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

[Spree Upgrade] Add on_hand and on_demand to the variants create and edit pages #3613

Merged

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Mar 17, 2019

What? Why?

Closes #3573

shared/_product_tabs

Here I de-deface shared/_product_tabs and remove the new Stock Management option from it.

Variants create/edit Page

In spree v2, on_hand and on_demand were removed from the variants edit page (on_hand here and on_demand here).

These fields were moved to a new submenu/page "Stock Management" but because this new page includes the complexity of stock management, it looks at lot easier (for both the dev and the user!) to just re-add these two fields in the variants edit page. Maybe when/if we make OFN work with multiple stock locations we can move OFN to use the stock management page.

This variants edit page is obviously needing a de-deface. But with the de-deface it will also need more specs and a new angular controller to do what's currently being done in on_demand_script.html.haml.deface for example. It will take some effort so I created a new tech debt issue for it #3615

Meanwhile, in this PR, I added a new deface override to add the two fields back to the page and adapted on_demand_script.html.haml.deface so that it works.
In terms of improvement, I didn't want to waste time improving the overrides so I decided to write feature specs on top of the page that will be valid after #3615 I added the specs in master #3617 these specs pass in master and pass in v2 with this PR (I tested locally and we can validate this when #3617 is merged into 2-0-stable).

What should we test?

The variants edit page should work as normal and the user should be able to manage on_hand and on_demand. These 2 fields should play well with changing the same values on the bulk edit page.
I have tested this locally and it's working correctly.

@luisramos0 luisramos0 self-assigned this Mar 17, 2019
@luisramos0 luisramos0 force-pushed the 2-0-update-variants-stock branch from 65dec6f to caa8cbf Compare March 18, 2019 11:33
@luisramos0 luisramos0 removed the pr-wip label Mar 18, 2019
@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP Add on_hand and on_demand to the variants edit page [Spree Upgrade] Add on_hand and on_demand to the variants edit page Mar 18, 2019
@luisramos0 luisramos0 changed the title [Spree Upgrade] Add on_hand and on_demand to the variants edit page [Spree Upgrade] WIP Add on_hand and on_demand to the variants edit page Mar 18, 2019
@luisramos0 luisramos0 force-pushed the 2-0-update-variants-stock branch from caa8cbf to 3324536 Compare March 18, 2019 18:27
We do not include stock management from v2 because we will use the variants edit page to manage on_hand and on_demand
@luisramos0 luisramos0 force-pushed the 2-0-update-variants-stock branch from 3324536 to 6ad952f Compare March 18, 2019 19:00
…and edit pagei), this is where they used to be in spree v1

The admin variants controller create action is adapted (same as in the admin ProductsController). This is necessary because variant.on_hand and on_demand cannot be mass assigned on creation in v2, see VariantStock for more details.

For now we are hiding the new stock management page that comes in spree v2 as we don't need the complexity added by the management of stock per stock location (only one stock location in ofn v2 for now)
@luisramos0 luisramos0 force-pushed the 2-0-update-variants-stock branch from 6ad952f to 21a10d0 Compare March 18, 2019 19:03
@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP Add on_hand and on_demand to the variants edit page [Spree Upgrade] Add on_hand and on_demand to the variants create and edit pages Mar 18, 2019
@luisramos0 luisramos0 removed the pr-wip label Mar 18, 2019
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.

Nice work! Thank you for doing it so thoroughly.

@luisramos0 luisramos0 added the pr-staged-au staging.openfoodnetwork.org.au label Mar 21, 2019
@lin-d-hop lin-d-hop self-assigned this Mar 22, 2019
@lin-d-hop
Copy link
Contributor

@luisramos0 Tested this one and found that:
As enterprise user nothing saves on the Edit Variant page
As superadmin user everything works as expected
https://docs.google.com/document/d/1K0inimZQxiY-Eh7oc45r_ttpjh23_9TPxA0ydfGhylE/edit#

Not sure whether this is caused by this change or whether this was an existing issue.
Up to you whether this is acceptable to move forward for now or if you want to look into this now. Possibly needs new issue for being able to save anything on the Edit Variant page?

@luisramos0
Copy link
Contributor Author

Thanks @lin-d-hop for testing!
I dont think it's related to user profile. I think it's just the variants, I found Rubhard and Potatoes1 to be broken. Artichoke for example, works well. Can you please confirm this is correct for you as well?

Tech explanation: this is a problem with variants in the system that do not have stock_items. I am not sure how did we end up with variants that do not have stock_items.
I could write code to handle this but I think we need to find out how this is happening.

So @lin-d-hop , I suggest you confirm the error is related to specific variants and not user profiles, and if that's correct we reset the database and try to re-create the problem (variants without stock_items).

@luisramos0
Copy link
Contributor Author

the problem is "related" to this PR (this PR makes the problem more visible) but the problem is not caused by this PR. So, I think we can merge this PR and create a separate issue for this problem.

@lin-d-hop
Copy link
Contributor

Thanks @luisramos0
Rechecked and yes you are right. It's not the profile it is the product.
Updated testing doc to reflect passing tests.
https://docs.google.com/document/d/1K0inimZQxiY-Eh7oc45r_ttpjh23_9TPxA0ydfGhylE/edit#

@luisramos0
Copy link
Contributor Author

luisramos0 commented Mar 25, 2019

ok, thanks!

for reference this is the invalid data:

openfoodnetwork=> select * from spree_variants where deleted_at is null and not exists (select * from spree_stock_items where variant_id = spree_variants.id);
 id | sku | weight | height | width | depth | deleted_at | is_master | product_id | cost_price | position | cost_currency | unit_value | unit_description | display_name | display_as | import_date
 10 | ABC |   0.00 |        |       |       |            | f         |          5 |      17.00 |        2 | AUD           |          1 |                  |              |            |
 22 |     |   0.00 |        |       |       |            | f         |          8 |            |        3 | AUD           |          3 |                  |              |            |
(2 rows)

I have reset the DB in staging AU and tried to replicate the issue by creating and deleting variants in both the bulk edit page and the new variant page... all looks good.
I think we should merge this PR and monitor this to see if it happens again.

@lin-d-hop can we move to ready to go?

@luisramos0
Copy link
Contributor Author

This is blocking #3640 so I am going to merge it. We can open new issue if required.

@luisramos0 luisramos0 removed the pr-staged-au staging.openfoodnetwork.org.au label Mar 26, 2019
@luisramos0 luisramos0 merged commit 9a771ec into openfoodfoundation:2-0-stable Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants