-
Notifications
You must be signed in to change notification settings - Fork 12
assign workinghours to participants on federated instances #1503
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
base: main
Are you sure you want to change the base?
Conversation
a545fbd to
7276810
Compare
|
|
||
| class Consequence(Model): | ||
| @dont_log # as we log the specific models | ||
| class AbstractConsequence(PolymorphicModel): |
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.
Currently, the code is not robust against consequence handlers being uninstalled (try removing the youth warden plugin). I think it should be, similar to missing structures and signup flow, it shouldn't crash the application. Maybe setting a custom _default_manager would be a simple solution?
|
Here's what we discussed about queryset filtering: def blub():
handlers = [...]
consequence_classes = AbstractConsequence.__subclasses__()
qs = AbstractConsequence.objects.filter(slug__in=map(operator.attrgetter("slug"), handlers)).distinct()
for handler in enabled_consequence_handlers:
for ConcreteConsequence in consequence_classes:
qs = ConcreteConsequence.filter_queryset(handler, qs, user)
# ==> rather ditch annotations and OR Q-Objects
return qs
def localConsequence_filter_queryset(handler, qs, user):
return handler.filter_local_queryset(qs, user) # filter for nonlocal or other handler or matches criteria
def federatedConsequence_filter_queryset(handler, qs, user):
return qs.filter(
Q("is not federated") | Q("matches criteria")
) |
ba01663 to
0a61e84
Compare
felixrindt
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.
I sent you a patch on Signal so you can look through my hacks during trying this out.
| if not self.instance and value != AbstractConsequence.States.NEEDS_CONFIRMATION: | ||
| raise serializers.ValidationError( | ||
| _("Consequences must be created in needs_confirmation state") | ||
| ) |
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 think this belongs in the concrete federated code, because in an normal API we would want to serialize consequences in any state?!
| queryset = WorkingHours.objects.all() | ||
|
|
||
|
|
||
| class ConsequenceViewSet(viewsets.ModelViewSet): |
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.
Again, this is specifically for federated consequences, the name should reflect that. Can it be moved to the plugin?
| def filter_editable_by_user(cls, user: UserProfile): | ||
| return Q(slug=cls.slug) & Q( | ||
| # Qualifications can be granted by people who... | ||
| | Q( # are responsible for the event the consequence originated from, if applicable | ||
| event_id__in=get_objects_for_user(user, perms="change_event", klass=Event), | ||
| Q( # are responsible for the event the consequence originated from, if applicable | ||
| data__event_id__in=get_objects_for_user(user, perms="change_event", klass=Event), | ||
| ) | ||
| | Q( # can edit the affected user anyway | ||
| user__in=get_objects_for_user( | ||
| localconsequence__user__in=get_objects_for_user( | ||
| user, perms="change_userprofile", klass=get_user_model() | ||
| ) | ||
| ) |
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.
The new version doesn't work for me. Postgres complains about having to implicitly typecast somewhere. I think it might be the json values in list checks.
| phone = models.CharField(_("phone number"), max_length=254, blank=True) | ||
| qualifications = models.ManyToManyField(Qualification) | ||
| federated_instance = models.ForeignKey(FederatedGuest, on_delete=models.CASCADE) | ||
| federated_instance_identifier = models.CharField(null=True, blank=True, max_length=255) |
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 is filled anywhere?
| if event_title: | ||
| s = _("{user} acquires '{qualification}' after participating in {event}.").format( | ||
| user=user, qualification=qualification_title, event=event_title | ||
| s = _("acquires '{qualification}' after participating in {event}.").format( |
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.
atm this is just "deleted event" when coming from a federated instance
closes #1501