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.

Sign up for GitHub

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

Jump to bottom

Report reminders #3990

Merged
merged 7 commits into from
Nov 1, 2024
Merged

Report reminders #3990

merged 7 commits into from
Nov 1, 2024

Conversation

bickelj
Copy link
Contributor

@bickelj bickelj commented Jun 14, 2024

Fixes #3989

Test Steps

Set up a fake SMTP server or use file-based email.

  • Configure reports for projects in Wagtail Admin
  • See that report reminders go out as configured

@bickelj bickelj requested a review from frjo July 30, 2024 16:48
@bickelj
Copy link
Contributor Author

bickelj commented Jul 30, 2024

@frjo Do you think this one is OK to merge?

@frjo
Copy link
Contributor

frjo commented Jul 31, 2024

Did a rebase and migration deconflict.

@frjo
Copy link
Contributor

frjo commented Jul 31, 2024

@bickelj Am I correct that this PR makes the following changes and additions to current functionality:

  1. Move the remind before x days option from a command argument to Wagtail settings.
  2. Allow to remind x days after or before report due date.
  3. Allow to have multiple reminders with only one cron job.

frjo
frjo requested changes Jul 31, 2024
View reviewed changes
hypha/apply/projects/models/project.py Outdated Show resolved Hide resolved
hypha/apply/projects/models/project.py Outdated Show resolved Hide resolved
@bickelj
Copy link
Contributor Author

bickelj commented Aug 8, 2024 •
edited
Loading

@frjo said:

Am I correct that this PR makes the following changes and additions to current functionality:

1. Move the remind before x days option from a command argument to Wagtail settings.

I think the the setup defaulted to some days before and monthly, but you could vary the settings a bit on an individual project after creation. So I don't think there was a way you could change that overall behavior. I don't know about a command argument. I see what you mean about hypha/apply/projects/management/commands/notify_report_due.py but I haven't tried that.

2. Allow to remind x days after or before report due date.

Yes, and let these settings apply to all new projects.

3. Allow to have multiple reminders with only one cron job.

I think so, yes.

@frankduncan did I get that right?

@frjo
Copy link
Contributor

frjo commented Aug 9, 2024

@bickelj If all titlecase can be changed to sentence case I can put this on test.

@bickelj
Copy link
Contributor Author

bickelj commented Aug 9, 2024

@frjo I think I got all the places in the latest push, let me know if I didn't!

@bickelj bickelj requested a review from frjo August 9, 2024 17:41
frjo
frjo requested changes Aug 10, 2024
View reviewed changes
Copy link
Contributor

@frjo frjo left a comment

Choose a reason for hiding this comment

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

Would also be good to add some documentation around this.

Otherwise it seems to be working very nicely.

hypha/apply/projects/models/project.py Show resolved Hide resolved
@frjo
Copy link
Contributor

frjo commented Aug 13, 2024

We have a migration numbering conflict with another PR on test but as soon as that is approved I will put this on test.

@frjo frjo added the Status: RTBC Internal Dev use only label Aug 13, 2024
@theskumar
Copy link
Member

This PR look neat!

@frjo @bickelj I think it would really help if we can add a how to guide in the docs on how to enable and setup the report reminders.

Probably at https://docs.hypha.app/setup/administrators/setup-notifications/

@theskumar
Copy link
Member

I don't think needs dedicated guide, but if it has to it will look something like this: https://docs.hypha.app/setup/administrators/setup-error-performance-monitoring/

@frjo frjo added Type: Feature This is something new (not an enhancement of an existing thing). Type: Minor Minor change, used in release drafter labels Aug 27, 2024
@bickelj
Copy link
Contributor Author

bickelj commented Aug 30, 2024

@theskumar I added some documentation in the most recent commit. Let me know what you think.

bickelj
bickelj commented Aug 30, 2024
View reviewed changes
docs/setup/administrators/setup-notifications.md

For information on configuring Slack notifications for Hypha, see the [Slack section](configuration.md/#slack-settings) of the configuration reference.

## Project report reminders
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want this to appear as if it were alike to "Email, Slack, ..." so I moved those into their own section before adding this one.

@bickelj
Copy link
Contributor Author

bickelj commented Oct 8, 2024

@frjo @theskumar Can you take a look at the documentation and see if it looks OK? Do you think these report reminders can be merged in the next iteration?

frankduncan and others added 5 commits October 18, 2024 10:54
@frankduncan @frjo
Add report reminder frequency
83878e2 
Adds a setting to the project settings for report reminders

Issue #3989
@frankduncan @frjo
Use new report frequency settings in notify_report_due command
49f4faf 
Instead of using a passed in variable, use the settings so that the
command can be configured in wagtail.

Issue #3989
@frjo
Fix migration conflict after rebase.
e955600
@bickelj @frjo
Clean up some variables and text
7fe8af0 
Issue #3989
@bickelj @frjo
Add a tiny bit of documentation
1b678dc 
Issue #3989
@frjo frjo added Status: Needs testing Tickets that need testing/qa Status: Needs dev testing 🧑‍💻 Tasks that should be tested by the dev team and removed Status: RTBC Internal Dev use only labels Oct 20, 2024
@frjo frjo mentioned this pull request Oct 20, 2024
Open
2 tasks
@wes-otf
Copy link
Contributor

wes-otf commented Oct 31, 2024

I've got a test report setup to be due saturday so should be able to merge this tomorrow assuming all notifies right

@wes-otf wes-otf added Status: Tested - approved for live ✅ and removed Status: Needs testing Tickets that need testing/qa Status: Needs dev testing 🧑‍💻 Tasks that should be tested by the dev team labels Nov 1, 2024
@wes-otf
Copy link
Contributor

wes-otf commented Nov 1, 2024

worked well - thanks @bickelj!

@frjo frjo merged commit 05f2a43 into main Nov 1, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wes-otf wes-otf wes-otf approved these changes

@frjo frjo frjo approved these changes

Assignees
No one assigned
Labels
Status: Tested - approved for live ✅ Type: Feature This is something new (not an enhancement of an existing thing). Type: Minor Minor change, used in release drafter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configurable project report reminders
5 participants
@bickelj @frjo @theskumar @wes-otf @frankduncan