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.

Sign up for GitHub

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

Jump to bottom

[WIP]849 send notification when broadcast is created #892

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

[WIP]849 send notification when broadcast is created #892

wants to merge 24 commits into from

Conversation

faithngetich
Copy link
Collaborator

@faithngetich faithngetich commented Oct 25, 2018 •
edited by roschaefer
Loading

Description

Fixes #849

Motivation and Context

It would be nice to send an email/slack notification informing that a new broadcast has been created.

How Has This Been Tested?

You will need to login to the site and create a broadcast, an easy way of doing so is as follows:

  • Go to this link and search for a broadcast that does not exist.
  • You'll be redirected to a page where you can create a broadcast. When you fill in the details and hit the create button you will be able to trigger the email.
  • Open mailer should send you to a link with the email details.
  • If you're not redirected, relax you can still view the emails on this link. This will show you all the emails that you have attempted sending.

Screenshots (if appropriate):

When you have successfully created a broadcast
screen shot 2018-10-25 at 19 26 00
Just before you save the broadcast
screen shot 2018-10-25 at 19 25 42

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@faithngetich faithngetich changed the title 849 send notification when broadcast is created [WIP]849 send notification when broadcast is created Oct 25, 2018
@roschaefer
Copy link
Owner

@faithngetich I updated your PR quite a bit and I hope I paved the way for you with some tests. I replaced letter_opener with mailhog which is the cleaner alternative, as it does not require a change to our rails code. Also, I configured sidekiq to process our mailers.

@roschaefer roschaefer force-pushed the 849_send_notification_when_broadcast_is_created branch from dcf3649 to 9c87ba1 Compare October 31, 2018 17:18
faithngetich and others added 22 commits October 31, 2018 18:20
@faithngetich @roschaefer
send emails on broadcast notification
e9421fd
@faithngetich @roschaefer
Add letter opener to view sent mail locally
2020e82
@faithngetich @roschaefer
Remove commented code
ffb4a1d
@faithngetich @roschaefer
Add email notification on broadcast creation job and test it
a88dd0a
@faithngetich @roschaefer
Test email notification on broadcast creation mailing method
52bcbc9
@faithngetich @roschaefer
Add email template
9ff6580
@roschaefer
Remove letter opener
ff8823e
@roschaefer
Add service mailhog to catch mails in development
f5df473 
..and configure sidekiq to process mailer queue
@roschaefer
Sketch UserMailer#ask_for_spam_check and test
04aa27f
@roschaefer
Make sure we have moderators in the future
1ab2073
@roschaefer
Add documentation for mailhog
0137a2b
@roschaefer
Remove obsolete code
3ab3aee
@roschaefer
Remove obsolete mailer view
e63cea3
@roschaefer
Turn the user into a moderator only on requests
9c87ba1
@roschaefer
Remove unnecessary code
1347de2
@roschaefer
Fix rspec test - test inside context block
2374c38
@NickSchimek
Add mail body
446b388
@faithngetich
test that email body provides a deep link to check the broadcast
ea1484a
@faithngetich
Add [rundfunk mitbestimmen] to the email subject
a4fa104
@NickSchimek
Add internationalization to email subject
323ecca
@ciremoussadia
Fixes the test that checks that notification are sent to all admins a…
6333fe9 
…nd moderators at Sidekiq queue list.
@ciremoussadia
Turns anyone who create or update a broadcast to a moderator
a02e295 
This lead to an error on specs on which email notification were sent to admins and moderator. We fix it by adding 1 to the size total consideing that the creator of the broadcats is no turned to a moderator.
@ciremoussadia ciremoussadia self-assigned this Jan 11, 2019
@ciremoussadia
Copy link
Collaborator

Hello @roschaefer, what can I do to help finish this PR?

@roschaefer
Copy link
Owner

roschaefer commented Jun 3, 2019 •
edited
Loading

@ciremoussadia sorry for responding so late. I kept the Github notification "unread" to motivate me to dig into this PR but I failed.

So, to answer your question: The build server tells me that push has succeeded but pr has not. This could mean that branch master was updated and has not been merged into this PR.

  1. git checkout 849_send_notification_when_broadcast_is_created
  2. git fetch
  3. git merge origin/master
  4. git push origin -u 849_send_notification_when_broadcast_is_created

Will update this PR. The expected outcome is that both pr and push fail. The build server tells me that there are rubocop errors (easy) and some rspec failures. You could fix the rubocops with bundle exec rubocop -a. As a next step you would need to fix the rspec tests. I think once this is done we could merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ciremoussadia ciremoussadia

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Send notification to users when new broadcast is created
4 participants
@faithngetich @roschaefer @ciremoussadia @NickSchimek