events: Update person dict in event for do_change_user_role to send role.

This commit changes the person dict in event sent by do_change_user_role
to send role instead of is_admin or is_guest.

This makes things much more straightforward for our upcoming primary
owners feature.

frontend_tests/node_tests/user_events.js

@@ -1,6 +1,7 @@
 set_global('$', global.make_zjquery());
 
 zrequire('people');
+const settings_config = zrequire('settings_config');
 zrequire('user_events');
 
 set_global('activity', {
@@ -75,14 +76,17 @@ run_test('updates', () => {
     };
     people.add_active_user(isaac);
 
-    user_events.update_person({user_id: isaac.user_id, is_guest: true});
+    user_events.update_person({user_id: isaac.user_id,
+                               role: settings_config.user_role_values.guest.code});
     person = people.get_by_email(isaac.email);
     assert(person.is_guest);
-    user_events.update_person({user_id: isaac.user_id, is_guest: false});
+    user_events.update_person({user_id: isaac.user_id,
+                               role: settings_config.user_role_values.member.code});
     person = people.get_by_email(isaac.email);
     assert(!person.is_guest);
 
-    user_events.update_person({user_id: isaac.user_id, is_admin: true});
+    user_events.update_person({user_id: isaac.user_id,
+                               role: settings_config.user_role_values.admin.code});
     person = people.get_by_email(isaac.email);
     assert.equal(person.full_name, 'Isaac Newton');
     assert.equal(person.is_admin, true);
@@ -101,7 +105,8 @@ run_test('updates', () => {
     assert.equal(user_id, isaac.user_id);
     assert.equal(full_name, 'Sir Isaac');
 
-    user_events.update_person({user_id: me.user_id, is_admin: false});
+    user_events.update_person({user_id: me.user_id,
+                               role: settings_config.user_role_values.member.code});
     assert(!global.page_params.is_admin);
 
     user_events.update_person({user_id: me.user_id, full_name: 'Me V2'});

static/js/settings_users.js

@@ -364,7 +364,7 @@ exports.update_user_data = function (user_id, new_data) {
         }
     }
 
-    if (new_data.is_admin !== undefined || new_data.is_guest !== undefined) {
+    if (new_data.role !== undefined) {
         user_row.find(".user_role").text(people.get_user_type(user_id));
     }
 };

static/js/user_events.js

@@ -2,7 +2,7 @@
 // server_events.js simple while breaking some circular
 // dependencies that existed when this code was in people.js.
 // (We should do bot updates here too.)
-
+const settings_config = require('./settings_config');
 exports.update_person = function update(person) {
     const person_obj = people.get_by_user_id(person.user_id);
 
@@ -47,12 +47,13 @@ exports.update_person = function update(person) {
         }
     }
 
-    if (Object.prototype.hasOwnProperty.call(person, 'is_admin')) {
+    if (Object.prototype.hasOwnProperty.call(person, 'role')) {
-        person_obj.is_admin = person.is_admin;
+        person_obj.is_admin = person.role === settings_config.user_role_values.admin.code;
+        person_obj.is_guest = person.role === settings_config.user_role_values.guest.code;
         settings_users.update_user_data(person.user_id, person);
 
-        if (people.is_my_user_id(person.user_id)) {
+        if (people.is_my_user_id(person.user_id) && page_params.is_admin !== person_obj.is_admin) {
-            page_params.is_admin = person.is_admin;
+            page_params.is_admin = person_obj.is_admin;
             gear_menu.update_org_settings_menu_item();
             settings_linkifiers.maybe_disable_widgets();
             settings_org.maybe_disable_widgets();
@@ -61,11 +62,6 @@ exports.update_person = function update(person) {
         }
     }
 
-    if (Object.prototype.hasOwnProperty.call(person, 'is_guest')) {
-        person_obj.is_guest = person.is_guest;
-        settings_users.update_user_data(person.user_id, person);
-    }
-
     if (Object.prototype.hasOwnProperty.call(person, 'avatar_url')) {
         const url = person.avatar_url;
         person_obj.avatar_url = url;

templates/zerver/api/changelog.md

@@ -19,6 +19,9 @@ below features are supported.
 * [`PATCH /users/{user_id}`](/api/update-user): The `is_admin` and
   `is_guest` parameters were removed in favor of the more general
   `role` parameter for specifying a change in user role.
+* [`GET /events`](/api/get-events-from-queue): `realm_user` events
+  sent when a user's role changes now include `role` property, instead
+  of the previous `is_guest` or `is_admin` booleans.
 
 **Feature level 6**
 * [`GET /events`](/api/get-events-from-queue): `realm_user` events to

zerver/lib/actions.py

@@ -3438,21 +3438,9 @@ def do_change_user_role(user_profile: UserProfile, value: int) -> None:
             RealmAuditLog.NEW_VALUE: value,
             RealmAuditLog.ROLE_COUNT: realm_user_count_by_role(user_profile.realm),
         }))
-    if UserProfile.ROLE_REALM_ADMINISTRATOR in [old_value, value]:
+    event = dict(type="realm_user", op="update",
-        event = dict(type="realm_user", op="update",
+                 person=dict(user_id=user_profile.id, role=user_profile.role))
-                     person=dict(user_id=user_profile.id,
+    send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id))
-                                 is_admin=value == UserProfile.ROLE_REALM_ADMINISTRATOR))
-        send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id))
-    if UserProfile.ROLE_GUEST in [old_value, value]:
-        event = dict(type="realm_user", op="update",
-                     person=dict(user_id=user_profile.id,
-                                 is_guest=value == UserProfile.ROLE_GUEST))
-        send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id))
-    if UserProfile.ROLE_REALM_OWNER in [old_value, value]:
-        event = dict(type="realm_user", op="update",
-                     person=dict(user_id=user_profile.id,
-                                 is_owner=value == UserProfile.ROLE_REALM_OWNER))
-        send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id))
 
 def do_change_is_api_super_user(user_profile: UserProfile, value: bool) -> None:
     user_profile.is_api_super_user = value

zerver/lib/events.py

@@ -48,7 +48,7 @@ from zerver.lib.actions import (
     get_owned_bot_dicts,
     get_available_notification_sounds,
 )
-from zerver.lib.users import get_cross_realm_dicts, get_raw_user_data
+from zerver.lib.users import get_cross_realm_dicts, get_raw_user_data, is_administrator_role
 from zerver.lib.user_groups import user_groups_in_realm_serialized
 from zerver.lib.user_status import get_user_info_dict
 from zerver.tornado.event_queue import request_event_queue, get_user_events
@@ -418,7 +418,11 @@ def apply_event(state: Dict[str, Any],
                     state['avatar_url'] = person['avatar_url']
                     state['avatar_url_medium'] = person['avatar_url_medium']
 
-                for field in ['is_admin', 'delivery_email', 'email', 'full_name']:
+                if 'role' in person:
+                    state['is_admin'] = is_administrator_role(person['role'])
+                    state['is_guest'] = person['role'] == UserProfile.ROLE_GUEST
+
+                for field in ['delivery_email', 'email', 'full_name']:
                     if field in person and field in state:
                         state[field] = person[field]
 
@@ -428,10 +432,10 @@ def apply_event(state: Dict[str, Any],
                 # realm.  This is ugly and probably better
                 # solved by removing the all-realm-bots data
                 # given to admin users from this flow.
-                if ('is_admin' in person and 'realm_bots' in state):
+                if ('role' in person and 'realm_bots' in state):
                     prev_state = state['raw_users'][user_profile.id]
                     was_admin = prev_state['is_admin']
-                    now_admin = person['is_admin']
+                    now_admin = is_administrator_role(person['role'])
 
                     if was_admin and not now_admin:
                         state['realm_bots'] = []
@@ -449,6 +453,9 @@ def apply_event(state: Dict[str, Any],
                 for field in p:
                     if field in person:
                         p[field] = person[field]
+                    if 'role' in person:
+                        p['is_admin'] = is_administrator_role(person['role'])
+                        p['is_guest'] = person['role'] == UserProfile.ROLE_GUEST
                     if 'custom_profile_field' in person:
                         custom_field_id = person['custom_profile_field']['id']
                         custom_field_new_value = person['custom_profile_field']['value']

zerver/lib/users.py

@@ -127,6 +127,9 @@ def check_valid_interface_type(interface_type: Optional[int]) -> None:
     if interface_type not in Service.ALLOWED_INTERFACE_TYPES:
         raise JsonableError(_('Invalid interface type'))
 
+def is_administrator_role(role: int) -> bool:
+    return role in {UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_REALM_OWNER}
+
 def bulk_get_users(emails: List[str], realm: Optional[Realm],
                    base_query: 'QuerySet[UserProfile]'=None) -> Dict[str, UserProfile]:
     if base_query is None:

zerver/tests/test_events.py

@@ -1830,7 +1830,7 @@ class EventsRegisterTest(ZulipTestCase):
             ('type', equals('realm_user')),
             ('op', equals('update')),
             ('person', check_dict_only([
-                ('is_admin', check_bool),
+                ('role', check_int_in(UserProfile.ROLE_TYPES)),
                 ('user_id', check_int),
             ])),
         ])

zerver/tests/test_users.py

@@ -148,7 +148,7 @@ class PermissionTest(ZulipTestCase):
         self.assertTrue(othello in admin_users)
         person = events[0]['event']['person']
         self.assertEqual(person['user_id'], othello.id)
-        self.assertEqual(person['is_admin'], True)
+        self.assertEqual(person['role'], UserProfile.ROLE_REALM_ADMINISTRATOR)
 
         # Taketh away
         req = dict(role=ujson.dumps(UserProfile.ROLE_MEMBER))
@@ -160,7 +160,7 @@ class PermissionTest(ZulipTestCase):
         self.assertFalse(othello in admin_users)
         person = events[0]['event']['person']
         self.assertEqual(person['user_id'], othello.id)
-        self.assertEqual(person['is_admin'], False)
+        self.assertEqual(person['role'], UserProfile.ROLE_MEMBER)
 
         # Cannot take away from last admin
         self.login('iago')
@@ -173,7 +173,7 @@ class PermissionTest(ZulipTestCase):
         self.assertFalse(desdemona in admin_users)
         person = events[0]['event']['person']
         self.assertEqual(person['user_id'], desdemona.id)
-        self.assertEqual(person['is_admin'], False)
+        self.assertEqual(person['role'], UserProfile.ROLE_MEMBER)
         with tornado_redirected_to_list([]):
             result = self.client_patch('/json/users/{}'.format(iago.id), req)
         self.assert_json_error(result, 'Cannot remove the only organization administrator')
@@ -366,7 +366,7 @@ class PermissionTest(ZulipTestCase):
         self.assertFalse(hamlet.can_access_all_realm_members())
         person = events[0]['event']['person']
         self.assertEqual(person['user_id'], hamlet.id)
-        self.assertTrue(person['is_guest'])
+        self.assertTrue(person['role'], UserProfile.ROLE_GUEST)
 
     def test_change_guest_to_regular_member(self) -> None:
         iago = self.example_user("iago")
@@ -384,7 +384,7 @@ class PermissionTest(ZulipTestCase):
         self.assertFalse(polonius.is_guest)
         person = events[0]['event']['person']
         self.assertEqual(person['user_id'], polonius.id)
-        self.assertFalse(person['is_guest'])
+        self.assertEqual(person['role'], UserProfile.ROLE_MEMBER)
 
     def test_change_admin_to_guest(self) -> None:
         iago = self.example_user("iago")
@@ -409,11 +409,7 @@ class PermissionTest(ZulipTestCase):
 
         person = events[0]['event']['person']
         self.assertEqual(person['user_id'], hamlet.id)
-        self.assertFalse(person['is_admin'])
+        self.assertEqual(person['role'], UserProfile.ROLE_GUEST)
-
-        person = events[1]['event']['person']
-        self.assertEqual(person['user_id'], hamlet.id)
-        self.assertTrue(person['is_guest'])
 
     def test_change_guest_to_admin(self) -> None:
         iago = self.example_user("iago")
@@ -437,7 +433,7 @@ class PermissionTest(ZulipTestCase):
 
         person = events[0]['event']['person']
         self.assertEqual(person['user_id'], polonius.id)
-        self.assertTrue(person['is_admin'])
+        self.assertEqual(person['role'], UserProfile.ROLE_REALM_ADMINISTRATOR)
 
     def test_admin_user_can_change_profile_data(self) -> None:
         realm = get_realm('zulip')