Refactor Signup Method: Signup Flow and Shift Structure #1202
Conversation
felixrindt
commented
Feb 9, 2024
•
felixrindt
commented
- QualificatioMixStructure
- race condition
- what happens when deleting a Qualification configured in the shift structures
- check PDF export
- update tests to work again
- add new tests
- update plugin dev docs
- update user docs and make sure it's linked correctly
- profiling and performance improvements
- display flow/structure descriptions
- migration
- delete old plugin
felixrindt
added
[C] enhancement
felixrindt
force-pushed
the
signup-flow-refactor
branch
from
ba6a74c
to
91c62c1
Compare
felixrindt
force-pushed
the
signup-flow-refactor
branch
from
58109db
to
849e582
Compare
felixrindt
force-pushed
the
signup-flow-refactor
branch
from
33bc821
to
9cf1be2
Compare
felixrindt
force-pushed
the
signup-flow-refactor
branch
from
ca1497a
to
268b8fe
Compare
felixrindt
force-pushed
the
signup-flow-refactor
branch
3 times, most recently
from
ce6d367
to
ff18ad2
Compare
felixrindt
force-pushed
the
signup-flow-refactor
branch
from
1867d01
to
9c2ad88
Compare
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class BaseSignupFlowConfigurationForm(SignupConfigurationForm): |
There was a problem hiding this comment.
I added those to have a common base class for the Flow Config Forms, although they aren't any different from the common SignupCOnfigurationForm
There was a problem hiding this comment.
i see.
i generally feel that the (class) structure of the code became a lot more complicated and is harder to understand. I think this is caused by a lot of classes (with empty ones like this) and very similar names (base and basic) that are hard to tell apart. maybe we can have a chat about whether we can improve on that
| from ephios.core.signup.flow.participant_validation import NoSignupSignupActionValidator | ||
|
|
||
|
|
||
| class ManualSignupActionValidator(NoSignupSignupActionValidator): |
There was a problem hiding this comment.
how is this different from NoSignupSignupActionValidator?
There was a problem hiding this comment.
Manual and Coupled both use that superclass but customize the message.
There was a problem hiding this comment.
couldn't that be part of the NoSignupSignupActionValidator? that already defines a message which feels kinda doubled
|
|
||
| class InstantConfirmSignupFlow(BaseSignupFlow): | ||
| slug = "instant_confirmation" | ||
| verbose_name = _("Direct signup") |
There was a problem hiding this comment.
I'm not sure whether it is clear what "direct" means here
There was a problem hiding this comment.
Please suggest a better name. This is hard 🙈
There was a problem hiding this comment.
i still like instant confirmation as it tells exactly what the user can expect to happen
| @property | ||
| def slug(self): | ||
| """ | ||
| A unique identifier for this signup method. |
There was a problem hiding this comment.
Yeah I got to search for mentions of signup_method and replace it in some more places...
| @@ -0,0 +1,116 @@ | |||
| from django import forms | |||
There was a problem hiding this comment.
why are the built-in flows in core but the structures in plugins?
There was a problem hiding this comment.
It's similar to what we had before, but I agree they could both be a plugin, or both in core. I think because they can be a plugin, it should be a plugin, but also because they provide a lot of the base implementation useful when building custom flows and structures, they should be in core.
I'm more leaning towards the plugin though. What do you think?
There was a problem hiding this comment.
would prefer both the same way regardless of what we decide on
I experienced multiple instances where people where confused why they couldn't add events and why they needed to enable such basic functionality upfront. Also it rarely makes sense to disable the builtin flows/structures, even if there are custom ones. so i would lean towards core, but the problem probably could be fixed by better pre-activating the builtin flows/methods
| def get_checkers(self): | ||
| def check_maximum_number_of_participants(shift, participant): | ||
| if ( | ||
| not shift.signup_flow.uses_requested_state |
There was a problem hiding this comment.
maybe we should make that configurable (happily as a followup)
| ^^^^^^^^^^^ | ||
|
|
||
| Configure named teams with individual min/max counts and qualification requirement to | ||
| sort participants into. Participants can even be asked for their preferred team. |
There was a problem hiding this comment.
maybe an example to better understand what this can be used for?
ephios/core/services/matching.py
Outdated
| ) | ||
| if not position.required_skill <= participant_skill: | ||
| # the participant does not have some required skill | ||
| return -1 # avoid 0 as that causes trouble with the matching lib |
There was a problem hiding this comment.
@BenBals thinks that it can occur that the algorithm will prefer a matching with someone unqualified because it guarantees that the matching fulfills \lvert M \rvert = \min(\lvert U \rvert, \lvert V \rvert),
There was a problem hiding this comment.
Can he provide an example where it breaks? When we thought through it we came to the conclusion it shouldn't happen, no proof obvsl...
There was a problem hiding this comment.
I adjusted the scoring so the case Ben constructed should be handled correctly (added a test too).
There was a problem hiding this comment.
At first glance, this looks like it should work.
There was a problem hiding this comment.
Thanks for having a look. When thinking about the complex matching I might look into the solvers you mentioned 🙈
felixrindt
force-pushed
the
signup-flow-refactor
branch
from
28ebcc7
to
bf14865
Compare
| @@ -0,0 +1,43 @@ | |||
| # Generated by Django 5.0.3 on 2024-03-25 18:45 | |||
There was a problem hiding this comment.
i don't think that belongs here
