Split up deprecated view notification to GitHub and other webhook endpoints #5083
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user nomenclature. * Add small amount of docs to point to
…/readthedocs.org into humitos/builds/notify-old-endpoints
…points This sets a date for deprecated of these endpoints as Mar 1st 2019. Too soon? * Reduce complexity and drop decorator pattern for Notification classmethod pattern used in other notifications * Add notifications for non-GitHub incoming webhooks * Add docs as well
humitos
reviewed
Jan 9, 2019
docs/webhooks.rst
Outdated
|
|
||
| .. _webhook-deprecated-endpoints: | ||
|
|
||
| I was warned that my project won't automatically build after Mar 1st |
There was a problem hiding this comment.
I would use the full month name and year here: March, 2019
|
Yeah, i was considering longer, 3 months seems reasonable |
Found out 2x messages are being generated, so this stops the automated mechanism for triggering these messages.
humitos
reviewed
Jan 10, 2019
humitos
reviewed
Jan 10, 2019
readthedocs/core/views/hooks.py
Outdated
| @@ -225,9 +221,6 @@ def github_build(request): # noqa: D205 | |||
| branches | |||
| ) | |||
| projects = repo_projects | ssh_projects | |||
| # TODO remove _build_url call and replace with a 4xx response after | |||
| # Jan 31st. | |||
| DeprecatedGitHubWebhookNotification.for_project_users(projects) | |||
There was a problem hiding this comment.
If we remove this from here, how is the idea to notify these users?
There was a problem hiding this comment.
Manually for now, we need this out soon. We can probably just keep this easy and ditch automating, and just do 2 or 3 scheduled emails instead.
humitos
reviewed
Jan 10, 2019
There was a problem hiding this comment.
I think we are close.
We were discussing about "Why the notification was shown as a Site Notification" and I think it's because we are not marking it as read=True as was my original idea (IIRC).
Also, by removing the notification from the webhook, we not warning users at all. So, I think that will notify them manually with a different script.
|
Already discussed in slack, but the message automation was creating more than one notification, as we were adding a site notification object to track the email notification. Manual notification is not ideal, but I feel like the addition Message instances for email notifications is creating some debt around on site messages we should avoid. If we think that we want to deduplicate email messages in the same way we deduplicate site messages, perhaps we can look into adding our own message storage and message storage db tables. I view this as low priority work however. |
agjohnson
pushed a commit
that referenced
this pull request
Jan 11, 2019
* Notify users about the usage of deprecated webhooks Each time a deprecated webhook is hit, a notification is created (without duplicating it) to be sent. * Extend notification to support de-dup and delayed email sent * Improve decorator to support generic and specific VCS webhook views * Remove no necessary settings * DeprecatedWebhookEndpointNotification tests and improvements * Better docstring * Lint * Update copy on notifications for github services deprecation (#5067) * Updated copy on webhooks * Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user nomenclature. * Add small amount of docs to point to * Update docs and point to docs in notification message * Add year * Split up deprecated view notification to GitHub and other webhook endpoints (#5083) * Updated copy on webhooks * Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user nomenclature. * Add small amount of docs to point to * Update docs and point to docs in notification message * Split up deprecated view notification to GitHub and other webhook endpoints This sets a date for deprecated of these endpoints as Mar 1st 2019. Too soon? * Reduce complexity and drop decorator pattern for Notification classmethod pattern used in other notifications * Add notifications for non-GitHub incoming webhooks * Add docs as well * More renaming and slight refactor Found out 2x messages are being generated, so this stops the automated mechanism for triggering these messages. * Update dates * Also update docs * Typo on date * Back out some more of the changes to notifications to make them operable without automation * Add admin method for notification * Add admin filter for project features
agjohnson
pushed a commit
that referenced
this pull request
Jan 11, 2019
* Notify users about the usage of deprecated webhooks Each time a deprecated webhook is hit, a notification is created (without duplicating it) to be sent. * Extend notification to support de-dup and delayed email sent * Improve decorator to support generic and specific VCS webhook views * Remove no necessary settings * DeprecatedWebhookEndpointNotification tests and improvements * Better docstring * Lint * Update copy on notifications for github services deprecation (#5067) * Updated copy on webhooks * Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user nomenclature. * Add small amount of docs to point to * Update docs and point to docs in notification message * Add year * Split up deprecated view notification to GitHub and other webhook endpoints (#5083) * Updated copy on webhooks * Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user nomenclature. * Add small amount of docs to point to * Update docs and point to docs in notification message * Split up deprecated view notification to GitHub and other webhook endpoints This sets a date for deprecated of these endpoints as Mar 1st 2019. Too soon? * Reduce complexity and drop decorator pattern for Notification classmethod pattern used in other notifications * Add notifications for non-GitHub incoming webhooks * Add docs as well * More renaming and slight refactor Found out 2x messages are being generated, so this stops the automated mechanism for triggering these messages. * Update dates * Also update docs * Typo on date * Back out some more of the changes to notifications to make them operable without automation * Add admin method for notification * Add admin filter for project features
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This sets a date for deprecated of these endpoints as Mar 1st 2019. Is this too soon?