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

Make edit order page work even if inventory_items dont have a corresponding line_item in the order #5253

Merged
merged 9 commits into from Apr 23, 2020

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Apr 17, 2020

What? Why?

Improves #4186 as the user will be able to use the edit order page without errors.
The data inconsistency and the underlying problem remains unknown and unresolved.

I have done a big refactoring of order_spec, it was needing one! The actual change is very small.

What should we test?

I can create the situation in 4186 by deleting the line item of an order in the database and keeping the inventory item in there but I am not sure how to do it with the app only. So, I am not sure how we should test this manually...

Release notes

Changelog Category: Fixed
Make edit order page work correctly even if the underlying order data is not consistent in terms of inventory items and line items.

@luisramos0 luisramos0 self-assigned this Apr 17, 2020
@luisramos0 luisramos0 changed the title Edit order Make edit order page work even if inventory_items dont have a corresponding line_item in the order Apr 17, 2020
@luisramos0 luisramos0 marked this pull request as ready for review April 17, 2020 19:30
@luisramos0 luisramos0 added the bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround. label Apr 17, 2020
Copy link
Member

@kristinalim kristinalim left a comment

Choose a reason for hiding this comment

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

Awesome work refactoring the specs, @luisramos0! 🎉

@sauloperez
Copy link
Contributor

I can create the situation in 4186 by deleting the line item of an order in the database

the easiest might be that the tester asks you to do so 🤷

@filipefurtad0 filipefurtad0 self-assigned this Apr 23, 2020
@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Apr 23, 2020
@filipefurtad0
Copy link
Contributor

Hi @luisramos0 !

Thank you so much for access to the FR server and all the help with the SQL commands for the table queries. Amazing stuff.

Here is a description of the process. I verified the bug in katuma- and fr-staging. Then, after staging this PR, proceeded as follows:

  1. ofn-app: created a two product order (two line items), as admin/or customer order, #R656741034 - OK

image

  1. psql console: checked if the the two line items appear
    => select * from spree_line_items where order_id in (select id from spree_orders where number like '#R656741034');

image

So, two line items visible, 336 and 337, variants 3106 and 3104, respectively.

  1. psql console: deleting one of the line items
    => delete from spree_line_items where id in (select max(id) from spree_line_items where order_id in (select id from spree_orders where number like '#R656741034'));

  2. psql console: checking only one of the line items appear (one got deleted) Basically, repeating the command in 2) , item 337 is gone.

image

  1. ofn-app: checking whether it is possible to edit the order.

It is indeed possible to edit the order. However I expected the remaining item to appear. Instead, none appears:

image

Shipping the order, I get the following confirmation email, where both items are shown:

image

However, on the customers side, the order appears as it should - with only the expected item remaining

image

So, in summary:

  • the purpose of the PR was to be able to edit the order and this is fulfilled.

However, there are discrepancies:

  • no items appear for the Admin under that order
  • what is shown in the SQL table matches the order from the customers perspective
  • shipping the order sends a Shipment Notification where the deleted item appears again.

I'm labelling this w/ feedback needed.

@filipefurtad0 filipefurtad0 added feedback-needed pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Apr 23, 2020
@luisramos0
Copy link
Contributor Author

Nice testing!

We are testing a data inconsistency scenario in itself. So, not all things will match... we are not fixing the root cause here.

The customer is seeing the correct info because that page is looking at the line items table 👍
The shipment email is showing both items because the inventory is still there in the shipment (that's the inconsistency that will be seen in these cases.
But I was expecting you to be able to see the remaining line item it in the admin page :-(
I tried it myself and it's not showing the line item for some reason, in my local tests the behavior was different.

Anyway, this is a little better than without the PR, isn't it? One of the now possible solutions here is to cancel the order.
I am going to merge this.
Tomorrow morning I can investigate this better and make another PR if I find some improvement.

@luisramos0 luisramos0 merged commit 2984829 into openfoodfoundation:master Apr 23, 2020
@luisramos0 luisramos0 deleted the edit_order branch April 23, 2020 18:27
@filipefurtad0
Copy link
Contributor

Hey @luisramos0 ,

Thank you for your feedback! Sure, definitively much better this way. I was wondering why the remaining line item did not show up it in the admin page, so I decided to check somewhere else, how it behaved - hopefully it also helps in improving this.

I am happy that it's merged and on time for the release 🎉

@luisramos0
Copy link
Contributor Author

yes, fantastic testing 👏

@luisramos0
Copy link
Contributor Author

ok Filipe, I have found why you were not seeing the other line item, that is fixed here:
#5350

@filipefurtad0
Copy link
Contributor

Nice! Thank you Luis 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants