From 4d1fb120b13287b791f625fa615c20c7f309ee27 Mon Sep 17 00:00:00 2001 From: Alejandro Alonso Date: Wed, 11 May 2016 12:41:16 +0200 Subject: [PATCH] Improving performance when updating objects --- taiga/permissions/service.py | 27 ++++++++++++++----- taiga/projects/models.py | 30 +++++++++++++++++++-- taiga/projects/notifications/services.py | 23 +++++------------ taiga/projects/references/models.py | 3 --- taiga/projects/services/tags_colors.py | 2 +- taiga/timeline/service.py | 29 +++++++++++++++++++-- taiga/timeline/signals.py | 33 +++--------------------- taiga/webhooks/apps.py | 2 -- tests/integration/test_notifications.py | 21 +++++++++------ tests/unit/test_timeline.py | 8 +++--- 10 files changed, 105 insertions(+), 73 deletions(-) diff --git a/taiga/permissions/service.py b/taiga/permissions/service.py index a56b3afc..32f79fdc 100644 --- a/taiga/permissions/service.py +++ b/taiga/permissions/service.py @@ -20,11 +20,18 @@ from .permissions import ADMINS_PERMISSIONS, MEMBERS_PERMISSIONS, ANON_PERMISSIO from django.apps import apps -def _get_user_project_membership(user, project): +def _get_user_project_membership(user, project, cache="user"): + """ + cache param determines how memberships are calculated trying to reuse the existing data + in cache + """ if user.is_anonymous(): return None - return user.cached_membership_for_project(project) + if cache == "user": + return user.cached_membership_for_project(project) + + return project.cached_memberships_for_user(user) def _get_object_project(obj): @@ -63,13 +70,17 @@ def is_project_admin(user, obj): return False -def user_has_perm(user, perm, obj=None): +def user_has_perm(user, perm, obj=None, cache="user"): + """ + cache param determines how memberships are calculated trying to reuse the existing data + in cache + """ project = _get_object_project(obj) if not project: return False - return perm in get_user_project_permissions(user, project) + return perm in get_user_project_permissions(user, project, cache=cache) def role_has_perm(role, perm): @@ -82,8 +93,12 @@ def _get_membership_permissions(membership): return [] -def get_user_project_permissions(user, project): - membership = _get_user_project_membership(user, project) +def get_user_project_permissions(user, project, cache="user"): + """ + cache param determines how memberships are calculated trying to reuse the existing data + in cache + """ + membership = _get_user_project_membership(user, project, cache=cache) if user.is_superuser: admins_permissions = list(map(lambda perm: perm[0], ADMINS_PERMISSIONS)) members_permissions = list(map(lambda perm: perm[0], MEMBERS_PERMISSIONS)) diff --git a/taiga/projects/models.py b/taiga/projects/models.py index 730a62e3..cd38c35b 100644 --- a/taiga/projects/models.py +++ b/taiga/projects/models.py @@ -44,7 +44,6 @@ from taiga.permissions.permissions import ANON_PERMISSIONS, MEMBERS_PERMISSIONS from taiga.projects.notifications.choices import NotifyLevel from taiga.projects.notifications.services import ( - get_notify_policy, set_notify_policy_level, set_notify_policy_level_to_ignore, create_notify_policy_if_not_exists) @@ -345,6 +344,33 @@ class Project(ProjectDefaults, TaggedMixin, models.Model): def cached_user_stories(self): return list(self.user_stories.all()) + @cached_property + def cached_notify_policies(self): + return {np.user.id: np for np in self.notify_policies.select_related("user", "project")} + + def cached_notify_policy_for_user(self, user): + """ + Get notification level for specified project and user. + """ + policy = self.cached_notify_policies.get(user.id, None) + if policy is None: + model_cls = apps.get_model("notifications", "NotifyPolicy") + policy = model_cls.objects.create( + project=self, + user=user, + notify_level= NotifyLevel.involved) + + del self.cached_notify_policies + + return policy + + @cached_property + def cached_memberships(self): + return {m.user.id: m for m in self.memberships.exclude(user__isnull=True).select_related("user", "project", "role")} + + def cached_memberships_for_user(self, user): + return self.cached_memberships.get(user.id, None) + def get_roles(self): return self.roles.all() @@ -426,7 +452,7 @@ class Project(ProjectDefaults, TaggedMixin, models.Model): set_notify_policy_level(notify_policy, notify_level) def remove_watcher(self, user): - notify_policy = get_notify_policy(self, user) + notify_policy = self.cached_notify_policy_for_user(user) set_notify_policy_level_to_ignore(notify_policy) def delete_related_content(self): diff --git a/taiga/projects/notifications/services.py b/taiga/projects/notifications/services.py index 0a3cb8e7..22d518cb 100644 --- a/taiga/projects/notifications/services.py +++ b/taiga/projects/notifications/services.py @@ -78,16 +78,6 @@ def create_notify_policy_if_not_exists(project, user, level=NotifyLevel.involved raise exc.IntegrityError(_("Notify exists for specified user and project")) from e -def get_notify_policy(project, user): - """ - Get notification level for specified project and user. - """ - model_cls = apps.get_model("notifications", "NotifyPolicy") - instance, _ = model_cls.objects.get_or_create(project=project, user=user, - defaults={"notify_level": NotifyLevel.involved}) - return instance - - def analize_object_for_watchers(obj:object, comment:str, user:object): """ Generic implementation for analize model objects and @@ -124,13 +114,13 @@ def _filter_by_permissions(obj, user): WikiPage = apps.get_model("wiki", "WikiPage") if isinstance(obj, UserStory): - return user_has_perm(user, "view_us", obj) + return user_has_perm(user, "view_us", obj, cache="project") elif isinstance(obj, Issue): - return user_has_perm(user, "view_issues", obj) + return user_has_perm(user, "view_issues", obj, cache="project") elif isinstance(obj, Task): - return user_has_perm(user, "view_tasks", obj) + return user_has_perm(user, "view_tasks", obj, cache="project") elif isinstance(obj, WikiPage): - return user_has_perm(user, "view_wiki_pages", obj) + return user_has_perm(user, "view_wiki_pages", obj, cache="project") return False @@ -149,7 +139,7 @@ def get_users_to_notify(obj, *, discard_users=None) -> list: project = obj.get_project() def _check_level(project:object, user:object, levels:tuple) -> bool: - policy = get_notify_policy(project, user) + policy = project.cached_notify_policy_for_user(user) return policy.notify_level in levels _can_notify_hard = partial(_check_level, project, @@ -226,8 +216,7 @@ def send_notifications(obj, *, history): # Get a complete list of notifiable users for current # object and send the change notification to them. notify_users = get_users_to_notify(obj, discard_users=[notification.owner]) - for notify_user in notify_users: - notification.notify_users.add(notify_user) + notification.notify_users.add(*notify_users) # If we are the min interval is 0 it just work in a synchronous and spamming way if settings.CHANGE_NOTIFICATIONS_MIN_INTERVAL == 0: diff --git a/taiga/projects/references/models.py b/taiga/projects/references/models.py index bf3b072c..3bc63b1c 100644 --- a/taiga/projects/references/models.py +++ b/taiga/projects/references/models.py @@ -110,6 +110,3 @@ models.signals.post_save.connect(attach_sequence, sender=UserStory, dispatch_uid models.signals.post_save.connect(attach_sequence, sender=Issue, dispatch_uid="refissue") models.signals.post_save.connect(attach_sequence, sender=Task, dispatch_uid="reftask") models.signals.post_delete.connect(delete_sequence, sender=Project, dispatch_uid="refprojdel") - - - diff --git a/taiga/projects/services/tags_colors.py b/taiga/projects/services/tags_colors.py index 9cebd044..90d44efa 100644 --- a/taiga/projects/services/tags_colors.py +++ b/taiga/projects/services/tags_colors.py @@ -54,7 +54,7 @@ def update_project_tags_colors_handler(instance): new_color = _get_new_color(tag, settings.TAGS_PREDEFINED_COLORS, exclude=used_colors) instance.project.tags_colors.append([tag, new_color]) - + remove_unused_tags(instance.project) if not isinstance(instance, Project): diff --git a/taiga/timeline/service.py b/taiga/timeline/service.py index 65e27a89..12f7f29e 100644 --- a/taiga/timeline/service.py +++ b/taiga/timeline/service.py @@ -78,8 +78,7 @@ def _add_to_objects_timeline(objects, instance:object, event_type:str, created_d _add_to_object_timeline(obj, instance, event_type, created_datetime, namespace, extra_data) -@app.task -def push_to_timeline(objects, instance:object, event_type:str, created_datetime:object, namespace:str="default", extra_data:dict={}): +def _push_to_timeline(objects, instance:object, event_type:str, created_datetime:object, namespace:str="default", extra_data:dict={}): if isinstance(objects, Model): _add_to_object_timeline(objects, instance, event_type, created_datetime, namespace, extra_data) elif isinstance(objects, QuerySet) or isinstance(objects, list): @@ -88,6 +87,32 @@ def push_to_timeline(objects, instance:object, event_type:str, created_datetime: raise Exception("Invalid objects parameter") +@app.task +def push_to_timelines(project, user, obj, event_type, created_datetime, extra_data={}): + if project is not None: + # Actions related with a project + + ## Project timeline + _push_to_timeline(project, obj, event_type, created_datetime, + namespace=build_project_namespace(project), + extra_data=extra_data) + + project.refresh_totals() + + if hasattr(obj, "get_related_people"): + related_people = obj.get_related_people() + + _push_to_timeline(related_people, obj, event_type, created_datetime, + namespace=build_user_namespace(user), + extra_data=extra_data) + else: + # Actions not related with a project + ## - Me + _push_to_timeline(user, obj, event_type, created_datetime, + namespace=build_user_namespace(user), + extra_data=extra_data) + + def get_timeline(obj, namespace=None): assert isinstance(obj, Model), "obj must be a instance of Model" from .models import Timeline diff --git a/taiga/timeline/signals.py b/taiga/timeline/signals.py index 887688fc..eda08031 100644 --- a/taiga/timeline/signals.py +++ b/taiga/timeline/signals.py @@ -22,42 +22,17 @@ from django.utils.translation import ugettext as _ from taiga.projects.history import services as history_services from taiga.projects.history.choices import HistoryType -from taiga.timeline.service import (push_to_timeline, +from taiga.timeline.service import (push_to_timelines, build_user_namespace, build_project_namespace, extract_user_info) -def _push_to_timeline(*args, **kwargs): +def _push_to_timelines(*args, **kwargs): if settings.CELERY_ENABLED: - push_to_timeline.delay(*args, **kwargs) + push_to_timelines.delay(*args, **kwargs) else: - push_to_timeline(*args, **kwargs) - - -def _push_to_timelines(project, user, obj, event_type, created_datetime, extra_data={}): - if project is not None: - # Actions related with a project - - ## Project timeline - _push_to_timeline(project, obj, event_type, created_datetime, - namespace=build_project_namespace(project), - extra_data=extra_data) - - project.refresh_totals() - - if hasattr(obj, "get_related_people"): - related_people = obj.get_related_people() - - _push_to_timeline(related_people, obj, event_type, created_datetime, - namespace=build_user_namespace(user), - extra_data=extra_data) - else: - # Actions not related with a project - ## - Me - _push_to_timeline(user, obj, event_type, created_datetime, - namespace=build_user_namespace(user), - extra_data=extra_data) + push_to_timelines(*args, **kwargs) def _clean_description_fields(values_diff): diff --git a/taiga/webhooks/apps.py b/taiga/webhooks/apps.py index a10cd1d2..9fff8e9b 100644 --- a/taiga/webhooks/apps.py +++ b/taiga/webhooks/apps.py @@ -27,8 +27,6 @@ def connect_webhooks_signals(): dispatch_uid="webhooks") - - def disconnect_webhooks_signals(): signals.post_save.disconnect(sender=apps.get_model("history", "HistoryEntry"), dispatch_uid="webhooks") diff --git a/tests/integration/test_notifications.py b/tests/integration/test_notifications.py index 85ea89fb..bb2cdfd8 100644 --- a/tests/integration/test_notifications.py +++ b/tests/integration/test_notifications.py @@ -60,7 +60,7 @@ def test_create_retrieve_notify_policy(): current_number = policy_model_cls.objects.all().count() assert current_number == 0 - policy = services.get_notify_policy(project, project.owner) + policy = project.cached_notify_policy_for_user(project.owner) current_number = policy_model_cls.objects.all().count() assert current_number == 1 @@ -182,6 +182,7 @@ def test_users_to_notify(): policy_member1.notify_level = NotifyLevel.all policy_member1.save() + del project.cached_notify_policies users = services.get_users_to_notify(issue) assert len(users) == 2 assert users == {member1.user, issue.get_owner()} @@ -190,6 +191,8 @@ def test_users_to_notify(): issue.add_watcher(member3.user) policy_member3.notify_level = NotifyLevel.all policy_member3.save() + + del project.cached_notify_policies users = services.get_users_to_notify(issue) assert len(users) == 3 assert users == {member1.user, member3.user, issue.get_owner()} @@ -199,12 +202,14 @@ def test_users_to_notify(): policy_member3.save() issue.add_watcher(member3.user) + del project.cached_notify_policies users = services.get_users_to_notify(issue) assert len(users) == 2 assert users == {member1.user, issue.get_owner()} # Test with watchers without permissions issue.add_watcher(member5.user) + del project.cached_notify_policies users = services.get_users_to_notify(issue) assert len(users) == 2 assert users == {member1.user, issue.get_owner()} @@ -231,7 +236,7 @@ def test_watching_users_to_notify_on_issue_modification_1(): issue = f.IssueFactory.create(project=project) watching_user = f.UserFactory() issue.add_watcher(watching_user) - watching_user_policy = services.get_notify_policy(project, watching_user) + watching_user_policy = project.cached_notify_policy_for_user(watching_user) issue.description = "test1" issue.save() watching_user_policy.notify_level = NotifyLevel.all @@ -250,7 +255,7 @@ def test_watching_users_to_notify_on_issue_modification_2(): issue = f.IssueFactory.create(project=project) watching_user = f.UserFactory() issue.add_watcher(watching_user) - watching_user_policy = services.get_notify_policy(project, watching_user) + watching_user_policy = project.cached_notify_policy_for_user(watching_user) issue.description = "test1" issue.save() watching_user_policy.notify_level = NotifyLevel.involved @@ -269,7 +274,7 @@ def test_watching_users_to_notify_on_issue_modification_3(): issue = f.IssueFactory.create(project=project) watching_user = f.UserFactory() issue.add_watcher(watching_user) - watching_user_policy = services.get_notify_policy(project, watching_user) + watching_user_policy = project.cached_notify_policy_for_user(watching_user) issue.description = "test1" issue.save() watching_user_policy.notify_level = NotifyLevel.none @@ -289,7 +294,7 @@ def test_watching_users_to_notify_on_issue_modification_4(): issue = f.IssueFactory.create(project=project) watching_user = f.UserFactory() project.add_watcher(watching_user) - watching_user_policy = services.get_notify_policy(project, watching_user) + watching_user_policy = project.cached_notify_policy_for_user(watching_user) issue.description = "test1" issue.save() watching_user_policy.notify_level = NotifyLevel.none @@ -309,7 +314,7 @@ def test_watching_users_to_notify_on_issue_modification_5(): issue = f.IssueFactory.create(project=project) watching_user = f.UserFactory() project.add_watcher(watching_user) - watching_user_policy = services.get_notify_policy(project, watching_user) + watching_user_policy = project.cached_notify_policy_for_user(watching_user) issue.description = "test1" issue.save() watching_user_policy.notify_level = NotifyLevel.all @@ -329,7 +334,7 @@ def test_watching_users_to_notify_on_issue_modification_6(): issue = f.IssueFactory.create(project=project) watching_user = f.UserFactory() project.add_watcher(watching_user) - watching_user_policy = services.get_notify_policy(project, watching_user) + watching_user_policy = project.cached_notify_policy_for_user(watching_user) issue.description = "test1" issue.save() watching_user_policy.notify_level = NotifyLevel.involved @@ -902,7 +907,7 @@ def test_watchers_assignation_for_us(client): def test_retrieve_notify_policies_by_anonymous_user(client): project = f.ProjectFactory.create() - policy = services.get_notify_policy(project, project.owner) + policy = project.cached_notify_policy_for_user(project.owner) url = reverse("notifications-detail", args=[policy.pk]) response = client.get(url, content_type="application/json") diff --git a/tests/unit/test_timeline.py b/tests/unit/test_timeline.py index e34906d6..18c679e5 100644 --- a/tests/unit/test_timeline.py +++ b/tests/unit/test_timeline.py @@ -26,12 +26,14 @@ from taiga.projects.models import Project import pytest +pytestmark = pytest.mark.django_db def test_push_to_timeline_many_objects(): with patch("taiga.timeline.service._add_to_object_timeline") as mock: users = [get_user_model(), get_user_model(), get_user_model()] + owner = get_user_model() project = Project() - service.push_to_timeline(users, project, "test", project.created_date) + service._push_to_timeline(users, project, "test", project.created_date) assert mock.call_count == 3 assert mock.mock_calls == [ call(users[0], project, "test", project.created_date, "default", {}), @@ -39,7 +41,7 @@ def test_push_to_timeline_many_objects(): call(users[2], project, "test", project.created_date, "default", {}), ] with pytest.raises(Exception): - service.push_to_timeline(None, project, "test") + service._push_to_timeline(None, project, "test") def test_add_to_objects_timeline(): @@ -54,7 +56,7 @@ def test_add_to_objects_timeline(): call(users[2], project, "test", project.created_date, "default", {}), ] with pytest.raises(Exception): - service.push_to_timeline(None, project, "test") + service._push_to_timeline(None, project, "test") def test_get_impl_key_from_model():