-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
4486 email pray and pay #4498
4486 email pray and pay #4498
Conversation
for more information, see https://pre-commit.ci
Some progress has been made on the email functionality. I looked at
|
Everything looks basically right to me, but I'll let Alberto take a more careful look. |
- Added email test assertions.
@v-anne I've applied some tweaks to the PR so that email sending works properly. One of the changes is to use email bulk sending, allowing us to send one email per recipient instead of using BCC. I think this is better due to AWS restrictions of 50 recipients when using the BCC or CC fields: This way, even if we have more than 50 users who need to be notified after a prayer is granted, all of them can receive the message.
@mlissner I think the only thing left to confirm is whether the email template is complete or if it requires some tweaks. Currently, the emails look as follows: Text version:
There are some CI actions failing related to authentication. I believe it can be something temporary. I'll retry them later. |
I made a little template that cribs from the alert email: https://docs.google.com/document/d/1jGjdZPueFXRHILqgI3Pp606EzrEaxvNTkcLdWNJdHTs/edit?usp=sharing I think it should be pretty easy, but some parts of it might be tricky and it probably isn't worth spending a ton of time on all of it. So consider it a notion, but hopefully one that gives some direction! Thank you both! |
@mlissner, I left some comments on your Google doc. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ourtlistener into 4486-email-pray-and-pay fixed
I added three things as you suggested in the Google Doc:
Despite our initial hesitance about the date requested component, I decided to include it. I think limiting the query for dates to the QuerySet described below makes this relatively efficient and is a minor use of resources. If you disagree, I'm happy to remove it. open_prayers = Prayer.objects.filter(
recap_document=instance, status=Prayer.WAITING
).select_related("user") |
I attempted adding the case name and court number to the alerts. I think the case name works but the court name doesn't. I couldn't quite figure out how Otherwise, I think this is almost ready to merge, after one more review. |
You just have to follow the FK chain. So if you have a RECAPDocument, the full chain would be:
I think the correct field on Court is |
- Added additional test assertions
I've tweaked the templates and email context to make them look more like the email template Mike shared. Added additional test assertions. Email subject remains as:
|
I think this is good to go. Thank you for your work on this, Alberto! |
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.
One quick comment from me, but Alberto, I leave it to you for full review.
Thank you @v-anne for sticking with this big feature! |
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.
In addition to Mike's suggestion, I would also recommend moving prayer_email.html
and prayer_email.txt
to favorites/templates
.
With those changes, I think this is good to go.
Thanks @v-anne
Both of your comments have now been resolved. |
Thanks LGTM! |
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.
Very cool. Thank you both.
@v-anne, deployments are blocked atm, but this will go live some time tomorrow, I expect.
Onward to the last pieces!
This PR is to add email alerts to #1346, the pray and pay project. It follows up on #4386, which implemented much of the logic for the backend in terms of the
Prayer
model.The logic for the email alerts is as follows:
When a document is confirmed to be available on CourtListener, the backend will identify whether any users had requested purchase of that document, and if so, will send all of them an email together informing them of its availability using BCC. The
Prayer
model will be updated to confirm that the document is now available, which will remove the document from the publicly available display of open requests.