[DFC API] Import known units when creating new products #11377

mkllnk · 2023-08-10T04:09:55Z

ℹ️ This is a funded feature. Please use the Clockify project with the following name when working on this, including review and test: #9170 OFN DFC Products

What? Why?

Whenever the DFC Prototype is updated, it's an opportunity to verify our implementation and update the specs.

Using DFC Connector for SuppliedProduct updates.
Updating name and unit as well.

Then adding support for most DFC units.

What should we test?

Specs only.

Release notes

Changelog Category: Technical changes

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

Dependencies

Documentation updates

I observed new data from the DFC Prototype. It now uses the DFC 1.8 ontology with the hasQuantity object. It now also uses PUT requests for updates because PATCH is not as well supported. Rails doesn't care though. I couldn't observe a request for the CatalogItem yet because the Prototype failed to send it.

Re-uses existing code and takes knowledge out of the controller.

I also updated the comments.

dacook

Great, I just noticed that the Tonne line needs fixing, and have a query about how the unit names are set.

dacook · 2023-08-30T06:53:10Z

engines/dfc_provider/spec/services/quantitative_value_builder_spec.rb

+      builder.apply(quantity, product)
+
+      expect(product.variant_unit).to eq "volume"
+      expect(product.variant_unit_name).to eq "liter"


I realise this is testing existing code, but I noticed the US spelling and was curious to see where this comes from.

I have to admit I always find it rather confusing how all the variant unit bits fit together, but I think I've narrowed it down to find that variant_unit_name is normally set with the values here: https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/services/weights_and_measures.rb#L35

IE it looks like a litre should be an (unambiguous?) "L"

Oh, you found the source of truth! I should have looked for that. You are right about the unit naming.

dacook · 2023-08-30T07:05:16Z

engines/dfc_provider/app/services/quantitative_value_builder.rb

@@ -78,6 +77,8 @@ def self.apply(quantity, product)
        ["weight", "gram", 1]
      when quantity_unit.KILOGRAM
        ["weight", "kg", 1_000]
+      when quantity_unit.TONNE
+        ["weight", "kg", 1_000_000]


I think this should be "T" (as in WeightsAndMeasures)

Yes you are right.

And I'm just realising another issue. I chose the most accurate values for imperial measures, thinking that it can always be rounded later, but WeightsAndMeasures uses rounded values as lookup keys. Well, I guess, we can add the rounding to that class to help it recognise these measures.

engines/dfc_provider/app/services/quantitative_value_builder.rb

We still have the scale stored in two places but in our current system that's part of a unit's "id". If the DFC adds that value to its standard then we can use it for lookup and don't need to repeat it. * datafoodconsortium/taxonomies#7

dacook

Great work, and on raising the specific issues with DFC too. It's interesting to learn more about the measurements of the world!

I just have a couple of suggestions...

dacook · 2023-08-30T23:59:26Z

app/services/weights_and_measures.rb

+      28.349523125 => { 'name' => 'oz', 'system' => 'imperial' },
      28.35 => { 'name' => 'oz', 'system' => 'imperial' },
+      453.59237 => { 'name' => 'lb', 'system' => 'imperial' },
      453.6 => { 'name' => 'lb', 'system' => 'imperial' },


Cool, so I guess this means we recognise both a) the more specific unit scale imported from DFC, and b) the less specific unit from the OFN admin interface or product importer.
It might be helpful to have a code comment to explain why there's two:

Suggested change

28.349523125 => { 'name' => 'oz', 'system' => 'imperial' },

28.35 => { 'name' => 'oz', 'system' => 'imperial' },

453.59237 => { 'name' => 'lb', 'system' => 'imperial' },

453.6 => { 'name' => 'lb', 'system' => 'imperial' },

28.349523125 => { 'name' => 'oz', 'system' => 'imperial' }, # More specific value imported from DFC

28.35 => { 'name' => 'oz', 'system' => 'imperial' },

453.59237 => { 'name' => 'lb', 'system' => 'imperial' }, # More specific value imported from DFC

453.6 => { 'name' => 'lb', 'system' => 'imperial' },

dacook · 2023-08-31T00:02:35Z

app/services/weights_and_measures.rb

-      1000.0 => { 'name' => 'kL', 'system' => 'metric' }
+      1000.0 => { 'name' => 'kL', 'system' => 'metric' },
+
+      0.25 => { 'name' => 'cu', 'system' => 'imperial' },


As you have pointed out, this is actually a metric cup, so should we not label it as such?

Suggested change

0.25 => { 'name' => 'cu', 'system' => 'imperial' },

0.25 => { 'name' => 'cu', 'system' => 'metric' },

This is tough. It's an imperial measure made to fit into the metric system. It's not included in the metric system which only uses multipliers of 10. But it's not an old imperial unit either. It's mainly used in Australia, Canada and New Zealand. It probably sits somewhere in between and belongs to neither, the imperial system or the metric system.

Although derived from the metric system, it is not an SI unit.[7]

https://en.wikipedia.org/wiki/Cup_(unit)

What do you think?

The other question is: does it matter? Now I came across the UnitPrice service which seems to assume that a variant's unit_value is always expressed in grams, litres, pounds or items. I actually find that class super confusing and I'm not sure it's covering all the cases or why we have to convert this way. When a product is created, the user chooses the unit and that should be enough, shouldn't it?

According to that logic, it does look safer to assign the metric system to a cup so that it doesn't assume it to be a weight. It looks like there's some bigger picture clean-up to do.

Hmm interesting. It's still commonly labelled "metric cup" which I would consider close enough.

But does it matter? No, in fact I would suggest we don't bother supporting this unit at all. We've clearly come this far without it.
Until it's clarified, or as you suggest we avoid scaling it and just treat it as a label, we probably shouldn't implement it.

Another thought: if "cup" is just a label that doesn't have a standard meaning, then why don't we import it as an items label? Could this work?

when quantity_unit.CUP # Interpreted as a label, because this is not an internationally measurable unit. # https://github.com/datafoodconsortium/taxonomies/issues/8 ["items", "cup", 1]

But does it matter? No, in fact I would suggest we don't bother supporting this unit at all.

Of course, thank you for thinking outside the cup and identifying this issue clearly as storm in a cup.

I also like your suggestion of importing the label. But there are a lot more DFC measures with a label we just import as generic "items". I would like to change that logic in general to use a label for all "items".

The DFC doesn't actually specify which cup it means. I don't expect anyone providing "cup" as unit to measure their produce and expect it to calculate as a regular volume. It can just be seen as items.

dacook

Ah, even better! 💯

dacook

Wait, there's a spec to fix :(

     NoMethodError:
       undefined method `semanticId' for "dfc-m:Gram":String
     
             label = unit.semanticId.split("#").last || "items"
                         ^^^^^^^^^^^
     # ./engines/dfc_provider/app/services/quantitative_value_builder.rb:113:in `map_unit'

engines/dfc_provider/app/services/quantitative_value_builder.rb

drummer83 · 2023-09-07T06:40:55Z

Hi @mkllnk,
by

What should we test?

Specs only.

do you mean there is no manual testing needed?
Or is there anything Filipe should do about the spec files?

mkllnk · 2023-09-07T23:05:17Z

@drummer83

What should we test?

This doesn't need testing. There's actually no other DFC software to test this with. So I'll merge this straight away.

RachL · 2023-09-08T07:08:29Z

@mkllnk actually we can test with demo.socleo.org on the other end. I will play with it a bit next week :)

Also I think we should have soon both UK and FR staging connected with the prototype if I understood correctly ping @RaggedStaff @lin-d-hop

mkllnk · 2023-09-11T00:28:52Z

@RachL

Also I think we should have soon both UK and FR staging connected with the prototype

Yes, that's deployed. We could actually try to transfer products form one platform to another. I haven't tested that yet. Would be interesting what you find.

mkllnk self-assigned this Aug 10, 2023

mkllnk added 10 commits August 28, 2023 17:13

Use DFC Connector for SuppliedProduct update

9ab75e0

Encapsulate DFC-OFN data model bridge

96b79bc

Re-uses existing code and takes knowledge out of the controller.

Spec current quantity unit conversion

996f3bb

Import DFC kilogram

5a8d95c

Import DFC milligram

9ddf536

Import imperial DFC units

43346e2

Import all metric DFC units

f096783

Import DFC bundle units like dozen or 6 pack

f96ab7c

Refactor DFC unit mapping for easier reading

8114430

I also updated the comments.

mkllnk force-pushed the dfc-update-request branch from 5b3232b to 8114430 Compare August 28, 2023 07:13

mkllnk changed the title ~~Update DFC SuppliedProduct with latest version 1.8 request~~ [DFC API] Import known units when creating new products Aug 28, 2023

mkllnk added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Aug 28, 2023

mkllnk marked this pull request as ready for review August 28, 2023 07:28

dacook requested changes Aug 30, 2023

View reviewed changes

mkllnk requested a review from dacook August 30, 2023 23:29

Use existing service to label weight&volume units

50f7177

We still have the scale stored in two places but in our current system that's part of a unit's "id". If the DFC adds that value to its standard then we can use it for lookup and don't need to repeat it. * datafoodconsortium/taxonomies#7

mkllnk force-pushed the dfc-update-request branch from ff64c8c to 50f7177 Compare August 30, 2023 23:48

dacook requested changes Aug 31, 2023

View reviewed changes

mkllnk added 2 commits August 31, 2023 16:11

Remove ambiguous unit "cup"

427d806

The DFC doesn't actually specify which cup it means. I don't expect anyone providing "cup" as unit to measure their produce and expect it to calculate as a regular volume. It can just be seen as items.

Import DFC unit labels like bunch and jar

514b8fa

mkllnk requested a review from dacook August 31, 2023 06:31

dacook approved these changes Aug 31, 2023

View reviewed changes

dacook requested changes Aug 31, 2023

View reviewed changes

Fix DFC context in spec example

57b4f61

mkllnk requested a review from dacook August 31, 2023 07:25

dacook approved these changes Aug 31, 2023

View reviewed changes

rioug reviewed Sep 4, 2023

View reviewed changes

engines/dfc_provider/app/services/quantitative_value_builder.rb Show resolved Hide resolved

rioug approved these changes Sep 5, 2023

View reviewed changes

mkllnk added api changes These pull requests change the API and can break integrations and removed feature toggled These pull requests' changes are invisible by default and are grouped in release notes labels Sep 7, 2023

drummer83 added the feedback-needed label Sep 7, 2023

mkllnk merged commit 1991970 into openfoodfoundation:master Sep 7, 2023

mkllnk deleted the dfc-update-request branch September 7, 2023 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DFC API] Import known units when creating new products #11377

[DFC API] Import known units when creating new products #11377

mkllnk commented Aug 10, 2023 •

edited

Loading

dacook left a comment

dacook Aug 30, 2023

mkllnk Aug 30, 2023

dacook Aug 30, 2023

mkllnk Aug 30, 2023

dacook left a comment

dacook Aug 30, 2023

dacook Aug 31, 2023

mkllnk Aug 31, 2023

dacook Aug 31, 2023

mkllnk Aug 31, 2023

dacook left a comment

dacook left a comment

drummer83 commented Sep 7, 2023

What should we test?

mkllnk commented Sep 7, 2023

RachL commented Sep 8, 2023

mkllnk commented Sep 11, 2023

	0.25 => { 'name' => 'cu', 'system' => 'imperial' },
	0.25 => { 'name' => 'cu', 'system' => 'metric' },

[DFC API] Import known units when creating new products #11377

[DFC API] Import known units when creating new products #11377

Conversation

mkllnk commented Aug 10, 2023 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 Sep 7, 2023

What should we test?

mkllnk commented Sep 7, 2023

RachL commented Sep 8, 2023

mkllnk commented Sep 11, 2023

mkllnk commented Aug 10, 2023 •

edited

Loading