{"action":"created","issue":{"url":"https://api.github.com/repos/hypothesis/h/issues/3246","repository_url":"https://api.github.com/repos/hypothesis/h","labels_url":"https://api.github.com/repos/hypothesis/h/issues/3246/labels{/name}","comments_url":"https://api.github.com/repos/hypothesis/h/issues/3246/comments","events_url":"https://api.github.com/repos/hypothesis/h/issues/3246/events","html_url":"https://github.com/hypothesis/h/pull/3246","id":150577884,"number":3246,"title":"Move email generation responsibilities out of `h.notification`","user":{"login":"nickstenning","id":3602,"avatar_url":"https://avatars.githubusercontent.com/u/3602?v=3","gravatar_id":"","url":"https://api.github.com/users/nickstenning","html_url":"https://github.com/nickstenning","followers_url":"https://api.github.com/users/nickstenning/followers","following_url":"https://api.github.com/users/nickstenning/following{/other_user}","gists_url":"https://api.github.com/users/nickstenning/gists{/gist_id}","starred_url":"https://api.github.com/users/nickstenning/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nickstenning/subscriptions","organizations_url":"https://api.github.com/users/nickstenning/orgs","repos_url":"https://api.github.com/users/nickstenning/repos","events_url":"https://api.github.com/users/nickstenning/events{/privacy}","received_events_url":"https://api.github.com/users/nickstenning/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-04-23T17:33:12Z","updated_at":"2016-04-23T17:33:13Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/hypothesis/h/pulls/3246","html_url":"https://github.com/hypothesis/h/pull/3246","diff_url":"https://github.com/hypothesis/h/pull/3246.diff","patch_url":"https://github.com/hypothesis/h/pull/3246.patch"},"body":"The preface: `h.notification` is some of the hairiest code in our codebase. We've all known it needs fixing for a while. I had a 10-hour flight, and [Sean had already done some of the hard work](https://github.com/hypothesis/h/pull/3238).\r\n\r\nThis is one of a couple of pull requests I have lined up to improve the state of this package. In rough order, here's how I think it's going to go down:\r\n\r\n- move email rendering/sending responsibilities out of `h.notification` and into `h.emails`\r\n- make `h.notification` less of a nightmare in general. A big part of this will be improving the test suite.\r\n- replace the `Subscriptions` model with one that does the opposite (records opt-outs)\r\n- move other email rendering responsibilities into `h.emails`\r\n- move email templates into MailChimp?\r\n\r\nThis PR only does the first of this list, and here's an extract from the relevant commit message:\r\n\r\n---\r\n\r\nRather than the `generate_notifications` function yielding rendered email data, we now have a business-logic only function which checks to see if a given annotation action merits a notification, and if so yields a tuple containing:\r\n\r\n     (the annotation being replied to,\r\n      the user who made the annotation being replied to,\r\n      the reply,\r\n      the user who made the reply,\r\n      the document object for the annotated page)\r\n\r\nThe responsibility for actually generating the email has moved into a new `h.emails` package, where `h.emails.reply_notification.generate` now accepts the above values as parameters and returns the data needed to construct an email.\r\n\r\nAs a result of the simplification of `h.notification.reply_template` in this commit, the following modules have also been removed:\r\n\r\n- `h.notification.gateway`\r\n- `h.notification.notifier`\r\n- `h.notification.types`\r\n\r\n---\r\n\r\nIt's worth noting that the tests for the new `h.email.reply_notification` module are a little bit involved, because I wanted to test against both the old and new `Annotation` models. While relying on `pytest` fixtures as heavily as these tests do is probably not a great idea in the long run, it does make this much easier to write.\r\n\r\nThe basic concept is that there's a fixture called `storage_driver` which takes one of two values: `'postgres'` or `'elastic'`. This bubbles up through other fixtures so that in the end, each of the tests in `test_reply_notification.py` are run twice, once with the old annotation model, once with the new."},"comment":{"url":"https://api.github.com/repos/hypothesis/h/issues/comments/213792042","html_url":"https://github.com/hypothesis/h/pull/3246#issuecomment-213792042","issue_url":"https://api.github.com/repos/hypothesis/h/issues/3246","id":213792042,"user":{"login":"codecov-io","id":8655789,"avatar_url":"https://avatars.githubusercontent.com/u/8655789?v=3","gravatar_id":"","url":"https://api.github.com/users/codecov-io","html_url":"https://github.com/codecov-io","followers_url":"https://api.github.com/users/codecov-io/followers","following_url":"https://api.github.com/users/codecov-io/following{/other_user}","gists_url":"https://api.github.com/users/codecov-io/gists{/gist_id}","starred_url":"https://api.github.com/users/codecov-io/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/codecov-io/subscriptions","organizations_url":"https://api.github.com/users/codecov-io/orgs","repos_url":"https://api.github.com/users/codecov-io/repos","events_url":"https://api.github.com/users/codecov-io/events{/privacy}","received_events_url":"https://api.github.com/users/codecov-io/received_events","type":"User","site_admin":false},"created_at":"2016-04-23T17:33:13Z","updated_at":"2016-04-23T17:33:13Z","body":"## [Current coverage][1] is `73.89%`\n> Merging **#3246** into **master** will decrease coverage by **-0.03%** as of [`207da80`][3]\n\n\n```diff\n@@            master   #3246   diff @@\n======================================\n  Files          126     125     -1\n  Stmts         4675    4642    -33\n  Branches       550     549     -1\n  Methods          0       0       \n======================================\n- Hit           3456    3430    -26\n- Partial         81      82     +1\n+ Missed        1138    1130     -8\n```\n\n> Review entire [Coverage Diff][4] as of [`207da80`][3]\n\n\n[1]: https://codecov.io/github/hypothesis/h?ref=207da80cdfc834393d3b99baa9ada294d0b255bd\n[2]: https://codecov.io/github/hypothesis/h/features/suggestions?ref=207da80cdfc834393d3b99baa9ada294d0b255bd\n[3]: https://codecov.io/github/hypothesis/h/commit/207da80cdfc834393d3b99baa9ada294d0b255bd\n[4]: https://codecov.io/github/hypothesis/h/compare/455bb7ef4859e06004686f9d56c7b784f76c86c0...207da80cdfc834393d3b99baa9ada294d0b255bd\n\n> Powered by [Codecov](https://codecov.io). Updated on successful CI builds."}}