Paginate Exchange Products API endpoint #4471

luisramos0 · 2019-11-18T14:52:01Z

What? Why?

Follow up from #4455
Closes #4476
This PR makes the OC pages able to handle exchanges with thousands of products by paginating the server queries to list exchange products.

More up to date demo:
http://g.recordit.co/HixXjlxPvk.gif

This PR opens up a few more major possibilities for improving performance, see isues #4480, #4481, #4482, #4487 and #4488

What should we test?

Verify the OC create and edit pages work correctly and specifically validate the list of products is loaded correctly when an exchange has more than 100 products (100 is the size of each page of products).

We should test the interaction between the load all products and the select all buttons in the exchange panel: if we click "select all" all checkboxes of the loaded products should be ticked.
One detail that may not be obvious to the user:

in the incoming settings, when user clicks on "select all" we need to load all products from the server and then select all the checkboxes (this process is done automatically by the page, the user sees a spinner and then all products appear loaded in the page and selected)
in the outgoing settings, when user clicks on "select all" we dont need to load all products before we select all - this is much faster but not all products/checkboxes will be visible in the page (only the first 100 or more if user has manually clicked "load more" or "load all")

Release notes

Changelog Category: Changed
The pages to create and edit Order Cycles can now handle enterprises providing or selling thousands of products. This is accomplished by handling the list of products in small pages instead of handling the full list of products all the time.

Dependencies

This cannot be merged before #4455

mkllnk

Nice. 👍

mkllnk · 2019-11-28T00:19:16Z

app/controllers/api/exchange_products_controller.rb

      {
-        results: results.total_count,
-        pages: results.num_pages,
+        results: paginated_products.total_count,


Is this the same as the count of the new endpoint?

no, this is products count, the new endpoint is variants count.

Matt-Yorkley · 2019-12-06T11:25:17Z

app/assets/javascripts/admin/order_cycles/controllers/incoming_controller.js.coffee

+  $scope.selectAllVariants = (exchange, selected) ->
+    $scope.loadAllExchangeProducts(exchange).then ->
+      $scope.setExchangeVariants(exchange, $scope.suppliedVariants(exchange.enterprise_id), selected)
+      $scope.$apply()


Having to manually call $scope.apply() is a code smell in Angular...

yes, agree. this is the only way I managed to refresh the page after the load process finishes. angular is not triggering a re-render.
anyway, this is supposed to disappear as we implement #4487

Matt-Yorkley · 2019-12-06T11:33:50Z

app/assets/javascripts/admin/order_cycles/controllers/incoming_controller.js.coffee


  $scope.addSupplier = ($event) ->
    $event.preventDefault()
    OrderCycle.addSupplier $scope.new_supplier_id
+
+  # To select all variants we first need to load them all from the server


Is this the best way to handle selecting all variants? Can't we decouple the saving of the "selected" state from loading all the products?

So instead of fetching and loading all the available variants in the exchange first, then sending another request to include all those variants in the exchange (and save it), can't we just have an endpoint on the server-side that handles the task of "select all the available variants in this exchange" and call that directly without doing the first step?

yes! that's exactly #4487

Ah sorry, I wasn't looking at all the issues at once.

no problem, it's good you described the issue in your comment without looking at it :-)

This commit breaks the OC page when there are more than 100 products in an exchange

…oducts

…include summary of number of products loaded already

…ucts before triggering the select all process

… by using the total count of variants to see if all variants are selected or not, even if not all variants are loaded

…ange_products_renderer

…nt to partial templates

luisramos0 · 2019-12-13T11:18:50Z

thanks @RachL
I looked at point 1 now.

Re numbers on the summary "x / y select " - I believe this is pre-existing problem and now I know why it happens. This is caused by products that changed suppliers. There are products selected in the OC that have a different supplier. Supplier caled "bb". If we change supplier again back to Dorv the numbers will become correct again. I think this can go live with this or maybe we should check if it is pre-existing. I'd open a separate issue for this.

re numbers of products - argh I have not thought about this problem! this will confuse the users... but the numbers are correct, above we see number of variants, below we see numbers of products :-(
what shall we do about this?
to change the loaded totals to number of variants instead of products, there's some extra calculation that needs to be done. it's not difficult but will take a few more hours of dev.
We could change from "376 / 400 selected" to "376 / 400 variants selected"

luisramos0 · 2019-12-13T12:22:29Z

Point 2 is a bug, those line should not show up in the simple form where we just load all the products for now.
I am fixing it now.

…include header and footer (product count and pagination) of the non-simple OC form

luisramos0 · 2019-12-13T14:14:21Z

I'd say we can skip code review as I just moved files around. Meanwhile reviews can comment/review if they think is required.
I'd say this is ready for test again: we need to recheck that the panels work on the multi exchange form and that now the simple form works and does not show the totals.

luisramos0 · 2019-12-13T15:38:43Z

I have redepoyed this last version to au staging (now running on ruby 2.3.7).

RachL · 2019-12-17T13:06:46Z

Alright so the error is fix! Moving to ready to go and creating the other issues.

luisramos0 · 2019-12-17T13:42:08Z

great, thanks @RachL
I have merged the first PR and changed the base of this one. I will wait for the master build and the build of this one to be green before I merge this one.

luisramos0 self-assigned this Nov 18, 2019

luisramos0 changed the base branch from master to oc_prods November 18, 2019 14:53

luisramos0 changed the title ~~Paginate exc prods~~ Paginate Exchange Products API endpoint Nov 18, 2019

luisramos0 changed the title ~~Paginate Exchange Products API endpoint~~ WIP Paginate Exchange Products API endpoint Nov 18, 2019

luisramos0 added the pr-wip label Nov 18, 2019

luisramos0 force-pushed the paginate_exc_prods branch 4 times, most recently from 9a44964 to 50237ee Compare November 21, 2019 18:19

luisramos0 force-pushed the oc_prods branch from b7eec49 to 974bae3 Compare November 22, 2019 13:14

luisramos0 force-pushed the paginate_exc_prods branch 2 times, most recently from 8368956 to 99cbbe6 Compare November 22, 2019 13:29

luisramos0 mentioned this pull request Nov 22, 2019

In the OC edit page, load products only when each products tab is opened by user #4455

Merged

luisramos0 force-pushed the paginate_exc_prods branch 4 times, most recently from 9477b55 to dfd9775 Compare November 22, 2019 18:24

This was referenced Nov 24, 2019

Move "select all" on incoming exchanges to a server call #4487

Open

OC performance - move "select all" on outgoing exchanges to a server call #4488

Open

luisramos0 force-pushed the paginate_exc_prods branch 5 times, most recently from 090841f to 8d90e66 Compare November 26, 2019 14:41

luisramos0 changed the title ~~WIP Paginate Exchange Products API endpoint~~ Paginate Exchange Products API endpoint Nov 26, 2019

luisramos0 removed the pr-wip label Nov 26, 2019

RachL added the pr-staged-au staging.openfoodnetwork.org.au label Nov 27, 2019

mkllnk approved these changes Nov 28, 2019

View reviewed changes

Matt-Yorkley reviewed Dec 6, 2019

View reviewed changes

luisramos0 force-pushed the oc_prods branch from 89f83f0 to 9451f1b Compare December 12, 2019 18:45

luisramos0 added 14 commits December 12, 2019 18:48

Paginate exchange products results

d5e42ee

This commit breaks the OC page when there are more than 100 products in an exchange

Add count variants endpoint and use it instead of loading exchange pr…

00478cc

…oducts

Add load more button to list of exchange products

cfe3f72

Add load all prouducts button to list of exchange products

0416521

Add link to load all products at the top of the list of products and …

2f7fd14

…include summary of number of products loaded already

Make select all work again in incoming exchanges by loading alll prod…

1a9c300

…ucts before triggering the select all process

Show exchange products list only after first page of products is loaded

3e5b7eb

Only show load more and load all if not all products are loaded already

a9598c5

Fix select all checkbox state in both incoming and outgoing exchanges…

7e3b6e2

… by using the total count of variants to see if all variants are selected or not, even if not all variants are loaded

Add some basic test coverage to exchange_products_controller and exch…

3264355

…ange_products_renderer

Add spinner when loading all products in an exchange

99e5959

DRY exchange products templates by extracting header and footer conte…

7ad8951

…nt to partial templates

Add spec for exchange products controller pagination

a039ef1

Add feature spec to validate load all and select all exchange products

a104bf8

luisramos0 force-pushed the paginate_exc_prods branch from 8d90e66 to a104bf8 Compare December 12, 2019 18:49

luisramos0 added 2 commits December 13, 2019 12:24

Restructure exchange products panels so that OC simple form does not …

c6af55d

…include header and footer (product count and pagination) of the non-simple OC form

Rename exchange products file names to increase clarity

db71460

luisramos0 force-pushed the paginate_exc_prods branch from 779f717 to db71460 Compare December 13, 2019 12:24

luisramos0 removed the feedback-needed label Dec 13, 2019

RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Dec 17, 2019

luisramos0 changed the base branch from oc_prods to master December 17, 2019 13:40

luisramos0 merged commit 25ded0d into openfoodfoundation:master Dec 17, 2019

luisramos0 deleted the paginate_exc_prods branch December 17, 2019 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paginate Exchange Products API endpoint #4471

Paginate Exchange Products API endpoint #4471

luisramos0 commented Nov 18, 2019 •

edited

Loading

mkllnk left a comment

mkllnk Nov 28, 2019

luisramos0 Nov 28, 2019

Matt-Yorkley Dec 6, 2019

luisramos0 Dec 6, 2019

Matt-Yorkley Dec 6, 2019 •

edited

Loading

luisramos0 Dec 6, 2019

Matt-Yorkley Dec 6, 2019

luisramos0 Dec 6, 2019

luisramos0 commented Dec 13, 2019 •

edited

Loading

luisramos0 commented Dec 13, 2019

luisramos0 commented Dec 13, 2019

luisramos0 commented Dec 13, 2019

RachL commented Dec 17, 2019

luisramos0 commented Dec 17, 2019

Paginate Exchange Products API endpoint #4471

Paginate Exchange Products API endpoint #4471

Conversation

luisramos0 commented Nov 18, 2019 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Nov 28, 2019

Choose a reason for hiding this comment

luisramos0 Nov 28, 2019

Choose a reason for hiding this comment

Matt-Yorkley Dec 6, 2019

Choose a reason for hiding this comment

luisramos0 Dec 6, 2019

Choose a reason for hiding this comment

Matt-Yorkley Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

luisramos0 Dec 6, 2019

Choose a reason for hiding this comment

Matt-Yorkley Dec 6, 2019

Choose a reason for hiding this comment

luisramos0 Dec 6, 2019

Choose a reason for hiding this comment

luisramos0 commented Dec 13, 2019 • edited Loading

luisramos0 commented Dec 13, 2019

luisramos0 commented Dec 13, 2019

luisramos0 commented Dec 13, 2019

RachL commented Dec 17, 2019

luisramos0 commented Dec 17, 2019

luisramos0 commented Nov 18, 2019 •

edited

Loading

Matt-Yorkley Dec 6, 2019 •

edited

Loading

luisramos0 commented Dec 13, 2019 •

edited

Loading