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.

Sign up for GitHub

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

Jump to bottom

Add redirect to shop on order cycle change #5748

Merged
merged 6 commits into from
Sep 4, 2020
Merged

Add redirect to shop on order cycle change #5748

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7e195c3
Add redirect to shop on order cycle change
Jul 8, 2020
3a722bc
Merge branch 'master' of https://github.com/openfoodfoundation/openfo…
Jul 14, 2020
79abc19
Move location change to PageSelection controller
Jul 15, 2020
429c88c
Add watcher to update filter after products are loaded
Jul 16, 2020
4e635e1
Merge branch 'master' of https://github.com/openfoodfoundation/openfo…
Aug 27, 2020
ef0038a
Change trigger to update filters after ProductsCtrl is loaded.
Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee
View file
Open in desktop
Expand Up @@ -11,14 +11,15 @@ Darkswarm.controller "OrderCycleCtrl", ($scope, $rootScope, $timeout, OrderCycle
$("#order_cycle_id").trigger("openTrigger")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

@mbudm mbudm Jul 16, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍



Darkswarm.controller "OrderCycleChangeCtrl", ($scope, $rootScope, $timeout, OrderCycle, Products, Variants, Cart, ChangeableOrdersAlert) ->
Darkswarm.controller "OrderCycleChangeCtrl", ($scope, $rootScope, $timeout, $location, OrderCycle, Products, Variants, Cart, ChangeableOrdersAlert) ->
# 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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already are handling it in Angular

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
ofn-shop-network-calls-720

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screen Shot 2020-07-15 at 3 36 48 pm

The commits around this area in git blame seem to cover functionality that still works with this code removed:
Screen Shot 2020-07-15 at 3 59 06 pm

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.

$timeout ->
$("#order_cycle_id").trigger("closeTrigger")

Expand Down