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 = True
PROJECT_TASKS_SYNC = True

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, #431
Related PRs: #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)

Signed-off-by: David Wallace <[email protected]>

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

# Conflicts: # rdmo/projects/handlers.py

…ange Fix: remove views from projects if catalog is changed

…hange in catalog Signed-off-by: David Wallace <[email protected]>

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

rdmo/tasks/managers.py

@@ -15,6 +15,16 @@ class TaskQuestionSet(CurrentSiteQuerySetMixin, GroupsQuerySetMixin, Availabilit
    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.

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

rdmo/projects/forms.py

@@ -160,6 +160,16 @@ class Meta:
            'catalog': forms.RadioSelect()
        }

+    def save(self, commit=True, *args, **kwargs):
+        if 'cancel' in self.data:


this was just still missing right? ;)

Did "Cancel" save the new instance? The update view was probably just a fuction at one point and cancel was handled there.

need to check again, if this was needed or not (cancel seems to work already)

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.

Signed-off-by: David Wallace <[email protected]>

…bled Signed-off-by: David Wallace <[email protected]>

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.

jochenklar · 2025-02-06T09:39:39Z

rdmo/projects/apps.py

@@ -10,5 +10,7 @@ class ProjectsConfig(AppConfig):
    def ready(self):
        from . import rules  # noqa: F401

-        if settings.PROJECT_REMOVE_VIEWS:
-            from . import handlers  # noqa: F401
+        if settings.PROJECT_VIEWS_SYNC:


I don't think this is correct. from . import handlers just tells django to "load" the file initially. We do nothing with the imported functions. m2m_changed_views and project_save_views do not even exist, maybe this is a leftover that somehow does not cause an error?

It should also be settings.PROJECT_REMOVE_VIEWS or settings.PROJECT_VIEWS_SYNC since we just want to load the file.

Ah now I get it, you created a handlers module. Then handlers.py should be deleted and its content should move to handlers/__init__.py (or a file in the directory).

jochenklar · 2025-02-06T09:42:21Z

rdmo/projects/forms.py

@@ -160,6 +160,16 @@ class Meta:
            'catalog': forms.RadioSelect()
        }

+    def save(self, commit=True, *args, **kwargs):
+        if 'cancel' in self.data:


Did "Cancel" save the new instance? The update view was probably just a fuction at one point and cancel was handled there.

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

@@ -180,6 +190,22 @@ class Meta:
            'tasks': forms.CheckboxSelectMultiple()
        }

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


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

jochenklar · 2025-02-06T09:57:06Z

rdmo/projects/handlers/project_save_views.py

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


Add newline.

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

@@ -21,7 +21,7 @@ <h2>{% trans 'Tasks' %}</h2>
                <th style="width: 15%">{% trans 'Time frame' %}</th>
                <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?

jochenklar · 2025-02-06T10:09:27Z

rdmo/tasks/managers.py

+            .exclude(catalogs=None)
+        )
+
+


Remove newline.

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

rdmo/tasks/managers.py

@@ -15,6 +15,16 @@ class TaskQuestionSet(CurrentSiteQuerySetMixin, GroupsQuerySetMixin, Availabilit
    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 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 from 15fd305 to d708437 Compare January 7, 2025 17:05

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

9065876

Signed-off-by: David Wallace <[email protected]>

MyPyDavid force-pushed the feat-sync-project-views-and-tasks branch 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

fix: remove views if catalog is changed

009d592

# Conflicts: # rdmo/projects/handlers.py

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 and others added 2 commits January 20, 2025 17:04

Merge pull request #1200 from rdmorganiser/fix/1198-views-catalogs-ch…

88e4583

…ange Fix: remove views from projects if catalog is changed

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

edb842e

…hange in catalog Signed-off-by: David Wallace <[email protected]>

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 added 4 commits January 22, 2025 12:15

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

ca40992

Signed-off-by: David Wallace <[email protected]>

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

d93f15f

Signed-off-by: David Wallace <[email protected]>

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

4dc9f04

Signed-off-by: David Wallace <[email protected]>

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

e19be64

…bled Signed-off-by: David Wallace <[email protected]>

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

Open

jochenklar requested changes Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signal handlers for syncing of project views and tasks #1218

signal handlers for syncing of project views and tasks #1218

MyPyDavid commented Dec 19, 2024 •

edited

Loading

jochenklar Jan 9, 2025

MyPyDavid Jan 20, 2025

jochenklar Feb 6, 2025

MyPyDavid Jan 22, 2025

jochenklar Feb 6, 2025

MyPyDavid Feb 6, 2025

jochenklar Feb 6, 2025

MyPyDavid Feb 6, 2025

MyPyDavid Jan 22, 2025

jochenklar Feb 6, 2025

MyPyDavid Jan 22, 2025

jochenklar Feb 6, 2025

MyPyDavid Feb 6, 2025

MyPyDavid Jan 22, 2025 •

edited

Loading

MyPyDavid Feb 6, 2025

MyPyDavid Jan 22, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025


		# Defer synchronization of views
		setattr(instance, DEFERRED_SYNC_VIEWS_KEY, True)

signal handlers for syncing of project views and tasks #1218

Are you sure you want to change the base?

signal handlers for syncing of project views and tasks #1218

Conversation

MyPyDavid commented Dec 19, 2024 • edited Loading

Description

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyPyDavid Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyPyDavid commented Dec 19, 2024 •

edited

Loading

MyPyDavid Jan 22, 2025 •

edited

Loading