signal handlers for syncing of project views and tasks #1218

MyPyDavid · 2024-12-19T18:34:32Z

Description

For the settings this PR removes:
PROJECT_REMOVE_VIEWS = True
and adds:

PROJECT_VIEWS_SYNC = False
PROJECT_TASKS_SYNC = False

The projects/handlers.py are refactored into a project/handlers package with a project/handlers/generic_handlers.py module that contains the logic for signal receivers.
It handles the signals for changes in View.catalogs, View.sites, View.groups as well as Task.catalogs, Task.sites, Task.groups and updates the project.views or projects.tasks.

Tests, that only use db view.catalog.add(...) methods and not the client, are included.

Related issues: #966, #1198, #345,
Related PRs: #431, #1200

Motivation and Context

ToBeDone:

Add tests
Decide on automatically adding views to projects (maybe also enabled by setting?)
Change to custom signal/include filtering of views based on user permissions
Implement adding of views to other signals.
...

How has this been tested?

Screenshots (if appropriate)

rdmo/projects/tests/test_handlers_tasks.py

jochenklar · 2025-01-09T09:48:38Z

rdmo/projects/handlers/utils.py

@@ -0,0 +1,10 @@
+


I don't think those methods should exist, this should be inline in the handlers (and without the getattr).

I could make it more explicit but it would require a lot more boilerplate code.
The signal handlers for View and Task are basically duplicates, so there would be 18 branches instead of the 9 if action == ... in total in the different functions for .catalogs, .sites and .groups .
But we already agreed about this in the meeting right?

Or would there be an extra use case that would require an update on the Issues as well, when project.tasks are handled?

Example for an explicit function

# Identify the catalogs affected by this change catalogs = model.objects.filter(pk__in=pk_set) # Determine the projects to be modified if action == 'post_add': projects = Project.objects.filter_catalogs(catalogs).exclude(views=instance) for project in projects: project.views.add(instance) elif action == 'post_remove': projects = Project.objects.filter_catalogs(catalogs).filter(views=instance) for project in projects: project.views.remove(instance) elif action == 'post_clear': projects = Project.objects.filter(views=instance) for project in projects: project.views.remove(instance)

No I mean just remove_instance_from_projects and add_instance_to_projects should be inlined to the generic handlers.

rdmo/projects/handlers/generic_handlers.py

MyPyDavid · 2025-01-22T09:52:16Z

rdmo/tasks/managers.py

    def filter_catalog(self, catalog):
        return self.filter(models.Q(catalogs=None) | models.Q(catalogs=catalog))

+    def filter_available_views_for_project(self, project):


I think that a Project should be able to find the views that are available for itself. Do you agree and would this query be correct for that?
I've changed the filter_group a little bit to handle a list of users as well as a single user.

I need to check after the meeting.

this kind of method is also needed for an endpoint (for project.views) in the new Project Detail in React I think

As discussed in the meeting.

change filter to filter_for_project_site and filter_project_owners_groups and add project.groups: set()

MyPyDavid · 2025-01-22T09:53:33Z

rdmo/projects/forms.py

+        if 'cancel' in self.data:
+            return self.instance
+
+        # if the catalog is the same, do nothing


I tried to prevent calling the save method if nothing was changed

I am not sure, but shouldn't this be handled by the UpdateView... maybe because we use only parts of the model here.

rdmo/projects/forms.py

MyPyDavid · 2025-01-22T09:56:52Z

rdmo/projects/handlers/project_save_tasks.py

+        return
+
+    # Defer synchronization of views
+    setattr(instance, DEFERRED_SYNC_TASKS_KEY, True)


I needed a pre_save to check if the catalog was changed or not. And afterwards it passes this boolean (with a "deferred" attribute name) to the post_save.
It seems complicated for just a project.save() but could not find another way.

remove setattr and just use, eg. instance._catalog_has_changed_sync_task

MyPyDavid · 2025-01-22T09:58:18Z

rdmo/projects/tests/helpers.py

+            for catalog in view.catalogs.all():
+                catalog_views_mapping[catalog.id].append({
+                    'id': view.id,
+                    'sites': list(view.sites.values_list('id', flat=True)),


I wanted to explicitly test this filter filter_available_views_for_project with this mapping but have not implemented that test yet.

jochenklar · 2025-02-06T09:35:56Z

rdmo/core/managers.py

-    def filter_group(self, user):
-        groups = user.groups.all()
+    def filter_group(self, users):
+


Remove newline.

rdmo/projects/apps.py

rdmo/projects/forms.py

jochenklar · 2025-02-06T09:43:47Z

rdmo/projects/forms.py

+        if 'cancel' in self.data:
+            return self.instance
+
+        # if the catalog is the same, do nothing


I am not sure, but shouldn't this be handled by the UpdateView... maybe because we use only parts of the model here.

jochenklar · 2025-02-06T09:44:54Z

rdmo/projects/forms.py

            'tasks': forms.CheckboxSelectMultiple()
        }

+    def save(self, commit=True, *args, **kwargs):


So this is just about not saving the instance when nothing changed, right?

yes, but it might be just an optional optimization, think I'll remove it again for now.

rdmo/projects/handlers/project_save_views.py

jochenklar · 2025-02-06T09:58:52Z

rdmo/projects/handlers/utils.py

@@ -0,0 +1,10 @@
+


No I mean just remove_instance_from_projects and add_instance_to_projects should be inlined to the generic handlers.

jochenklar · 2025-02-06T10:00:45Z

rdmo/projects/templates/projects/project_detail_issues.html

                <th style="width: 15%">{% trans 'Status' %}</th>
                <th style="width: 10%" class="text-right">
-                {% if can_change_project %}
+                {% if can_change_project and not settings.PROJECT_TASKS_SYNC  %}


This part is missing from the sidebar:

But only for tasks it seems.

No, its for both, but there is another error: A view/task without catalog should be available to all projects. Also the update-task/view-for-project forms should be disabled when the setting is True. Check /projects/1/update/views/ and /projects/1/update/tasks/. Maybe 404?

So, I am adding now a filter to the managers that is used for all cases where a project needs its available views/tasks.
There is no request.user in the loop anymore. Is that a problem? The filter is determined by the project's properties (like project.site, .catalogs and the new .groups )

def filter_available_views_for_project(self, project): site_filter = Q(sites=project.site) | Q(sites__isnull=True) catalogs_filter = Q(catalogs=project.catalog) | Q(catalogs__isnull=True) groups_filter = Q(groups__in=project.groups) | Q(groups__isnull=True) availability_filter = Q(available=True) return self.filter(site_filter & catalogs_filter & groups_filter & availability_filter)

rdmo/tasks/managers.py

jochenklar · 2025-02-06T10:12:22Z

rdmo/tasks/managers.py

    def filter_catalog(self, catalog):
        return self.filter(models.Q(catalogs=None) | models.Q(catalogs=catalog))

+    def filter_available_views_for_project(self, project):


I need to check after the meeting.

MyPyDavid · 2025-02-14T13:34:34Z

rdmo/core/settings.py


-PROJECT_REMOVE_VIEWS = True
+PROJECT_VIEWS_SYNC = True
+PROJECT_TASKS_SYNC = True


should these actually be enabled by default?

I think not.

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

# Conflicts: # rdmo/projects/handlers.py

…hange in catalog Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

…bled Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de> # Conflicts: # rdmo/core/settings.py

… handlers Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

…ps to Project Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid · 2025-02-14T15:26:02Z

rebased to 2.3.0

jochenklar

The main issue is about the new managers and availability. An reviewer/editor/admin should be able to assign a non available Task/View to a project to test it.

jochenklar · 2025-02-17T12:20:52Z

rdmo/core/settings.py


-PROJECT_REMOVE_VIEWS = True
+PROJECT_VIEWS_SYNC = True
+PROJECT_TASKS_SYNC = True


I think not.

jochenklar · 2025-02-17T12:27:36Z

rdmo/views/managers.py

+        catalogs_filter = Q(catalogs=project.catalog) | Q(catalogs__isnull=True)
+        groups_filter = Q(groups__in=project.groups) | Q(groups__isnull=True)
+        availability_filter = Q(available=True)
+        return self.filter(site_filter & catalogs_filter & groups_filter & availability_filter)


I am sorry but there is a newline in rdmo.tasks.managers right here 🙃

jochenklar · 2025-02-17T12:28:50Z

testing/config/settings/base.py

 PROJECT_SEND_INVITE = True

-PROJECT_REMOVE_VIEWS = True
+PROJECT_VIEWS_SYNC = True


Depending on the default settings and the tests, maybe this should be False (or removed from this file).

jochenklar · 2025-02-17T12:34:18Z

rdmo/tasks/managers.py

+        site_filter = Q(sites=project.site) | Q(sites__isnull=True)
+        catalogs_filter = Q(catalogs=project.catalog) | Q(catalogs__isnull=True)
+        groups_filter = Q(groups__in=project.groups) | Q(groups__isnull=True)
+        availability_filter = Q(available=True)


I think you removed the check for the managers and admins here, this was done by .filter_availability(self.request.user) and needs the user. This comes from AvailabilityQuerySetMixin. I think you also reimplemented the other thinks in core/managers.py.

Maybe the availability part should stay in AvailabilityQuerySetMixin and we need only filter_for_project and use it like this

tasks = Task.objects.filter_for_project(project).filter_availability(self.request.user)

alternatively 3 methods

tasks = Task.objects.filter_for_project_site(project) \ .filter_for_project_catalog(project) \ .filter_for_project_group(project) \ .filter_availability(self.request.user)

ok, Ive added it now like this in the code where it is already several times indented.
What do you think about this type of notation? ;)

tasks = ( Task.objects .filter_for_project(project) .filter_availability(self.request.user) )

I like "my" style better, but I think this is fine (at least no \). I would indent the two '.filter...' lines by 4.

I've used one-liner queries when there is only a single filter and it fits on one line, for multiple filters I've used this multi-line parentheses notation with the extra indent. So basically best of both worlds ;)

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

jochenklar

Done, thanks!

MyPyDavid self-assigned this Dec 19, 2024

MyPyDavid changed the base branch from main to 2.3.0 December 19, 2024 18:34

MyPyDavid added the status:work-in-progress label Dec 19, 2024

MyPyDavid force-pushed the feat-sync-project-views-and-tasks branch 2 times, most recently from d708437 to 9065876 Compare January 8, 2025 09:54

MyPyDavid added status:acceptance and removed status:work-in-progress labels Jan 8, 2025

MyPyDavid added this to the RDMO 2.3.0 milestone Jan 8, 2025

jochenklar requested changes Jan 9, 2025

View reviewed changes

MyPyDavid added the status:work-in-progress label Jan 20, 2025

MyPyDavid mentioned this pull request Jan 20, 2025

Fix: remove views from projects if catalog is changed #1200

Merged

4 tasks

MyPyDavid commented Jan 22, 2025

View reviewed changes

rdmo/projects/forms.py Outdated Show resolved Hide resolved

MyPyDavid commented Jan 22, 2025

View reviewed changes

MyPyDavid requested a review from jochenklar January 22, 2025 09:58

MyPyDavid marked this pull request as ready for review January 22, 2025 09:58

MyPyDavid changed the title ~~Draft: signal handlers sync project views and tasks~~ signal handlers for syncing of project views and tasks Jan 22, 2025

MyPyDavid force-pushed the feat-sync-project-views-and-tasks branch from 55905ba to e19be64 Compare January 24, 2025 13:40

MyPyDavid linked an issue Jan 31, 2025 that may be closed by this pull request

"view" doesn't change when "catalog" is changed #1198

Closed

jochenklar requested changes Feb 6, 2025

View reviewed changes

MyPyDavid added the type:feature-request label Feb 13, 2025

MyPyDavid commented Feb 14, 2025

View reviewed changes

MyPyDavid and others added 6 commits February 14, 2025 16:18

feat(project,handlers): add sync for views and tasks, #966 #1198

3036a33

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

fix: remove views if catalog is changed

1a03525

# Conflicts: # rdmo/projects/handlers.py

feat(project, handlers): refactor and add handler for project save, c…

8924baf

…hange in catalog Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

fix(project,handlers): ignore signals at project create

55d24f7

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

refactor(project,handlers): simplify pre and post save

bdb6f7e

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

fix(managers,tasks,views): use project.owners for filter

02137dd

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid added 11 commits February 14, 2025 16:23

feat(project, update): prevent views or tasks update when sync is ena…

b09af76

…bled Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de> # Conflicts: # rdmo/core/settings.py

refactor(project,handlers): move add and remove inline to the generic…

ecefbaf

… handlers Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

refactor(project,handlers): not use setattr and add created check

42f54e0

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

refactor(tasks,views,manager): update query filter for project

e0799c3

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

feat(projects,sync): use tasks and views for project filter, add grou…

2371728

…ps to Project Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

feat(managers,task,views): include isnull in filter for project

20ca32d

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

refactor(project,handlers): remove handlers.py

9b2dedc

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

style(managers,tasks,views): fix newlines

d3f94a4

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

style(projects,handlers): fix args and remove docstrings

4f32f00

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

feat(projects,sync): prevent update tasks or views when sync is enabled

9cb4429

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

revert(core,managers): keep filter group the same

50f25dd

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid force-pushed the feat-sync-project-views-and-tasks branch from 4860e95 to 50f25dd Compare February 14, 2025 15:25

MyPyDavid requested a review from jochenklar February 17, 2025 10:33

jochenklar requested changes Feb 17, 2025

View reviewed changes

MyPyDavid added 6 commits February 17, 2025 21:50

feat(projects,sync): disable sync by default

90c32a3

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

refactor(managers,sync): add filter_for_project method

3c2ea28

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

tests(project, handlers): add fixtures to enable sync

fa69180

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

feat(project,tasks,views): add user to availability filter

1b42dd1

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

tests(project, handlers): fix import of enable_project_tasks_sync

3da6110

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

style(project,tasks,views): update sync queries

7fc4740

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

jochenklar approved these changes Feb 20, 2025

View reviewed changes

MyPyDavid merged commit 7ada4e1 into 2.3.0 Feb 20, 2025
19 checks passed

MyPyDavid deleted the feat-sync-project-views-and-tasks branch February 20, 2025 12:32

MyPyDavid mentioned this pull request Feb 20, 2025

RDMO 2.3.0 ⭐ #1195

Merged

signal handlers for syncing of project views and tasks #1218

signal handlers for syncing of project views and tasks #1218

Uh oh!

Conversation

MyPyDavid commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MyPyDavid Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MyPyDavid Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MyPyDavid commented Feb 14, 2025

Uh oh!

jochenklar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MyPyDavid commented Dec 19, 2024 •

edited

Loading

MyPyDavid Jan 22, 2025 •

edited

Loading

MyPyDavid Feb 14, 2025 •

edited

Loading