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

Notify users about the usage of deprecated webhooks #4898

Merged
merged 9 commits into from Jan 11, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Nov 13, 2018

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

Closes #4568

request,
user,
)
notification.send()
Copy link
Member Author

@humitos humitos Nov 13, 2018

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

Copy link
Contributor

@agjohnson agjohnson Nov 13, 2018

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

Copy link
Member Author

@humitos humitos Nov 13, 2018

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?

Copy link
Member Author

@humitos humitos Nov 13, 2018

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

Copy link
Contributor

@agjohnson agjohnson Nov 13, 2018

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.

Copy link
Contributor

@agjohnson agjohnson left a comment

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 Outdated Show resolved Hide resolved
readthedocs/projects/notifications.py Show resolved Hide resolved
'task': 'readthedocs.projects.tasks.send_deprecated_endpoint_notifications',
'schedule': crontab(minute=0, hour='10', day_of_week='monday'),
'options': {'queue': 'web'},
},
Copy link
Contributor

@agjohnson agjohnson Nov 13, 2018

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


<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>
Copy link
Contributor

@agjohnson agjohnson Nov 13, 2018

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
Copy link
Member Author

@humitos humitos commented Nov 19, 2018

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

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.

@humitos humitos force-pushed the humitos/builds/notify-old-endpoints branch from bf8798d to 4611180 Nov 19, 2018
@codecov
Copy link

@codecov codecov bot commented Nov 19, 2018

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
Copy link
Member Author

@humitos humitos commented Nov 19, 2018

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:

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

Then, later in the same day:

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

After 7 days:

  1. the user hits the deprecated endpoint
  2. a notification is instantiated and no Message is created (there is already one)
  3. when .send is called, as the .created datetime is 7 days old we send an email
  4. 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 humitos requested a review from Nov 21, 2018
@@ -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):
Copy link
Member Author

@humitos humitos Nov 21, 2018

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 Outdated Show resolved Hide resolved
@humitos humitos force-pushed the humitos/builds/notify-old-endpoints branch from 4b60ea3 to 8a6bb8a Dec 10, 2018
@humitos humitos added this to the 2.9 milestone Dec 26, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Jan 2, 2019

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
Copy link
Contributor

@agjohnson agjohnson commented Jan 8, 2019

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
Copy link
Contributor

@agjohnson agjohnson commented Jan 9, 2019

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
Copy link
Member Author

@humitos humitos commented Jan 9, 2019

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).

message='{}: {}'.format(self.name, self.get_subject()),
level=self.level,
user=self.user,
extra_tags='email_delayed',
Copy link
Member Author

@humitos humitos Jan 10, 2019

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
Copy link
Contributor

@agjohnson agjohnson commented Jan 11, 2019

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.

@agjohnson agjohnson merged commit a8814b3 into master Jan 11, 2019
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/builds/notify-old-endpoints branch Jan 11, 2019
agjohnson added a commit that referenced this issue 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants