-
Notifications
You must be signed in to change notification settings - Fork 54
2.4.0 fix multisite not all sites #1488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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>
jochenklar
left a comment
There was a problem hiding this 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>
|
Wait, maybe I need to change this behaviour in the
Like so: #1436 (comment) |
|
I am not sure, I think projects already always have a |
yes, I mean to add this new
|
|
Ah, yes, makes sense. |
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
|
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
rdmo/management/managers.py
Outdated
| ) | ||
|
|
||
|
|
||
| class ForProjectQuerySet(ForSiteQuerySetMixin, ForGroupsQuerySetMixin, ForCatalogQuerySetMixin, |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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>
|
In principle its fine, but ...
I would: move the 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 |
|
Maybe |
Are |
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: |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 👀
| def get_queryset(self) -> ViewQuerySet: | ||
| return ViewQuerySet(self.model, using=self._db) | ||
|
|
||
| def filter_catalog(self, catalog): |
There was a problem hiding this comment.
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..
jochenklar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕺
Description
Related issue: #1481
Motivation and Context
How has this been tested?
Screenshots (if appropriate)