realms: Create default system user groups for internal realm. #22352

sahil839 · 2022-06-29T14:54:37Z

Since we include internal realms while creating system groups
in "0382_create_role_based_system_groups.py", we should do it
when creating new internal realms as well to be consistent.
This PR also modifies bulk_create_users to add the users
to the respective system groups.

And we would also need a migration to add the groups to internal
realms created after the previous migration.

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

zerver/migrations/0395_create_role_based_groups_for_internal_realms.py

sahil839 · 2022-07-12T18:08:04Z

@timabbott this is ready for your review.

sahil839 · 2022-07-21T17:52:31Z

@timabbott can you review this? Would be good to get this one completed as I will open the next PR for adding a new setting based on groups model tommorrow, and would be nice to get this one off the list.

timabbott · 2022-07-27T00:25:23Z

This looks to me to be a correct implementation of fixing this inconsistency by adding these groups to the system bot realm.

I feel like it might be more correct to instead remove them from the system bot realm, if that realm doesn't contain users beyond the system bots... but I'm not sure how to code that in a safe fashion.

@mateuszmandera thoughts? I can see going either way on this, given our eventual goal to eliminate the system bot realm as a concept...

sahil839 · 2022-07-27T18:08:15Z

We currently define internal bots in settings.INTERNAL_BOTS so we can probably check through emails I think. Not think there would be many of them with users other than system bots.

mateuszmandera · 2022-07-27T18:50:41Z

I feel like it might be more correct to instead remove them from the system bot realm, if that realm doesn't contain users beyond the system bots... but I'm not sure how to code that in a safe fashion.

Hmm, what would the advantage be? Isn't it simpler to just have the clear property the system realm has system user groups rather than it being conditioned on whether the realm has additional human users or not, with all the extra code that would be needed for ensuring that conditionality?

mateuszmandera · 2022-07-27T19:02:14Z

zerver/lib/bulk_create.py

@@ -116,6 +118,26 @@ def bulk_create_users(

    Subscription.objects.bulk_create(subscriptions_to_create)

+    full_members_system_group = UserGroup.objects.get(


This codepath is also called via do_import_system_bots calling create_users, which calls this bulk_create_users - in import_realm.py - have you checked if this will work fine? E.g. if importing from another server, which has these UserGroupMemberships in the internal realm, won't an error happen due to the UserGroupMemberships being attempted to be created twice - once through this codepath and once through the import of user groups?

I haven't looked at this in-depth, just a tricky edge case that came to mind that is not clear to me if was addressed - we can audit further if needed

I checked it once again now and this will work fine. do_import_system_bots is called after importing realm, and we also check whether the bots are already created or not and I think we can assume that the UserGroupMemberships objects in the import data will only be present if UserProfile objects are present in import data.

Okay, cool thanks for checking!

I think the comment here is incorrect right now, though, since this is called from the data import code path. However, as noted in the thread I started here:

https://chat.zulip.org/#narrow/stream/3-backend/topic/do_import_system_bots/near/1417790

I think that the data import code path is a noop (and would be a bug if it weren't) and should be deleted, so the comment is likely fine as written.

mateuszmandera

Looks good I think with those couple of tiny tweaks mentioned

mateuszmandera · 2022-08-01T16:38:05Z

zerver/lib/bulk_create.py

@@ -116,6 +118,26 @@ def bulk_create_users(

    Subscription.objects.bulk_create(subscriptions_to_create)

+    full_members_system_group = UserGroup.objects.get(
+        name="@role:fullmembers", realm=realm, is_system_group=True


Should these "@role:fullmembers" etc. string be deduplicated and instead be centrally hard-coded in UserGroup.FULL_MEMBERS etc. or something like that? There's some other places where these strings are repeated. Or is that intended since maybe this popped up in the reviews of the original implementation?

I don't think we discussed in the original implementation, but I liked the idea of deduplicating this, does UserGroup.FULL_MEMBERS_GROUP_NAME sounds good. For other cases we can probably use UserGroup.SYSTEM_USER_GROUP_ROLE_MAP, I know this would be long but we already declare name of user groups there.

Yeah, that name sounds good to me

Yeah those names sound great.

Reading this, I think the UserGroup.SYSTEM_USER_GROUP_ROLE_MAP references feel cumbersome. I think it's pretty reasonable to define constants like EVERYONE_ON_INTERNET_GROUP_NAME = "@role:internet" for all 5-6 of them, and have SYSTEM_USER_GROUP_ROLE_MAP use those values in its definitions, rather than this approach.

I'll drop the last commit and also tweak the other two final commits to fit that proposed pattern.

zerver/lib/bulk_create.py

zerver/migrations/0401_create_role_based_groups_for_internal_realms.py

sahil839 · 2022-08-02T07:11:25Z

Added the assert statement and a comment in migration as asked above.

mateuszmandera

Awesome, thanks!

sahil839 · 2022-08-08T17:45:22Z

Added commits to add constants for user group names wherever requried and using them and SYSTEM_USER_GROUP_ROLE_MAP to get user groups from names and in tests.

timabbott · 2022-08-10T00:35:07Z

This is great, merged with a few changes:

The tweaks mentioned here: realms: Create default system user groups for internal realm. #22352 (comment)
Made sure the comment for the migration appears at the top of the file; that's more likely to be visible when looking for the information. I left a copy at the bottom as well.

sahil839 · 2022-08-10T16:36:09Z

@timabbott This was not merge automatically because CI was failing due to linter error. I fixed it and also added commits for other group names. CI is not passing due to puppeteer error, which I think would be fixed by #22692.

Since we include internal realms while creating system groups in "0382_create_role_based_system_groups.py", we should do it when creating new internal realms as well to be consistent. Tests are changed accordingly as UserGroup objects are created. We also change the user group ids used in api docs examples such that user groups are of correct realm.

This commit modifies bulk_create_users to add the users to the respective system groups. And due to this change, now bots in development environment are also added to system groups. Tests are changed accordingly as more UserGroupMembeship objects are created.

There may be some internal realms which were created after applying "0382_create_role_based_system_groups.py" migration and this migration is used to create system groups for those realms.

We now use FULL_MEMBERS_GROUP_NAME instead of writing the actual full members system group name at multiple places, so that we can have all the group names coded at one place only.

We now use EVERYONE_ON_INTERNET_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

We now use OWNERS_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

We now use ADMINISTRATORS_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

We now use MODERATORS_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

We now use MEMBERS_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

We now use EVERYONE_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

sahil839 · 2022-08-11T11:43:10Z

Ohh, this got auto-merged as I repushed again for CI to pass. But I added some more commits too.

timabbott · 2022-08-11T21:12:28Z

Yeah, that's a possibly surprising interaction with how GitHub's "merge once CI passes" feature works -- basically it'll merge the next time the PR passes CI, regardless of whether there are actual changes or not.

In this case, I've reviewed the new commits and they look great, so not a problem :)

zulipbot added the size: XL label Jun 29, 2022

sahil839 commented Jun 29, 2022

View reviewed changes

zerver/migrations/0395_create_role_based_groups_for_internal_realms.py Outdated Show resolved Hide resolved

sahil839 force-pushed the internal-realm branch 3 times, most recently from fdca74c to e4c2725 Compare June 30, 2022 07:54

sahil839 force-pushed the internal-realm branch 4 times, most recently from 64852f9 to bf91d04 Compare July 12, 2022 17:01

sahil839 force-pushed the internal-realm branch 3 times, most recently from c73dfd7 to b66e2c8 Compare July 21, 2022 17:13

sahil839 force-pushed the internal-realm branch from b66e2c8 to 7f05eec Compare July 26, 2022 14:14

sahil839 mentioned this pull request Jul 26, 2022

Add can_remove_subscribers_group stream setting. #22589

Merged

12 tasks

timabbott requested a review from mateuszmandera July 27, 2022 00:23

timabbott force-pushed the internal-realm branch from 7f05eec to c4a27c4 Compare July 27, 2022 00:24

mateuszmandera reviewed Jul 27, 2022

View reviewed changes

mateuszmandera reviewed Aug 1, 2022

View reviewed changes

sahil839 force-pushed the internal-realm branch from c4a27c4 to ecb4273 Compare August 1, 2022 18:06

mateuszmandera approved these changes Aug 2, 2022

View reviewed changes

sahil839 force-pushed the internal-realm branch 2 times, most recently from f8d95da to 4656dc6 Compare August 8, 2022 17:40

timabbott force-pushed the internal-realm branch from 4656dc6 to 6c2bcc1 Compare August 10, 2022 00:34

timabbott enabled auto-merge (rebase) August 10, 2022 00:34

sahil839 force-pushed the internal-realm branch from 6c2bcc1 to 7bc0d4e Compare August 10, 2022 11:28

sahil839 added 10 commits August 11, 2022 16:38

migrations: Add migration to create system groups for internal realms.

5a31090

There may be some internal realms which were created after applying "0382_create_role_based_system_groups.py" migration and this migration is used to create system groups for those realms.

user_groups: Add FULL_MEMBERS_GROUP_NAME constant.

9a7bf71

We now use FULL_MEMBERS_GROUP_NAME instead of writing the actual full members system group name at multiple places, so that we can have all the group names coded at one place only.

user_groups: Add EVERYONE_ON_INTERNET_GROUP_NAME constant.

64c0008

We now use EVERYONE_ON_INTERNET_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

user_groups: Add OWNERS_GROUP_NAME constant.

6b83238

We now use OWNERS_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

user_groups: Add ADMINISTRATORS_GROUP_NAME constant.

09b438b

We now use ADMINISTRATORS_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

user_groups: Add MODERATORS_GROUP_NAME constant.

7086259

We now use MODERATORS_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

user_groups: Add MEMBERS_GROUP_NAME constant.

3772307

We now use MEMBERS_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

user_groups: Add EVERYONE_GROUP_NAME constant.

8c5f94c

We now use EVERYONE_GROUP_NAME instead of writing the actual group name at multiple places, so that we can have all the group names coded at one place only.

sahil839 force-pushed the internal-realm branch from 7bc0d4e to 8c5f94c Compare August 11, 2022 11:08

timabbott merged commit 544d58a into zulip:main Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

realms: Create default system user groups for internal realm. #22352

realms: Create default system user groups for internal realm. #22352

sahil839 commented Jun 29, 2022 •

edited

Loading

sahil839 commented Jul 12, 2022

sahil839 commented Jul 21, 2022

timabbott commented Jul 27, 2022

sahil839 commented Jul 27, 2022

mateuszmandera commented Jul 27, 2022

mateuszmandera Jul 27, 2022 •

edited

Loading

sahil839 Jul 28, 2022

mateuszmandera Aug 1, 2022

timabbott Aug 10, 2022

mateuszmandera left a comment

mateuszmandera Aug 1, 2022

sahil839 Aug 2, 2022

mateuszmandera Aug 2, 2022

timabbott Aug 10, 2022

timabbott Aug 10, 2022

sahil839 commented Aug 2, 2022

mateuszmandera left a comment

sahil839 commented Aug 8, 2022

timabbott commented Aug 10, 2022

sahil839 commented Aug 10, 2022

sahil839 commented Aug 11, 2022 •

edited

Loading

timabbott commented Aug 11, 2022

		@@ -116,6 +118,26 @@ def bulk_create_users(

		Subscription.objects.bulk_create(subscriptions_to_create)

		full_members_system_group = UserGroup.objects.get(

realms: Create default system user groups for internal realm. #22352

realms: Create default system user groups for internal realm. #22352

Conversation

sahil839 commented Jun 29, 2022 • edited Loading

sahil839 commented Jul 12, 2022

sahil839 commented Jul 21, 2022

timabbott commented Jul 27, 2022

sahil839 commented Jul 27, 2022

mateuszmandera commented Jul 27, 2022

mateuszmandera Jul 27, 2022 • edited Loading

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

mateuszmandera left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil839 commented Aug 2, 2022

mateuszmandera left a comment

Choose a reason for hiding this comment

sahil839 commented Aug 8, 2022

timabbott commented Aug 10, 2022

sahil839 commented Aug 10, 2022

sahil839 commented Aug 11, 2022 • edited Loading

timabbott commented Aug 11, 2022

sahil839 commented Jun 29, 2022 •

edited

Loading

mateuszmandera Jul 27, 2022 •

edited

Loading

sahil839 commented Aug 11, 2022 •

edited

Loading