Fix Grumpy Cat on shop page (cart with items from a closed OC) #5440

luisramos0 · 2020-05-15T13:30:33Z

What? Why?

This issue was caused by landing on a shop page with some variants in the cart from an Order Cycle that was no longer visible to the user (either closed or some new tag rules). The code is now smart enough to verify the order_cycle is not visible and will empty the order.

Thsi was a simple if statement on the enterprise_controller but sometimes we have to make the world a better place, I mean OFN, so I went on an afternoon of refactoring by extracting new OrderCartRest service (and add some unit tests) and renaming 2 other services according to service naming conventions so now we have related services together:

OrderCheckoutRestart (restart order workflow)
OrderCompletionReset (reset order after completion)
OrderCartReset (reset order on shop page).

It's when we extract these services (and see how complex they are) that we see how obviously overloaded our controllers are!!! We need more of this in OFN.

What should we test?

Verify the issue is fixed. When OC is closed and there are items in the cart, the cart will be cleared up and the normal redirection rules will apply:
1- no other OCs: flash message and redirect to shops page
2- if there is only one more OC, that one will be selected and displayed
3- if there are two or more OCs, the red Oc selector will show up.

We should also try to break it by changing things in the Backoffice and load different shops pages #variation #exploratorytesting

Note: now I realize I havent tested with 3 OCs.

Release notes

Changelog Category: Fixed
Fixed grumpy cat page (422 http error) when Order Cycle was closed while user was shopping (the broken case was only when there other Order Cycles open for the same shop)

…e naming convention and also to make it more specific to completed orders

…rvice naming convention

…ted_order_cycles

Matt-Yorkley · 2020-05-18T10:03:43Z

spec/controllers/enterprises_controller_spec.rb

+      expect(controller.current_order.distributor).to eq(distributor)
+      expect(controller.current_order.order_cycle).to eq(order_cycle2)
+      expect(controller.current_order.line_items).to be_empty
+    end


Do you think it's worth adding specs for the cases here?

master...Matt-Yorkley:enterprise-controller-stock

Line item variants are all in stock

Line item variants are not all in stock

A line item's variant has been soft-deleted

A line item's variant has been removed from the OC

I was planning to do this as part of this PR but now that Pau moved it to Test Ready with the two approvals, we can probably get this into the release.
I will work on these specs anyway and create a new PR. I am a bit all over 10's of PRs, I may get to this only on Monday.

IMO it's fine as is. The services seem mostly covered with unit tests and we might just need to increase the integration coverage at controller level a little bit.

Matt-Yorkley

👍

sauloperez · 2020-05-21T12:36:49Z

app/controllers/enterprises_controller.rb

-      OrderCycle.not_closed.with_distributor(current_distributor)
-    end
-  end
-


You'll go to heaven for this 😍

sauloperez · 2020-05-21T12:37:50Z

app/services/order_completion_reset.rb

+# frozen_string_literal: false
+
+# Resets a completed order by building a new order based on the one specified.
+# This implements the "continue shopping" feature once an order is completed.


pretty useful 👌

filipefurtad0 · 2020-05-22T15:50:38Z

Hi @luisramos0

I've checked this bug and the test cases you describe.

In summary, the bug is gone 🎉
However, I can't seem to trigger the flash warning - which was displayed before the PR, when being redirected to the homepage:

The test cases:
Closing OC while on checkout, with a product on cart from that OC:

1- no other OCs: redirect to shops page, but a flash message does not appear

2- if there is only one more OC, that one will be selected and displayed - OK

3- if there are two or more OCs, the red Oc selector will show up - OK (verified this with 3 open OCs as well)

If two open OCs have the same products, and one of them is closes before checkout, the products on the cart are not automatically allocated to the OC remaining open. Instead, one is redirected to the /shop page, either with remaining OC open (test case 2) or with the selector showing up in red (case 3).

Other tests:

changing prices to items in cart - all good
keeping the OC open but removing item - leads to the now fixed stuck on cart issue 🎉
adding other items to OC - all good

This is ready to go. It think the warning would be nice to have, but I don't think we should block this, for this reason. I can open a separate issue to add this.

luisramos0 · 2020-05-22T21:22:11Z

ok, thanks Filipe. Yes, let's open an issue for that missing message. We need to tackle it now.
I dont understand how it can redirect but not show the warning: the two things are just one after the other in the code... maybe some problem displaying the flash message?

filipefurtad0 · 2020-05-23T17:35:21Z

Hi @luisramos0 ,

I checked this PR again, and found a small but relevant imprecision in my notes above (confusion between shops /shops /shop). In test case 1, the user gets redirected to the /shop page, as the pic shows (not confuse with /shops - sorry for that).

To see which flash message to expect in these test cases, I had a look in the code. So, should it be this error:

a) "The shop you are looking for doesn't exist or is inactive on OFN. Please check other shops."

or this info?

b) "The order cycle you've selected has just closed. Please try again!"

According to the code it should be a), right? I could trigger both these flash messages in my system, but only when visiting or getting redirected to the /shops page. This can happen if the user is in the checkout menu with items in cart, and:

a) the shop closes (by changing the package from shop/hub to producer profile) and the user attempts to edit is cart (navigating back with the browser):

b) the OC closes on checkout page and the user clicks Boutiques:

So, both of these flash messages appear in the /shops# page. In the covered test cases the user rather gets redirected to the /shop page (not /shops or shops), so could this be the reason why I am not observing any the flash messages?

Should this be dealt in a separate issue (should I open one)?

PS - I found other issues which I don't think are related to this PR (opening issues on these):

Bug switching between different shops, which can be observed by the disagreement between the address bar and the actual page the user finds himself in (gif):
Update: I can't reproduce this in production, nor on the latest release (draft)... So perhaps, this was fixed, which is good news.

Missing translation for the flash error b) - it seems not to exist in the en.yml

luisramos0 · 2020-05-25T17:32:24Z

Hey Filipe, thanks for the detailed update!

I understand the problem, the user has a cart and is shopping, the manager closes the OC in the backoffice (there are no other OCs), user loads the shopfront and the cart is empty and the closed shop message displays. The closed shop message is already evidently saying the cause of the empty cart. I think we are ok here.

So we would change the test case and leave it as is:
Instead of:
"no other OCs: flash message and redirect to shops page"
We have:
"no other OCs: stay on shop page where the closed shop message is displayed"

I think you did find a bug though: the missing translation of that flash message in b) 👍

luisramos0 · 2020-05-25T17:35:42Z

re that gif... @Matt-Yorkley will be happy to use his new catchphrase about caching issues!
Can you have a look at this last gif Matt?

If anyone sees that problem again I think we should open an issue for it.

luisramos0 · 2020-05-25T17:55:32Z

actually, I can replicate this problem in staging FR.
You just have to access the shop with a direct link I think.
click here: https://staging.openfoodfrance.org/fricando-hub/shop
and then click here: https://staging.openfoodfrance.org/chop-suey-farm/shop
Look at the pages...
If you refresh the page with the chop suey url, the chop suey shop will load.

luisramos0 · 2020-05-25T17:57:52Z

I cant replicate this in prod. Maybe a problem with the release?

luisramos0 · 2020-05-25T18:01:12Z

I can replicate this in staging FR by clicking around actually 🙈

click /shops in main menu
click fricando shop
click /shops in main menu
click chop seuy shop (I see Fricando shop 🙈 )

luisramos0 added 2 commits May 15, 2020 14:25

Extract method to prepare work ahead

c3f0c0e

Select first default OC only if no OC is already selected

52e7ca2

sigmundpetersen assigned luisramos0 May 15, 2020

luisramos0 added 9 commits May 15, 2020 19:29

If selected OC is not in the available OCs, empty the order

94bb958

Fix problem in spec where wrong enterprise was being used

493adc8

Refactor order.set_order_cycle, return early and remove indentation

6827ce5

Remove dead code

21d1a7b

Rename ResetOrderService to OrderCompletionReset to follow new servic…

ba58506

…e naming convention and also to make it more specific to completed orders

Rename restartCheckout to order_checkout_restart to make it follow se…

35824c7

…rvice naming convention

Extract order_cart_reset service from enterprises_controller

a438317

Adapt spec to validate issue openfoodfoundation#5340

f22eae7

Extract order_cycle_not_listed? method and rename order_cycles to lis…

1c749a8

…ted_order_cycles

luisramos0 force-pushed the grumpy_cat branch from 90d3c8e to 1c749a8 Compare May 15, 2020 18:42

Add unit tests to order_cart_reset

ace73be

luisramos0 marked this pull request as ready for review May 15, 2020 19:24

luisramos0 changed the title ~~Grumpy cat~~ Fix Grumpy Cat on shop page (cart with items from a closed OC) May 15, 2020

Matt-Yorkley reviewed May 18, 2020

View reviewed changes

Matt-Yorkley approved these changes May 18, 2020

View reviewed changes

sauloperez approved these changes May 21, 2020

View reviewed changes

filipefurtad0 self-assigned this May 22, 2020

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels May 22, 2020

filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label May 22, 2020

sauloperez merged commit 3f593d6 into openfoodfoundation:master May 22, 2020

luisramos0 deleted the grumpy_cat branch May 22, 2020 21:20

RachL mentioned this pull request May 25, 2020

Release v2.9.10 Chop suey #5488

Closed

7 tasks

This was referenced May 25, 2020

Switching between shops does not work correctly #5496

Closed

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

Merged

filipefurtad0 mentioned this pull request Jun 9, 2020

Attempting to check out with a closed OC redirects to homepage #5571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Grumpy Cat on shop page (cart with items from a closed OC) #5440

Fix Grumpy Cat on shop page (cart with items from a closed OC) #5440

luisramos0 commented May 15, 2020 •

edited

Loading

Matt-Yorkley May 18, 2020

luisramos0 May 21, 2020

sauloperez May 22, 2020

Matt-Yorkley left a comment

sauloperez May 21, 2020

luisramos0 May 21, 2020

luisramos0 May 21, 2020

sauloperez May 21, 2020

filipefurtad0 commented May 22, 2020 •

edited

Loading

luisramos0 commented May 22, 2020

filipefurtad0 commented May 23, 2020 •

edited

Loading

luisramos0 commented May 25, 2020 •

edited

Loading

luisramos0 commented May 25, 2020 •

edited

Loading

luisramos0 commented May 25, 2020 •

edited

Loading

luisramos0 commented May 25, 2020

luisramos0 commented May 25, 2020

Fix Grumpy Cat on shop page (cart with items from a closed OC) #5440

Fix Grumpy Cat on shop page (cart with items from a closed OC) #5440

Conversation

luisramos0 commented May 15, 2020 • edited Loading

What? Why?

What should we test?

Release notes

Matt-Yorkley May 18, 2020

Choose a reason for hiding this comment

luisramos0 May 21, 2020

Choose a reason for hiding this comment

sauloperez May 22, 2020

Choose a reason for hiding this comment

Matt-Yorkley left a comment

Choose a reason for hiding this comment

sauloperez May 21, 2020

Choose a reason for hiding this comment

luisramos0 May 21, 2020

Choose a reason for hiding this comment

luisramos0 May 21, 2020

Choose a reason for hiding this comment

sauloperez May 21, 2020

Choose a reason for hiding this comment

filipefurtad0 commented May 22, 2020 • edited Loading

luisramos0 commented May 22, 2020

filipefurtad0 commented May 23, 2020 • edited Loading

luisramos0 commented May 25, 2020 • edited Loading

luisramos0 commented May 25, 2020 • edited Loading

luisramos0 commented May 25, 2020 • edited Loading

luisramos0 commented May 25, 2020

luisramos0 commented May 25, 2020

luisramos0 commented May 15, 2020 •

edited

Loading

filipefurtad0 commented May 22, 2020 •

edited

Loading

filipefurtad0 commented May 23, 2020 •

edited

Loading

luisramos0 commented May 25, 2020 •

edited

Loading

luisramos0 commented May 25, 2020 •

edited

Loading

luisramos0 commented May 25, 2020 •

edited

Loading