-
Notifications
You must be signed in to change notification settings - Fork 202
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
Add management command to send api move announcement email #4229
Add management command to send api move announcement email #4229
Conversation
Upgrading this to high priority so that we can send it out early next week along with the make post on Monday. #2037 for context. @WordPress/openverse-api pinging y'all for expedited review to match the priority. |
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.
I was under the impression that we've sent bulk emails before and there was an existing management command to send out emails. I think I was wrong about that.
I think it's worth generalising this command to send any email, and keep the message as a separate file that can be passed to the command via CLI args. This way we can send emails in the future, with the same command and different content.
However, I'm okay with taking up this idea^ as a future improvement. This PR LGTM and I'm unblocking these except for some ideas to improve the message text.
|
||
For the best experience, please update code referencing api.openverse.engineering to use api.openverse.org. | ||
|
||
To read more about this change, including opportunities to ask questions or give feedback, please read the Make Openverse blog post announcing it: |
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 line should be wrapped like the others.
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.
LGTM, tests are great. +1 to @dhruvkb's suggestions for the message text but approving to unblock.
I like the suggestion to make the command more general but strongly agree that should be a future iteration 👍
The issue references the old management command that we sent an email with before: #3742 A general one is out of scope, as you've both said, but I want to share further context, as I don't believe we even need one. We've sent two emails in as many years, and the first was extremely specialised in the selection of who received it. It's hard to imagine it would be worth the time to write, test, and maintain a generalised management command when this one can be copy/pasted (and crucially in the meantime deleted, and not kept as unused code for who knows how long) and whatever modifications made as needed. Without knowing who the audience of a particular email is, you'd also need to work out how to specify that, and I'd really rather not have to dredge up memories of dealing with CSVs of SQL-exported email addresses in Django admin. |
Agree on both points: generalising is out of scope for this PR and this code can be deleted when unused (at 2 emails in ~3 years this is basically 100% of the time). |
Because application emails are not unique, there is a chance of accidentally emailing people twice, if their multiple applications registered to a single email are pulled; that is, there is a chance of this if the @dhruvkb and @stacimc Just an FYI that I did ask for editorial reviews from everyone in the maintainer group before marking this PR ready for review; ideally these editorial remarks would have been shared there, as some of them overlap with content in the Make post. Just giving context for where the text came from, and the process it's already been through. I appreciate the editorial feedback, but it would have been good to have it when the text was originally discussed, so that it could align with the Make post. |
Co-authored-by: Dhruv Bhanushali <hi@dhruvkb.dev>
85d4f95
to
147b056
Compare
The emails are sent as of 2024-05-08T00:59:58.366Z. |
Fixes
Part of #3742 by @sarayourfriend (does not fix because the issue also requires sending the email, i.e., running the command in production).
Description
Review the management command. The message text has already undergone review. The placeholder for the make post URL will be replaced before merging, after the post is available next week.
Testing Instructions
Review the code and confirm tests are adequate and pass.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin