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

System emails design #4818

Merged
merged 9 commits into from
Jun 3, 2022
Merged

System emails design #4818

merged 9 commits into from
Jun 3, 2022

Conversation

decabeza
Copy link
Collaborator

References

In the System Emails section there are 12 emails that are sent from CONSUL.

As the styles for these emails must be inline, it's complicated to modify them to add custom styles. Also, as the emails were created at different times there were some inconsistencies in the design.

Objectives

This PR adds the following changes:

  • Add mailer helpers to simplify customization: all styles can now be changed from the helper and are common to all emails, maintaining the design.

  • Remove icon images on mailers: these images do not contribute anything to the user and are not used on the rest of the website.

  • Add missing heading title on mailers: for consistency, titles have been added to all emails that did not have one.

Visual Changes 👨‍🎨

email_1

email_2

@javierm javierm added this to Reviewing in Consul Democracy via automation Apr 28, 2022
@decabeza
Copy link
Collaborator Author

The failing spec it seems not related with these changes. 🤔

@javierm
Copy link
Member

javierm commented Apr 28, 2022

The failing spec it seems not related with these changes

That's right, that spec has been failing all day... maybe there's an update in chrome/chromedriver causing it. We'll take a look next week!

@taitus taitus self-assigned this May 4, 2022
@microweb10
Copy link
Member

That's right, that spec has been failing all day... maybe there's an update in chrome/chromedriver causing it. We'll take a look next week!

Hi!
We have the same spec failing in our repo that is synced with the CONSUL version 1.4.1.
We have added the commits on the PR 4784 and PR 4824 but the spec is still failing.
Is there any other commit we should add to avoid that spec to fail? 🤔
Thanks in advance!

@Senen
Copy link
Member

Senen commented May 4, 2022

That's right, that spec has been failing all day... maybe there's an update in chrome/chromedriver causing it. We'll take a look next week!

Hi! We have the same spec failing in our repo that is synced with the CONSUL version 1.4.1. We have added the commits on the PR 4784 and PR 4824 but the spec is still failing. Is there any other commit we should add to avoid that spec to fail? 🤔 Thanks in advance!

Hi @microweb10,

I think we fixed it in:

@taitus taitus moved this from Reviewing to Doing in Consul Democracy May 4, 2022
@taitus taitus moved this from Doing to Reviewing in Consul Democracy May 5, 2022
@taitus taitus moved this from Reviewing to Doing in Consul Democracy May 5, 2022
@taitus taitus moved this from Doing to Reviewing in Consul Democracy May 5, 2022
@taitus taitus moved this from Reviewing to Doing in Consul Democracy May 5, 2022
@taitus taitus moved this from Doing to Reviewing in Consul Democracy May 11, 2022
@javierm javierm self-assigned this Jun 1, 2022
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

@taitus I've left some comments; let me know what you think!

app/views/mailer/budget_investment_unfeasible.html.erb Outdated Show resolved Hide resolved
app/views/mailer/comment.html.erb Outdated Show resolved Hide resolved
app/views/mailer/evaluation_comment.html.erb Outdated Show resolved Hide resolved
app/controllers/admin/system_emails_controller.rb Outdated Show resolved Hide resolved
app/views/mailer/comment.html.erb Show resolved Hide resolved
app/views/mailer/budget_investment_unfeasible.html.erb Outdated Show resolved Hide resolved
app/views/mailer/proposal_notification_digest.html.erb Outdated Show resolved Hide resolved
@javierm javierm moved this from Reviewing to Doing in Consul Democracy Jun 1, 2022
@taitus taitus moved this from Doing to Reviewing in Consul Democracy Jun 2, 2022
Currently with both seeds and dev_seeds, not only was this email not
displayed from the system emails section, but it also caused an error in
the application.

@email_to had an empty value and in the view we tried to access
@email_to.name which caused the error. We kept the same logic but
added the current_user to make sure it always has a valid value. We add
the current_user because the current_user is always present in this controller..
The notification digest title did not look the same as other mail.  We
removed the table for the title to make it more consistent with the rest of
the emails.
In the comments and direct message emails, the "attributes" was
missing and in the reply email it was not in the right place.
This section was being displayed differently in each mailing. We are trying
to unify it so that they are displayed the same.
expect(page).to have_content comment.body

expect(page).to have_link "Cleaner city",
href: admin_budget_budget_investment_url(investment.budget, investment, anchor: "comments", host: app_host)
Copy link

Choose a reason for hiding this comment

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

Layout/LineLength: Line is too long. [117/110] (https://rubystyle.guide#max-line-length)

@javierm javierm linked an issue Jun 2, 2022 that may be closed by this pull request
@javierm javierm added the 1.5 label Jun 2, 2022
decabeza and others added 4 commits June 2, 2022 17:56
In the mail section we have very different indentations and formatting in
 texts with sanitize, links and texts with interpolations. In my opinion it
helps a lot to have clearer indentations in these cases.

This may not be the best way to indent them, but at least I think it is
clearer than it was and at least relatively unified.
Consul Democracy automation moved this from Reviewing to Testing Jun 2, 2022
@taitus taitus merged commit 77825ed into master Jun 3, 2022
Consul Democracy automation moved this from Testing to Release 1.5.0 Jun 3, 2022
@taitus taitus deleted the emails_styles branch June 3, 2022 06:40
@javierm javierm removed the 1.5 label Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error 500 on Admin system emails
5 participants