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

Enterprises Controller: reset_distributor must be called before any call to memoized current_distributor #5501

Merged
merged 2 commits into from
May 26, 2020

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented May 26, 2020

What? Why?

Closes #5496

The bug was caused by a hidden logical problem created by memoized current_distributor, current_customer that uses current_distributor cant be called before reset_distributor, otherwise the original (and outdated) distributor was memoized and returned by current_distributor...

The new spec in enterprises_controller validates the problem is fixed.

Moving current_distributor and current_customer to a service would be awesome here but would take too much time because they are called from quite a few places.

What should we test?

Verify bug is resolved.
We should probably give it a simple validation of #5440 as well

Release notes

Fix stale data problem when navigating across different shops.

@luisramos0 luisramos0 self-assigned this May 26, 2020
@luisramos0 luisramos0 changed the title Break OrderCartReset in two steps so that memoized method current_cus… Fix problem in Enterprises Controller: reset_distributor must be done before any call to memoized current_distributor May 26, 2020
…tomer (that uses memoized current_distributor) is called after reset_distributor
@luisramos0 luisramos0 force-pushed the grumpy_cat_take_2 branch from d3317e2 to 52810b0 Compare May 26, 2020 10:26
@luisramos0 luisramos0 changed the title Fix problem in Enterprises Controller: reset_distributor must be done before any call to memoized current_distributor Fix problem in Enterprises Controller: reset_distributor must be called before any call to memoized current_distributor May 26, 2020
@luisramos0 luisramos0 changed the title Fix problem in Enterprises Controller: reset_distributor must be called before any call to memoized current_distributor Enterprises Controller: reset_distributor must be called before any call to memoized current_distributor May 26, 2020
@RachL RachL added the pr-staged-fr staging.coopcircuits.fr label May 26, 2020
@RachL RachL self-assigned this May 26, 2020
@RachL
Copy link
Contributor

RachL commented May 26, 2020

I've tested the scenario in the issue:

Log in (admin, customer...)
Click /shops in main menu
Choose an open shop A
Click again in /shops in main menu
Choose another shop (other than the one you are), shop B
You should observe shop A again, while the address bar clearly shows the address of shop B...

looking good!
About grumpy cat:

  1. Single OC, all good:
    image

  2. However with 2 OC open or 3 and if I close one of them(e.g. with Phillippe shop), I'm redirected to the home page like in test number 1.

Is this intended? It's not clear to me while reading #5440 . In any case I don't think it is a blocker and we can merge this work as is and improve in a second round. At least we don't have a grumpy cat anymore :)

ping @luisramos0 @filipefurtad0 @kristinalim

@filipefurtad0
Copy link
Contributor

I agree @RachL, this is good to go.

If the OC is closed while on /checkout and I click the back button on the browser (like in Nick's video on the issue #5340 (comment)) I always land on the /shop page, and observe OC selector (if any left), but no flash message.

I've searched a lot (as this was indicated as an outcome of a test case) and the only way I found to observe the flash was clicking directly in Shops, in the menu, instead of clicking back on the browser, from /checkout. So... How do you do it? (magic!) :-)

Well, main point is probably that the release is not blocked any more 🎉

@luisramos0 luisramos0 merged commit 7313e3c into openfoodfoundation:master May 26, 2020
@luisramos0 luisramos0 deleted the grumpy_cat_take_2 branch May 26, 2020 22:07
@luisramos0
Copy link
Contributor Author

luisramos0 commented May 26, 2020

yeah, that redirect is not correct Rachel, can you replicate it? when you say "I am redirected", in what exact page are you and what do you do? page refresh or click somewhere?

@luisramos0 luisramos0 mentioned this pull request May 26, 2020
7 tasks
@RachL
Copy link
Contributor

RachL commented May 26, 2020

@luisramos0 @filipefurtad0 yes I can replicate. You need to do the following:

  1. Go to shop with several OC open and add products to the cart, but don't go any further. Stay on the shop page, leave the browser tab opened

  2. Close the OC you have just used with another browser window

  3. Come back to your tab with the shop

  4. Click on the cart on the main menu and "proceed to checkout"

There the redirect to the home page with flash message happens.I also reproduced it while being on the cart page and trying to go to the checkout page after closing the OC. I'm sorry, I didn't think we could have different behavior, so I didn't test the checkout page before. Now I get a snail while trying to checkout with an OC that closed while I was on the checkout page...might be my internet connexion, I will retry in the morning...

@RachL
Copy link
Contributor

RachL commented May 27, 2020

Outch, wasn't my internet access, I could reproduce the snail this morning it's a 404 error:

My test was: I had a user who was on /checkout, I closed the OC, then I made this user click on the checkout button at the bottom of the page. I ended up with a 404:

image

However when I press the browser back button I indeed ended up with the shop open on the other OC, no flash message.

Is that 404 something you saw earlier @filipefurtad0 ?

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented May 27, 2020

Hey @RachL ,

Thanks for checking this again.
Yes I saw this, it's #5372

However when I press the browser back button I indeed ended up with the shop open on the other OC, no flash message.

That's what I observe too - so, all good I think.

@RachL
Copy link
Contributor

RachL commented May 27, 2020

@filipefurtad0 great!

@RachL RachL removed the pr-staged-fr staging.coopcircuits.fr label May 27, 2020
@luisramos0
Copy link
Contributor Author

Hey, do we need a new issue for the redirect problem? I mean, if there are 2 OCs and the hub closes one of them the redirect should go to the order OC, not to /shops.

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Jun 9, 2020

Yes @luisramos0,
Redirection should be to /shop, not the homepage. I'll open an issue and link it #5440.
Thanks for mentioning this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching between shops does not work correctly
5 participants