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.

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

BOM: Ensure orders informations are loaded for each line items #10375

Conversation

jibees
Copy link
Contributor

@jibees jibees commented Feb 3, 2023

What? Why?

This PR:

  • change pagination from orders to line_items: we display on that page line_items and not orders, pagination should reflect that.

  • Load orders that are linked to the line_items displayed in the page and not ones that match pagination, search or whatever (we could actually only link one order if this order as at list 15 line_items ; 15 is the default per_page items count)

  • Closes BOM page is not finding or displaying name, email or order number for some orders #10363

What should we test?

The bug occur if, for the current admin user, there was at least one order with more that one line_items (and therefore pagination was desynchronized). Then the test is pretty simple: go to BOM, and then to page 2. Please ensure you that you could reproduce the bug before staging the PR ;)

Release notes

Changelog Category: User facing changes

This should not be related to any pagination, search or whatever: actually we consolidate line_items with order additional info based on line_items.order.id.
@jibees jibees self-assigned this Feb 3, 2023
@jibees jibees force-pushed the 10363-bom-page-is-not-finding-or-displaying-name-email-or-order-number-for-some-orders branch from d2cb2c1 to bddd887 Compare February 3, 2023 11:25
@sigmundpetersen sigmundpetersen added the priority We focus on this issue right now label Feb 3, 2023
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Code looks good. Great work!

@filipefurtad0 filipefurtad0 self-assigned this Feb 6, 2023
@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Feb 6, 2023
@jibees
Copy link
Contributor Author

jibees commented Feb 7, 2023

Back into In Dev since #10383 raised some questions around automatic specs... Seems like there is a big issue, that should probably be resolved.

EDIT: as it is not reliable also on master, I think this PR can go forward like this. We should be able to solve issues around pagination and specs with the PR #10383 that @filipefurtad0 opened.

@filipefurtad0
Copy link
Contributor

Hey @jibees thanks for fixing this.

I guess the previous test was made on a hub with a lot of orders, and the bug becomes unnoticeable at first sight. It's easily spotted if we look at the last page:

image

We see plenty of missing information, and that pagination information is not correct.

After staging the PR:

image

We can verify that the pagination information is correct, and that there are no orders missing.

I've also tested this PR with the spec on #10383 which considers two orders with plenty of line items.

Right now, the spec is failing because the pagination information refers two results - from two orders - although the UI displays plenty of entries, from line items:

     Failure/Error: expect(page).to have_content "20 Results found. Viewing 1 to 15."
       expected to find text "20 Results found. Viewing 1 to 15." in "Logged in as : esta_fay@tromp.us Account Logout Store DASHBOARD PRODUCTS ORDER CYCLES ORDERS REPORTS CONFIGURATION ENTERPRISES CUSTOMERS GROUPS USERS ORDERS BULK ORDER MANAGEMENT SUBSCRIPTIONS Bulk Order Management What's this? SEARCH PRODUCER All SHOP All ORDER CYCLE All DATE RANGE FILTER RESULTS CLEAR FILTERS ACTIONS COLUMNS 15 per page 2 Results found. Viewing 1 to 2.

Running the spec over this branch passes, so I'd say that shows that your PR fixes the issue 🎉
I'll merge here and rebase #10383.

Thank you!

@audez
Copy link
Collaborator

audez commented Feb 8, 2023

Hi! Awesome! 🙌
Do you think this issue was linked to the following: if you do a “Quick search”, it only filters the items on the page you’re on. For instance, you're on the BOM page on an OC where “Brocolis” have been ordered.
There are other lineitems on page one, and "Brocolis" appear only from page 2.
If you search “Brocolis” while being on page 1, it will display "No orders found", just because this line item was on page 2.
That means that if a producer wants to do adjustments on a specific product, they will have to click on all the pages to apply the filter on each of them.

I don't remember that this problem was here before but I might be wrong 🤔

@filipefurtad0
Copy link
Contributor

Nice, thanks for reporting that @audez 👍
I think it could be related - pagination was scoping orders instead of line items - so I guess the quick search will check which orders, on page xyz were having them on...

I can observe it (as of now) in staging-FR, which I believe has not been updated since this fix was merged. I'll stage master and check the behavior again.

@filipefurtad0
Copy link
Contributor

Humm, seems to still affect master:

searching for "name" - which in this case can be a customer name, but also a line item:

Page 1
image

Page 2
image

Page 3
image

And so on... Ideally these would all appear on the first 15 results, first page. So this seems to be an issue indeed.

Not sure if really a bug, perhaps an improvement? It needs an issue either way. Great catch Aude!

@drummer83
Copy link
Contributor

drummer83 commented Feb 10, 2023

Testing the release v4.2.34 it seems there is still some information missing (sometimes).
Please see below, where 'Producer' and 'Order Cycle' are empty for some lines. All other columns seem to be fixed.
@filipefurtad0 or @jibees could you please double check or if this is something else which has nothing to do with this PR?
Please reopen, if you think it makes sense.

This is on staging AU:
image

@drummer83 drummer83 mentioned this pull request Feb 10, 2023
12 tasks
@jibees
Copy link
Contributor Author

jibees commented Feb 13, 2023

@drummer83 Do you confirm that your find is related to openfoodfoundation/wishlist#402 and not to this one?

@drummer83
Copy link
Contributor

@jibees Yes, I confirm.
It is due to the wishlist #402 and due to #10421. But it's not related to this PR here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority We focus on this issue right now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BOM page is not finding or displaying name, email or order number for some orders
6 participants