[Product Refactor] Move variant unit sizes to variant #12787

rioug · 2024-08-19T04:46:54Z

⚠️ Please use clockify code #9105 REST API - Products when working on this ⚠️

What? Why?

Last step of the first part of product refactor #9069
Move variant unit sizes to variant :

variant_unit,
variant_unit_scale,
variant_unit_name

Review Note:

We should really refactor app/webpacker/controllers/variant_controller.js and app/webpacker/controllers/variant_controller.js to extract anything that's common for the "unit popout". We could probably move it to component. This PR is already big enough so I did not want to do it here.

What should we test?

Anything that touches unit sizes (non exhaustive list), check that unit sizes are displayed properly and can be updated when applicable :

Create a new Product
Create a new Variant
Update a variant
Bulk product edit page legacy, create/update variant
Bulk product edit page create/update variant
Inventory page
Producer shop page
order and fulfilment report

Update variant page, I replaced angular with StimulusJS, beside checking unit sizes check there is no feature loss.

Release notes

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

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

dacook

Reviewed first half(up to "Prettify javascript").

Sorry lots of comments, mostly just comprehension but I have a few queries as well.

db/migrate/20240618013850_add_variant_unit_to_variant.rb

spec/services/variant_units/option_value_namer_spec.rb

app/models/spree/variant.rb

app/models/product_import/entry_validator.rb

dacook · 2024-08-20T01:07:05Z

spec/services/sets/product_set_spec.rb

+          # updating variant doesn't increment saved_count
+          # expect(product_set.saved_count).to eq 1


🤔 Hmm, it probably should..

Indeed, this PR was not the place for it though

spec/javascripts/unit/admin/products/services/option_value_namer_spec.js.coffee

app/assets/javascripts/admin/directives/maintain_unit_scale.js.coffee

app/assets/javascripts/admin/bulk_product_update.js.coffee

mkllnk

Review in progress:

Fix DFC supplied product builder

db/migrate/20240618013850_add_variant_unit_to_variant.rb

mkllnk

Epic! Great that you improved the code along the way and fixed little things.

I have to admit that I didn't read every line. But it looks pretty robust.

app/models/spree/product.rb

dacook

Phew, done!
I have several comments, probably not all of them need changes, but I think some do (whether now or in a follow-up PR).
I will review my own review and mark any required changes with a ± in the comment.

dacook · 2024-08-26T03:25:32Z

app/webpacker/js/services/price_parser.js

+    price = parseFloat(price);
+
+    if (isNaN(price)) {
+      return null;
+    }
+
+    return price;


This code made sense in the beautifully terse coffeescript, but the ugly JS caused me to wonder if this works just as well:

Suggested change

price = parseFloat(price);

if (isNaN(price)) {

return null;

}

return price;

return parseFloat(price) || null

It's not 100% the same logic, but the specs should be able to tell us if it matters or not.

app/webpacker/controllers/edit_variant_controller.js

spec/lib/spree/core/product_duplicator_spec.rb

spec/system/admin/products_v3/actions_spec.rb

spec/system/admin/products_v3/update_spec.rb

app/webpacker/controllers/edit_variant_controller.js

app/webpacker/css/admin_v3/pages/edit_variant.scss

app/models/spree/product.rb

dacook

I've added some ± and one ❓ that I think require addressing, can you please check these?
Other comments might be worth addressing too, but I don't consider them blocking.

spec/system/admin/products_v3/update_spec.rb

dacook

🏅 Awesome, an epic job!

drummer83 · 2024-08-29T16:29:23Z

@rioug There's a conflict here, sorry. Could you please have a look?
Also Github is asking for more reviews. Do we need that?

rioug · 2024-09-02T05:33:04Z

@drummer83 it's now ready to go.

mkllnk · 2024-09-03T01:23:44Z

@rioug, sorry, just created another merge conflict.

rioug · 2024-09-03T05:05:33Z

@rioug, sorry, just created another merge conflict.

Sorted !

rioug · 2024-09-03T06:30:04Z

Simplecov failed, I think it's because it re ran only the failed jobs, maybe something we need to look into.

Anyway, it doesn't block merging.

drummer83 · 2024-09-04T16:43:17Z

Hi @rioug,

I'm sorry, I'm getting an error 500 on the /admin/products page on staging AU.
Bugsnag is here: https://app.bugsnag.com/yaycode/openfoodnetwork-aus/errors?filters[event.since]=30d&filters[error.status]=open&filters[app.release_stage]=staging

I'll move this back to In Progress until fixed. 🙏

- make sure the weight is only cleared when needed - make sure the displayed unit is up to date

I disabled Metrics/AbcSize for ensure_standard_variant as I don't think that's hard to understand the code. And utimately it will be removed once product actually becomes optional.

The current spec is useless, but it has been addressed on master

After the product redactor it only checked for the "description" on product, which is actually skipped when doing an update.

Co-authored-by: David Cook <[email protected]>

Client side validation messages depend on the browser's locale, which we have no controll over. Now we just check a message is set.

There is no easy way to make the original product invalid, but we can make sure the cloned product will be invalid. The cloned product add "COPe OF " in front of the product's name, so by starting with a name that's long enough, the cloned product will have a name longer that 255 char and will then be invalid.

Co-authored-by: Maikel <[email protected]>

rioug · 2024-09-30T02:04:28Z

Hi @filipefurtad0,
I am not sure how you "deleted the migration" but is look likes it wasn't done from a database perspective. I fixed the database on both au-staging and fr-staging.

FYI if you need to retest the migration, while this PR is staged, you can run the following to revert the migration done by the PR:

bundle exec rails db:migrate:down VERSION=20240819045115
bundle exec rails db:migrate:down VERSION=20240618013850

However, I can see some issues pointed by @drummer83 are still relevant, like being able to select an empty unit value, when creating an product in the products page:

To be clear this has not been fixed so it's expected to be able to select an empty unit value (Rachel is happy for this to be fixed later on). However, if a variant was created before the PR, it shouldn't have an empty unit value after the PR has been staged.

github-actions bot assigned rioug Aug 19, 2024

rioug force-pushed the move-variant-unit-attributes-to-variant branch from 29d26d0 to 07c2b4e Compare August 19, 2024 05:41

rioug marked this pull request as ready for review August 19, 2024 05:48

dacook self-requested a review August 20, 2024 01:05

dacook reviewed Aug 20, 2024

View reviewed changes

mkllnk reviewed Aug 20, 2024

View reviewed changes

db/migrate/20240618013850_add_variant_unit_to_variant.rb Show resolved Hide resolved

mkllnk added the api changes These pull requests change the API and can break integrations label Aug 21, 2024

mkllnk approved these changes Aug 21, 2024

View reviewed changes

app/models/spree/product.rb Outdated Show resolved Hide resolved

app/models/spree/product.rb Show resolved Hide resolved

dacook reviewed Aug 26, 2024

View reviewed changes

dacook requested changes Aug 26, 2024

View reviewed changes

rioug force-pushed the move-variant-unit-attributes-to-variant branch from 333e7bc to 57eee15 Compare August 27, 2024 01:07

rioug requested review from dacook and mkllnk August 27, 2024 01:22

mkllnk approved these changes Aug 27, 2024

View reviewed changes

spec/system/admin/products_v3/update_spec.rb Outdated Show resolved Hide resolved

dacook approved these changes Aug 27, 2024

View reviewed changes

dacook added the priority We focus on this issue right now label Aug 27, 2024

drummer83 added the feedback-needed label Aug 29, 2024

rioug force-pushed the move-variant-unit-attributes-to-variant branch from d10359a to a2c4fd5 Compare September 2, 2024 05:24

dacook approved these changes Sep 2, 2024

View reviewed changes

sigmundpetersen removed the feedback-needed label Sep 2, 2024

rioug force-pushed the move-variant-unit-attributes-to-variant branch from a2c4fd5 to 76cd3e2 Compare September 3, 2024 05:05

drummer83 self-assigned this Sep 4, 2024

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

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

rioug and others added 25 commits September 30, 2024 11:46

Fix legacy bulk edit product system spec

7a152f5

Post rebase fix product import system spec

d562b97

Fix Unit price system spec

e45cb88

Fix minor differences in local env and CI

095ea00

Fix orders and fulfillment report

caf86b3

Fix bulk order management page

7f23d4b

Fix product import controller

2e5a3eb

Fix small bug on edit variant page

0959202

- make sure the weight is only cleared when needed - make sure the displayed unit is up to date

Move unit popout css to a partial

5e56c1d

Add stying for the unit pop out

9cf4889

Improve variant related validation when creating product

b6a7a5f

I disabled Metrics/AbcSize for ensure_standard_variant as I don't think that's hard to understand the code. And utimately it will be removed once product actually becomes optional.

Fix rebase issue

cfd8e74

Add all columns moved to variant to ignored_columns

b36fdf8

Fix bulk coop report

98cd1b1

The current spec is useless, but it has been addressed on master

Remove non updatable check when updating a product

cd077fb

After the product redactor it only checked for the "description" on product, which is actually skipped when doing an update.

Update spec/models/spree/variant_spec.rb

7fcbf04

Co-authored-by: David Cook <[email protected]>

Remove commented out code

fcad8ac

Per review, remove the use of raw

22e7300

Per review, check value are saved in the database

3a31d76

Fix spec to remove reliance on browser's message

32e02dd

Client side validation messages depend on the browser's locale, which we have no controll over. Now we just check a message is set.

Update spec/system/admin/products_v3/update_spec.rb

25c8f87

Co-authored-by: Maikel <[email protected]>

Fix rebase issue

2618b4e

Use AdminTooltipComponent, instead of partial

27dfac7

Fix invoice print specs

ee4f775

rioug force-pushed the move-variant-unit-attributes-to-variant branch from 46d9ca7 to ee4f775 Compare September 30, 2024 01:47

Post rebase, fix product cloning spec

00b7f24

rioug removed pr-staged-au staging.openfoodnetwork.org.au feedback-needed labels Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Product Refactor] Move variant unit sizes to variant #12787

[Product Refactor] Move variant unit sizes to variant #12787

rioug commented Aug 19, 2024 •

edited

Loading

dacook left a comment

dacook Aug 20, 2024

rioug Aug 26, 2024

mkllnk left a comment

mkllnk left a comment

dacook left a comment

dacook Aug 26, 2024

dacook left a comment

dacook left a comment

drummer83 commented Aug 29, 2024

rioug commented Sep 2, 2024

mkllnk commented Sep 3, 2024

rioug commented Sep 3, 2024

rioug commented Sep 3, 2024

drummer83 commented Sep 4, 2024

rioug commented Sep 30, 2024

		# updating variant doesn't increment saved_count
		# expect(product_set.saved_count).to eq 1

[Product Refactor] Move variant unit sizes to variant #12787

Are you sure you want to change the base?

[Product Refactor] Move variant unit sizes to variant #12787

Conversation

rioug commented Aug 19, 2024 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

dacook left a comment

Choose a reason for hiding this comment

dacook Aug 20, 2024

Choose a reason for hiding this comment

rioug Aug 26, 2024

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook Aug 26, 2024

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

drummer83 commented Aug 29, 2024

rioug commented Sep 2, 2024

mkllnk commented Sep 3, 2024

rioug commented Sep 3, 2024

rioug commented Sep 3, 2024

drummer83 commented Sep 4, 2024

rioug commented Sep 30, 2024

rioug commented Aug 19, 2024 •

edited

Loading