Skip to content

Conversation

@MyPyDavid
Copy link
Member

Description

Related issue: #1481

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

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

@jochenklar jochenklar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Some mostly cosmetic remarks.

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
@MyPyDavid
Copy link
Member Author

MyPyDavid commented Nov 27, 2025

Wait, maybe I need to change this behaviour in the Task and View managers as well.
For some reason I added the CurrentSiteQuerySetMixin to the TaskQuerySet but it does not even use the filter_current_site but instead a filter_for_project_site because I wanted to filter for the site that the projects has.
I think that I should refactor that filter_for_project_site or filter_for_site into core/managers and add this new multisite behaviour (and do the same for the View and add some tests as well).
What do you think @jochenklar?

Like so: #1436 (comment)

@jochenklar
Copy link
Member

I am not sure, I think projects already always have a site they belong to, right? If I remember correctly, it was important that for Issues and Views the site of the project is checked and not the site of the user.

@MyPyDavid
Copy link
Member Author

MyPyDavid commented Nov 27, 2025

I am not sure, I think projects already always have a site they belong to, right? If I remember correctly, it was important that for Issues and Views the site of the project is checked and not the site of the user.

yes, I mean to add this new if settings.MULTISITE ... behaviour(not all when None) also to the queries on Task.sites and View.sites , and then it will indeed use the project.site as argument.

@jochenklar
Copy link
Member

Ah, yes, makes sense.

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
@MyPyDavid
Copy link
Member Author

  • still need to add tests for the new multisite filter behaviour

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


class ForProjectQuerySet(ForSiteQuerySetMixin, ForGroupsQuerySetMixin, ForCatalogQuerySetMixin,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about this @jochenklar??
It cleans up a lot of code and is currently only used by Views and Tasks..

Copy link
Member Author

@MyPyDavid MyPyDavid Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I would keep the View and Task QuerySet classes and just add a mixin to it (not remove them completely)..

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

jochenklar commented Dec 1, 2025

In principle its fine, but ...

  1. The tasks and views django apps, should not be dependent on on management (its the other way around).

  2. This is one layer of indirection to much for me. The ForSiteQuerySetMixin, ForGroupsQuerySetMixin, ForCatalogQuerySetMixin are not needed.

I would:

move the ForProjectManagerMixin to core and call it MultisiteManagerMixin or ProjectManagerMixin.

Alternatively (and after giving it some more thought): maybe this is not a good case for managers. After all those are project related functions and should be part of the projects django app. Why not create a filter_for_project(model, project) method in project.utils containing all the code and be done with it. Also easy testable.

@jochenklar
Copy link
Member

Maybe filter_elements_for_project(Task, project)?

@MyPyDavid
Copy link
Member Author

Maybe filter_elements_for_project(Task, project)?

Are Task and View also considered as normal elements? ;)

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

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

def filter_current_site(self):
return self.filter(models.Q(sites=None) | models.Q(sites=settings.SITE_ID))
if settings.MULTISITE:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the fix for the issue #1481

cc=[request.user.email], reply_to=[request.user.email])


def filter_tasks_or_views_for_project(task_or_view, project) -> TaskQuerySet | ViewQuerySet:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the new util function,
it has the nice return type annotation 👀

@MyPyDavid MyPyDavid requested a review from jochenklar December 1, 2025 20:30
def get_queryset(self) -> ViewQuerySet:
return ViewQuerySet(self.model, using=self._db)

def filter_catalog(self, catalog):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this was used anywhere even before this PR..

Copy link
Member

@jochenklar jochenklar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕺

@MyPyDavid MyPyDavid merged commit 2969be0 into 2.4.0 Dec 4, 2025
18 checks passed
@MyPyDavid MyPyDavid deleted the 2.4.0-fix-multisite-not-all-sites branch December 4, 2025 15:30
@MyPyDavid MyPyDavid mentioned this pull request Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[multisite] A catalog can be made available to other sites when removing all of the sites from the catalog

3 participants