From 4d843b55cb48b806283efb6929dd84e9425b7524 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 19 Jan 2023 17:24:00 +0100 Subject: [PATCH 01/10] reduce # of SQL queries for GET /schedules endpoints by >90%, plus: - update types in ical_utils.py - document user_is_authorized function --- engine/apps/api/permissions/__init__.py | 11 ++++ .../serializers/schedules_polymorphic.py | 1 + engine/apps/public_api/views/schedules.py | 2 + engine/apps/schedules/ical_utils.py | 57 +++++++++---------- 4 files changed, 40 insertions(+), 31 deletions(-) diff --git a/engine/apps/api/permissions/__init__.py b/engine/apps/api/permissions/__init__.py index 00311d7572..bb12182e02 100644 --- a/engine/apps/api/permissions/__init__.py +++ b/engine/apps/api/permissions/__init__.py @@ -72,6 +72,17 @@ def get_most_authorized_role( def user_is_authorized(user, required_permissions: typing.List[LegacyAccessControlCompatiblePermission]) -> bool: + """ + This function checks whether `user` has all permissions in `required_permissions`. RBAC permissions are used + if RBAC is enabled for the organization, otherwise the fallback basic role is checked. + + Parameters + ---------- + user : apps.user_management.models.user.User + The user to check permissions for + required_permissions : typing.List[LegacyAccessControlCompatiblePermission] + A list of permissions that a user must have to be considered authorized + """ if user.organization.is_rbac_permissions_enabled: user_permissions = [u["action"] for u in user.permissions] required_permissions = [p.value for p in required_permissions] diff --git a/engine/apps/public_api/serializers/schedules_polymorphic.py b/engine/apps/public_api/serializers/schedules_polymorphic.py index 4445fdc35f..cdad2725d9 100644 --- a/engine/apps/public_api/serializers/schedules_polymorphic.py +++ b/engine/apps/public_api/serializers/schedules_polymorphic.py @@ -10,6 +10,7 @@ class PolymorphicScheduleSerializer(EagerLoadingMixin, PolymorphicSerializer): SELECT_RELATED = ["organization"] + PREFETCH_RELATED = ["custom_shifts__users", "custom_on_call_shifts"] resource_type_field_name = "type" diff --git a/engine/apps/public_api/views/schedules.py b/engine/apps/public_api/views/schedules.py index 8029de1226..fd6c068e73 100644 --- a/engine/apps/public_api/views/schedules.py +++ b/engine/apps/public_api/views/schedules.py @@ -43,6 +43,8 @@ def get_queryset(self): if name is not None: queryset = queryset.filter(name=name) + queryset = self.serializer_class.setup_eager_loading(queryset) + return queryset.order_by("id") def get_object(self): diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index 962766d512..c317d38533 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -3,6 +3,7 @@ import datetime import logging import re +import typing from collections import namedtuple from typing import TYPE_CHECKING @@ -35,19 +36,34 @@ """ if TYPE_CHECKING: from apps.schedules.models import OnCallSchedule - from apps.user_management.models import User + from apps.user_management.models import Organization, User + from apps.user_management.models.user import UserQuerySet +logger = logging.getLogger(__name__) +logger.setLevel(logging.DEBUG) -def users_in_ical(usernames_from_ical, organization, include_viewers=False): + +def users_in_ical( + usernames_from_ical: typing.List[str], organization: Organization, include_viewers=False +) -> UserQuerySet: """ - Parse ical file and return list of users found - NOTE: only grafana username will be used, consider adding grafana email and id + This method returns a `UserQuerySet`, 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. + + Parameters + ---------- + usernames_from_ical : typing.List[str] + A list of usernames present in the ical feed + organization : apps.user_management.models.organization.Organization + The organization in question + include_viewers : bool + Whether or not the list should be further filtered to exclude users based on granted permissions """ from apps.user_management.models import User users_found_in_ical = organization.users if not include_viewers: - # TODO: this is a breaking change.... users_found_in_ical = users_found_in_ical.filter( **User.build_permissions_query(RBACPermission.Permissions.SCHEDULES_WRITE, organization) ) @@ -57,38 +73,15 @@ def users_in_ical(usernames_from_ical, organization, include_viewers=False): (Q(username__in=usernames_from_ical) | Q(email__lower__in=user_emails)) ).distinct() - # Here is the example how we extracted users previously, using slack fields too - # user_roles_found_in_ical = team.org_user_role.filter(role__in=[ROLE_ADMIN, ROLE_USER]).filter( - # Q( - # Q(amixr_user__slack_user_identities__slack_team_identity__amixr_team=team) & - # Q( - # Q(amixr_user__slack_user_identities__profile_display_name__in=usernames_from_ical) | - # Q(amixr_user__slack_user_identities__cached_name__in=usernames_from_ical) | - # Q(amixr_user__slack_user_identities__slack_id__in=[username.split(" ")[0] for username in - # usernames_from_ical]) | - # Q(amixr_user__slack_user_identities__cached_slack_login__in=usernames_from_ical) | - # Q(amixr_user__slack_user_identities__profile_real_name__in=usernames_from_ical) - # ) - # ) - # | - # Q(username__in=usernames_from_ical) - # ).annotate(is_deleted_sui=Subquery(slack_user_identity_subquery.values("deleted")[:1])).filter( - # ~Q(is_deleted_sui=True) | Q(is_deleted_sui__isnull=True)).distinct() - # return user_roles_found_in_ical - return users_found_in_ical @timed_lru_cache(timeout=100) -def memoized_users_in_ical(usernames_from_ical, organization): +def memoized_users_in_ical(usernames_from_ical: typing.List[str], organization: Organization) -> UserQuerySet: # using in-memory cache instead of redis to avoid pickling python objects return users_in_ical(usernames_from_ical, organization) -logger = logging.getLogger(__name__) -logger.setLevel(logging.DEBUG) - - # used for display schedule events on web def list_of_oncall_shifts_from_ical( schedule, @@ -288,7 +281,7 @@ def list_of_empty_shifts_in_schedule(schedule, start_date, end_date): return sorted(empty_shifts, key=lambda dt: dt.start) -def list_users_to_notify_from_ical(schedule, events_datetime=None, include_viewers=False): +def list_users_to_notify_from_ical(schedule, events_datetime=None, include_viewers=False) -> UserQuerySet: """ Retrieve on-call users for the current time """ @@ -298,7 +291,9 @@ def list_users_to_notify_from_ical(schedule, events_datetime=None, include_viewe ) -def list_users_to_notify_from_ical_for_period(schedule, start_datetime, end_datetime, include_viewers=False): +def list_users_to_notify_from_ical_for_period( + schedule, start_datetime, end_datetime, include_viewers=False +) -> UserQuerySet: # get list of iCalendars from current iCal files. If there is more than one calendar, primary calendar will always # be the first calendars = schedule.get_icalendars() From 222c35cba2c078eb4c874607d1e48344aa35751d Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 23 Jan 2023 14:06:47 +0000 Subject: [PATCH 02/10] Draft removing SQL queries when getting on-call users for multiple schedules --- engine/apps/api/serializers/schedule_base.py | 10 +-- engine/apps/api/views/schedule.py | 10 +++ engine/apps/schedules/ical_utils.py | 80 +++++++++++++++++-- .../apps/schedules/models/on_call_schedule.py | 16 +--- engine/apps/slack/models/slack_usergroup.py | 2 +- 5 files changed, 91 insertions(+), 27 deletions(-) diff --git a/engine/apps/api/serializers/schedule_base.py b/engine/apps/api/serializers/schedule_base.py index c2eb6ea608..e4acf4c41c 100644 --- a/engine/apps/api/serializers/schedule_base.py +++ b/engine/apps/api/serializers/schedule_base.py @@ -1,8 +1,6 @@ -from django.utils import timezone from rest_framework import serializers from apps.api.serializers.user_group import UserGroupSerializer -from apps.schedules.ical_utils import list_users_to_notify_from_ical from apps.schedules.models import OnCallSchedule from apps.schedules.tasks import schedule_notify_about_empty_shifts_in_schedule, schedule_notify_about_gaps_in_schedule from common.api_helpers.custom_fields import TeamPrimaryKeyRelatedField @@ -67,11 +65,9 @@ def get_warnings(self, obj): return warnings def get_on_call_now(self, obj): - users_on_call = list_users_to_notify_from_ical(obj, timezone.datetime.now(timezone.utc)) - if users_on_call is not None: - return [user.short() for user in users_on_call] - else: - return [] + # Serializer context is set here: apps.api.views.schedule.ScheduleView.get_serializer_context + users = self.context["oncall_users"][obj.pk] + return [user.short() for user in users] def get_number_of_escalation_chains(self, obj): # num_escalation_chains param added in queryset via annotate. Check ScheduleView.get_queryset diff --git a/engine/apps/api/views/schedule.py b/engine/apps/api/views/schedule.py index fa61c5ddc4..7d475f7fc6 100644 --- a/engine/apps/api/views/schedule.py +++ b/engine/apps/api/views/schedule.py @@ -104,9 +104,19 @@ def can_update_user_groups(self): return user_group.can_be_updated + @cached_property + def oncall_users(self): + """ + The result of this method is cached and is reused for the whole lifetime of a request, + since self.get_serializer_context() is called multiple times for every instance in the queryset. + """ + queryset = self.get_queryset() + return queryset.get_oncall_users() + def get_serializer_context(self): context = super().get_serializer_context() context.update({"can_update_user_groups": self.can_update_user_groups}) + context.update({"oncall_users": self.oncall_users}) return context def _annotate_queryset(self, queryset): diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index c317d38533..a064497c23 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -44,7 +44,10 @@ def users_in_ical( - usernames_from_ical: typing.List[str], organization: Organization, include_viewers=False + usernames_from_ical: typing.List[str], + organization: Organization, + include_viewers=False, + users_to_filter=None, ) -> UserQuerySet: """ This method returns a `UserQuerySet`, filtered by users whose username, or case-insensitive e-mail, @@ -62,6 +65,14 @@ def users_in_ical( """ from apps.user_management.models import User + # Filter users without making SQL queries if users_to_filter arg is provided + # users_to_filter is passed in apps.schedules.ical_utils.get_oncall_users_for_multiple_schedules + if users_to_filter is not None: + emails_from_ical = [username.lower() for username in usernames_from_ical] + return list( + {user for user in users_to_filter if user.username in usernames_from_ical or user.email in emails_from_ical} + ) + users_found_in_ical = organization.users if not include_viewers: users_found_in_ical = users_found_in_ical.filter( @@ -281,18 +292,28 @@ def list_of_empty_shifts_in_schedule(schedule, start_date, end_date): return sorted(empty_shifts, key=lambda dt: dt.start) -def list_users_to_notify_from_ical(schedule, events_datetime=None, include_viewers=False) -> UserQuerySet: +def list_users_to_notify_from_ical( + schedule, events_datetime=None, include_viewers=False, users_to_filter=None +) -> UserQuerySet: """ Retrieve on-call users for the current time """ events_datetime = events_datetime if events_datetime else timezone.datetime.now(timezone.utc) return list_users_to_notify_from_ical_for_period( - schedule, events_datetime, events_datetime, include_viewers=include_viewers + schedule, + events_datetime, + events_datetime, + include_viewers=include_viewers, + users_to_filter=users_to_filter, ) def list_users_to_notify_from_ical_for_period( - schedule, start_datetime, end_datetime, include_viewers=False + schedule, + start_datetime, + end_datetime, + include_viewers=False, + users_to_filter=None, ) -> UserQuerySet: # get list of iCalendars from current iCal files. If there is more than one calendar, primary calendar will always # be the first @@ -311,7 +332,9 @@ def list_users_to_notify_from_ical_for_period( parsed_ical_events.setdefault(current_priority, []).extend(current_usernames) # find users by usernames. if users are not found for shift, get users from lower priority for _, usernames in sorted(parsed_ical_events.items(), reverse=True): - users_found_in_ical = users_in_ical(usernames, schedule.organization, include_viewers=include_viewers) + users_found_in_ical = users_in_ical( + usernames, schedule.organization, include_viewers=include_viewers, users_to_filter=users_to_filter + ) if users_found_in_ical: break if users_found_in_ical: @@ -320,6 +343,53 @@ def list_users_to_notify_from_ical_for_period( return users_found_in_ical +def get_oncall_users_for_multiple_schedules(schedules, events_datetime=None): + """ + TODO: add comments + """ + from apps.user_management.models import User + + if events_datetime is None: + events_datetime = timezone.datetime.now(timezone.utc) + + # Exit early if there are no schedules + if not schedules.exists(): + return {} + + # Assume all schedules from the queryset belong to the same organization + organization = schedules[0].organization + + # Gather usernames from all events from all schedules + usernames = set() + for schedule in schedules.all(): + calendars = schedule.get_icalendars() + for calendar in calendars: + if calendar is None: + continue + events = ical_events.get_events_from_ical_between(calendar, events_datetime, events_datetime) + for event in events: + current_usernames, _ = get_usernames_from_ical_event(event) + usernames.update(current_usernames) + + # Fetch relevant users from the db + emails = [username.lower() for username in usernames] + users = organization.users.filter( + Q(**User.build_permissions_query(RBACPermission.Permissions.SCHEDULES_WRITE, organization)) + & (Q(username__in=usernames) | Q(email__lower__in=emails)) + ) + + # Get on-call users + oncall_users = {} + for schedule in schedules.all(): + # pass user list to list_users_to_notify_from_ical + schedule_oncall_users = list_users_to_notify_from_ical( + schedule, events_datetime=events_datetime, users_to_filter=users + ) + oncall_users.update({schedule.pk: schedule_oncall_users}) + + return oncall_users + + def parse_username_from_string(string): """ Parse on-call shift user from the given string diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index d746e81c53..df012d598c 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -18,10 +18,10 @@ from apps.schedules.ical_utils import ( fetch_ical_file_or_get_error, + get_oncall_users_for_multiple_schedules, list_of_empty_shifts_in_schedule, list_of_gaps_in_schedule, list_of_oncall_shifts_from_ical, - list_users_to_notify_from_ical, ) from apps.schedules.models import CustomOnCallShift from common.public_primary_keys import generate_public_primary_key, increase_public_primary_key_length @@ -43,19 +43,7 @@ def generate_public_primary_key_for_oncall_schedule_channel(): class OnCallScheduleQuerySet(PolymorphicQuerySet): def get_oncall_users(self, events_datetime=None): - if events_datetime is None: - events_datetime = timezone.datetime.now(timezone.utc) - - users = set() - - for schedule in self.all(): - schedule_oncall_users = list_users_to_notify_from_ical(schedule, events_datetime=events_datetime) - if schedule_oncall_users is None: - continue - - users.update(schedule_oncall_users) - - return list(users) + return get_oncall_users_for_multiple_schedules(self, events_datetime) class OnCallSchedule(PolymorphicModel): diff --git a/engine/apps/slack/models/slack_usergroup.py b/engine/apps/slack/models/slack_usergroup.py index a319f1337d..fe613b69bd 100644 --- a/engine/apps/slack/models/slack_usergroup.py +++ b/engine/apps/slack/models/slack_usergroup.py @@ -69,7 +69,7 @@ def can_be_updated(self) -> bool: @property def oncall_slack_user_identities(self): - users = self.oncall_schedules.get_oncall_users() + users = set(self.oncall_schedules.get_oncall_users().values()) slack_user_identities = [user.slack_user_identity for user in users if user.slack_user_identity is not None] return slack_user_identities From 19db41a82facd0ab3104e10db6d3087c56e6d30e Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 24 Jan 2023 12:02:24 +0100 Subject: [PATCH 03/10] revert changes to schedules public api endpoint --- engine/apps/public_api/serializers/schedules_polymorphic.py | 1 - engine/apps/public_api/views/schedules.py | 2 -- 2 files changed, 3 deletions(-) diff --git a/engine/apps/public_api/serializers/schedules_polymorphic.py b/engine/apps/public_api/serializers/schedules_polymorphic.py index cdad2725d9..4445fdc35f 100644 --- a/engine/apps/public_api/serializers/schedules_polymorphic.py +++ b/engine/apps/public_api/serializers/schedules_polymorphic.py @@ -10,7 +10,6 @@ class PolymorphicScheduleSerializer(EagerLoadingMixin, PolymorphicSerializer): SELECT_RELATED = ["organization"] - PREFETCH_RELATED = ["custom_shifts__users", "custom_on_call_shifts"] resource_type_field_name = "type" diff --git a/engine/apps/public_api/views/schedules.py b/engine/apps/public_api/views/schedules.py index fd6c068e73..8029de1226 100644 --- a/engine/apps/public_api/views/schedules.py +++ b/engine/apps/public_api/views/schedules.py @@ -43,8 +43,6 @@ def get_queryset(self): if name is not None: queryset = queryset.filter(name=name) - queryset = self.serializer_class.setup_eager_loading(queryset) - return queryset.order_by("id") def get_object(self): From b9e1f6123ad3dad04aaf1b21200e3c1b4c7fe293 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 24 Jan 2023 12:16:20 +0100 Subject: [PATCH 04/10] update CHANGELOG --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9d4779da0..1b3a573893 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add Slack slash command allowing to trigger a direct page via a manually created alert group +### Changed + +- Improve performance of `GET /api/internal/v1/schedules` endpoint ([#1169](https://github.com/grafana/oncall/pull/1169)) + ## v1.1.18 (2023-01-18) ### Added @@ -26,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Modified how the `Organization.is_rbac_permissions_enabled` flag is set, -based on whether we are dealing with an open-source, or cloud installation + based on whether we are dealing with an open-source, or cloud installation - Backend implementation to support direct user/schedule paging - Changed documentation links to open in new window - Remove helm chart signing From 66e8d5a1f0d94deeaa92bdec2f288a3f27a90ac9 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 24 Jan 2023 12:23:41 +0100 Subject: [PATCH 05/10] update CHANGELOG --- CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da7a7cdf5a..ea215bcec7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,15 +17,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Allow users with `viewer` role to fetch cloud connection status using the internal API ([#1181](https://github.com/grafana/oncall/pull/1181)) - When removing the Slack ChatOps integration, make it more explicit to the user what the implications of doing so are +- Improve performance of `GET /api/internal/v1/schedules` endpoint ([#1169](https://github.com/grafana/oncall/pull/1169)) ### Fixed - Removed duplicate API call, in the UI on plugin initial load, to `GET /api/internal/v1/alert_receive_channels` -### Changed - -- Improve performance of `GET /api/internal/v1/schedules` endpoint ([#1169](https://github.com/grafana/oncall/pull/1169)) - ## v1.1.18 (2023-01-18) ### Added From 781184000ed4141706b742883396751ebd802cf7 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 24 Jan 2023 12:37:34 +0100 Subject: [PATCH 06/10] address KeyError in failing tests --- engine/apps/api/serializers/schedule_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/apps/api/serializers/schedule_base.py b/engine/apps/api/serializers/schedule_base.py index e4acf4c41c..ffe6f6ac37 100644 --- a/engine/apps/api/serializers/schedule_base.py +++ b/engine/apps/api/serializers/schedule_base.py @@ -66,7 +66,7 @@ def get_warnings(self, obj): def get_on_call_now(self, obj): # Serializer context is set here: apps.api.views.schedule.ScheduleView.get_serializer_context - users = self.context["oncall_users"][obj.pk] + users = self.context["oncall_users"].get(obj.pk, []) return [user.short() for user in users] def get_number_of_escalation_chains(self, obj): From eea31d5487e152352819c071df9c9a57d3e4b594 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 24 Jan 2023 13:01:32 +0100 Subject: [PATCH 07/10] fix failing test_get_oncall_users_for_empty_schedule test --- engine/apps/schedules/tests/test_custom_on_call_shift.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/apps/schedules/tests/test_custom_on_call_shift.py b/engine/apps/schedules/tests/test_custom_on_call_shift.py index 0a40a879f3..1b4669585e 100644 --- a/engine/apps/schedules/tests/test_custom_on_call_shift.py +++ b/engine/apps/schedules/tests/test_custom_on_call_shift.py @@ -1295,7 +1295,7 @@ def test_get_oncall_users_for_empty_schedule( schedule = make_schedule(organization, schedule_class=OnCallScheduleCalendar) schedules = OnCallSchedule.objects.filter(pk=schedule.pk) - assert schedules.get_oncall_users() == [] + assert schedules.get_oncall_users()[schedule.pk] == [] @pytest.mark.django_db From 1f4105a3bc9eb89fc6e76ca0bf6e3d1ef97a3a54 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 25 Jan 2023 10:05:43 +0100 Subject: [PATCH 08/10] update test_get_oncall_users_for_multiple_schedules --- .../tests/test_custom_on_call_shift.py | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/engine/apps/schedules/tests/test_custom_on_call_shift.py b/engine/apps/schedules/tests/test_custom_on_call_shift.py index 1b4669585e..7974ad912f 100644 --- a/engine/apps/schedules/tests/test_custom_on_call_shift.py +++ b/engine/apps/schedules/tests/test_custom_on_call_shift.py @@ -1357,15 +1357,29 @@ def test_get_oncall_users_for_multiple_schedules( schedules = OnCallSchedule.objects.filter(pk__in=[schedule_1.pk, schedule_2.pk]) - expected = set(schedules.get_oncall_users(events_datetime=now + timezone.timedelta(seconds=1))) + def _extract_oncall_users_from_schedules(schedules): + return set(user for schedule in schedules.values() for user in schedule) + + expected = _extract_oncall_users_from_schedules( + schedules.get_oncall_users(events_datetime=now + timezone.timedelta(seconds=1)) + ) assert expected == {user_1, user_2} - expected = set(schedules.get_oncall_users(events_datetime=now + timezone.timedelta(minutes=10, seconds=1))) + expected = _extract_oncall_users_from_schedules( + schedules.get_oncall_users(events_datetime=now + timezone.timedelta(minutes=10, seconds=1)) + ) assert expected == {user_1, user_2, user_3} - assert schedules.get_oncall_users(events_datetime=now + timezone.timedelta(minutes=30, seconds=1)) == [user_3] + assert _extract_oncall_users_from_schedules( + schedules.get_oncall_users(events_datetime=now + timezone.timedelta(minutes=30, seconds=1)) + ) == {user_3} - assert schedules.get_oncall_users(events_datetime=now + timezone.timedelta(minutes=40, seconds=1)) == [] + assert ( + _extract_oncall_users_from_schedules( + schedules.get_oncall_users(events_datetime=now + timezone.timedelta(minutes=40, seconds=1)) + ) + == set() + ) @pytest.mark.django_db From 14e71be112faff2e2cf4682f6e52ea5f7fc94786 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 25 Jan 2023 10:06:03 +0100 Subject: [PATCH 09/10] update slack_usergroup oncall_slack_user_identities method + test --- engine/apps/slack/models/slack_usergroup.py | 2 +- engine/apps/slack/tests/test_user_group.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/engine/apps/slack/models/slack_usergroup.py b/engine/apps/slack/models/slack_usergroup.py index fe613b69bd..6cd98b664b 100644 --- a/engine/apps/slack/models/slack_usergroup.py +++ b/engine/apps/slack/models/slack_usergroup.py @@ -69,7 +69,7 @@ def can_be_updated(self) -> bool: @property def oncall_slack_user_identities(self): - users = set(self.oncall_schedules.get_oncall_users().values()) + users = set(user for schedule in self.oncall_schedules.get_oncall_users().values() for user in schedule) slack_user_identities = [user.slack_user_identity for user in users if user.slack_user_identity is not None] return slack_user_identities diff --git a/engine/apps/slack/tests/test_user_group.py b/engine/apps/slack/tests/test_user_group.py index e03d1a727d..d1a53c1410 100644 --- a/engine/apps/slack/tests/test_user_group.py +++ b/engine/apps/slack/tests/test_user_group.py @@ -39,7 +39,7 @@ def test_oncall_slack_user_identities( ) user_3 = make_user_for_organization(organization) - with patch.object(OnCallScheduleQuerySet, "get_oncall_users", return_value=[user_1, user_2, user_3]): + with patch.object(OnCallScheduleQuerySet, "get_oncall_users", return_value={"schedule1": [user_1, user_2, user_3]}): assert user_group.oncall_slack_user_identities == [slack_user_identity_1, slack_user_identity_2] From 8eff4a61e6afaf73e3e9426c74e452a28e1228a4 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 25 Jan 2023 10:26:42 +0100 Subject: [PATCH 10/10] add comments + type hints --- engine/apps/schedules/ical_utils.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index a064497c23..fd6cd34172 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -47,7 +47,7 @@ def users_in_ical( usernames_from_ical: typing.List[str], organization: Organization, include_viewers=False, - users_to_filter=None, + users_to_filter: typing.Optional[UserQuerySet] = None, ) -> UserQuerySet: """ This method returns a `UserQuerySet`, filtered by users whose username, or case-insensitive e-mail, @@ -62,13 +62,15 @@ def users_in_ical( The organization in question include_viewers : bool Whether or not the list should be further filtered to exclude users based on granted permissions + users_to_filter : typing.Optional[UserQuerySet] + Filter users without making SQL queries if users_to_filter arg is provided + users_to_filter is passed in `apps.schedules.ical_utils.get_oncall_users_for_multiple_schedules` """ from apps.user_management.models import User - # Filter users without making SQL queries if users_to_filter arg is provided - # users_to_filter is passed in apps.schedules.ical_utils.get_oncall_users_for_multiple_schedules + emails_from_ical = [username.lower() for username in usernames_from_ical] + if users_to_filter is not None: - emails_from_ical = [username.lower() for username in usernames_from_ical] return list( {user for user in users_to_filter if user.username in usernames_from_ical or user.email in emails_from_ical} ) @@ -79,9 +81,8 @@ def users_in_ical( **User.build_permissions_query(RBACPermission.Permissions.SCHEDULES_WRITE, organization) ) - user_emails = [v.lower() for v in usernames_from_ical] users_found_in_ical = users_found_in_ical.filter( - (Q(username__in=usernames_from_ical) | Q(email__lower__in=user_emails)) + (Q(username__in=usernames_from_ical) | Q(email__lower__in=emails_from_ical)) ).distinct() return users_found_in_ical @@ -343,10 +344,9 @@ def list_users_to_notify_from_ical_for_period( return users_found_in_ical -def get_oncall_users_for_multiple_schedules(schedules, events_datetime=None): - """ - TODO: add comments - """ +def get_oncall_users_for_multiple_schedules( + schedules, events_datetime=None +) -> typing.Dict[OnCallSchedule, typing.List[User]]: from apps.user_management.models import User if events_datetime is None: