Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support for disabled modules, multiple database connections, lazy models creation, further implementation of core modules according to original idea given by Andrija Petrovic, support for submenu items ... #37

Closed
wants to merge 29 commits into from

Conversation

veljkopopovic
Copy link
Contributor

First of all, thank you for a fantastic project you have delivered to us :D I hope I will be able to help a bit.
Few things has been addressed here:

  1. support for module disabling. Adding disabled_modules list in configuration file will allow module loader to skip loading specified modules allowing developers to turn off modules got from repo without messing with original repo structure avoiding, that way, problems with pulling latest changes.
  2. multiple mongo DB connections situation. Adding dbs map in configuration file in format db_alias:connection_path will create connections to given mongodb paths. Once connecting databases is done, two functions are added to mongoose instance in order to make connection map available to outer world:
  • get_MEANIODB_connection : which will return connection for given db_alias. If record in map, default connection is returned
  • alias_MEANIODB_exists: helper function which allows to check if alias has been registered.

After setting these functions, rest of bootstrap is done so these functions remains visible to the rest of program ...
3. model loading helper has been introduced. It is backward compatible to support current implementation. If model.js file set to module.exports register field with model settings, model will be registered within requireModel function. This way, model file writer has some typing help: with previous solution, programmer should, in every model write something like this

mongoose.get_MEANIODB_connection(dbalias).model(model, schema, [collection])

With this solution model programmer should set :

module.exports = {
  register : [
    { schema: UserSchema, model: 'User', dbs:['users'] }
  ]
};

and requireModel function will do rest of job ...
On the other hand if UserSchema is a simple hash, not instance of mongoose.Schema, lazy model creation is invoked. This way every file invoked through module load process can extend schema object. Once all modules are loaded, lazy models creation is started: createModels will take resulting structure and create schema and model on designated db connection out of it ...
4. Last (for now) but not least, core modules has been implemented as far as possible ...

Best regards ...

@veljkopopovic veljkopopovic changed the title support for disabled modules support for disabled modules and multiple database connections Sep 11, 2014
@veljkopopovic veljkopopovic changed the title support for disabled modules and multiple database connections support for disabled modules, multiple database connections, further implementation of core modules according to original idea given by Andrija Petrovic Sep 12, 2014
@veljkopopovic veljkopopovic changed the title support for disabled modules, multiple database connections, further implementation of core modules according to original idea given by Andrija Petrovic support for disabled modules, multiple database connections, lazy models creation, further implementation of core modules according to original idea given by Andrija Petrovic Sep 12, 2014
@veljkopopovic
Copy link
Contributor Author

Here is lazy model creation example structure, bit twisted original users schema :

var UserSchema = {
  fields: {
    _id: {
      type: String,
      unique: [true, 'There is already a user with that username'],
      required: [true, 'Please provide a username']
    },
    snapshot: {
      type:Schema.Types.ObjectId,
      ref:'UserSnapshot'
    },
    hashed_password: {
      type: String,
      validate: [validatePresenceOf, 'Password cannot be blank']
    },
    provider: {
      type: String,
      default: 'local'
    },  
    roles: {
      type: Array,
      default: ['notapproved']
    },

    salt: String,
    resetPasswordToken: String,
    resetPasswordExpires: Date
  },
  virtual: {
    password: {
      set: function(password) {
        this._password = password;
        this.salt = this.makeSalt();
        this.hashed_password = this.hashPassword(password);
      },
      get: function () {
        return this._password;
      }
    }
  },
  pre: {
    'save': function(next) {
      if (this.isNew && this.provider === 'local' && this.password && !this.password.length)
      return next(new Error('Invalid password'));
      next();
    }
  },
  statics : {
    getUser: function (id, cb, full_profile) {
      this.findOne({ _id:id }, full_profile ? undefined : '-salt -hashed_password -__v')
        .populate('snapshot', '-_id -__v')
        .exec(cb);
    },
    createUser: function (userdata, cb) {
      var Ussm = mongoose.get_MEANIODB_connection('users').model('UserSnapshot');
      var uss = new Ussm (userdata);
      uss.save(function (e,d) {
        if (e) return cb (e);
        userdata._id = userdata.username;
        userdata.snapshot = d._id;
        var User = mongoose.get_MEANIODB_connection('users').model('User');
        var user = new User(userdata);
        user.save (cb);
      });
    }
  },
  methods: {
    /**
     * HasRole - check if the user has required role
     *
     * @param {String} plainText
     * @return {Boolean}
     * @api public
     */
    hasRole: function(role) {
      var roles = this.roles;
      return roles.indexOf('admin') !== -1 || roles.indexOf(role) !== -1;
    },

    /**
     * IsAdmin - check if the user is an administrator
     *
     * @return {Boolean}
     * @api public
     */
    isAdmin: function() {
      return this.roles.indexOf('admin') !== -1;
    },

    /**
     * Authenticate - check if the passwords are the same
     *
     * @param {String} plainText
     * @return {Boolean}
     * @api public
     */
    authenticate: function(plainText) {
      return this.hashPassword(plainText) === this.hashed_password;
    },

    /**
     * Make salt
     *
     * @return {String}
     * @api public
     */
    makeSalt: function() {
      return crypto.randomBytes(16).toString('base64');
    },

    /**
     * Hash password
     *
     * @param {String} password
     * @return {String}
     * @api public
     */
    hashPassword: function(password) {
      if (!password || !this.salt) return '';
      var salt = new Buffer(this.salt, 'base64');
      return crypto.pbkdf2Sync(password, salt, 10000, 64).toString('base64');
    }
  }
};

@andrija-hers
Copy link
Contributor

I'll take the opportunity to point out how all the core functionality is now split into core modules:

  • bootstrap (refactored to comply to "core module rules", with just a hook function and no return value)
  • config (encapsulates the Config class functionality)
  • db (most of the total job done in this PR has been done here)
  • menu (encapsulates the Menus functionality)
  • module (contains the basic Module functionality that other modules may extend by adding methods to the Meanio.prototype.Module.prototype)

Another important refactor: Config is now created without a callback in the constructor. So, Config is created before the DBs are initiated. Config initially does the util.loadSettings() and this function is now called only during Config creation. It makes no sense (both logically and in performance terms) to let other layers of the software do the util.loadSettings again. After the Config is created, the Meanio.prototype.loadSettings method refers to Meanio.Singleton.config.clean, of course.

@liorkesos
Copy link
Member

Its the weekend here but I can't wait to check this.
When we complete integrating your ideas + the mean.json + login and ux
fixes we will bump a version to 0.5x.
On Sep 12, 2014 6:10 PM, "Andrija Petrovic" notifications@github.com
wrote:

I'll take the opportunity to point out how all the core functionality is
now split into core modules:

  • bootstrap (refactored to comply to "core module rules", with just a
    hook function and no return value)
  • config (encapsulates the Config class functionality)
  • db (most of the total job done in this PR has been done here)
  • menu (encapsulates the Menus functionality)
  • module (contains the basic Module functionality that other modules
    may extend by adding methods to the Meanio.prototype.Module.prototype)

Another important refactor: Config is now created without a callback in
the constructor. So, Config is created before the DBs are initiated. Config
initially does the util.loadSettings() and this function is now called only
during Config creation. It makes no sense (both logically and in
performance terms) to let other layers of the software do the
util.loadSettings again. After the Config is created, the
Meanio.prototype.loadSettings method refers to
Meanio.Singleton.config.clean, of course.


Reply to this email directly or view it on GitHub
#37 (comment).

@veljkopopovic veljkopopovic changed the title support for disabled modules, multiple database connections, lazy models creation, further implementation of core modules according to original idea given by Andrija Petrovic support for disabled modules, multiple database connections, lazy models creation, further implementation of core modules according to original idea given by Andrija Petrovic, support for submenu items ... Sep 16, 2014
@ellman
Copy link
Collaborator

ellman commented Sep 16, 2014

hi @andrija-hers firstlly thanks so much for this awesome contribution. I like the direction now we just have to go over the PR, better understand it and see when/how it will integrate with the project etc

@veljkopopovic
Copy link
Contributor Author

Submenu items support has just been implemented ... Unfortunately, without client side code nothing can be seen, so I am about to create a PR for client side within few minutes as well ... Complete implementation is fully backwards compatible, so anyone who uses old code should be ok. News are path and name fields within dictionary given to Modulename.menus.add function. Path is slash separated string which should point to parent menu item. Name is, of course, name of given menu item which will serve for future reference.
So, if you want to add a menu item to main node, you should provide something like this:

  Test.menus.add({
    title: 'Test',
    link: 'test example page',
    name: 'test',
    path: 'main'
  });

Omitting path will add item to main menu node by default.
Now, if you want to add submenu item to an item with title 'Test', hash like this one should be provided:

Test.menus.add({
    title:'Test1',
    link:'test1',
    name: 'test1',
    path:'main/test'
});

This way, test1 will become submenu item of test ... And so on ...
Client side PR

Best regards ...

@ellman
Copy link
Collaborator

ellman commented Sep 17, 2014

error when using mean-admin package on fresh version of linnovate/mean branch master

TypeError: Cannot call method 'get' of undefined
at Menus.get (/home/yonatan/Repos/linnovate/tests/t1/node_modules/meanio/lib/menu.js:133:18)
at /home/yonatan/Repos/linnovate/tests/t1/packages/system/server/routes/menus.js:15:30
at Layer.handle as handle_request
at next (/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/route.js:100:13)
at Route.dispatch (/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/route.js:81:3)
at Layer.handle as handle_request
at /home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/index.js:227:24
at param (/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/index.js:324:14)
at param (/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/index.js:340:14)
at Function.proto.process_params (/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/index.js:384:3)

@andrija-hers
Copy link
Contributor

Dear Yonatan,

many thanks for the info! We'll get down to it - and eventually ask for
more info on how to regenerate the error (because we didn't experience this
problem).

On Wed, Sep 17, 2014 at 11:49 AM, Yonatan Ellman notifications@github.com
wrote:

error when using mean-admin package on fresh version of linnovate/mean
branch master

TypeError: Cannot call method 'get' of undefined
at Menus.get
(/home/yonatan/Repos/linnovate/tests/t1/node_modules/meanio/lib/menu.js:133:18)
at
/home/yonatan/Repos/linnovate/tests/t1/packages/system/server/routes/menus.js:15:30
at Layer.handle as handle_request
http://home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/layer.js:76:5
at next
(/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/route.js[image:
💯]13)
at Route.dispatch
(/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/route.js:81:3)
at Layer.handle as handle_request
http://home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/layer.js:76:5
at
/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/index.js:227:24
at param
(/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/index.js:324:14)
at param
(/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/index.js:340:14)
at Function.proto.process_params
(/home/yonatan/Repos/linnovate/tests/t1/node_modules/express/lib/router/index.js:384:3)


Reply to this email directly or view it on GitHub
#37 (comment).

@veljkopopovic
Copy link
Contributor Author

@ellman : thanx, got it, will fix it asap ...

@ellman
Copy link
Collaborator

ellman commented Sep 17, 2014

@veljkopopovic thanks. I see that sorted the issue with the menu I will continue testing.

@veljkopopovic
Copy link
Contributor Author

@ellman you've been bit faster :D Anyway, this should fix defaultMenu problem, allowing menu-admin package to actually display side bar, but my humble opinion is that defaultMenu is bit a dirty hack. In other words, why admin package does not register menu items within, let's say, 'side' menu within its register function staying consistent with other packages?
Regarding disabledModules, ok, will change it in order to follow convention from other settings fields. Did not think about it at the time proposal was given ...

@veljkopopovic
Copy link
Contributor Author

@ellman, @liorkesos : will write some comments tonight, no problem ... as well as bit of doc on this lazy mechanism as well

@liorkesos
Copy link
Member

@veljkopopovic - We've been looking in to this - it looks very cool till now.
I think of creating a 0.4.1 branch which will be a candidate to the 0.5.x - we will probably merge in to that.
There is a lot of documentation ti be done but we like the direction and are pretty sure this stuff makes it in.
In the future please work with smaller PR's so we can integrate them seperatly if possible.

@ellman
Copy link
Collaborator

ellman commented Sep 17, 2014

https://github.com/linnovate/mean-cli/tree/0.4.1 created for now as a common tinkering ground in preparation for the next major release.

@fyockm We merged the PR and pushed 0.4.1 branch which will mainly just be a placeholder for 0.5.0. In this branch we will test, benchmark and refine overall direction. If you get any time it will be great to get your opinion on the overall direction

@ellman
Copy link
Collaborator

ellman commented Sep 17, 2014

@veljkopopovic regarding the defaultMenu being a dirty hack.... Yes, you are 100% correct but we are kind of stuck with it at least until a major version.

Main problem is changes breaking packages. In theory we could just refactor mean-admin and maybe one or 2 others. Which would probably be a better direction. @liorkesos what do you think?

@veljkopopovic
Copy link
Contributor Author

@ellman, @liorkesos sorry being late for this doc thing, we're in bit a rush here in Belgrade, had no time to write something ... Hopefully, I'll grab some time this weekend ...
Best regards ...

fyockm and others added 8 commits September 20, 2014 09:38
* lirantal-cleanup-spellings:
  fixing spellings
  adding my own CLA to the list
…/mean-cli into lirantal-cleanup-jshint-warnings

* 'cleanup-jshint-warnings' of https://github.com/lirantal/mean-cli:
  cleaning up jshints issues
  adding javascript implicit strict
  cleaning up jshint semi-colon issues
  fixed jshint issues - removed anonymous function() definition within a loop, with no 'code' variable being passed to
@veljkopopovic
Copy link
Contributor Author

Let's close these and move along

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants