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

Update spec for quotation mark replacement #12584

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jun 19, 2024

What? Why?

Related to:

I'm not sure why, but the pre-compiling of assets triggered Rails to render style="..." instead of style='...' in this case. But when assets are compiled on-demand, we get the single quotes. So I changed the spec to be agnostic of this detail. We actually just want to know about the link and its href.

What should we test?

  • Specs only

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

I'm not sure why, but the pre-compiling of assets triggered Rails to
render `style="..."` instead of `style='...'` in this case. But when
assets are compiled on-demand, we get the single quotes. So I changed
the spec to be agnostic of this detail. We actually just want to know
about the link and its href.
@mkllnk mkllnk self-assigned this Jun 19, 2024
@mkllnk mkllnk added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jun 19, 2024
@mkllnk mkllnk marked this pull request as ready for review June 19, 2024 01:49
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

💯 Great change!

let(:shop) { create(:enterprise, allow_order_changes: true) }

let(:content) { Capybara::Node::Simple.new(body) }
Copy link
Member

Choose a reason for hiding this comment

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

Clever! Now we can use capybara expectations just like on a web page. ✨

🤔 I wonder if we should build this in to a mailer spec helper...

Copy link
Member

Choose a reason for hiding this comment

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

Do you know what the style check was for? I doubt it's relevant to these test expectations so I'm happy to see it go.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it was just part of the regular expression to match the link but nothing else. It wasn't checking the style.

@dacook
Copy link
Member

dacook commented Jun 19, 2024

I'm going to merge, but FYI @rioug, we can use normal Capybara expectations in mailer specs ☝️

@dacook dacook merged commit 66606ee into openfoodfoundation:master Jun 19, 2024
52 checks passed
@mkllnk mkllnk deleted the spec-refactor branch June 19, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants