[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 <david@redcliffs.net>

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 <maikel@email.org.au>

filipefurtad0 · 2024-10-15T18:58:26Z

Hey @rioug,

I can confirm that this PR introduces the "possibility" to select an empty value from the list; However, it is not possible to save these changes, attempting to do so, triggers the warning below:

For comparison, before this PR:

This is valid for new and old variants, in the sense that an empty choice is appearing in the UI in both cases. However, all variants have attributed unit values.

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).

Alright, thank you for pointing this out. I'll open an issue on this. Thanks for all the effort and resilience. Merging (finally!) 🎉

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 19 commits October 14, 2024 15:01

Fix small bug on edit variant page

64f60d1

- 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

630c398

Add stying for the unit pop out

a500c75

Improve variant related validation when creating product

9db4173

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

0695b43

Add all columns moved to variant to ignored_columns

dbca2e2

Fix bulk coop report

377f035

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

Remove non updatable check when updating a product

3bb2232

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

7c2c614

Co-authored-by: David Cook <david@redcliffs.net>

Remove commented out code

2a671d4

Per review, remove the use of raw

ce0c792

Per review, check value are saved in the database

04e14bf

Fix spec to remove reliance on browser's message

755a394

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

ef1f320

Co-authored-by: Maikel <maikel@email.org.au>

Fix rebase issue

40afe7e

Use AdminTooltipComponent, instead of partial

67c1133

Fix invoice print specs

f8eeca8

Post rebase, fix product cloning spec

01337c1

rioug force-pushed the move-variant-unit-attributes-to-variant branch from 00b7f24 to 01337c1 Compare October 14, 2024 04:02

filipefurtad0 self-assigned this Oct 15, 2024

filipefurtad0 added no-staging-AU A tag which does not trigger deployments, indicating a server is being used pr-staged-au staging.openfoodnetwork.org.au and removed no-staging-AU A tag which does not trigger deployments, indicating a server is being used labels Oct 15, 2024

filipefurtad0 merged commit 97b6289 into openfoodfoundation:master Oct 15, 2024
55 checks passed

filipefurtad0 removed the pr-staged-au staging.openfoodnetwork.org.au label Oct 15, 2024

filipefurtad0 mentioned this pull request Oct 15, 2024

[Products page] Empty option on unit scale dropdown #12919

Open

rioug mentioned this pull request Oct 16, 2024

Use Alpine Image for Docker #12905

Open

4 tasks

filipefurtad0 mentioned this pull request Oct 22, 2024

Release v5.0.0 Grünkohl #12920

Closed

12 tasks

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

filipefurtad0 commented Oct 15, 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

[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

filipefurtad0 commented Oct 15, 2024

rioug commented Aug 19, 2024 •

edited

Loading