-
Notifications
You must be signed in to change notification settings - Fork 20
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
Events: auto reject registration applications to event after the event ends #1934
base: dev
Are you sure you want to change the base?
Events: auto reject registration applications to event after the event ends #1934
Conversation
b868334
to
5c0fba4
Compare
a90f24a
to
154549e
Compare
For auto rejecting registration application after an event is over, like MIT-LCP#1906 a management command which will be executed through cronjob seems like a good idea.
Added management command to reject pending registration applications for events that have ended. an email notification is sent to the applicant.
Like other cron job, the idea here is that we dont want to overwhelm the resources so the cron jobs will run everyhour for a small number of applications
154549e
to
7e550fb
Compare
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.
@tompollard @alistairewj I'm leaving a few comments on this PR - in my opinion this still requires some thought and is not mergeable.
Right now it looks a bit wonky in my opinion - some cron job processes every event and rejects some fixed number of participants (not sure why it's limited this way). If we wanted to make something else happen when an event ends (for example remove the access grant from it's participants assuming we use it to manage access. Just an example) then we'd need to add another cron job, with more logic etc. or rework the current command into something that handles everything related to events ending. The command would keep growing and growing with new functionality.
In general we'd probably like to be able to hook when certain things happen. E.g. whenever an event ends, it should send a event_ended
message to some listeners which would trigger their specific logic. This would decouple the source of the message (i.e. when it happens) from what happens. So adding new handlers would be trivial. This is the pub/sub approach but it's completely absent from the codebase at the moment so it's a question of how it would fit in here.
We can try to simulate it without introducing any new architecture by scheduling a background_task
to execute at the date when the event is ending (rescheduling it whenever the date is changed and making sure we check whether it ended in the task logic).
total_applications_per_event_to_reject = (options['number'] | ||
or settings.DEFAULT_NUMBER_OF_APPLICATIONS_TO_REJECT_PER_EVENT) | ||
past_events = Event.objects.filter( | ||
end_date__lt=timezone.now(), |
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 seems like it would introduce a lot of redundancy - it will fetch all the events that were finished in the past, even if this command already processed them. Intuitively, this is something that should happen once per event. Fetching events from years ago makes no sense for this command.
if not settings.ENABLE_EVENT_REGISTRATION_AUTO_REJECTION: | ||
LOGGER.info('Auto rejection of event applications is disabled.') |
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.
If it's disabled then, in my opinion, this command shouldn't even be called. Also, there is no point in logging a string every time the cron job is executed.
applications = event.applications.filter(status=EventApplication.EventApplicationStatus.WAITLISTED)[ | ||
:total_applications_per_event_to_reject | ||
] |
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.
Batching the rejections this way doesn't make a lot of sense to me. Doing a bulk_update
(one query) and sending a mass email would take care of the entire thing at once.
Context
On Events, if the Event Host doesn't respond to participation request for whatever reason and the event ends, the participation requests will still be pending and the participations wont have received any response.
we should be closing all the participation applications after the event has ended automatically(in cases where the event host doesnot do that)
On this PR, i added a management commands which will automatically reject applications for events once the event ends and informs the participants. Just like #1906, this feature is configurable and can be turned off(is turned off by default) with .env variables. Similarly, by default the management command will only reject 5 applications per event at a time. The cronjob is set to trigger management command every hour.