-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
Updates and moves consumer/account folder into system #8319
Updates and moves consumer/account folder into system #8319
Conversation
You did not delete the old files? |
I did, but somehow the changes were not picked up... I've used It seems it's best to move files one by one. |
Best to do it that way, to assure nothing gets lost. Moving back to in dev 👍 |
4c27d2d
to
8640404
Compare
To nuke all changes in a branch I'm doing: While on the to-be-nuked branch
This closes the pull request as it removes all commits. After committing again one can click the re-open (it is only active with new once there are new commits) with the button below. Not sure there's a better way to do this, but this seems to work. |
@@ -64,7 +64,6 @@ | |||
# Allows switching of default card | |||
within(".card#card#{non_default_card.id}") do | |||
find_field('default_card').click | |||
accept_alert |
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.
Spec fails with this line, so I removed it - as it is not necessary, the modal is accepted by default.
Regardless of that we now have a warning here which goes:
Modal window with text `Changing your default card will remove shops' existing authorizations to charge it. You can re-authorize shops after updating the default card. Do you wish to change the default card?"` has been opened, but you didn't wrap your code into (`accept_prompt` | `dismiss_prompt` | `accept_confirm` | `dismiss_confirm` | `accept_alert`), accepting by default
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 old spec just called accept_alert
once the alert was displayed. But now we need to "expect" an alert before it happens and wrap the alert-causing code in a block. I added a commit to show you how we should use it now.
And as it turns out, it revealed a mistake in the translation file! This is a good example why testing for explicit strings is much better than expect(text).to eq I18n.t(:key)
.
The `strip` method removes a trailing newline at the end.
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.
Nice one! 😄
Now we just need a green build...
@@ -64,7 +64,6 @@ | |||
# Allows switching of default card | |||
within(".card#card#{non_default_card.id}") do | |||
find_field('default_card').click | |||
accept_alert |
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 old spec just called accept_alert
once the alert was displayed. But now we need to "expect" an alert before it happens and wrap the alert-causing code in a block. I added a commit to show you how we should use it now.
And as it turns out, it revealed a mistake in the translation file! This is a good example why testing for explicit strings is much better than expect(text).to eq I18n.t(:key)
.
Thanks @mkllnk for pushing this forward! And for explaining your approach, too - that's really helpful 👍 |
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.
👍
What? Why?
Relates to #7929
Moves and updates the specs in
feature/consumer/account
into system specs.What should we test?
Green build.
Release notes
Moves and updates the specs in
feature/consumer/account
into system specs.Changelog Category: Technical changes