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

Show file tree
Hide file tree
Changes from 3 commits
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
9 changes: 0 additions & 9 deletions app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,6 @@ Darkswarm.controller "OrderCycleCtrl", ($scope, $rootScope, $timeout, OrderCycle
$scope.order_cycle = OrderCycle.order_cycle
$scope.OrderCycle = OrderCycle

# 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 'orderCycleSelected'
if !$scope.OrderCycle.selected()
$("#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) ->
# Track previous order cycle id for use with revertOrderCycle()
$scope.previous_order_cycle_id = OrderCycle.order_cycle.order_cycle_id
Expand Down
7 changes: 6 additions & 1 deletion app/assets/javascripts/darkswarm/controllers/page_selection_ctrl.js.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Darkswarm.controller "PageSelectionCtrl", ($scope, $location) ->
Darkswarm.controller "PageSelectionCtrl", ($scope, $rootScope, $location) ->
$scope.selectedPage = ->
# The path looks like `/contact` for the URL `https://ofn.org/shop#/contact`.
# We remove the slash at the beginning.
Expand All @@ -15,3 +15,8 @@ Darkswarm.controller "PageSelectionCtrl", ($scope, $location) ->
$scope.whitelistPages = (pages) ->
$scope.whitelist = pages
$scope.lastPage = pages[0]

# when an order cycle is changed, ensure the shop tab is active to save a click
$rootScope.$on "orderCycleSelected", ->
if $scope.selectedPage() != "shop"
$location.path("shop")