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

Notifications: email about DONT_SHALLOW_CLONE deprecated feature flag #10463

Closed
wants to merge 4 commits into from

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 22, 2023

This PR prepare an email notification to be sent to users with projects using this feature flag. We use a similar pattern than the one used for the deprecation of the configuration file: 1 email per user listing all their projects affected 1.

There are set of different changes in this PR (each set has it's own commit to make it easier to review):

  • Expand the contact_owners.py management command
    • Follow the convention we have in other notifications regarding the context: us production_uri instead of domain
    • Add all the projects for a particular user to the context, so we can show actions specific to those projects
    • Use the internal get_context_data() and extra_context= to follow the pattern stablished for the Notification class.
  • Add a template filter to check if a Project has a feature flag or not
  • Write an initial email to be reviewed by the team

Preview

Screenshot_2023-06-22_18-19-48

Related: #9779

Footnotes

  1. I'd like to standardize this pattern in a generic way somehow, but I don't want to deviate too much right, so I made some small changes to allow me to do this in a simple way for now.

humitos added 4 commits June 22, 2023 17:10
- Follow the convention we have in other notifications regarind the context: use
  `production_uri` instead of `domain
- Add all the `projects` for a particular user to the context, so we can show
  actions specific to those projects
- Use the internal `get_context_data()` and `extra_context=` to follow the
  pattern stablished for the `Notification` class.

Besides, make the new linter to pass.
`structlog` give us a better experience here.
I'll be using this filter to send emails to projects with a particular feature flag.
I'm commiting this to the repository so we we do review and we have some history
about how we send emails. I had a hard time today figure it out how we used this
in the past.
@humitos humitos requested a review from ericholscher June 22, 2023 16:18
@humitos humitos requested a review from a team as a code owner June 22, 2023 16:18
@stsewd
Copy link
Member

stsewd commented Jun 22, 2023

I think we should stop trying to adapt the command each time we want to send an email, and just call it from a script, as mentioned in #9882 (comment).

@humitos
Copy link
Member Author

humitos commented Jun 22, 2023

Is expanding the script a problem? This PR does not make breaking changes on the script; it just add functionality. This also helps to keep the script maintained and working when we need it.

@stsewd
Copy link
Member

stsewd commented Jun 22, 2023

This is injecting all projects by default in the context, and adding a filter, which shouldn't be required for all types of emails/notifications. This can be just a custom context function, with all the proper projects already filtered.

@ericholscher
Copy link
Member

ericholscher commented Jun 22, 2023

I tend to agree... it seems like if we're not going to be sending these emails in an automated fashion in the application, just having a script that we run to send them is a lot simpler, and doesn't require a deploy or code changes. It would make it much easier to iteratively send notifications to each of our feature flag users in a pretty quick way, and we could keep the scripts in VCS if we wanted to review/version control them. I don't feel super strongly, though, but I do see how requiring deploys makes it more annoying to ship these notifications.

@humitos
Copy link
Member Author

humitos commented Jun 26, 2023

OK. I'm closing this PR and I will create a different script. I opened #10470 with the changes required to follow the conventions we have and keep consistency.

@humitos humitos closed this Jun 26, 2023
@stsewd stsewd deleted the humitos/notifications-management-cmd branch June 26, 2023 16:41
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