Add redirect to shop on order cycle change #5748

mbudm · 2020-07-08T10:28:41Z

What? Why?

Adds a location change to the shop path when an order cycle is changed.

Not sure if this is the nicest way to do it. I was looking around for something to use other than a straight 'shop' string. I thought there may be a possibility of using the whitelist loaded in app/assets/javascripts/darkswarm/controllers/page_selection_ctrl.js.coffee but that relies on knowing the index order of the shop tab, which is also brittle as it changes depending on the shop settings.

Any ideas on a pattern for providing some authoritative constants in angularJs that are generated by rails?

What should we test?

Navigate to a shop, that has a home page message it will default to the home page.
Select an order cycle
The tabs should change to the shop tab

This should also behave the same if an item has been added to a checkout and the shopper is prompted about clearing their cart.

Navigate to a shop, that has a home page message it will default to the home page.
Select an order cycle
The tabs should change to the shop tab
add an item
navigate to the shop home (or any other tab)
Select another order cycle
confirm the prompt to clear your cart
The tabs should change to the shop tab

Release notes

Redirect to shop when order cycle selected

Changelog Category: Changed

luisramos0 · 2020-07-08T11:48:08Z

app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee

  # Track previous order cycle id for use with revertOrderCycle()
  $scope.previous_order_cycle_id = OrderCycle.order_cycle.order_cycle_id
  $scope.$watch 'order_cycle.order_cycle_id', (newValue, oldValue)->
    $scope.previous_order_cycle_id = oldValue

  $scope.changeOrderCycle = ->
    OrderCycle.push_order_cycle $scope.orderCycleChanged
+    $location.path("shop")


Spree does it with app/views/spree/admin/shared/_routes.html.erb
I dont think we should do routing in angular. The main reason is that we don't really need it and it can get complicated if we go for it.
In this case, I'd do this on the server. For example, making push_order_cycle return a 301 and follow it on the callback?

This changes the hash only, that's done client side no?

I think in this case we already are handling it in Angular, and we update the browser path when different tabs are clicked...

@mbudm it's a call to the server, see app/assets/javascripts/darkswarm/services/order_cycle.js.coffee
that goes to app/controllers/shop_controller.rb#order_cycle

we already are handling it in Angular

Matt, where? I think the actual route is on the server side, isnt it?

@luisramos0 the shop is essentially a SPA. Changing tabs is done with an anchor URL. See: https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/views/shopping_shared/_tabs.html.haml#L12

So changing the hash via javascript is the same as a click on <a href="#/shop">.

The network traffic shows this when changing tabs - it's all XHR (and one PNG). This network traffic is initiated from javascript by the OrderCycleCtrl when the shop tab is rendered. Those calls every time are probably not necessary most of the time as I doubt shops product change that quickly but thats a future optimisation.

What I did find in my travels is that in the admin we are using a tab directive, that has a method for selecting tabs, and broadcasts this change to any listeners. Perhaps this should also be used in the shop as it's nicer than changing the location directly. As an aside this directive uses the same $location.path('tabname') approach (as do a few other examples).

Hey Steve, that tabset directive is used in the frontoffice, it's used in the tabs on the user account page.

That tbaset code is a good example where I dont think we should go. Even on this shopfront page, I think it's best to avoid SPA altogether and keep with rails routes. Simple, it works, we dont need the extra complication.

But I understand that the current routing is done through the fragment/hash already. Because of that I dont think this is a major problem. I think the routing needs to be done at least in an isolated manner though, for example, in a specific service? Having the OrderCyycleChangeController know about routes looks wrong.

Not to mention the fact that we are talking about an angular file with two controllers defined in it...

I am just sharing my concerns and why I think we should make it a little better as we change it.

OK I found a nicer way - using the existing broadcast event of an orderCycleChange, which is picked by by the shop tab controller - generically called PageSelectionCtrl (rename?). To make this work I also removed what seems to be a redundant piece of code that fires this event whenever the OrderCycleCtrl is instantiated.

I suspect that this was a way to open the select box (clue is in the "openTrigger" trigger argument) to prompt users to select an order, which has been superceded by the red style applied to the order cycle selector:

The commits around this area in git blame seem to cover functionality that still works with this code removed:

There is defs a lot more refactoring potential in the shop, and probably a lot more redundant code that is hard to spot at first - it is also high value given this is the crucial customer facing experience. Possibly a candidate if we look at a library upgrade/change.

…odnetwork into issue/5072

luisramos0

Nice, thanks for investigating Steve. Much better 👍

luisramos0 · 2020-07-15T14:03:08Z

app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee

-  $timeout =>
-    $rootScope.$broadcast 'orderCycleSelected'
-    if !$scope.OrderCycle.selected()
-      $("#order_cycle_id").trigger("openTrigger")


there's something related to tooltips with this openTrigger, I dont know what is it...
here: /javascripts/darkswarm/darkswarm.js.coffee

I'm pretty sure the OpenTrigger bit relates to a popover message that has now been removed, so let's ditch that part 👍

I think we still need the timeout and $rootScope.$broadcast 'orderCycleSelected' bit here though, it's used elsewhere. It triggers the shop's list of product filters to be loaded or refreshed for example (which is causing the 2 failing specs in the current build).

See: app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee:16. Maybe it could be adjusted to fit your needs here?

ok, so lets remove that line in javascripts/darkswarm/darkswarm.js.coffee as well?

@Matt-Yorkley The problem with broadcasting 'orderCycleSelected' every time the OrderCycleCtrl is initiated is that this occurs every time a tab is loaded, so the user is always navigated to /shop.

So I've pushed an alternate way of ensuring update_filters() is run after a product load, using a watcher on the Product.loading flag.

Alternatively I could leave this timeout and use a different event, (naming is hard) and fire update_filter() from that event:

# Timeout forces this to be evaluated after everything is loaded # This is a hack. We should probably write our own "popover" directive # That takes an expression instead of a trigger, and binds to that $timeout => $rootScope.$broadcast 'orderCycleChangeReady'

@luisramos0 I also removed the other redundant tooltipProvider reference you mentioned.

Also I'm getting different results with tests locally so will see how this lates commit goes, in theory it should pass as the filters are now updating again.

EDIT: they passed, just the variant_override problem spec failing

I've pushed an alternate way of ensuring update_filters() is run...

I think this is moving in the right direction, but in this case the value of Product.loading can change many times while the user is scrolling through the product list, submitting a search, selecting filters etc, but we need to be calling update_filters() only once, when an order cycle is selected...

Yes I was afraid of that, hence the alternate suggestion. My question on update_filters being triggered too often is, can. the filters change as more products are loaded? It seems likely as I assume the filters shown are only relevant to what is loaded so far...?

Nonetheless.

Update_filters() is called when the order cycle changes, via the existing broadcast.

The remaining problem is when the page is loaded and an order_cycle is already selected.

I'll look at this next week, there must be some safe/more appropriate hook for calling update_filters, my alternate solution, reinstating the timeout in the unrelated OrderCycleCtrl doesn't feel like the best option - it smells like potential edge case race condition.

The filters are per order cycle, so they won't change when more products are loaded for the same OC.

I'm sure there's a nice way to resolve it 👍

Also remove the other redundant trigger

daniellemoorhead · 2020-08-14T05:52:57Z

Hey @mbudm any movement on this PR?

…odnetwork into issue/5072

mbudm · 2020-08-27T08:03:39Z

Keeping it simple, using a $timeout to trigger the update filters after the Products Controller is loaded.

daniellemoorhead · 2020-08-28T05:40:01Z

@Matt-Yorkley @luisramos0 @filipefurtad0 would be great to get this in the next release if we can - any chance of getting the code reviews and testing done before then? Isn't difficult to test and this is the second round of code review so hopefully will be quick 🤞

Filipe, I'll be asleep by the time this gets reviewed (if that's today) thus why I'm hoping you can do it 🙏

luisramos0 · 2020-09-01T11:08:38Z

I am ok with this if it works 👍

daniellemoorhead · 2020-09-04T00:58:14Z

@luisramos0 do we need to wait for @Matt-Yorkley to re-review or is it a small enough change to come straight back to testing?

mkllnk

This seems to do the trick. 👍

daniellemoorhead · 2020-09-04T06:27:36Z

Testing notes:

Tested on:

Mac / Chrome + Safari
Windows 10 / Chrome

Works a treat:

When on another tab, I select the order cycle for the first time. It takes me to the shop tab.
I then go to another tab, and then select the different order cycle option. It takes me to the shop tab.
Same happens when I add products, go to another tab, change order cycle, then confirm the change will remove the products from my cart. It takes me to the shop tab.

Good to go! @luisramos0 perhaps this can go into the release for next week?

Add redirect to shop on order cycle change

7e195c3

mbudm self-assigned this Jul 8, 2020

luisramos0 reviewed Jul 8, 2020

View reviewed changes

Steve Roberts added 2 commits July 14, 2020 13:31

Merge branch 'master' of https://github.com/openfoodfoundation/openfo…

3a722bc

…odnetwork into issue/5072

Move location change to PageSelection controller

79abc19

luisramos0 approved these changes Jul 15, 2020

View reviewed changes

Add watcher to update filter after products are loaded

429c88c

Also remove the other redundant trigger

Steve Roberts added 2 commits August 27, 2020 14:48

Merge branch 'master' of https://github.com/openfoodfoundation/openfo…

4e635e1

…odnetwork into issue/5072

Change trigger to update filters after ProductsCtrl is loaded.

ef0038a

sigmundpetersen requested a review from Matt-Yorkley September 1, 2020 10:32

luisramos0 approved these changes Sep 1, 2020

View reviewed changes

mkllnk approved these changes Sep 4, 2020

View reviewed changes

daniellemoorhead added pr-staged-fr staging.coopcircuits.fr pr-staged-es and removed pr-staged-fr staging.coopcircuits.fr labels Sep 4, 2020

daniellemoorhead self-assigned this Sep 4, 2020

luisramos0 merged commit 4fe24da into openfoodfoundation:master Sep 4, 2020

sigmundpetersen removed the pr-staged-es label Sep 4, 2020

filipefurtad0 mentioned this pull request Jan 24, 2024

Selecting OC redirects to shop's home page #12076

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add redirect to shop on order cycle change #5748

Add redirect to shop on order cycle change #5748

mbudm commented Jul 8, 2020

luisramos0 Jul 8, 2020

mbudm Jul 8, 2020

Matt-Yorkley Jul 8, 2020

luisramos0 Jul 8, 2020

luisramos0 Jul 8, 2020

mbudm Jul 14, 2020

luisramos0 Jul 14, 2020 •

edited

mbudm Jul 15, 2020

luisramos0 left a comment

luisramos0 Jul 15, 2020

Matt-Yorkley Jul 15, 2020 •

edited

luisramos0 Jul 15, 2020

mbudm Jul 16, 2020

mbudm Jul 16, 2020 •

edited

Matt-Yorkley Jul 16, 2020 •

edited

mbudm Jul 17, 2020

Matt-Yorkley Jul 17, 2020

daniellemoorhead commented Aug 14, 2020

mbudm commented Aug 27, 2020

daniellemoorhead commented Aug 28, 2020 •

edited

luisramos0 commented Sep 1, 2020

daniellemoorhead commented Sep 4, 2020

mkllnk left a comment

daniellemoorhead commented Sep 4, 2020 •

edited

Add redirect to shop on order cycle change #5748

Add redirect to shop on order cycle change #5748

Conversation

mbudm commented Jul 8, 2020

What? Why?

What should we test?

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 Jul 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley Jul 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbudm Jul 16, 2020 • edited

Choose a reason for hiding this comment

Matt-Yorkley Jul 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniellemoorhead commented Aug 14, 2020

mbudm commented Aug 27, 2020

daniellemoorhead commented Aug 28, 2020 • edited

luisramos0 commented Sep 1, 2020

daniellemoorhead commented Sep 4, 2020

mkllnk left a comment

Choose a reason for hiding this comment

daniellemoorhead commented Sep 4, 2020 • edited

Testing notes:

luisramos0 Jul 14, 2020 •

edited

Matt-Yorkley Jul 15, 2020 •

edited

mbudm Jul 16, 2020 •

edited

Matt-Yorkley Jul 16, 2020 •

edited

daniellemoorhead commented Aug 28, 2020 •

edited

daniellemoorhead commented Sep 4, 2020 •

edited