Allow individuals to modify registration details past the modification deadline #6152
Conversation
tomasr8
left a comment
tomasr8
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left you some comments :)
indico/modules/events/registration/controllers/management/reglists.py
Outdated
Show resolved
Hide resolved
Thanhphan1147
requested review from
GovernmentPlates,
ThiefMaster and
tomasr8
indico/modules/events/registration/controllers/management/reglists.py
Outdated
Show resolved
Hide resolved
kewisch
left a comment
kewisch
left a comment
There was a problem hiding this comment.
I discussed this with Thanh internally, wanted to relay our conversation here: For the allow/update UI, maybe there is a chance for simplification. Here is what I am thinking:
- Default state as you posted, allow modification button to get started
- Modal allows you to set a date as you do, the date is set - client side - to 1 week from now (this covers the most common use case, short modification allowance). More on the modal later.
- When an exceptional deadline is set, the button is not "close modification" and no update link, but rather one edit button that simply opens the modal again.
- The modal also allows you to delete the date (either a separate button or simply the X), which when saved will reset the exceptional modification to None so it falls back to the form settings.
This way we don't need the somewhat small (update) link, and there is just one button to concentrate on. We wouldn't need a default date in the backend, and could maybe even use the same controller for both set and disable the exception deadline, just nulling it out for the latter. We might have to do some validation to ensure the deadline is not in the past, potentially client side, but that should be it.
Thanh mentioned the delete button was to reduce clicks in case a manager needs to change a number of registrations, which is a good point. My suggestion was to see how two buttons side by side might look. It did bring up the question, is there a way to just clear out all the exceptions, in case a manager really wants to close modifications. Worth looking into maybe adding some UI in the registration settings.
indico/modules/events/registration/controllers/management/reglists.py
Outdated
Show resolved
Hide resolved
indico/modules/events/registration/controllers/management/reglists.py
Outdated
Show resolved
Hide resolved
indico/modules/events/registration/templates/management/_registration_details.html
Outdated
Show resolved
Hide resolved
|
Thanks everyone for the helpful reviews. I have added a commit simplifying the button labels, changed the naming of db column and class names, renamed relevant urls, as well as reworked the UI now with 2 buttons to edit the deadline or to close it. Therefore I'll allow myself to close all of the ongoing comments and ask for another round of review! |
Thanhphan1147
requested review from
GovernmentPlates,
ThiefMaster and
kewisch
|
Please avoid merge commits and rebase onto the latest mater instead. This is much easier for use to deal with when testing (we like to rebase to the latest master in case the branch is a bit outdated again by the time we review an test). Also some general advice for future contributions: Do not use your own |
|
I see, got it, thanks for the tip. I'll make sure to keep that in mind the next time around! |
|
Hi @ThiefMaster, just to avoid any confusion, is there anything else I need to do from my side to update the PR? If not it'd be nice if you can take a look to see if everything is ok. Thanks! |
|
I still need to properly review and test the latest version. The only thing you could do in the meantime is getting rid of that merge commit and rebasing it on top of the latest master. |
|
I think something went wrong when you rebased (you pushed an empty branch). After force-pushing your previous version you should be able to reopen it though. |
|
I rebased the PR onto the latest master and also updated the PR's description with images of updated UI elements. |
Thanhphan1147
force-pushed
the
master
branch
from
8671425 to
31f089a
Compare
ThiefMaster
force-pushed
the
master
branch
2 times, most recently
from
75da4a3 to
8d91e6b
Compare
|
I rebased the PR and fixed a few small things I noticed. Will add a few more comments on stuff that needs to be fixed/changed. |
indico/modules/events/registration/controllers/management/reglists.py
Outdated
Show resolved
Hide resolved
indico/modules/events/registration/controllers/management/reglists.py
Outdated
Show resolved
Hide resolved
indico/modules/events/registration/templates/management/_registration_details.html
Outdated
Show resolved
Hide resolved
…l names and use dashes
…ists.py Co-authored-by: Dominic H. <[email protected]>
* Change update link to edit button * Shorten button name * Change db column name to modification_end_dt * Change blueprint naming * Update docstring * Set default time to +1 week * Add flash message
Co-authored-by: Adrian <[email protected]>
…ists.py Co-authored-by: Adrian <[email protected]>
…tration_details.html Co-authored-by: Adrian <[email protected]>
…low on regform level
…s not allow on regform level" This reverts commit 98d1282.
Remove optional in form Update rendering logic
It was quite weird before that we had this split over both models, now it's just in a single place.
|
LGTM, some ideas that may make for a nice future PR:
|
5 participants
Closes #5264
Added a toggle button within the registration management that managers can see to allow for participants to modify their registration as well as a nullable date field form to set the deadline up to which the participant can modify their registration (defaults to 1 week after the current date).
The toggle will take effect only when the global modification deadline is passed and the configuration allows for modification.
Example of UI elements:
