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

[WIP] 390 monthly email list revised #727

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

Conversation

mz99
Copy link
Collaborator

@mz99 mz99 commented Jul 30, 2018

Since the last PR was accidentally merged already, here is the new branch. Should reasons_for_notifications be refactored to be more decoupled (separate each scenario into own methods)?

close #390

@mz99 mz99 requested a review from roschaefer July 30, 2018 16:26
Copy link
Owner

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work 🥇

Here are a few suggestions for you. Also consider to try out some of the mail interceptor gems for rails: https://github.com/ryanb/letter_opener
https://mailcatcher.me/

backend/app/mailers/user_mailer.rb Outdated Show resolved Hide resolved
backend/app/models/user.rb Outdated Show resolved Hide resolved
backend/app/models/user.rb Outdated Show resolved Hide resolved
backend/app/models/user.rb Outdated Show resolved Hide resolved
backend/app/views/user_mailer/monthly_news.html.erb Outdated Show resolved Hide resolved
backend/app/views/user_mailer/monthly_news.html.erb Outdated Show resolved Hide resolved
backend/lib/tasks/monthly_mailer.rake Show resolved Hide resolved
backend/spec/models/user_spec.rb Show resolved Hide resolved
@mz99 mz99 force-pushed the 390_monthly_email_list_revised branch 2 times, most recently from 1e8a1e6 to a3772c3 Compare July 31, 2018 14:57
@mz99 mz99 force-pushed the 390_monthly_email_list_revised branch from a3772c3 to 72961c9 Compare July 31, 2018 14:58
hello: Hallo
welcome: Willkommen zu den Updates dieses Monats:
body:
recently_created_broadcasts: Hier sind die neuen Sendungen, die seit Ihrer letzten Anmeldung den Rundfunk Mitbeständen hinzugefügt wurden:
Copy link
Owner

@roschaefer roschaefer Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throughout the app we're using the informal, not very polite way of addressing "Du" instead of "Sie". The translation is perfect 👌 but very formal. I will fix the translations as soon as this PR is ready for merge

@roschaefer
Copy link
Owner

Nice work! @Kachulio1 and maybe @aonomike do you want to pair with @mz99 on this PR maybe? Pretty interesting stuff for you, I guess. I can chip tomorrow if you like.

@roschaefer roschaefer force-pushed the 390_monthly_email_list_revised branch from b271bc0 to 4c5ee5c Compare August 9, 2018 16:41
@faithngetich
Copy link
Collaborator

faithngetich commented Sep 18, 2018

@mz99 It looks like the pending migrations are what is making the build fail. You might want to run them and see if the build passes.

To resolve this issue, run: bin/rails db:migrate RAILS_ENV=test

@roschaefer
Copy link
Owner

@faithngetich feel free to fix it yourself. There is no such thing as code ownership, but collective code ownership. The source branch of this PR is from our origin repository, so you should have write permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regular emails to increase user engagement
3 participants