Fix product with ml unit display dl #11741

abdellani · 2023-10-31T18:01:47Z

What? Why?

Closes Products with 'mL' unit displaying as 'dL' on shopfront #11724

What should we test?

In addition to the scenario described in the original issue. I highlighted some fields that require a special focus

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

abdellani · 2023-10-31T18:03:40Z

Do we want to list all the available Units on the product creation form?

mkllnk

Looking good!

Do we want to list all the available Units on the product creation form?

I would display only the configured available units unless someone complains...

mkllnk · 2023-11-01T03:44:32Z

spec/services/weights_and_measures_spec.rb

+      units = WeightsAndMeasures::UNITS.values.flat_map(&:values).pluck("name").sort.uniq
+      expect(units).to eq(all_units.sort.uniq)


If this is the same, shall we call WeightsAndMeasures::UNITS.values.flat_map(&:values).pluck("name") from all_units? Then we don't need to maintain two lists. We just need to make sure that the units are in the right order.

abdellani · 2023-11-01T11:14:05Z

spec/javascripts/unit/admin/services/variant_unit_manager_spec.js.coffee


  describe "compatibleUnitScales", ->
    it "returns a sorted set of compatible scales based on the scale and unit type provided", ->
      expect(VariantUnitManager.compatibleUnitScales(1, "weight")).toEqual [1.0, 1000.0, 1000000.0]
      expect(VariantUnitManager.compatibleUnitScales(453.6, "weight")).toEqual [28.35, 453.6]

+    pending "should load only available unit", ->


this new test is failing.
I don't know how to update availableUnits before running the tests.
I tried directly assigning a new value to VariantUnitManager.availableUnits, but it didn't work.

I'm not familiar with this testing framework to suggest how to fix it directly sorry. But I thought that we could re-arrange it to resolve the problem, so tried this in another commit. Let me know what you think.

dacook

Great work! You identified an inconsistency in the app and changed it to respect the configured preferences, which is a great end result.

I noticed we can use a better way of accessing the preference, so have added a commit for this to demonstrate.
I also thought of a way to make the JS test pass and added another commit.

I think we could tidy up the JS a bit, but don't know how, so I approve of this ;)

dacook · 2023-11-03T01:53:05Z

spec/javascripts/unit/admin/services/variant_unit_manager_spec.js.coffee


  describe "compatibleUnitScales", ->
    it "returns a sorted set of compatible scales based on the scale and unit type provided", ->
      expect(VariantUnitManager.compatibleUnitScales(1, "weight")).toEqual [1.0, 1000.0, 1000000.0]
      expect(VariantUnitManager.compatibleUnitScales(453.6, "weight")).toEqual [28.35, 453.6]

+    pending "should load only available unit", ->


I'm not familiar with this testing framework to suggest how to fix it directly sorry. But I thought that we could re-arrange it to resolve the problem, so tried this in another commit. Let me know what you think.

dacook · 2023-11-03T02:06:53Z

app/assets/javascripts/admin/products/services/variant_unit_manager.js.coffee

+      if availableUnits
+        available = availableUnits.split(",")
+        (parseFloat(scale) for scale, scaleInfo of @units[unitType] when scaleInfo['system'] == scaleSystem and available.includes(scaleInfo['name'])).sort (a, b) ->
+          a - b
+      else
+        (parseFloat(scale) for scale, scaleInfo of @units[unitType] when scaleInfo['system'] == scaleSystem).sort (a, b) ->
+          a - b


It looks like the condition could have been included into the existing code, rather than copy to a separate branch.
In fact, it looks like all functions in this class need to filter by available units.

I think there should be a filtered units list, which would probably be shared with the other functions.
But I have to admit I'm struggling with this kind of Javascript and am not sure how to refactor it!

So if you'd like to try, I'd be very thankful, but don't expect you to!

dacook · 2023-11-03T02:11:55Z

spec/system/admin/product_import_spec.rb

+  before do
+    Spree::Config.set_preference(:available_units, available_units)
+  end


We should actually mock the preference, so that it doesn't leak into other specs. In fact, there's and example below in this file!
It wasn't working because you used a different method for accessing the preference. I've added a commit to change this to use the more standard format (Spree::Config.available_units), and now product_import_spec.rb works without any further changes.

mkllnk

Great improvements.

drummer83 · 2023-11-08T17:56:46Z

Hi @abdellani,
Thanks for working on this important fix! 💪

Before staging

previous (old) products & variants still showed ml
edited and new variants converted volume to dl

After staging

New units in instance configuration settings:
- mg (milligram) ✔️
- cl (centilitre) ✔️
- dl (decilitre) ✔️
- gal (gallon) ✔️
New units in instance configuration settings are inactive by default. ✔️
Previously active/inactive units are still active/inactive after staging. ✔️
Previous units in shopfront are unchanged (e.g. dl), even if the unit is inactive in configuration settings.
- I wonder if a migration to convert dl and cl to ml would make sense? @openfoodfoundation/train-drivers-product-owners ❓
On order summary page and on order confirmation page only active units are being used (e.g. dl is converted to ml) ✔️
Edit order cycle pages needs to be checked again if the unit values of older products are converted to largest suitable unit scale and inactive units are used as well.:question:
On bulk edit products page only the active units are being used. ✔️
On edit product page only the active units are being used. ✔️
On product details page only active unit scales can be selected. ✔️
We again see rounding issues in the shopfront for 700 ml. ❌ (out of scope here, reopened Inconsistent rounding on some units #7504)
Error 500 in shopfront after deactivating certain unit scales (oz, ml in my case) Bugsnag here ❌

I think we should look at the error 500 and think about a migration before merging.

Will move this back to In Dev but we're close.

Thanks again!

…hat are selected on 'Available Units' list

added a test to make sure that all units on WeightsAndMeasures::UNITS are list on all_units

…re selected

And mock the preference value, rather than setting it. Before, the set preference could have leaked to other tests. (I noticed that it was already like this in product_import_spec.rb)

Best viewed with white-space ignored

… untis when needed

abdellani · 2023-11-13T09:14:54Z

Error 500 in shopfront after deactivating certain unit scales (oz, ml in my case) Bugsnag here ❌

Nice catch @drummer83 👍
I fixed that issue now.

Thank you

drummer83 · 2023-11-13T09:42:31Z

@openfoodfoundation/train-drivers-product-owners

Can you please comment on the question about converting existing dl and cl to ml?

lin-d-hop · 2023-11-13T19:52:45Z

So only the new/edited products were updated to dl since the change?
In that case probably not so many are impacted. A migration would be good but more important to get this out fast to minimise products impacted.
Perhaps we get this out, then add a papercut for a migration and instances can choose it if it is bugging them?

dacook

A migration seems like a good idea, because we're confident that dL was not intended on any products. It is possible that someone might intentionally use dL, in which case a future migration could cause an undesired outcome. So I'd suggest it's better to choose now or never.

But it's easy to manually fix and hopefully doesn't affect too many people, so "never" is probably a reasonable option :)

dacook · 2023-11-14T01:15:00Z

app/services/weights_and_measures.rb

+  def scales_for_variant_unit(ignore_available_units: false)
+    return @units[@variant.product.variant_unit] if ignore_available_units
+
+    @units[@variant.product.variant_unit]&.reject { |_scale, unit_info|


This no longer caches the result, but I assume caching wasn't needed anyway. Even if it gets called twice, it seems like an inexpensive process.

drummer83 · 2023-11-16T18:47:26Z

Hi @abdellani,

I have tested my scenarios from above again. Everything is still looking good and I couldn't reproduce the error 500 anymore. 💪

One thing I need to mention: Staging failed on FR server. I assume this is because I had staged the first version of this PR on that server earlier already. It worked well on AU server.

So - I'm not merging this one because I'm not sure about the failed staging. But apart from that this PR is ready to go!

Thanks! 🙏

Ping @filipefurtad0, since you will be drafting the release tomorrow.

filipefurtad0 · 2023-11-16T19:35:26Z

Thanks for testing and the heads-up @drummer83.
On what the deployment concerns: We'll still go through release testing, so if there is something really nasty about it I think we would still catch it there.

I'd propose to still stage on UK-staging (doing so now), and all is well, then merge?

filipefurtad0 · 2023-11-16T19:51:13Z

Ok, deployment was fine on staging-UK:

What do you recon @openfoodfoundation/developers ?

dacook · 2023-11-16T22:41:03Z

I just checked the logs for the fr-staging deployments and it appears to have failed due to this error during asset compilation:

Compiling...
Compilation failed:
Browserslist: caniuse-lite is outdated. Please run:
  npx update-browserslist-db@latest
  Why you should do it regularly: https://github.com/browserslist/update-db#readme

(deprecation warnings follow..)

I think I've seen this before. This is certainly not related to this PR, so we're good to go 👍

But we should pay attention to the warning. I'll give that a try..

filipefurtad0 · 2023-11-17T09:22:22Z

Thanks @dacook ! 🙏

mkllnk reviewed Nov 1, 2023

View reviewed changes

abdellani marked this pull request as ready for review November 1, 2023 11:11

abdellani commented Nov 1, 2023

View reviewed changes

abdellani requested a review from mkllnk November 2, 2023 08:53

sigmundpetersen assigned abdellani Nov 2, 2023

dacook self-requested a review November 3, 2023 00:47

dacook approved these changes Nov 3, 2023

View reviewed changes

mkllnk approved these changes Nov 3, 2023

View reviewed changes

drummer83 self-assigned this Nov 8, 2023

drummer83 added no-staging-UK A tag which does not trigger deployments, indicating a server is being used pr-staged-uk staging.openfoodnetwork.org.uk and removed no-staging-UK A tag which does not trigger deployments, indicating a server is being used labels Nov 8, 2023

drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Nov 8, 2023

drummer83 removed their assignment Nov 8, 2023

abdellani and others added 9 commits November 13, 2023 09:29

update WeightsAndMeasures#scale_for_unit_value to only return units t…

642cd85

…hat are selected on 'Available Units' list

update units list on Spree::Admin::GeneralSettingsHelper#all_units

fb24678

added a test to make sure that all units on WeightsAndMeasures::UNITS are list on all_units

load GeneralSettingsHelper#all_units from WeightsAndMeasures::UNITS

e10d441

update the list of the unit on VariantUnitManager

2733e45

fix existing tests

a9d85fd

update VariantUnitManger specs

864f63b

update VariantUnitManager#compatibleUnitScales to return units that a…

72d8544

…re selected

Use named config method

91cb3df

And mock the preference value, rather than setting it. Before, the set preference could have leaked to other tests. (I noticed that it was already like this in product_import_spec.rb)

Re-arrange spec

b872d7c

Best viewed with white-space ignored

abdellani force-pushed the fix-product-with-ml-unit-display-dl branch 2 times, most recently from 4d51a43 to d1017c6 Compare November 13, 2023 08:35

update WeightsAndMeasures#scales_for_variant_unit to ignore available…

847c3af

… untis when needed

abdellani force-pushed the fix-product-with-ml-unit-display-dl branch from d1017c6 to 847c3af Compare November 13, 2023 08:59

abdellani requested a review from mkllnk November 13, 2023 09:12

abdellani requested a review from dacook November 13, 2023 09:12

dacook approved these changes Nov 14, 2023

View reviewed changes

mkllnk approved these changes Nov 14, 2023

View reviewed changes

drummer83 self-assigned this Nov 16, 2023

drummer83 added feedback-needed and removed pr-staged-au staging.openfoodnetwork.org.au pr-staged-fr staging.coopcircuits.fr labels Nov 16, 2023

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Nov 16, 2023

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Nov 16, 2023

dacook merged commit 2960f75 into openfoodfoundation:master Nov 16, 2023
60 of 62 checks passed

dacook mentioned this pull request Nov 16, 2023

Update browserslist caniuse db #11819

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix product with ml unit display dl #11741

Fix product with ml unit display dl #11741

abdellani commented Oct 31, 2023 •

edited

Loading

abdellani commented Oct 31, 2023

mkllnk left a comment

mkllnk Nov 1, 2023

abdellani Nov 1, 2023

dacook Nov 3, 2023

dacook left a comment

dacook Nov 3, 2023

dacook Nov 3, 2023

dacook Nov 3, 2023

mkllnk left a comment

drummer83 commented Nov 8, 2023

abdellani commented Nov 13, 2023

drummer83 commented Nov 13, 2023

lin-d-hop commented Nov 13, 2023

dacook left a comment

dacook Nov 14, 2023

drummer83 commented Nov 16, 2023

filipefurtad0 commented Nov 16, 2023 •

edited

Loading

filipefurtad0 commented Nov 16, 2023

dacook commented Nov 16, 2023 •

edited

Loading

filipefurtad0 commented Nov 17, 2023

		units = WeightsAndMeasures::UNITS.values.flat_map(&:values).pluck("name").sort.uniq
		expect(units).to eq(all_units.sort.uniq)

Fix product with ml unit display dl #11741

Fix product with ml unit display dl #11741

Conversation

abdellani commented Oct 31, 2023 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

abdellani commented Oct 31, 2023

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Nov 1, 2023

Choose a reason for hiding this comment

abdellani Nov 1, 2023

Choose a reason for hiding this comment

dacook Nov 3, 2023

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook Nov 3, 2023

Choose a reason for hiding this comment

dacook Nov 3, 2023

Choose a reason for hiding this comment

dacook Nov 3, 2023

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

drummer83 commented Nov 8, 2023

Before staging

After staging

abdellani commented Nov 13, 2023

drummer83 commented Nov 13, 2023

lin-d-hop commented Nov 13, 2023

dacook left a comment

Choose a reason for hiding this comment

dacook Nov 14, 2023

Choose a reason for hiding this comment

drummer83 commented Nov 16, 2023

filipefurtad0 commented Nov 16, 2023 • edited Loading

filipefurtad0 commented Nov 16, 2023

dacook commented Nov 16, 2023 • edited Loading

filipefurtad0 commented Nov 17, 2023

abdellani commented Oct 31, 2023 •

edited

Loading

filipefurtad0 commented Nov 16, 2023 •

edited

Loading

dacook commented Nov 16, 2023 •

edited

Loading