Password change logout #2850
Password change logout #2850
Conversation
Matt-Yorkley
force-pushed
the
password_change_logout
branch
from
57eb5dc to
55411af
Compare
| @@ -27,5 +31,18 @@ | |||
| click_link I18n.t('spree.users.show.tabs.settings') | |||
| expect(page).to have_content I18n.t('spree.users.show.unconfirmed_email', unconfirmed_email: '[email protected]') | |||
| end | |||
|
|
|||
| it "allows the user to change their password" do | |||
| allow(Spree::Auth::Config).to receive(:[]).with(:signout_after_password_change).and_return(false) | |||
There was a problem hiding this comment.
@mkllnk I've added this to make the spec work, as the test environment doesn't pick up the value set in the spree initializer. Do you think this is ok, or should I try to set that config variable globally in the test environment instead? I'm wondering if it might interfere with other specs or changes to factories if it's set globally...
There was a problem hiding this comment.
That's the way IMO. Changes to global state in tests => 💥
There was a problem hiding this comment.
I agree that changes to the global state are not good in tests in general. And I find this solution okay, but I see a problem in the fact that the logic is different for production and testing here. We have to add this line to the test case to make it behave the same way. That's basically code duplication in a very hidden place. Imagine we had to define the checkout state machine in the same spec that verifies that it's correct. It's not really testing the code used in production then.
| @@ -27,5 +31,18 @@ | |||
| click_link I18n.t('spree.users.show.tabs.settings') | |||
| expect(page).to have_content I18n.t('spree.users.show.unconfirmed_email', unconfirmed_email: '[email protected]') | |||
| end | |||
|
|
|||
| it "allows the user to change their password" do | |||
| allow(Spree::Auth::Config).to receive(:[]).with(:signout_after_password_change).and_return(false) | |||
There was a problem hiding this comment.
I agree that changes to the global state are not good in tests in general. And I find this solution okay, but I see a problem in the fact that the logic is different for production and testing here. We have to add this line to the test case to make it behave the same way. That's basically code duplication in a very hidden place. Imagine we had to define the checkout state machine in the same spec that verifies that it's correct. It's not really testing the code used in production then.
|
I agree @mkllnk that changing the settings for the production environment but not for tests doesn't seem like a great idea. I've switched to setting it in the global test config, and all the specs are green, so I guess it's ok. @sauloperez are you happy with that? 4760ebb |
|
Staged on https://staging.katuma.org/ |
|
testing notes: https://docs.google.com/document/d/1gtzQWewyiLawJEyyH85xUSNQHNHcAitVvLPL4lu63qQ/edit?usp=sharing @Matt-Yorkley i randomly did another test as change password was affected I tested the "forgot password" process and it seems that PR is breaking it, the email recovery email is not sent. See notes. I tested on current French production (on v1.21) and it is working so it feels like it can be connected to that PR... back to in dev then for now. Precision: I did create a new user so I received correctly other confirmation message so it doesn't seem related to Katuma staging setup. |
|
@myriamboure I've just put this up on UK staging, and reset password emails are working fine with this PR. I'll leave it up if you want to re-test it. |
|
I've retested and everything is indeed fine ! I've updated @myriamboure testing notes: |
What? Why?
Closes #2848
Adjusts a config setting so that users are not logged out when updating their password.
What should we test?
Users can change password via /account page without being logged out.
Release notes
Users will not be logged out whilst changing their password.
Changelog Category: Changed