Load closed shops in a separate request on /shops page #5156

Matt-Yorkley · 2020-04-04T11:25:59Z

What? Why?

Part of #5143

Loads only open shops by default on the /shops page, and loads closed shops separately when the "Show Closed Shops" button is clicked.

What should we test?

/shops page loads a lot faster than before.
Closed shops are displayed as before, after the "Show Closed Shops" button is clicked.
Filtering works as before.

Release notes

Load closed shops separately on /shops page

Changelog Category: Changed

luisramos0 · 2020-04-04T11:54:53Z

This makes me think about API first: what is the enterprises and shops endpoints we want to have?

Maybe we should separate enterprises from shops? An enterprise includes profiles and producers, etc Shops must have respective OrderCycle? Or are all producers closed_shops?

I think the concept of "recently closed shops" is important. An enterprise that never had an OC is very different from an enterprise that just closed it's weekly OC on a Friday and will open in on Tuesday as usual. The API should be prepared to take this.

Maybe we can have enterprises list all enterprises:
/api/enterprises list all enterprises
and then have shops endpoints:
/api/enterprises/shops lists all open shops
/api/enterprises/shops/closed lists all enterprises not ready for checkout

what do you think?

Matt-Yorkley · 2020-04-04T12:41:56Z

Maybe we should separate enterprises from shops?

Yes, I was thinking about looking at that (after making sure it all works). We now have two "shop" actions in api/enterprises_controller now; shopfront (which is basically a show action) and closed_shops. I'm not sure about the best way to arrange them.

Both of these actions require this in the enterprise controller: skip_authorization_check only: [:shopfront, :closed_shops] but all the other actions don't; the shop actions are public but the others are not.

I think the concept of "recently closed shops" is important. An enterprise that never had an OC is very different from an enterprise that just closed it's weekly OC on a Friday and will open in on Tuesday as usual. The API should be prepared to take this.

In theory yes, but we don't currently have anything that needs that specific info from the API, so I'd say YAGNI...

Maybe we can have enterprises list all enterprises: ... /api/enterprises list all enterprises

I don't think we have a current use for that endpoint either...

luisramos0 · 2020-04-04T13:51:05Z

awesome! yes to api/shops_controller!

we dont have /api/enterprises, the list, just because we do inject_enterprises, otherwise we would need it. It's one of those that we should have, it's a very basic feature missing from the OFN api right now: list all enterprises (regardless of open/closed shops).

kirstenalarsen · 2020-04-04T23:34:48Z

agree with @luisramos0 - having enterprises on the API would be ace - not least opening up our abilities to simply pull that information into other, better directory and mapping interfaces - so that we could potentially quickly and cheaply have a much more effective directory function :)

mkllnk

This is great but the filter options are not updating correctly. And three buttons show for a very short time. A loading indicator would be awesome.

mkllnk · 2020-04-07T04:57:33Z

app/controllers/api/shops_controller.rb

+      @active_distributor_ids = []
+      @earliest_closing_times = []


What do we need these for?

These are used in OpenFoodNetwork::EnterpriseInjectionData.new a few lines later, and will trigger big queries (that we don't need in the case of closed shops) unless these two ivars are already set.

…" button

Matt-Yorkley · 2020-04-07T08:44:38Z

@mkllnk

...three buttons show for a very short time

Fixed 👍

A loading indicator would be awesome

Added 👍

the filter options are not updating correctly

Give it another try. The Taxons in "Filter by Type" are loading correctly and working. Are they set up correctly in your environment? Clicking the "Filter by Property" buttons also works correctly...

mkllnk

Wow, that's great.

I'm not sure what my problem with the type filter is. I used production data and it worked fine in master but broke on this branch. When I reset the sample data, it all seems to work fine though. So I hope that it will all be good. Something that should get tested in staging.

sauloperez · 2020-04-08T07:34:24Z

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

@@ -24,7 +24,7 @@ Darkswarm.controller "HubNodeCtrl", ($scope, HashNavigation, CurrentHub, $http,
    $scope.shopfront_loading = true
    $scope.toggle_tab(event)

-    $http.get("/api/enterprises/" + $scope.hub.id + "/shopfront")
+    $http.get("/api/shops/" + $scope.hub.id)


sauloperez · 2020-04-08T07:37:47Z

app/controllers/api/shops_controller.rb

+      render text: Api::EnterpriseShopfrontSerializer.new(enterprise).to_json, status: :ok
+    end
+
+    def closed_shops


IMO this should be a filter specified by URL query param for an API::ShopsController#index so we follow Rails REST conventions. But I want to discuss this in another PR and without any rush. This is awesome as it is.

filipefurtad0 · 2020-04-08T15:26:12Z

Hey @Matt-Yorkley,

I had a look at the upload times of the /shops page, including closed shops, before and after staging this in UK-staging. It was in both cases around 30s, but I noticed my CPUs were over the top, so perhaps the limiting step is my system.

After UK-staging:

Type, Property and Delivery filters worked fine, showing both open and closed shops. I also had a closer look at the "Type" filter, since @mkllnk observed some issues before. I chose different combinations, found no issues.
Closed shops can be toggled, as usual.

Good to go.

Matt-Yorkley self-assigned this Apr 4, 2020

Load closed shops in a separate request on /shops page

af48cac

Matt-Yorkley force-pushed the closed-shops branch from 93ca0bf to af48cac Compare April 4, 2020 12:06

Matt-Yorkley force-pushed the closed-shops branch 2 times, most recently from c59c463 to aa085d1 Compare April 4, 2020 14:43

Add api/shops_controller and refactor

bc859cf

Matt-Yorkley force-pushed the closed-shops branch from aa085d1 to bc859cf Compare April 4, 2020 15:02

mkllnk requested changes Apr 7, 2020

View reviewed changes

Matt-Yorkley added 2 commits April 7, 2020 10:31

Fix flicker effect showing 3 buttons when clicking "Show Closed Shops…

94f8ea2

…" button

Add loading indicator when showing closed shops

003341e

mkllnk approved these changes Apr 8, 2020

View reviewed changes

sauloperez approved these changes Apr 8, 2020

View reviewed changes

filipefurtad0 self-assigned this Apr 8, 2020

filipefurtad0 added pr-staged-es pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-es labels Apr 8, 2020

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Apr 8, 2020

Matt-Yorkley merged commit fc5e346 into openfoodfoundation:master Apr 8, 2020

luisramos0 mentioned this pull request May 22, 2020

Shopfront search box returns all records when none is expected #5467

Open

johansenja mentioned this pull request Aug 29, 2024

Optimise shops page: Only inject distributors with active order cycles #12827

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load closed shops in a separate request on /shops page #5156

Load closed shops in a separate request on /shops page #5156

Matt-Yorkley commented Apr 4, 2020 •

edited

Loading

luisramos0 commented Apr 4, 2020

Matt-Yorkley commented Apr 4, 2020 •

edited

Loading

luisramos0 commented Apr 4, 2020

kirstenalarsen commented Apr 4, 2020

mkllnk left a comment •

edited

Loading

mkllnk Apr 7, 2020

Matt-Yorkley Apr 7, 2020

Matt-Yorkley commented Apr 7, 2020 •

edited

Loading

mkllnk left a comment

sauloperez Apr 8, 2020

sauloperez Apr 8, 2020

filipefurtad0 commented Apr 8, 2020

Load closed shops in a separate request on /shops page #5156

Load closed shops in a separate request on /shops page #5156

Conversation

Matt-Yorkley commented Apr 4, 2020 • edited Loading

What? Why?

What should we test?

Release notes

luisramos0 commented Apr 4, 2020

Matt-Yorkley commented Apr 4, 2020 • edited Loading

luisramos0 commented Apr 4, 2020

kirstenalarsen commented Apr 4, 2020

mkllnk left a comment • edited Loading

Choose a reason for hiding this comment

mkllnk Apr 7, 2020

Choose a reason for hiding this comment

Matt-Yorkley Apr 7, 2020

Choose a reason for hiding this comment

Matt-Yorkley commented Apr 7, 2020 • edited Loading

mkllnk left a comment

Choose a reason for hiding this comment

sauloperez Apr 8, 2020

Choose a reason for hiding this comment

sauloperez Apr 8, 2020

Choose a reason for hiding this comment

filipefurtad0 commented Apr 8, 2020

Matt-Yorkley commented Apr 4, 2020 •

edited

Loading

Matt-Yorkley commented Apr 4, 2020 •

edited

Loading

mkllnk left a comment •

edited

Loading

Matt-Yorkley commented Apr 7, 2020 •

edited

Loading