diff --git a/CHANGELOG.md b/CHANGELOG.md index 55151194..c59e1908 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix regression in public actions endpoint handling user field by @mderynck ([#3053](https://github.com/grafana/oncall/pull/3053)) +### Changed + +- Rework how users are fetched from DB when getting users from schedules ical representation ([#3067](https://github.com/grafana/oncall/pull/3067)) + ## v1.3.38 (2023-09-19) ### Fixed diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index cdd02ffc..285ad176 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -67,11 +67,10 @@ IcalEvents = typing.List[IcalEvent] def users_in_ical( usernames_from_ical: typing.List[str], organization: "Organization", -) -> "UserQuerySet": +) -> typing.List["User"]: """ This method returns a sequence of `User` objects, filtered by users whose username, or case-insensitive e-mail, - is present in `usernames_from_ical`. If `include_viewers` is set to `True`, users are further filtered down - based on their granted permissions. + is present in `usernames_from_ical`. Parameters ---------- @@ -80,24 +79,26 @@ def users_in_ical( organization : apps.user_management.models.organization.Organization The organization in question """ - from apps.user_management.models import User + required_permission = RBACPermission.Permissions.SCHEDULES_WRITE emails_from_ical = [username.lower() for username in usernames_from_ical] - # users_found_in_ical = organization.users users_found_in_ical = organization.users.filter( - **User.build_permissions_query(RBACPermission.Permissions.SCHEDULES_WRITE, organization) - ) - - users_found_in_ical = users_found_in_ical.filter( (Q(username__in=usernames_from_ical) | Q(email__lower__in=emails_from_ical)) ).distinct() - return users_found_in_ical + if organization.is_rbac_permissions_enabled: + # it is more efficient to check permissions on the subset of users filtered above + # than performing a regex query for the required permission + users_found_in_ical = [u for u in users_found_in_ical if {"action": required_permission.value} in u.permissions] + else: + users_found_in_ical = users_found_in_ical.filter(role__lte=required_permission.fallback_role.value) + + return list(users_found_in_ical) @timed_lru_cache(timeout=100) -def memoized_users_in_ical(usernames_from_ical: typing.List[str], organization: "Organization") -> UserQuerySet: +def memoized_users_in_ical(usernames_from_ical: typing.List[str], organization: "Organization") -> typing.List["User"]: # using in-memory cache instead of redis to avoid pickling python objects return users_in_ical(usernames_from_ical, organization) @@ -354,7 +355,7 @@ def list_users_to_notify_from_ical_for_period( schedule: "OnCallSchedule", start_datetime: datetime.datetime, end_datetime: datetime.datetime, -) -> UserQuerySet: +) -> typing.List["User"]: users_found_in_ical: typing.Sequence["User"] = [] events = schedule.final_events(start_datetime, end_datetime) usernames = [] diff --git a/engine/apps/user_management/migrations/0015_auto_20230926_2203.py b/engine/apps/user_management/migrations/0015_auto_20230926_2203.py new file mode 100644 index 00000000..15ce31e5 --- /dev/null +++ b/engine/apps/user_management/migrations/0015_auto_20230926_2203.py @@ -0,0 +1,21 @@ +# Generated by Django 3.2.20 on 2023-09-26 22:03 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('user_management', '0014_auto_20230728_0802'), + ] + + operations = [ + migrations.AddIndex( + model_name='user', + index=models.Index(fields=['is_active', 'organization', 'username'], name='user_manage_is_acti_385fc4_idx'), + ), + migrations.AddIndex( + model_name='user', + index=models.Index(fields=['is_active', 'organization', 'email'], name='user_manage_is_acti_7e930d_idx'), + ), + ] diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index 2043f0f9..8c05bb62 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -169,6 +169,10 @@ class User(models.Model): # Including is_active to unique_together and setting is_active to None allows to # have multiple deleted users with the same user_id, but user_id is unique among active users unique_together = ("user_id", "organization", "is_active") + indexes = [ + models.Index(fields=["is_active", "organization", "username"]), + models.Index(fields=["is_active", "organization", "email"]), + ] public_primary_key = models.CharField( max_length=20,