From 2ab6767b7312adda10918f41368e770bbc632736 Mon Sep 17 00:00:00 2001 From: sahil839 Date: Sun, 31 May 2020 01:13:19 +0530 Subject: [PATCH] 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 | 13 +++++++++---- static/js/settings_users.js | 2 +- static/js/user_events.js | 16 ++++++---------- templates/zerver/api/changelog.md | 3 +++ zerver/lib/actions.py | 18 +++--------------- zerver/lib/events.py | 15 +++++++++++---- zerver/lib/users.py | 3 +++ zerver/tests/test_events.py | 2 +- zerver/tests/test_users.py | 18 +++++++----------- 9 files changed, 44 insertions(+), 46 deletions(-) diff --git a/frontend_tests/node_tests/user_events.js b/frontend_tests/node_tests/user_events.js index e3d9bbf722..d9c58bae5c 100644 --- a/frontend_tests/node_tests/user_events.js +++ b/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'}); diff --git a/static/js/settings_users.js b/static/js/settings_users.js index fd458b02d8..b2ccf6d1d3 100644 --- a/static/js/settings_users.js +++ b/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)); } }; diff --git a/static/js/user_events.js b/static/js/user_events.js index 8c84aef111..ef7db197dc 100644 --- a/static/js/user_events.js +++ b/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')) { - person_obj.is_admin = person.is_admin; + if (Object.prototype.hasOwnProperty.call(person, 'role')) { + 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)) { - page_params.is_admin = person.is_admin; + if (people.is_my_user_id(person.user_id) && page_params.is_admin !== person_obj.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; diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index a2a002e790..bded4d95a8 100644 --- a/templates/zerver/api/changelog.md +++ b/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 diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 0f032e65ad..08e530e3eb 100644 --- a/zerver/lib/actions.py +++ b/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", - person=dict(user_id=user_profile.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)) + event = dict(type="realm_user", op="update", + person=dict(user_id=user_profile.id, role=user_profile.role)) + 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 diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 0d31c51ae6..df3dec5fab 100644 --- a/zerver/lib/events.py +++ b/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'] diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 894c6fce96..609600ad72 100644 --- a/zerver/lib/users.py +++ b/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: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 5ffaffa4ab..0f5ec02876 100644 --- a/zerver/tests/test_events.py +++ b/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), ])), ]) diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index f687282620..c2c85c0e4c 100644 --- a/zerver/tests/test_users.py +++ b/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']) - - person = events[1]['event']['person'] - self.assertEqual(person['user_id'], hamlet.id) - self.assertTrue(person['is_guest']) + self.assertEqual(person['role'], UserProfile.ROLE_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')