Skip to content
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

Slide reminder #113

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

detunjiSamuel
Copy link

@detunjiSamuel detunjiSamuel commented Mar 3, 2023

Changes made #110

  • Added field to conference model -slide_reminder_days
  • Added slides reminder task to conference reminder job
  • Added new mail(speker_remind_slides Template to be sent

@detunjiSamuel detunjiSamuel changed the title Slide reminder #110 Slide reminder Mar 3, 2023
@mhagander
Copy link
Member

Without having done a deep review of the code, a couple of quick commets:

  1. Why does this need it's own scheduled job, when we already have a generic one for reminders?
  2. The text in the email comes out as it being mandatory to upload slides. For all events I have been involved in it's been optional, and that usecase definitely needs to be covered. But I can see wanting to cover both, in which case that needs to be configurable.
  3. Won't this code keep sending reminders once the deadline is passed, once every time the job is run, for ever and ever?
  4. The queries for if it should run or not look exremely weird. You get all conferences, then do an extra subquery against all conferences that have reminders, and then in the python code verify the result of the subquery against the result of the main query? Surely this is just a super-trivial one-level query?
  5. The cancel_upload_reminder part looks very strange. As soon as a single speaker has uploaded a single slide, you turn off the reminder. And you do so by reconfiguring the entire conference? Apart from that logic being strange, a background job should never change the actual configuration of the conference.
  6. (You've also accidentally left a debugging print statement in there)

@andreasscherbaum
Copy link
Member

The text in the email comes out as it being mandatory to upload slides

I don't think any of our own conferences make it mandatory to upload slides.

Now that's something other conferences might want, but in this case there should be at least two config options:

  1. Uploading slides is mandatory
  2. Uploading slides is required before the conference starts (review and such)

I'd consider this a different enhancement, and so far not a priority one.

Let's focus on the regular reminder instead.

@detunjiSamuel
Copy link
Author

Without having done a deep review of the code, a couple of quick commets:

  1. Why does this need it's own scheduled job, when we already have a generic one for reminders?
  2. The text in the email comes out as it being mandatory to upload slides. For all events I have been involved in it's been optional, and that usecase definitely needs to be covered. But I can see wanting to cover both, in which case that needs to be configurable.
  3. Won't this code keep sending reminders once the deadline is passed, once every time the job is run, for ever and ever?
  4. The queries for if it should run or not look exremely weird. You get all conferences, then do an extra subquery against all conferences that have reminders, and then in the python code verify the result of the subquery against the result of the main query? Surely this is just a super-trivial one-level query?
  5. The cancel_upload_reminder part looks very strange. As soon as a single speaker has uploaded a single slide, you turn off the reminder. And you do so by reconfiguring the entire conference? Apart from that logic being strange, a background job should never change the actual configuration of the conference.
  6. (You've also accidentally left a debugging print statement in there)

will make changes to address these in the next set of commits. Thank you

@detunjiSamuel
Copy link
Author

detunjiSamuel commented Mar 6, 2023

@mhagander I have made changes to meet the points you mentioned earlier

Copy link
Member

@mhagander mhagander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick look (need a more detailed one), but doesn't this one do it wrong if you atually set the value to 0 in the config? If you do that, it will still send reminders because of it?

Also, doesn't modifying the lastmodified field risk interefering with the other reminders?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants