Notify users about the usage of deprecated webhooks #4898

humitos · 2018-11-13T10:43:50Z

Each time a deprecated webhook is hit, a notification is created (without duplicating it) to be sent.

humitos · 2018-11-13T10:51:55Z

readthedocs/notifications/decorators.py

+                request,
+                user,
+            )
+            notification.send()


By calling .send() here we will be sending an email notification each time the endpoint is hit and that's not right.

Since we only have email (send email immediately) and site (make a persistent notification) backends I'm thinking on creating a new called delayed_email which would be a mixture between these two: save the notification in the storage to be sent as an email later.

I think we need here:

create notifications on each endpoint hit

deduplicate them automatically when created

store them somewhere

recover the notification stored (and filter by type deprecated_webhook_endpoint) from a Celery task

send them and remove them from the storage

cc @agjohnson

Either our code or message-extends already does this for us. It de-duplicates messages. I'd have to dig up where this is, but a third backend shouldn't be necessary

Yes, but it's a custom storage https://github.com/rtfd/readthedocs.org/blob/5fb61ac617741dc3c9adcacb64d5e7154deedab2/readthedocs/notifications/storages.py#L28

Since we are not calling storage.add when using the EmailNotification that code is never called, from what I understand. Right?

What I'm proposing is to create a custom backend but use that same storage that already de-duplicate the messages.

Yeah, our custom fallback storage backend:
https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/notifications/storages.py#L20-L80

So instead of using a Django level message type, you use a persistent level message type. As long as the message text matches, the user will only get one message.

agjohnson

Looking good so far! Your implementation is really close to being usable, I think the notification level will solve your issue on duplication. Next, tests!

readthedocs/notifications/decorators.py

readthedocs/projects/notifications.py

agjohnson · 2018-11-13T15:31:55Z

readthedocs/settings/base.py

+            'task': 'readthedocs.projects.tasks.send_deprecated_endpoint_notifications',
+            'schedule': crontab(minute=0, hour='10', day_of_week='monday'),
+            'options': {'queue': 'web'},
+        },


I don't think we need a task here, messages will just go out from our notification system.

agjohnson · 2018-11-13T15:33:11Z

readthedocs/templates/projects/notifications/deprecated_webhook_endpoint_email.html

+
+<p>Once you have done that, you need to go to your <a href="{% url "projects_integrations" project.slug %}">project's Integrations</a> under Read the Docs project's Admin, click integration and then in "Resync webhook" button.</p>
+
+<p>Thanks!</p>


We can probably point users to documentation on this. Maybe i'll put together a doc update and blog post. I'll come back to this copy

humitos · 2018-11-19T10:12:32Z

This PR still need some things to be ready to merge:

decide how to send the emails (Notify users about the usage of deprecated webhooks #4898 (comment))
copy on HTML
blog post

Regarding emails, it seems that you (@agjohnson) have something in mind that I'm not getting. I think what you are suggesting is not possible, actually.

codecov · 2018-11-19T18:33:37Z

Codecov Report

Merging #4898 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4898      +/-   ##
==========================================
+ Coverage    76.8%   76.95%   +0.14%     
==========================================
  Files         158      158              
  Lines        9937    10019      +82     
  Branches     1241     1258      +17     
==========================================
+ Hits         7632     7710      +78     
- Misses       1973     1980       +7     
+ Partials      332      329       -3

Impacted Files	Coverage Δ
readthedocs/core/views/hooks.py	`80.21% <ø> (ø)`	⬆️
readthedocs/notifications/notification.py	`95% <100%> (+0.06%)`	⬆️
readthedocs/notifications/backends.py	`94.73% <100%> (ø)`	⬆️
readthedocs/projects/admin.py	`91.5% <100%> (ø)`	⬆️
readthedocs/projects/notifications.py	`100% <100%> (ø)`	⬆️
readthedocs/builds/views.py	`90.74% <0%> (-5.26%)`	⬇️
readthedocs/core/signals.py	`82% <0%> (-4.67%)`	⬇️
readthedocs/vcs_support/backends/bzr.py	`31.03% <0%> (-1.7%)`	⬇️
readthedocs/doc_builder/constants.py	`86.36% <0%> (-1.14%)`	⬇️
readthedocs/oauth/services/github.py	`42.76% <0%> (-0.83%)`	⬇️
... and 20 more

humitos · 2018-11-19T21:40:24Z

I updated the approach used here. read attribute wasn't to clear, so I decided to use extra_tags with email_delayed and email_sent which are more explicit.

The workflow is as follows:

the user hits the deprecated endpoint for the first time
a notification and a Message is created as email_delayed
an email is sent immediately

Then, later in the same day:

the user hits the deprecated endpoint
a notification is instantiated but as there is already an exact Message for this user/project no new Message is created
no email is sent

After 7 days:

the user hits the deprecated endpoint
a notification is instantiated and no Message is created (there is already one)
when .send is called, as the .created datetime is 7 days old we send an email
we also create a new Message with email_delayed so the next hit of the endpoint doesn't create n' send the email as the first time

humitos · 2018-11-21T15:21:41Z

readthedocs/projects/notifications.py

@@ -11,3 +15,69 @@ class ResourceUsageNotification(Notification):
    context_object_name = 'project'
    subject = 'Builds for {{ project.name }} are using too many resources'
    level = REQUIREMENT
+
+
+class DeprecatedWebhookEndpointNotification(Notification):


This notification could be generalized in the future so other notifications can extend from the base class and use the same workflow for de-duplication and sending emails.

readthedocs/core/views/hooks.py

Each time a deprecated webhook is hit, a notification is created (without duplicating it) to be sent.

humitos · 2019-01-02T14:12:02Z

It would be good to merge and deploy this as soon as possible so we give some time to our users to perform the migration.

January 31, 2019: GitHub stops delivering installed services' events on GitHub.com.

(from https://developer.github.com/changes/2018-04-25-github-services-deprecation/)

* 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

agjohnson · 2019-01-08T23:40:27Z

I didn't catch this initially, but this notification is for all webhook endpoints. I'm fixing this up now to be not GitHub specific.

agjohnson · 2019-01-09T00:53:08Z

Also, have we tested this on our commercial side? I'm mostly certain that Project.users is the correct mechanism to get owners, but not 100%

humitos · 2019-01-09T10:26:37Z

Also, have we tested this on our commercial side? I'm mostly certain that Project.users is the correct mechanism to get owners, but not 100%

Project.users.all() works on both (.com and .org)

I didn't catch this initially, but this notification is for all webhook endpoints. I'm fixing this up now to be not GitHub specific.

Yeah, originally we want to let users know about all the deprecated endpoints on our side but now we are more worried about GitHub Services. I mean, /bitbucket and /generic should be also warn the user (now, with a different message probably).

humitos · 2019-01-10T09:02:15Z

readthedocs/projects/notifications.py

+            message='{}: {}'.format(self.name, self.get_subject()),
+            level=self.level,
+            user=self.user,
+            extra_tags='email_delayed',


At this point, I think we want to check

if created and self.send_email: message.read = True

to avoid showing the email notification as a site notification.

…points (#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 · 2019-01-11T00:27:43Z

We're archiving this at https://github.com/rtfd/readthedocs.org/tree/humitos/de-duplicate-email-notifications and merging this with the automation rules removed for now.

* 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

humitos force-pushed the humitos/builds/notify-old-endpoints branch from fab04cc to bf8798d Compare November 13, 2018 10:45

humitos mentioned this pull request Nov 13, 2018

Deprecate old /github, /bitbucket, /build endpoints #4568

Closed

humitos added the PR: work in progress Pull request is not ready for full review label Nov 13, 2018

humitos commented Nov 13, 2018

View reviewed changes

agjohnson reviewed Nov 13, 2018

View reviewed changes

agjohnson added Needed: tests Tests are required Needed: documentation Documentation is required labels Nov 13, 2018

humitos force-pushed the humitos/builds/notify-old-endpoints branch from bf8798d to 4611180 Compare November 19, 2018 18:33

humitos removed PR: work in progress Pull request is not ready for full review Needed: tests Tests are required labels Nov 19, 2018

humitos requested a review from a team November 21, 2018 15:20

humitos commented Nov 21, 2018

View reviewed changes

stsewd reviewed Nov 27, 2018

View reviewed changes

readthedocs/core/views/hooks.py Outdated Show resolved Hide resolved

humitos added 7 commits December 10, 2018 18:13

Notify users about the usage of deprecated webhooks

0778294

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

ebf278b

Improve decorator to support generic and specific VCS webhook views

ec54cfa

Remove no necessary settings

095607b

DeprecatedWebhookEndpointNotification tests and improvements

6123d45

Better docstring

ecb1f8d

Lint

8a6bb8a

humitos force-pushed the humitos/builds/notify-old-endpoints branch from 4b60ea3 to 8a6bb8a Compare December 10, 2018 17:14

humitos added this to the 2.9 milestone Dec 26, 2018

humitos mentioned this pull request Jan 9, 2019

cider.readthedocs.io is not building #5079

Closed

humitos removed the Needed: documentation Documentation is required label Jan 9, 2019

humitos commented Jan 10, 2019

View reviewed changes

agjohnson merged commit a8814b3 into master Jan 11, 2019

delete-merged-branch bot deleted the humitos/builds/notify-old-endpoints branch January 11, 2019 01:51

stsewd mentioned this pull request Jan 21, 2019

Build not triggered anymore after commit #5142

Closed

dojutsu-user mentioned this pull request Feb 12, 2019

Project stopped having its builds run on Git push without warning #5268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notify users about the usage of deprecated webhooks #4898

Notify users about the usage of deprecated webhooks #4898

humitos commented Nov 13, 2018

humitos Nov 13, 2018

agjohnson Nov 13, 2018

humitos Nov 13, 2018

humitos Nov 13, 2018

agjohnson Nov 13, 2018 •

edited

Loading

agjohnson left a comment

agjohnson Nov 13, 2018

agjohnson Nov 13, 2018

humitos commented Nov 19, 2018

codecov bot commented Nov 19, 2018 •

edited

Loading

humitos commented Nov 19, 2018 •

edited

Loading

humitos Nov 21, 2018

humitos commented Jan 2, 2019

agjohnson commented Jan 8, 2019

agjohnson commented Jan 9, 2019

humitos commented Jan 9, 2019

humitos Jan 10, 2019

agjohnson commented Jan 11, 2019


		<p>Once you have done that, you need to go to your <a href="{% url "projects_integrations" project.slug %}">project's Integrations</a> under Read the Docs project's Admin, click integration and then in "Resync webhook" button.</p>

		<p>Thanks!</p>

Notify users about the usage of deprecated webhooks #4898

Notify users about the usage of deprecated webhooks #4898

Conversation

humitos commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Nov 19, 2018

codecov bot commented Nov 19, 2018 • edited Loading

Codecov Report

humitos commented Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

humitos commented Jan 2, 2019

agjohnson commented Jan 8, 2019

agjohnson commented Jan 9, 2019

humitos commented Jan 9, 2019

Choose a reason for hiding this comment

agjohnson commented Jan 11, 2019

agjohnson Nov 13, 2018 •

edited

Loading

codecov bot commented Nov 19, 2018 •

edited

Loading

humitos commented Nov 19, 2018 •

edited

Loading