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

Very severe gotcha in 'group' method #4

Open
jfrank14 opened this issue Sep 26, 2014 · 6 comments
Open

Very severe gotcha in 'group' method #4

jfrank14 opened this issue Sep 26, 2014 · 6 comments

Comments

@jfrank14
Copy link

The group method is documented to imply that it adds users to the group, but in fact it REPLACES the users in the group. If you add users A and B to a group X, and then you call it again to add users C and D, I found that the ONLY USERS IN THE GROUP are C and D, while A and B are no longer in the group.

I just learned this via a nasty shock when I was trying to add one user to a group with many existing users and found that after doing this that user was the ONLY user in the group.

The way Discourse itself adds a user to a group seems to be a POST to:

/admin/users/{userId}/groups
data: group_id={groupId}

@solhuebner
Copy link

Good point

@timolaine
Copy link
Contributor

That is indeed a good point. Would it be a good solution to make the group function only create groups that do not exist yet, and add a separate addUserToGroup function or similar?

@timolaine
Copy link
Contributor

Something like this I mean?

@jfrank14
Copy link
Author

I'm not sure I follow what this code change does. It looks like it deals with actually creating groups, which is important but I think unrelated.

I think the general answer to the problem is that there should be a method like:

 replaceGroupUsers(groupIdOrName, userIdOrNames)

which would make the specified group contain only the specified users and remove any previous ones.

And there should be a method like

addGroupUsers(groupIdOrName, userIdOrNames)

which would add the specified users to the group, PRESERVING any existing users that were previously in the group. (And there should probably be a removeGroupUsers too.)

Does that seem reasonable?

On 9/30/2014 7:14 AM, Timo Laine wrote:

Something like this timolaine@7436f0e I mean?


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

@timolaine
Copy link
Contributor

Sounds good. I believe my addUserToGroup function is close to your addGroupUsers, apart from the limitation that it can only add one user at a time.

I am sure a lot more functions are needed, I have only added ones that were necessary for my purposes. :-)

@jfrank14
Copy link
Author

Fair enough. But even as is, this has been a great resource for me for automating some things in Discourse. Thanks for open sourcing this!

On 9/30/2014 10:03 AM, Timo Laine wrote:

Sounds good. I believe my addUserToGroup function is close to your addGroupUsers, apart from the limitation that it can only add one user at a time.

I am sure a lot more functions are needed, I have only added ones that were necessary for my purposes. :-)


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

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

No branches or pull requests

3 participants