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

johansenja · 2024-08-29T11:28:26Z

What? Why?

This follows on from some of my previous changes, to optimise the load time for the /shops page (esp. the UK instance).

When looking at the html on the https://openfoodnetwork.org.uk/shops page, I could see that >4000 shops are being injected - but only a few dozen (tops) are displayed as open and active; there are client-side filters which filter all of them to pick out the active ones (active.js.coffee/closed_shops.js.coffee). From what I can work out, the rest of the shops are superfluous because the closed shops are fetched via API (#5156).

The active attribute (as read by the filters on the front end) is determined by:

    def active
      options[:data].active_distributor_ids&.include? object.id
    end

where active_distributor_ids is determined by:

    def active_distributor_ids
      @active_distributor_ids ||=
        begin
          enterprises = Enterprise.distributors_with_active_order_cycles.ready_for_checkout
          enterprises = enterprises.where(id: @enterprise_ids) if @enterprise_ids.present?
          enterprises.pluck(:id)
        end
    end

so it seems logical to me to move distributors_with_active_order_cycles into the open_shops method, because it's the same logic that will be used to filter the open shops, but just doing it sooner on the back end rather than client-side.

What should we test?

Visit /shops
Check that the expected open shops are listed
Check that the filters are displayed and working as before
Click on "Show closed shops"
Check that the expected closed shops load ok

ShopsListService#open_shops is only used on this page, so it shouldn't have any other knock-on effects

Release notes

Changelog Category (reviewers may add a label for the release notes):

Technical changes only

Dependencies

Documentation updates

sigmundpetersen · 2024-09-02T08:47:40Z

Hi @johansenja , there are some build failures here that seem relevant.
We have also merged some fixes to the build recently, so you could rebase to get a cleaner build 👍

johansenja · 2024-09-03T16:45:30Z

spec/controllers/api/v0/shops_controller_spec.rb

@@ -32,10 +32,19 @@
    end

    describe "#closed_shops" do
+      let!(:hub_open_order_cycle) {


Without this, techinically none of the shops in this test are open & active (as in, visible in the "open shops" on the /shops page)

johansenja · 2024-09-03T16:46:40Z

Hi johansenja , there are some build failures here that seem relevant.
We have also merged some fixes to the build recently, so you could rebase to get a cleaner build 👍

Thanks for the heads-up! Looks l missed some cases but I'm hoping it should be a bit more comprehensive now 🤞🏻

sigmundpetersen · 2024-09-03T17:27:11Z

Yes looks good now.
Simplecov is failing but that is also in master. Maybe you want to have a look at that as well in another PR? :)

johansenja · 2024-09-03T17:55:40Z

I hadn't noticed that previously, but I am hoping #12839 should cover it!

rioug

Looks good ! thanks @johansenja 🙏

So now that we are doing the filtering in the backend, do you think we could remove it from the front end ?

rioug · 2024-09-03T23:30:54Z

app/services/shops_list_service.rb

  def closed_shops
-    shops_list.not_ready_for_checkout.all
+    shops_list.where.not(id: open_shops.reselect("enterprises.id"))


TIL about reselect

🙌🏻 It can be very useful! There is also rewhere, reorder and regroup, though I don't use them so much

rioug · 2024-09-03T23:39:46Z

app/services/shops_list_service.rb

+    shops_list.
+      ready_for_checkout.
+      distributors_with_active_order_cycles


I noticed in active_distributor_ids we do the filtering with order cycle first and then we filter for ready for check out, extract from the method :

enterprises = Enterprise.distributors_with_active_order_cycles.ready_for_checkout

Any reason you are doing it the other way around here ? I don't think it matters by maybe one is more efficient than the other ?

This wasn't an intential choice, but it shouldn't matter too much when it comes to querying the DB because it only affects the order of some of the wheres and joins - and postgres should be smart enough to plan and execute both queries in the same way. On the application side, I think any differences would be marginal

mkllnk

Thank you.

I added some comments but changes are optional.

We should add to the test list though to check that all filters display correctly. We had bugs around that before when working on the shops list.

mkllnk · 2024-09-04T01:06:09Z

spec/services/shops_list_service_spec.rb

+      open_enterprise_ids = Enterprise.distributors_with_active_order_cycles.pluck(:id).to_set
+      expect(open_enterprise_ids).not_to be_empty
+      expect(shops.pluck(:id)).to all be_in open_enterprise_ids


It's good to cover the code but the expectation doesn't achieve much. If you call Enterprise.distributors_with_active_order_cycles in the spec and in the code under test and check if they equal then the test will always pass. A better test would be to manually select the enterprise ids from your test data.

So I guess that distributors[0] was chosen to have an order cycle here. Reading the spec, I would expect that id to be returned. Can we check that in the spec?

mkllnk · 2024-09-04T01:08:09Z

spec/services/shops_list_service_spec.rb

+      open_enterprise_ids = Enterprise.distributors_with_active_order_cycles.pluck(:id).to_set
+      expect(open_enterprise_ids).not_to be_empty
+
+      shops.pluck(:id).each do |shop_id|


Here I would find it much clearer to state something like expect(shops).to eq distributors[1..2].

mkllnk · 2024-09-04T01:12:44Z

app/services/shops_list_service.rb

-    shops_list.ready_for_checkout.all
+    shops_list.
+      ready_for_checkout.
+      distributors_with_active_order_cycles.
+      all


You are showing nice TDD here. While I find that super helpful in development, I think that the spec and the changed code can be in the same commit. Then a reviewer can see straight away which spec is passing for this new code.

Another guideline is to make specs always pass in the commit history. This enables us to run git bisect on a failing spec to see when it started failing. So if you want to commit your spec first, and that's totally legit as well, then you need to declare the spec pending first, so that it doesn't fail. Then when you implement the code, you remove the pending to make it pass properly. That way you clearly mark the spec as now fulfilled with your code change. Sometimes you need several commits to actually make a spec pass and this way you are transparent about when it's done.

Does that make sense to you?

Yes that makes sense - I must say I don't often use pending but it definitely makes sense for cases like this

drummer83 · 2024-09-05T11:04:08Z

Hi @johansenja,
Thanks for continuing your work on this! 💪

I have tested your PR on our staging AU server and it's working well:

Open shops are loading correctly. ✔️
Product category filters are working. ✔️
Shipping methods filters are working. ✔️
Search (shop name, town, etc.) is working. ✔️
Closed shops are loading correctly. ✔️
Note: Closed shops are displayed in a different (random?) order - it's not a big deal, I think.

Other findings

Product categories of closed shops

The displayed filters for type/product category show many blank spaces when closed shops are listed. I assume those are placeholders for categories from the closed shops, which can't load the category name properly.
This was NOT introduced by this PR. We have it in current master as well. However, maybe it was introduced by one of your earlier PRs? I'm absolutely not sure, maybe it has always been there, but if you could check, I think this would be great! 🙏

Inconsistent product categories

The available filters show more product categories than the available shops. I assume that the filters is checking for ALL product categories in use by that shop, while for the shop only the ones in an active order cycle are considered. This can be confusing to customers. I don't think this was introduced by any of your PRs, just mentioning it here because I came across this for the first time. I will create a separate issue for this and check with product what's the desired behavior.

Conclusion

My findings have not been introduced by this PR. All test cases passed successfully.
This one is ready to go! Merging! 🚀
Thanks again for your great contributions! 🥳

github-actions bot assigned johansenja Aug 29, 2024

johansenja changed the title ~~Only inject distributors with active order cycles into shops page~~ Optimise shops page: Only inject distributors with active order cycles into HTML Aug 29, 2024

johansenja changed the title ~~Optimise shops page: Only inject distributors with active order cycles into HTML~~ Optimise shops page: Only inject distributors with active order cycles Aug 29, 2024

johansenja force-pushed the only-fetch-active-open-shops branch 3 times, most recently from 537eba8 to ae7d433 Compare September 3, 2024 16:43

johansenja commented Sep 3, 2024

View reviewed changes

johansenja mentioned this pull request Sep 3, 2024

Enable include-hidden-files for uploading code coverage chunks #12839

Merged

1 task

rioug self-requested a review September 3, 2024 23:22

rioug approved these changes Sep 3, 2024

View reviewed changes

rioug added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Sep 3, 2024

mkllnk approved these changes Sep 4, 2024

View reviewed changes

johansenja added 4 commits September 4, 2024 11:46

Add specs for ShopsListService

cf5e182

Add implementation

6df0b24

Ensure excluded shops are captured by #closed_shops

7d2f3bf

Improve specificity of closed_shops API test data

9cd39d5

johansenja force-pushed the only-fetch-active-open-shops branch from ae7d433 to 9cd39d5 Compare September 4, 2024 10:46

drummer83 self-assigned this Sep 5, 2024

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Sep 5, 2024

drummer83 merged commit cf21c03 into openfoodfoundation:master Sep 5, 2024
53 checks passed

drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Sep 5, 2024

mkllnk mentioned this pull request Sep 20, 2024

Micromarché Hubs on coopcircuits.fr opens very slowly #12870

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johansenja commented Aug 29, 2024 •

edited by mkllnk

Loading

sigmundpetersen commented Sep 2, 2024

johansenja Sep 3, 2024 •

edited

Loading

johansenja commented Sep 3, 2024 •

edited

Loading

sigmundpetersen commented Sep 3, 2024

johansenja commented Sep 3, 2024

rioug left a comment

rioug Sep 3, 2024

johansenja Sep 5, 2024

rioug Sep 3, 2024

johansenja Sep 5, 2024

mkllnk left a comment

mkllnk Sep 4, 2024

mkllnk Sep 4, 2024

mkllnk Sep 4, 2024

johansenja Sep 5, 2024

drummer83 commented Sep 5, 2024

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

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

Conversation

johansenja commented Aug 29, 2024 • edited by mkllnk Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

sigmundpetersen commented Sep 2, 2024

johansenja Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

johansenja commented Sep 3, 2024 • edited Loading

sigmundpetersen commented Sep 3, 2024

johansenja commented Sep 3, 2024

rioug left a 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

mkllnk left a 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

drummer83 commented Sep 5, 2024

Other findings

Product categories of closed shops

Inconsistent product categories

Conclusion

johansenja commented Aug 29, 2024 •

edited by mkllnk

Loading

johansenja Sep 3, 2024 •

edited

Loading

johansenja commented Sep 3, 2024 •

edited

Loading