Groups as objects about meteor-roles HOT 7 CLOSED

Hi Kevin,

I think your approach is correct of having a separate groups collection and using the group id as the group wrt the Roles package. Rather than changing the Roles code to detect ObjectId I'd suggest using the string version of the ObjectId. You may be able to use 'toString()' or if that doesn't work, try 'toHexString()'. Here's a SO question that seemed relevant:

http://stackoverflow.com/questions/15041128/meteor-0-5-7-how-to-handle-use-meteor-collection-objectid

from meteor-roles.

Thanks for this.

Can I just check - what's the rationale for not allowing an ObjectId as a key for the groups dictionary?

I'm happy to use the toString() value here but obviously it's more convenient to keep the same data type for a field throughout the code. Also, apparently using ObjectId is more performant than string (although I don't know if that applies when were talking about an embedded document and not the _id field.)

from meteor-roles.

I would be happy to accept a pull request if you'd like to work on that. Just be sure to add a test or two to cover the new case.

On Apr 24, 2014, at 9:27 PM, Kevin Bennett [email protected] wrote:

Thanks for this.

Can I just check - what's the rationale for not allowing an ObjectId as a key for the groups dictionary?

I'm happy to use the toString() value here but obviously it's more convenient to keep the same data type for a field throughout the code. Also, apparently using ObjectId is more performant than string (although I don't know if that applies when were talking about an embedded document and not the _id field.)

—
Reply to this email directly or view it on GitHub.

from meteor-roles.

Heh, I forked and started implementing it and then discovered that in MondgoDB keys must be strings. So that puts an end to that. It could be possible by restructuring the roles doc to be an array, but a clunky solution and breaks backwards compatibility. You can mark this as closed : )

On a related note, would you accept a pull for the following:

• Roles.getAllGroups() - list all groups that exist for all users
• Roles.getGroupsForUser(role) - list all groups a user belongs to, optional restrict by role
• Automatically creating a group collection and insert a group each time a user is added to a new group, same as roles collection.

Thanks

from meteor-roles.

Thanks for looking into it.

Re: the other PRs, definitely for the first two.

Having a group collection that Roles creates is something I'd probably avoid. I debated whether to have the roles collection when I first wrote this package. I added it because I thought it would be useful for UI purposes to have a list of all roles but it's caused a lot of confusion for people so I think it was a mistake.

Can you expand a bit of your use case for how an automatically created groups collection would help?

Sent from my phone

On Apr 25, 2014, at 12:19 PM, Kevin Bennett [email protected] wrote:

Heh, I forked and started implementing it and then discovered that in MondgoDB keys must be strings. So that puts an end to that. It could be possible by restructuring the roles doc to be an array, but a clunky solution and breaks backwards compatibility. You can mark this as closed : )

On a related note, would you accept a pull for the following:

Roles.getAllGroups() - list all groups that exist for all users
Roles.getGroupsForUser(role) - list all groups a user belongs to, optional restrict by role
Automatically creating a group collection and insert a group each time a user is added to a new group, same as roles collection.
Thanks

—
Reply to this email directly or view it on GitHub.

from meteor-roles.

I was thinking along the same lines as you were for 'roles' - I would like to display a list of all groups in the UI and it would be easier with a collection of all groups. But you're probably right that it's more confusing than necessary.

I'm gonna implement the getAllGroups and getGroupsForUser methods for now. If I need the create groups collection functionality later, we could achieve it by adding in hooks like onCreateGroupHook or something, and allow the new group to be published to a collection if required.

from meteor-roles.

I sent a pull request for Roles.getGroupsForUser with tests.

I see now that Roles.getAllGroups will be an expensive query, pulling the whole users collection and filtering down to unique keys in user.roles. For my use-case, this isn't acceptable. So I'm thinking I create a groups collection outside of the meteor-roles package.

I'll get back to you if I'm thinking of adding anything else.

from meteor-roles.