FEATURE: Allow group owners promote more owners #19768

Drenmi · 2023-01-06T08:44:16Z

What is this feature?

Based on this feature request in Meta. Allows group owners (in addition to admins) to promote other members to owners.

What's not in this PR?

Some of the things mentioned in the Meta thread that I think might warrant their own PRs.

Demoting other group owners. (A bit of a rabbit hole here with potentially displaying warnings if you're demoting yourself, making the group "ownerless", etc.)
Clean-up of the admin logs.
Clean-up of code that was "lifted and shifted" between controllers.

What's the implementation like?

There were two pre-existing permission checks: "can edit group" and "can admin group". The former was used to allow group owners to add new members to the group.

This change makes it so that users with "can edit group" permission (i.e. group owners) can also promote owners.

To make this possible, the #add_owners controller action has been migrated from the admin namespace (since that is hidden behind a staff constraint) which required just some minor tweaking of the parameters since they were not in line between the two groups controllers.

The old test cases still hold, except for non-owners which should now receive 403 instead of 404 (from the routing constraint). In addition there's a test added for the group owner happy path.

Verification

While logged in as a non-admin group owner:

Desktop:

Desktop (bulk):

Mobile:

app/assets/javascripts/discourse/app/controllers/group-index.js

Drenmi · 2023-01-06T08:46:38Z

spec/requests/groups_controller_spec.rb

+      context "when logged in as a non-owner" do
+        before { sign_in(user) }
+
+        it "prevents adding of owners with a 403 response" do


Changed from 404 (routing constraint) to 403 (authorization) since this is now in a public controller.

Drenmi · 2023-01-06T08:46:50Z

spec/requests/groups_controller_spec.rb

+        end
+      end
+
+      context "when logged in as an owner" do


Test case for the new happy path.

app/controllers/groups_controller.rb

spec/requests/groups_controller_spec.rb

jjaffeux · 2023-01-06T09:09:11Z

spec/requests/groups_controller_spec.rb

+            put "/groups/#{group.id}/owners.json", params: {
+              usernames: user.username
+            }
+          end.to_not change { group.group_users.count }


Im always suspicious of this kind of code without reload it might regress due to some callbacks and we wouldn't get it I think

Reading this again, it wasn't very clear. What I meant: group.group_users.reload.count and group.group_users.count could have different output based on callbacks added to models

Interesting take! I was always a bit suspicious about code that required reload to test, but I guess we're onto the same thing, just from opposite directions. 🙂 Do you think this should be reloaded in anticipation of future changes, or you think whoever adds a callback later should notice the test failing and address?

jjaffeux · 2023-01-06T09:10:05Z

@Drenmi is this something you wanted to be merged for 3.0?

jjaffeux · 2023-01-06T09:10:39Z

@Drenmi note the backend failures look legit

Drenmi · 2023-01-06T09:41:13Z

is this something you wanted to be merged for 3.0?

Nat confirmed it's for 3.1.

note the backend failures look legit

Fixed! 🤞

app/assets/javascripts/discourse/app/components/groups-form-interaction-fields.js

nbianca · 2023-01-06T09:40:09Z

app/assets/javascripts/discourse/app/templates/group-index.hbs

@@ -54,6 +54,7 @@
                    <BulkGroupMemberDropdown
                      @bulkSelection={{this.bulkSelection}}
                      @canAdminGroup={{this.model.can_admin_group}}
+                      @canEditGroup={{this.model.can_edit_group}}


Having both can_admin_group and can_edit_group is a bit confusing, but not related to your change and there is already a comment in the code that explains that can_edit_group is just for membership changes.

I agree with you. I think the naming exacerbates the problem a bit, as it's not apparent what "edit" and "admin" means. I was actually thinking ahead, how permissions can be structured in a way that one can easily look at it and see what a certain permission can do and not. 🙂

app/controllers/groups_controller.rb

nbianca · 2023-01-06T09:46:43Z

app/controllers/groups_controller.rb

+      end
+    end
+
+    group.restore_user_count!


What does this line do?

Managed to dig this out. It's part of Rails dirty-checking. It's a method that's generated here. Why we need that here I still haven't figured out, but I trust it for now. 😬

nbianca · 2023-01-06T09:51:44Z

app/controllers/groups_controller.rb

+
+    users.each do |user|
+      if !group.users.include?(user)
+        group.add(user)


This could be group.add_owner and then you can drop some of the code around this.

Also, not related to your code, but I hate that we have both add and add_owner, when the latter should just be an argument of the first method.

Looking at the methods, it seems to me #add_owner should perhaps just be #make_owner or #promote_owner. Then we'd have two methods with two distinct responsibilities, whereas right now #add_owner is overstepping a bit.

Drenmi · 2023-01-09T06:04:26Z

Thank you for the reviews, @jjaffeux, @nbianca! 🙇

I have addressed your comments with a mix of amendments and topics to revisit some things later. I am weary to change too much of the existing code at the same time as introducing new behaviour. Please re-review and see if you're okay with the current version. 💌

Drenmi · 2023-01-10T10:33:25Z

Any changes you see as required before approving, @jjaffeux, @nbianca? Need one GitHub approval at least to proceed. 😅

app/assets/javascripts/discourse/app/components/groups-form-interaction-fields.js

nbianca

Looks good to me.

discoursebot · 2023-01-12T02:07:39Z

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/allow-group-owners-to-assign-other-members-as-owners/182227/18

Drenmi commented Jan 6, 2023

View reviewed changes

app/assets/javascripts/discourse/app/controllers/group-index.js Outdated Show resolved Hide resolved

Drenmi commented Jan 6, 2023

View reviewed changes

Drenmi force-pushed the feature/group-owners-promote-owners branch from 76c3249 to 798ecdd Compare January 6, 2023 09:02

jjaffeux reviewed Jan 6, 2023

View reviewed changes

app/controllers/groups_controller.rb Show resolved Hide resolved

jjaffeux reviewed Jan 6, 2023

View reviewed changes

spec/requests/groups_controller_spec.rb Outdated Show resolved Hide resolved

jjaffeux reviewed Jan 6, 2023

View reviewed changes

jjaffeux requested a review from nbianca January 6, 2023 09:10

Drenmi force-pushed the feature/group-owners-promote-owners branch from 798ecdd to 6ce1808 Compare January 6, 2023 09:29

nbianca added the 3.1 label Jan 6, 2023

nbianca reviewed Jan 6, 2023

View reviewed changes

Drenmi force-pushed the feature/group-owners-promote-owners branch from 6ce1808 to 264cca3 Compare January 9, 2023 02:43

Drenmi force-pushed the feature/group-owners-promote-owners branch from 264cca3 to d22a671 Compare January 9, 2023 10:27

nbianca reviewed Jan 10, 2023

View reviewed changes

app/assets/javascripts/discourse/app/components/groups-form-interaction-fields.js Outdated Show resolved Hide resolved

nbianca approved these changes Jan 10, 2023

View reviewed changes

Drenmi added 2 commits January 11, 2023 15:43

Add can_edit_group attribute to BasicGroupSerializer

dea7fe4

Include 'Make owner' option in drop-down for existing group owners

456146e

Drenmi force-pushed the feature/group-owners-promote-owners branch 2 times, most recently from cc8a15e to 664cb7e Compare January 11, 2023 08:16

Drenmi added 2 commits January 11, 2023 16:24

Move group ownership endpoint out of admin namespace

c61da98

We only need one GroupActionLogger per action

289d56a

Drenmi force-pushed the feature/group-owners-promote-owners branch from 664cb7e to 289d56a Compare January 11, 2023 08:24

Drenmi merged commit d2e9ea6 into discourse:main Jan 11, 2023

Drenmi deleted the feature/group-owners-promote-owners branch January 11, 2023 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Allow group owners promote more owners #19768

FEATURE: Allow group owners promote more owners #19768

Drenmi commented Jan 6, 2023 •

edited

Drenmi Jan 6, 2023

Drenmi Jan 6, 2023

jjaffeux Jan 6, 2023

jjaffeux Jan 6, 2023

Drenmi Jan 9, 2023

jjaffeux commented Jan 6, 2023

jjaffeux commented Jan 6, 2023

Drenmi commented Jan 6, 2023

nbianca Jan 6, 2023

Drenmi Jan 9, 2023

nbianca Jan 6, 2023

Drenmi Jan 9, 2023

nbianca Jan 6, 2023

Drenmi Jan 9, 2023 •

edited

Drenmi commented Jan 9, 2023

Drenmi commented Jan 10, 2023

nbianca left a comment

discoursebot commented Jan 12, 2023

FEATURE: Allow group owners promote more owners #19768

FEATURE: Allow group owners promote more owners #19768

Conversation

Drenmi commented Jan 6, 2023 • edited

What is this feature?

What's not in this PR?

What's the implementation like?

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjaffeux commented Jan 6, 2023

jjaffeux commented Jan 6, 2023

Drenmi commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drenmi Jan 9, 2023 • edited

Choose a reason for hiding this comment

Drenmi commented Jan 9, 2023

Drenmi commented Jan 10, 2023

nbianca left a comment

Choose a reason for hiding this comment

discoursebot commented Jan 12, 2023

Drenmi commented Jan 6, 2023 •

edited

Drenmi Jan 9, 2023 •

edited