-
Notifications
You must be signed in to change notification settings - Fork 58
Add bulk unenrol functionality #261
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
| . get_string('cancellationsentmgr', 'facetoface'); | ||
|
|
||
| // Handle bulk signup cancellation | ||
| if (!empty($fromform->f)) { |
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.
It looks like this conditional does not include the logging and event triggers.
golenkovm
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.
Thanks @owenherbert-catalyst
My main concern is the location of the Button to Cancel all bookings. I believe it should be redered once, same as Sign-up for stream, above the session list. Required/optional params for cancelsignup.php will have to be revisited as well.
One more minor thing is that the testing instructions seem to have missing the step where Signup type is set to Multiple.
Cheers,
Misha
|
Hi @golenkovm ,
Can I please get some clarification for this point |
|
Heads up I fixed the existing codechecker issues in a different PR, so you should be able to get the CI green now 🟢 |
|
Thanks @owenherbert-catalyst Please find a few edge cases I found:
Regarding button location:
Cheers, |
|
Hi @golenkovm , I've taken another look through this and have addressed the points you raised:
|
|
Thanks @owenherbert-catalyst Gave it a test and in general it looks good. The only thing I picked up is that the button is being rendered even if Signup type set to single. I'm kinda on the fence, but it feels a bit redundant. If we set Signup type to Single we don't render Sign-up for stream button and therefore shouldn't render "Cancel all bookings" one. Let me know your thoughts. Cheers, |
|
Hi @golenkovm , I'm on the fence too. I think we should render it, it makes debugging easier and also adds a quick and repeatable action for students. |
264b2af to
af805f1
Compare
|
Sorry, I might confused you in our internal comms. What I actually meant is #261 (comment) - If we set Signup type to Single we shouldn't render "Cancel all bookings". With the latest patch when I set Can you please tweak the patch so Thanks, |
|
Hi @golenkovm , I've just pushed and tested those changes on my environment and they now meet those conditions. |
golenkovm
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.
Thanks @owenherbert-catalyst merging

This merge requests adds
bulk unenrol from streamfunctionality to themod_facetofaceplugin. The current plugin implementation only allows a user to cancel a signup one by one.Other MR: #260
Preview:

What's changed?
cancel all bookingsbutton.Testing instructions: