discourse-api-php icon indicating copy to clipboard operation
discourse-api-php copied to clipboard

Very severe gotcha in 'group' method

Open jfrank14 opened this issue 10 years ago • 6 comments

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}

jfrank14 avatar Sep 26 '14 17:09 jfrank14

Good point

solhuebner avatar Sep 28 '14 21:09 solhuebner

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 avatar Sep 30 '14 07:09 timolaine

Something like this I mean?

timolaine avatar Sep 30 '14 11:09 timolaine

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 https://github.com/timolaine/discourse-api-php/commit/7436f0ed7b9756676bed73c3caa4632be96f318c I mean?

— Reply to this email directly or view it on GitHub https://github.com/discoursehosting/discourse-api-php/issues/4#issuecomment-57298333.

jfrank14 avatar Sep 30 '14 13:09 jfrank14

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. :-)

timolaine avatar Sep 30 '14 14:09 timolaine

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 https://github.com/discoursehosting/discourse-api-php/issues/4#issuecomment-57318123.

jfrank14 avatar Sep 30 '14 14:09 jfrank14