Sevenzo / challenge-platform

The Sevenzo Challenge Platform
https://www.sevenzo.org
0 stars 3 forks source link

Email digest #171

Closed hugopeixoto closed 7 years ago

hugopeixoto commented 7 years ago

This implements the email digest feature (connects to #156). Added three modes: immediate ("as they happen"), daily and weekly.

When we saw @krnjn's comment on waffle we had already finished most of the code. We implemented this as a notification queue, after trying the live query method. The latter was a bit heavy on the queries, and it relied on reimplementing Comment#send_notifications logic in a slightly different way, so we decided to go with the former. It was a more straightforward change, and it will be easier to add notifications in the future. We also implemented a flush action when the user updates their preferences.

This feature relies on a cron job (we're using the whenever gem) to trigger the daily/weekly digests. Deployment steps will need to be adapted depending on the setup. Let us know if we need to change something to accommodate the deployment scripts.

krnjn commented 7 years ago

Hi @hugopeixoto – thanks for the heavy lift for this. This was some solid work and I like the approach you all took in setting up the scheduled_notifications and the Notifications service module to handle notifying users of new comments!

In the interest of time to avoid going back / forth with changes (there weren't very many that I made – mostly style consistency / cleaning up the views), I split up most of the work you did here into smaller PRs that introduced the changes piece meal so that I could make a bit more sense of the code. You can see them in separate PRs here: https://github.com/Sevenzo/challenge-platform/pulls?utf8=%E2%9C%93&q=is%3Apr%20is%3Amerged%20LoM. Essentially, I would do the following with this main branch to get these into separate PRs:

  1. git checkout -b kj-some-branch-name
  2. git reset master
  3. git add -p – adding just a few changes at a time that seemed "grouped" together

We can obviously revisit this approach if you all would prefer a different method / means to merging in the code, but as I mentioned above, this was just the fastest way I could review, merge, and deploy all the work you've completed here since I have limited time to do so. Let me know if you have thoughts / questions and we can go from there. Thanks again!

juliosantos commented 7 years ago

@krnjn:

Thanks for your feedback — we'll be issuing separate commits from now on.

Also: could you explain the changes you made in spec/models/user_spec.rb? We're trying to better understand your testing style so we stick with it.

krnjn commented 7 years ago

Sure, it's fairly simple in that the test was nested inside a describe block for another method. In general, I think unit tests for each method is probably the way to go (obviously test coverage is lacking in that respect, given time constraints we didn't finish them all), but I moved out your tests into different describe blocks for each method that you added.