From e10361a832217b331a8b71525378a2eba4882bbe Mon Sep 17 00:00:00 2001 From: Rishi Gupta Date: Fri, 4 Oct 2019 17:35:07 -0700 Subject: [PATCH] models: Replace is_guest and is_realm_admin with UserProfile.role. This new data model will be more extensible for future work on features like a primary administrator. --- analytics/views.py | 2 +- corporate/lib/stripe.py | 4 +- corporate/tests/test_stripe.py | 11 +++-- zerver/data_import/hipchat.py | 14 ++++-- zerver/data_import/hipchat_user.py | 6 ++- zerver/data_import/import_util.py | 6 +-- zerver/data_import/mattermost.py | 11 +++-- zerver/data_import/slack.py | 6 ++- zerver/lib/actions.py | 20 ++++++-- zerver/lib/cache.py | 6 +-- zerver/lib/create_user.py | 7 +-- zerver/lib/email_notifications.py | 2 +- zerver/lib/events.py | 4 +- .../migrations/0248_userprofile_role_start.py | 49 +++++++++++++++++++ .../0249_userprofile_role_finish.py | 31 ++++++++++++ zerver/models.py | 34 ++++++++++--- zerver/tests/test_home.py | 6 +-- zerver/tests/test_mattermost_importer.py | 8 ++- zerver/tests/test_signup.py | 3 +- zerver/tests/test_slack_importer.py | 7 +-- zerver/tests/test_subs.py | 10 ++-- zerver/tests/test_users.py | 4 -- zerver/views/users.py | 16 +++--- zilencer/management/commands/populate_db.py | 4 +- 24 files changed, 191 insertions(+), 80 deletions(-) create mode 100644 zerver/migrations/0248_userprofile_role_start.py create mode 100644 zerver/migrations/0249_userprofile_role_finish.py diff --git a/analytics/views.py b/analytics/views.py index 3ec909f8c2..de8d5b7c7d 100644 --- a/analytics/views.py +++ b/analytics/views.py @@ -574,7 +574,7 @@ def realm_summary_table(realm_minutes: Dict[str, float]) -> str: # Fetch all the realm administrator users realm_admins = defaultdict(list) # type: Dict[str, List[str]] for up in UserProfile.objects.select_related("realm").filter( - is_realm_admin=True, + role=UserProfile.ROLE_REALM_ADMINISTRATOR, is_active=True ): realm_admins[up.realm.string_id].append(up.delivery_email) diff --git a/corporate/lib/stripe.py b/corporate/lib/stripe.py index da8febe43f..6463d2c7ee 100644 --- a/corporate/lib/stripe.py +++ b/corporate/lib/stripe.py @@ -40,9 +40,9 @@ DEFAULT_INVOICE_DAYS_UNTIL_DUE = 30 def get_seat_count(realm: Realm) -> int: non_guests = UserProfile.objects.filter( - realm=realm, is_active=True, is_bot=False, is_guest=False).count() + realm=realm, is_active=True, is_bot=False).exclude(role=UserProfile.ROLE_GUEST).count() guests = UserProfile.objects.filter( - realm=realm, is_active=True, is_bot=False, is_guest=True).count() + realm=realm, is_active=True, is_bot=False, role=UserProfile.ROLE_GUEST).count() return max(non_guests, math.ceil(guests / 5)) def sign_string(string: str) -> Tuple[str, str]: diff --git a/corporate/tests/test_stripe.py b/corporate/tests/test_stripe.py index f1635e6f38..93f7bada14 100644 --- a/corporate/tests/test_stripe.py +++ b/corporate/tests/test_stripe.py @@ -218,8 +218,8 @@ class StripeTestCase(ZulipTestCase): realm = get_realm('zulip') seat_count = get_seat_count(realm) assert(seat_count >= 6) - for user in UserProfile.objects.filter(realm=realm, is_active=True, is_bot=False, is_guest=False) \ - .exclude(email__in=[ + for user in UserProfile.objects.filter(realm=realm, is_active=True, is_bot=False) \ + .exclude(role=UserProfile.ROLE_GUEST).exclude(email__in=[ self.example_email('hamlet'), self.example_email('iago')])[:seat_count-6]: user.is_active = False @@ -740,17 +740,18 @@ class StripeTest(StripeTestCase): # Test guests # Adding a guest to a realm with a lot of members shouldn't change anything - UserProfile.objects.create(realm=realm, email='user3@zulip.com', pointer=-1, is_guest=True) + UserProfile.objects.create(realm=realm, email='user3@zulip.com', pointer=-1, role=UserProfile.ROLE_GUEST) self.assertEqual(get_seat_count(realm), initial_count) # Test 1 member and 5 guests realm = Realm.objects.create(string_id='second', name='second') UserProfile.objects.create(realm=realm, email='member@second.com', pointer=-1) for i in range(5): UserProfile.objects.create(realm=realm, email='guest{}@second.com'.format(i), - pointer=-1, is_guest=True) + pointer=-1, role=UserProfile.ROLE_GUEST) self.assertEqual(get_seat_count(realm), 1) # Test 1 member and 6 guests - UserProfile.objects.create(realm=realm, email='guest5@second.com', pointer=-1, is_guest=True) + UserProfile.objects.create(realm=realm, email='guest5@second.com', pointer=-1, + role=UserProfile.ROLE_GUEST) self.assertEqual(get_seat_count(realm), 2) def test_sign_string(self) -> None: diff --git a/zerver/data_import/hipchat.py b/zerver/data_import/hipchat.py index 51b8bdc9f2..fd7f9ac2c8 100755 --- a/zerver/data_import/hipchat.py +++ b/zerver/data_import/hipchat.py @@ -21,6 +21,7 @@ from zerver.lib.utils import ( from zerver.models import ( RealmEmoji, Recipient, + UserProfile, ) from zerver.data_import.import_util import ( @@ -101,17 +102,21 @@ def convert_user_data(user_handler: UserHandler, email = in_dict['email'] full_name = in_dict['name'] id = user_id_mapper.get(in_dict['id']) - is_realm_admin = in_dict['account_type'] == 'admin' - is_guest = in_dict['account_type'] == 'guest' is_mirror_dummy = False short_name = in_dict['mention_name'] timezone = in_dict['timezone'] + role = UserProfile.ROLE_MEMBER + if in_dict['account_type'] == 'admin': + role = UserProfile.ROLE_REALM_ADMINISTRATOR + if in_dict['account_type'] == 'guest': + role = UserProfile.ROLE_GUEST + date_joined = int(timezone_now().timestamp()) is_active = not in_dict['is_deleted'] if not email: - if is_guest: + if role == UserProfile.ROLE_GUEST: # Hipchat guest users don't have emails, so # we just fake them. email = 'guest-{id}@example.com'.format(id=id) @@ -140,8 +145,7 @@ def convert_user_data(user_handler: UserHandler, full_name=full_name, id=id, is_active=is_active, - is_realm_admin=is_realm_admin, - is_guest=is_guest, + role=role, is_mirror_dummy=is_mirror_dummy, realm_id=realm_id, short_name=short_name, diff --git a/zerver/data_import/hipchat_user.py b/zerver/data_import/hipchat_user.py index 8730166c28..46d000eed8 100644 --- a/zerver/data_import/hipchat_user.py +++ b/zerver/data_import/hipchat_user.py @@ -5,6 +5,9 @@ from django.utils.timezone import now as timezone_now from zerver.data_import.import_util import ( build_user_profile, ) +from zerver.models import ( + UserProfile, +) class UserHandler: ''' @@ -55,8 +58,7 @@ class UserHandler: full_name=full_name, id=user_id, is_active=False, - is_realm_admin=False, - is_guest=False, + role=UserProfile.ROLE_MEMBER, is_mirror_dummy=True, realm_id=realm_id, short_name=short_name, diff --git a/zerver/data_import/import_util.py b/zerver/data_import/import_util.py index a532eff322..ac8b8713c6 100644 --- a/zerver/data_import/import_util.py +++ b/zerver/data_import/import_util.py @@ -63,8 +63,7 @@ def build_user_profile(avatar_source: str, full_name: str, id: int, is_active: bool, - is_realm_admin: bool, - is_guest: bool, + role: int, is_mirror_dummy: bool, realm_id: int, short_name: str, @@ -79,8 +78,7 @@ def build_user_profile(avatar_source: str, id=id, is_mirror_dummy=is_mirror_dummy, is_active=is_active, - is_realm_admin=is_realm_admin, - is_guest=is_guest, + role=role, pointer=pointer, realm_id=realm_id, short_name=short_name, diff --git a/zerver/data_import/mattermost.py b/zerver/data_import/mattermost.py index f435902da2..b4948ab654 100644 --- a/zerver/data_import/mattermost.py +++ b/zerver/data_import/mattermost.py @@ -15,7 +15,7 @@ from django.conf import settings from django.utils.timezone import now as timezone_now from django.forms.models import model_to_dict -from zerver.models import Recipient, RealmEmoji, Reaction +from zerver.models import Recipient, RealmEmoji, Reaction, UserProfile from zerver.lib.utils import ( process_list_in_batches, ) @@ -64,12 +64,14 @@ def process_user(user_dict: Dict[str, Any], realm_id: int, team_name: str, id = user_id_mapper.get(user_dict['username']) delivery_email = user_dict['email'] email = user_dict['email'] - is_realm_admin = is_team_admin(user_dict) - is_guest = False short_name = user_dict['username'] date_joined = int(timezone_now().timestamp()) timezone = 'UTC' + role = UserProfile.ROLE_MEMBER + if is_team_admin(user_dict): + role = UserProfile.ROLE_REALM_ADMINISTRATOR + if user_dict["is_mirror_dummy"]: is_active = False is_mirror_dummy = True @@ -85,8 +87,7 @@ def process_user(user_dict: Dict[str, Any], realm_id: int, team_name: str, full_name=full_name, id=id, is_active=is_active, - is_realm_admin=is_realm_admin, - is_guest=is_guest, + role=role, is_mirror_dummy=is_mirror_dummy, realm_id=realm_id, short_name=short_name, diff --git a/zerver/data_import/slack.py b/zerver/data_import/slack.py index c53a4ad53f..5531e3ec17 100755 --- a/zerver/data_import/slack.py +++ b/zerver/data_import/slack.py @@ -161,7 +161,9 @@ def users_to_zerver_userprofile(slack_data_dir: str, users: List[ZerverFieldsT], avatar_url = build_avatar_url(slack_user_id, user['team_id'], user['profile']['avatar_hash']) build_avatar(user_id, realm_id, email, avatar_url, timestamp, avatar_list) - realm_admin = get_admin(user) + role = UserProfile.ROLE_MEMBER + if get_admin(user): + role = UserProfile.ROLE_REALM_ADMINISTRATOR timezone = get_user_timezone(user) if slack_user_id in slack_user_id_to_custom_profile_fields: @@ -186,7 +188,7 @@ def users_to_zerver_userprofile(slack_data_dir: str, users: List[ZerverFieldsT], avatar_source='U', is_bot=user.get('is_bot', False), pointer=-1, - is_realm_admin=realm_admin, + role=role, bot_type=1 if user.get('is_bot', False) else None, date_joined=timestamp, timezone=timezone, diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 42a3bec0c4..cf435f28d3 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -220,7 +220,7 @@ def private_stream_user_ids(stream_id: int) -> Set[int]: def public_stream_user_ids(stream: Stream) -> Set[int]: guest_subscriptions = get_active_subscriptions_for_stream_id( - stream.id).filter(user_profile__is_guest=True) + stream.id).filter(user_profile__role=UserProfile.ROLE_GUEST) guest_subscriptions = {sub['user_profile_id'] for sub in guest_subscriptions.values('user_profile_id')} return set(active_non_guest_user_ids(stream.realm_id)) | guest_subscriptions @@ -3466,9 +3466,14 @@ def do_change_default_all_public_streams(user_profile: UserProfile, value: bool, def do_change_is_admin(user_profile: UserProfile, value: bool, permission: str='administer') -> None: + # TODO: This function and do_change_is_guest should be merged into + # a single do_change_user_role function in a future refactor. if permission == "administer": - user_profile.is_realm_admin = value - user_profile.save(update_fields=["is_realm_admin"]) + if value: + user_profile.role = UserProfile.ROLE_REALM_ADMINISTRATOR + else: + user_profile.role = UserProfile.ROLE_MEMBER + user_profile.save(update_fields=["role"]) elif permission == "api_super_user": user_profile.is_api_super_user = value user_profile.save(update_fields=["is_api_super_user"]) @@ -3483,8 +3488,13 @@ def do_change_is_admin(user_profile: UserProfile, value: bool, send_event(user_profile.realm, event, active_user_ids(user_profile.realm_id)) def do_change_is_guest(user_profile: UserProfile, value: bool) -> None: - user_profile.is_guest = value - user_profile.save(update_fields=["is_guest"]) + # TODO: This function and do_change_is_admin should be merged into + # a single do_change_user_role function in a future refactor. + if value: + user_profile.role = UserProfile.ROLE_GUEST + else: + user_profile.role = UserProfile.ROLE_MEMBER + user_profile.save(update_fields=["role"]) event = dict(type="realm_user", op="update", person=dict(email=user_profile.email, user_id=user_profile.id, diff --git a/zerver/lib/cache.py b/zerver/lib/cache.py index de07ab25db..5ab83be3da 100644 --- a/zerver/lib/cache.py +++ b/zerver/lib/cache.py @@ -351,8 +351,8 @@ def user_profile_by_api_key_cache_key(api_key: str) -> str: realm_user_dict_fields = [ 'id', 'full_name', 'short_name', 'email', 'avatar_source', 'avatar_version', 'is_active', - 'is_realm_admin', 'is_bot', 'realm_id', 'timezone', - 'date_joined', 'is_guest', 'bot_owner_id' + 'role', 'is_bot', 'realm_id', 'timezone', + 'date_joined', 'bot_owner_id' ] # type: List[str] def realm_user_dicts_cache_key(realm_id: int) -> str: @@ -434,7 +434,7 @@ def flush_user_profile(sender: Any, **kwargs: Any) -> None: cache_delete(active_user_ids_cache_key(user_profile.realm_id)) cache_delete(active_non_guest_user_ids_cache_key(user_profile.realm_id)) - if changed(kwargs, ['is_guest']): + if changed(kwargs, ['role']): cache_delete(active_non_guest_user_ids_cache_key(user_profile.realm_id)) if changed(kwargs, ['email', 'full_name', 'short_name', 'id', 'is_mirror_dummy']): diff --git a/zerver/lib/create_user.py b/zerver/lib/create_user.py index 62424408b5..c84ca83f3d 100644 --- a/zerver/lib/create_user.py +++ b/zerver/lib/create_user.py @@ -65,7 +65,6 @@ def create_user_profile(realm: Realm, email: str, password: Optional[str], default_language=realm.default_language, twenty_four_hour_time=realm.default_twenty_four_hour_time, delivery_email=email) - if bot_type or not active: password = None if user_profile.email_address_is_realm_public(): @@ -91,8 +90,10 @@ def create_user(email: str, password: Optional[str], realm: Realm, user_profile = create_user_profile(realm, email, password, active, bot_type, full_name, short_name, bot_owner, is_mirror_dummy, tos_version, timezone) - user_profile.is_realm_admin = is_realm_admin - user_profile.is_guest = is_guest + if is_realm_admin: + user_profile.role = UserProfile.ROLE_REALM_ADMINISTRATOR + if is_guest: + user_profile.role = UserProfile.ROLE_GUEST user_profile.avatar_source = avatar_source user_profile.timezone = timezone user_profile.default_sending_stream = default_sending_stream diff --git a/zerver/lib/email_notifications.py b/zerver/lib/email_notifications.py index 36c700fff2..0eb55d1346 100644 --- a/zerver/lib/email_notifications.py +++ b/zerver/lib/email_notifications.py @@ -549,7 +549,7 @@ def enqueue_welcome_emails(user: UserProfile, realm_creation: bool=False) -> Non 'realm_name': user.realm.name, 'realm_creation': realm_creation, 'email': user.email, - 'is_realm_admin': user.is_realm_admin, + 'is_realm_admin': user.role == UserProfile.ROLE_REALM_ADMINISTRATOR, }) if user.is_realm_admin: context['getting_started_link'] = (user.realm.uri + diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 5f90d48eb1..2b54eb11d6 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -86,8 +86,8 @@ def get_raw_user_data(realm: Realm, client_gravatar: bool) -> Dict[int, Dict[str client_gravatar=client_gravatar, ) - is_admin = row['is_realm_admin'] - is_guest = row['is_guest'] + is_admin = row['role'] == UserProfile.ROLE_REALM_ADMINISTRATOR + is_guest = row['role'] == UserProfile.ROLE_GUEST is_bot = row['is_bot'] # This format should align with get_cross_realm_dicts() and notify_created_user result = dict( diff --git a/zerver/migrations/0248_userprofile_role_start.py b/zerver/migrations/0248_userprofile_role_start.py new file mode 100644 index 0000000000..23a236a1a1 --- /dev/null +++ b/zerver/migrations/0248_userprofile_role_start.py @@ -0,0 +1,49 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.24 on 2019-10-03 22:27 +from __future__ import unicode_literals + +from django.db import migrations, models +from django.db.backends.postgresql_psycopg2.schema import DatabaseSchemaEditor +from django.db.migrations.state import StateApps + +# The values at the time of this migration +ROLE_REALM_ADMINISTRATOR = 200 +ROLE_MEMBER = 400 +ROLE_GUEST = 600 + +def update_role(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None: + UserProfile = apps.get_model('zerver', 'UserProfile') + for user in UserProfile.objects.all(): + if user.is_realm_admin: + user.role = ROLE_REALM_ADMINISTRATOR + elif user.is_guest: + user.role = ROLE_GUEST + else: + user.role = ROLE_MEMBER + user.save(update_fields=['role']) + +def reverse_code(apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None: + UserProfile = apps.get_model('zerver', 'UserProfile') + for user in UserProfile.objects.all(): + if user.role == ROLE_REALM_ADMINISTRATOR: + user.is_realm_admin = True + user.save(update_fields=['is_realm_admin']) + elif user.role == ROLE_GUEST: + user.is_guest = True + user.save(update_fields=['is_guest']) + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0247_realmauditlog_event_type_to_int'), + ] + + operations = [ + migrations.AddField( + model_name='userprofile', + name='role', + field=models.PositiveSmallIntegerField(null=True), + ), + + migrations.RunPython(update_role, reverse_code=reverse_code), + ] diff --git a/zerver/migrations/0249_userprofile_role_finish.py b/zerver/migrations/0249_userprofile_role_finish.py new file mode 100644 index 0000000000..aab95ed7a1 --- /dev/null +++ b/zerver/migrations/0249_userprofile_role_finish.py @@ -0,0 +1,31 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.24 on 2019-10-03 23:40 +from __future__ import unicode_literals + +from django.db import migrations, models + +# The values at the time of this migration +ROLE_MEMBER = 400 + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0248_userprofile_role_start'), + ] + + operations = [ + migrations.RemoveField( + model_name='userprofile', + name='is_guest', + ), + migrations.RemoveField( + model_name='userprofile', + name='is_realm_admin', + ), + + migrations.AlterField( + model_name='userprofile', + name='role', + field=models.PositiveSmallIntegerField(db_index=True, default=ROLE_MEMBER), + ), + ] diff --git a/zerver/models.py b/zerver/models.py index 3944ccbb8d..cd61fd18d8 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -397,7 +397,7 @@ class Realm(models.Model): notifications to all administrator users. """ # TODO: Change return type to QuerySet[UserProfile] - return UserProfile.objects.filter(realm=self, is_realm_admin=True, + return UserProfile.objects.filter(realm=self, role=UserProfile.ROLE_REALM_ADMINISTRATOR, is_active=True) def get_human_admin_users(self) -> Sequence['UserProfile']: @@ -406,7 +406,8 @@ class Realm(models.Model): realm's administrators (bots don't have real email addresses). """ # TODO: Change return type to QuerySet[UserProfile] - return UserProfile.objects.filter(realm=self, is_bot=False, is_realm_admin=True, + return UserProfile.objects.filter(realm=self, is_bot=False, + role=UserProfile.ROLE_REALM_ADMINISTRATOR, is_active=True) def get_active_users(self) -> Sequence['UserProfile']: @@ -793,16 +794,24 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): # See also `long_term_idle`. is_active = models.BooleanField(default=True, db_index=True) # type: bool - is_realm_admin = models.BooleanField(default=False, db_index=True) # type: bool is_billing_admin = models.BooleanField(default=False, db_index=True) # type: bool - # Guest users are limited users without default access to public streams (etc.) - is_guest = models.BooleanField(default=False, db_index=True) # type: bool - is_bot = models.BooleanField(default=False, db_index=True) # type: bool bot_type = models.PositiveSmallIntegerField(null=True, db_index=True) # type: Optional[int] bot_owner = models.ForeignKey('self', null=True, on_delete=models.SET_NULL) # type: Optional[UserProfile] + # Each role has a superset of the permissions of the next higher + # numbered role. When adding new roles, leave enough space for + # future roles to be inserted between currently adjacent + # roles. These constants appear in RealmAuditLog.extra_data, so + # changes to them will require a migration of RealmAuditLog. + # ROLE_REALM_OWNER = 100 + ROLE_REALM_ADMINISTRATOR = 200 + # ROLE_MODERATOR = 300 + ROLE_MEMBER = 400 + ROLE_GUEST = 600 + role = models.PositiveSmallIntegerField(default=ROLE_MEMBER, db_index=True) # type: int + # Whether the user has been "soft-deactivated" due to weeks of inactivity. # For these users we avoid doing UserMessage table work, as an optimization # for large Zulip organizations with lots of single-visit users. @@ -1018,6 +1027,14 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): def __str__(self) -> str: return "" % (self.email, self.realm) + @property + def is_realm_admin(self) -> bool: + return self.role == UserProfile.ROLE_REALM_ADMINISTRATOR + + @property + def is_guest(self) -> bool: + return self.role == UserProfile.ROLE_GUEST + @property def is_incoming_webhook(self) -> bool: return self.bot_type == UserProfile.INCOMING_WEBHOOK_BOT @@ -2136,8 +2153,9 @@ def active_user_ids(realm_id: int) -> List[int]: def active_non_guest_user_ids(realm_id: int) -> List[int]: query = UserProfile.objects.filter( realm_id=realm_id, - is_active=True, - is_guest=False, + is_active=True + ).exclude( + role=UserProfile.ROLE_GUEST ).values_list('id', flat=True) return list(query) diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 0e7dfd27ae..2d0fdddd89 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -634,7 +634,7 @@ class HomeTest(ZulipTestCase): html = result.content.decode('utf-8') self.assertNotIn('Invite more users', html) - user_profile.is_realm_admin = True + user_profile.role = UserProfile.ROLE_REALM_ADMINISTRATOR user_profile.save() result = self._get_home_page() html = result.content.decode('utf-8') @@ -672,9 +672,9 @@ class HomeTest(ZulipTestCase): self.assertIn('Billing', result_html) # billing admin, with CustomerPlan -> show billing link - user.is_realm_admin = False + user.role = UserProfile.ROLE_REALM_ADMINISTRATOR user.is_billing_admin = True - user.save(update_fields=['is_realm_admin', 'is_billing_admin']) + user.save(update_fields=['role', 'is_billing_admin']) result_html = self._get_home_page().content.decode('utf-8') self.assertIn('Billing', result_html) diff --git a/zerver/tests/test_mattermost_importer.py b/zerver/tests/test_mattermost_importer.py index 1210c030f4..287e735ca2 100644 --- a/zerver/tests/test_mattermost_importer.py +++ b/zerver/tests/test_mattermost_importer.py @@ -74,8 +74,7 @@ class MatterMostImporter(ZulipTestCase): self.assertEqual(user["full_name"], "Harry Potter") self.assertEqual(user["id"], 1) self.assertEqual(user["is_active"], True) - self.assertEqual(user["is_realm_admin"], True) - self.assertEqual(user["is_guest"], False) + self.assertEqual(user["role"], UserProfile.ROLE_REALM_ADMINISTRATOR) self.assertEqual(user["is_mirror_dummy"], False) self.assertEqual(user["realm"], 3) self.assertEqual(user["short_name"], "harry") @@ -84,7 +83,7 @@ class MatterMostImporter(ZulipTestCase): # A user with a `null` team value shouldn't be an admin. harry_dict["teams"] = None user = process_user(harry_dict, realm_id, team_name, user_id_mapper) - self.assertEqual(user["is_realm_admin"], False) + self.assertEqual(user["role"], UserProfile.ROLE_MEMBER) team_name = "slytherin" snape_dict = username_to_user["snape"] @@ -96,8 +95,7 @@ class MatterMostImporter(ZulipTestCase): self.assertEqual(user["full_name"], "Severus Snape") self.assertEqual(user["id"], 2) self.assertEqual(user["is_active"], False) - self.assertEqual(user["is_realm_admin"], False) - self.assertEqual(user["is_guest"], False) + self.assertEqual(user["role"], UserProfile.ROLE_MEMBER) self.assertEqual(user["is_mirror_dummy"], True) self.assertEqual(user["realm"], 3) self.assertEqual(user["short_name"], "snape") diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index bf2524dbcd..269d0f3e85 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -3234,7 +3234,8 @@ class DeactivateUserTest(ZulipTestCase): def test_do_not_deactivate_final_user(self) -> None: realm = get_realm('zulip') - UserProfile.objects.filter(realm=realm, is_realm_admin=False).update(is_active=False) + UserProfile.objects.filter(realm=realm).exclude( + role=UserProfile.ROLE_REALM_ADMINISTRATOR).update(is_active=False) email = self.example_email("iago") self.login(email) result = self.client_delete('/json/users/me') diff --git a/zerver/tests/test_slack_importer.py b/zerver/tests/test_slack_importer.py index fd106c09e0..4e8c6a1ac0 100644 --- a/zerver/tests/test_slack_importer.py +++ b/zerver/tests/test_slack_importer.py @@ -45,6 +45,7 @@ from zerver.models import ( get_realm, RealmAuditLog, Recipient, + UserProfile, ) import ujson @@ -272,13 +273,13 @@ class SlackImporter(ZulipTestCase): self.assertEqual(zerver_userprofile[0]['is_bot'], False) self.assertEqual(zerver_userprofile[0]['is_active'], True) self.assertEqual(zerver_userprofile[0]['is_mirror_dummy'], False) - self.assertEqual(zerver_userprofile[0]['is_realm_admin'], False) + self.assertEqual(zerver_userprofile[0]['role'], UserProfile.ROLE_MEMBER) self.assertEqual(zerver_userprofile[0]['enable_desktop_notifications'], True) self.assertEqual(zerver_userprofile[0]['email'], 'jon@gmail.com') self.assertEqual(zerver_userprofile[0]['full_name'], 'John Doe') self.assertEqual(zerver_userprofile[1]['id'], test_slack_user_id_to_zulip_user_id['U0CBK5KAT']) - self.assertEqual(zerver_userprofile[1]['is_realm_admin'], True) + self.assertEqual(zerver_userprofile[1]['role'], UserProfile.ROLE_REALM_ADMINISTRATOR) self.assertEqual(zerver_userprofile[1]['is_staff'], False) self.assertEqual(zerver_userprofile[1]['is_active'], True) self.assertEqual(zerver_userprofile[0]['is_mirror_dummy'], False) @@ -292,7 +293,7 @@ class SlackImporter(ZulipTestCase): self.assertEqual(zerver_userprofile[2]['avatar_source'], 'U') self.assertEqual(zerver_userprofile[3]['id'], test_slack_user_id_to_zulip_user_id['UHSG7OPQN']) - self.assertEqual(zerver_userprofile[3]['is_realm_admin'], False) + self.assertEqual(zerver_userprofile[3]['role'], UserProfile.ROLE_MEMBER) self.assertEqual(zerver_userprofile[3]['is_staff'], False) self.assertEqual(zerver_userprofile[3]['is_active'], False) self.assertEqual(zerver_userprofile[3]['email'], 'matt.perry@foreignteam.slack.com') diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index d1e0064795..e06d6c6fe5 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -2268,18 +2268,18 @@ class SubscriptionAPITest(ZulipTestCase): def test_can_create_streams(self) -> None: othello = self.example_user('othello') - othello.is_realm_admin = True + othello.role = UserProfile.ROLE_REALM_ADMINISTRATOR self.assertTrue(othello.can_create_streams()) - othello.is_realm_admin = False + othello.role = UserProfile.ROLE_MEMBER othello.realm.create_stream_policy = Realm.CREATE_STREAM_POLICY_ADMINS self.assertFalse(othello.can_create_streams()) othello.realm.create_stream_policy = Realm.CREATE_STREAM_POLICY_MEMBERS - othello.is_guest = True + othello.role = UserProfile.ROLE_GUEST self.assertFalse(othello.can_create_streams()) - othello.is_guest = False + othello.role = UserProfile.ROLE_MEMBER othello.realm.waiting_period_threshold = 1000 othello.realm.create_stream_policy = Realm.CREATE_STREAM_POLICY_WAITING_PERIOD othello.date_joined = timezone_now() - timedelta(days=(othello.realm.waiting_period_threshold - 1)) @@ -3596,7 +3596,7 @@ class GetSubscribersTest(ZulipTestCase): # Send private stream subscribers to all realm admins. def test_admin_case() -> None: - self.user_profile.is_realm_admin = True + self.user_profile.role = UserProfile.ROLE_REALM_ADMINISTRATOR # Test realm admins can get never subscribed private stream's subscribers. never_subscribed = get_never_subscribed() diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 74b6769817..470fbab3a9 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -403,10 +403,6 @@ class PermissionTest(ZulipTestCase): self.assertEqual(person['email'], polonius.email) self.assertTrue(person['is_admin']) - person = events[1]['event']['person'] - self.assertEqual(person['email'], polonius.email) - self.assertFalse(person['is_guest']) - def test_admin_user_can_change_profile_data(self) -> None: realm = get_realm('zulip') self.login(self.example_email("iago")) diff --git a/zerver/views/users.py b/zerver/views/users.py index cea09b8023..9dba87e468 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -87,11 +87,10 @@ def update_user_backend(request: HttpRequest, user_profile: UserProfile, user_id validator=check_list(check_dict([('id', check_int)])))) -> HttpResponse: target = access_user_by_id(user_profile, user_id, allow_deactivated=True, allow_bots=True) - # This condition is a bit complicated, because the user could - # already be a guest/admin, or the request could be to make the - # user a guest/admin. In any case, the point is that we outright - # reject requests that would result in a user who is both an admin - # and a guest. + # Historically, UserProfile had two fields, is_guest and is_realm_admin. + # This condition protected against situations where update_user_backend + # could cause both is_guest and is_realm_admin to be set. + # Once we update the frontend to just send a 'role' value, we can remove this check. if (((is_guest is None and target.is_guest) or is_guest) and ((is_admin is None and target.is_realm_admin) or is_admin)): return json_error(_("Guests cannot be organization administrators")) @@ -415,10 +414,9 @@ def get_members_backend(request: HttpRequest, user_profile: UserProfile, 'realm_id', 'full_name', 'is_bot', - 'is_realm_admin', 'is_active', - 'is_guest', 'bot_type', + 'role', 'avatar_source', 'avatar_version', 'bot_owner__email', @@ -447,9 +445,9 @@ def get_members_backend(request: HttpRequest, user_profile: UserProfile, full_name=row['full_name'], is_bot=row['is_bot'], is_active=row['is_active'], - is_admin=row['is_realm_admin'], bot_type=row['bot_type'], - is_guest=row['is_guest'], + is_admin=row['role'] == UserProfile.ROLE_REALM_ADMINISTRATOR, + is_guest=row['role'] == UserProfile.ROLE_GUEST, timezone=row['timezone'], ) diff --git a/zilencer/management/commands/populate_db.py b/zilencer/management/commands/populate_db.py index 3c3c0c876c..eca2229be2 100644 --- a/zilencer/management/commands/populate_db.py +++ b/zilencer/management/commands/populate_db.py @@ -219,8 +219,8 @@ class Command(BaseCommand): iago.save(update_fields=['is_staff']) guest_user = get_user("polonius@zulip.com", zulip_realm) - guest_user.is_guest = True - guest_user.save(update_fields=['is_guest']) + guest_user.role = UserProfile.ROLE_GUEST + guest_user.save(update_fields=['role']) # These bots are directly referenced from code and thus # are needed for the test suite.