Skip to content

Add missing DOM ids to rails/mailers/email.html template #42470

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

Merged

Conversation

nfedyashev
Copy link
Contributor

Summary

Instead of using Capybara/Selenium my team uses Cypress for all end to end tests. We have a test that checks that all Mailer Preview actions of all mailers could be opened and rendered without an error. This helped us to catch bugs earlier because there are lots of mailer and mailer actions.

The issue that we faced was that important most of the dd elements like(to, from, subject etc) were really hard to get to in DOM because they didn't have a unique selector. This PR fixes it.

Below is what our Cypress test look like:

context('Mailer Preview', () => {
  it('works for all mailer actions', () => {
    cy.visit('/rails/mailers')

    cy.get('li a').each($a => {
      const href = $a.attr('href');
      cy.log(href)
      cy.visit(href)

      cy.get('#from').then($dd => {
        cy.log('FROM :' + $dd.get(0).innerText)
        //expect($dd.get(0).innerText).to.eq('from@example.com')
      })

      cy.get('#to').then($dd => {
        cy.log('TO: ' + $dd.get(0).innerText)
        //expect($dd.get(0).innerText).to.eq('foo@bar.com')
      })

      cy.get('#subject').then($dd => {
        cy.log('SUBJECT: ' + $dd.get(0).innerText)
        //expect($dd.get(0).innerText).to.eq('Test subject')
      })

      cy.get('#mime_type').then($dd => {
        cy.log('MIME TYPE: ' + $dd.get(0).innerText)
        //expect($dd.get(0).innerText).to.eq('HTML email')
      })

      cy.get('[download="icon.png"]').should('exist')
    })
  })
})

Other Information

Attachment elements weren't updated because they already have download attribute with a file name.
I've used markup validator from https://validator.w3.org/ and it didn't find any errors or warnings related to this update.

Below is a screenshot of a very simplified test run in Cypress:

cypress-run-results

@rails-bot rails-bot bot added the railties label Jun 13, 2021
@zzak
Copy link
Member

zzak commented Jun 14, 2021

I think this makes sense, but what is stopping you from using your own template instead of the generated one?

@@ -56,35 +56,35 @@
<dl>
<% if @email.respond_to?(:smtp_envelope_from) && Array(@email.from) != Array(@email.smtp_envelope_from) %>
<dt>SMTP-From:</dt>
<dd><%= @email.smtp_envelope_from %></dd>
<dd id="smtp-from"><%= @email.smtp_envelope_from %></dd>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe namespacing these to email to ensure their uniqueness?
I'm not sure if we should use underscores or dashes.

Suggested change
<dd id="smtp-from"><%= @email.smtp_envelope_from %></dd>
<dd id="email-smtp-from"><%= @email.smtp_envelope_from %></dd>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@p8 I was thinking about this approach but eventually decided not to do it because of existing DOM elements on this page which don't have that email- prefix in id attributes here, here and here.

Changing it everywhere and adding email- prefix to each element doesn't make this PR fully backwards-compatible. Let me know what do you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@p8 PR has been updated to follow existing naming convention for id attributes(underscore). Thanks for noticing the inconsistency

@nfedyashev
Copy link
Contributor Author

@zzak

I think this makes sense, but what is stopping you from using your own template instead of the generated one?

Just trying to reduce the boilerplate - I have several Rails apps and they all have a similar specs/testing stack.
Hopefully this would beneficial for Rails users as well.

@nfedyashev
Copy link
Contributor Author

id attribute values have been updated to follow existing(underscrore) convention.

ag id= railties/lib/rails/templates/rails/mailers/email.html.erb
59:      <dd id="smtp_from"><%= @email.smtp_envelope_from %></dd>
64:      <dd id="smtp_to"><%= @email.smtp_envelope_to %></dd>
68:    <dd id="from"><%= @email.header['from'] %></dd>
72:      <dd id="reply_to"><%= @email.header['reply-to'] %></dd>
76:    <dd id="to"><%= @email.header['to'] %></dd>
80:      <dd id="cc"><%= @email.header['cc'] %></dd>
84:    <dd id="date"><%= Time.current.rfc2822 %></dd>
87:    <dd><strong id="subject"><%= @email.subject %></strong></dd>
102:        <select id="part" onchange="refreshBody(false);">
108:      <dd id="mime_type" data-mime-type="<%= part_query(@part.mime_type) %>"><%= @part.mime_type == 'text/html' ? 'HTML email' : 'plain-text email' %></dd>
110:      <dd id="mime_type" data-mime-type=""></dd>
116:        <select id="locale" onchange="refreshBody(true);">

@rails-bot
Copy link

rails-bot bot commented Sep 13, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Sep 13, 2021
@rafaelfranca rafaelfranca merged commit 4a5d8fc into rails:main Sep 15, 2021
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.

5 participants