-
Notifications
You must be signed in to change notification settings - Fork 499
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
AO3-6835 Create change_email mailer preview #4970
AO3-6835 Create change_email mailer preview #4970
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments regarding the tests.
end | ||
mail( | ||
to: @old_email, | ||
subject: default_i18n_subject(app_name: ArchiveConfig.APP_SHORT_NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
visit change_email_user_path(user) | ||
end | ||
|
||
When "{string} old email {string} should be emailed" do |user, old_email| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubocop already pointed it out, the username is unused here. I would propose to remove the username from the step name (something like "the email address {string} should be emailed") and to move this to email_custom_steps.rb
@users | ||
Feature: | ||
As a registered user | ||
I should be able to change my email |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enthusiasm regarding tests is appreciated, however all these things except for the "Translated email is sent when email is changed" scenario are already tested infeatures/other_a/profile_edit.feature
:/
I would suggest to move the translated email scenario to that file and remove the rest of the new tests.
And the email should contain "Translated footer" | ||
And the email should not contain "fan-run and fan-supported archive" | ||
And the email should not contain "translation missing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make this into a step in email_custom_steps.rb
and call it something like "the email to email address {string} should be translated"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing
)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-6835
Purpose
Create
change_email
mailer preview and update theI18n.with_locale
block.Include cucumber test for sending a translated
change_email
notification to user.Testing Instructions
See Jira.
Credit
Amy Lee