-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
Feat: manufacturing place + origins of ingredients knowledge panels + Normalize all panels #6069
Conversation
After discussing with @jasmeet0817 , we decided to make some changes to the knowledge panels API and to normalize all of them:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some minor comments.
}; | ||
}; | ||
|
||
$template_data_ref->{encode_json} = sub($) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$template_data_ref->{encode_json} = sub($) { | |
# add json encoding function to templates | |
$template_data_ref->{encode_json} = sub($) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a comment above that code:
# Add functions and values that are passed to all templates
So I'm not sure if it makes sense to have 1 comment per function too.
$log->debug("create_manufacturing_place_panel", { code => $product_ref->{code} }) if $log->is_debug(); | ||
|
||
# Go through the product packaging codes, keep the first one with associated geo coordinates | ||
if (defined $product_ref->{emb_codes_tags}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you changed indent width ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm guessing github only use indent_size 2, but does not switch to 4 for Perl files as defined in .editorconfig
[*]
charset = utf-8
end_of_line = lf
indent_style = space
indent_size = 2
insert_final_newline = true
trim_trailing_whitespace = true
Perl files: tab indentation (4 spaces)
[*.{pm,pl,t}]
indent_style = tab
indent_size = 4
Python files: 4 spaces indentation
[*.py]
indent_size = 4
|
||
# Go through the product packaging codes, keep the first one with associated geo coordinates | ||
if (defined $product_ref->{emb_codes_tags}) { | ||
foreach my $packager_code_tagid (@{$product_ref->{emb_codes_tags}}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foreach my $packager_code_tagid (@{$product_ref->{emb_codes_tags}}) { | |
# we will create panel for the first known localtion | |
foreach my $packager_code_tagid (@{$product_ref->{emb_codes_tags}}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'm adding it
packager_code_data => $packager_codes{$packager_code_tagid}, | ||
lat => $lat + 0.0, | ||
lng => $lng + 0.0, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily for action, but as we talked about de-intricating modules:
This is typically a case, where I think we could separate concerns :
- you want to find a valid packager location from a list of packages codes --> this is a good function to have in the packager code package (and an iterator would even be better)
- having this, you use it here to get the data and generate your template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, filed #6072 to do it later.
"subtitle": "[% panel.packager_code_data.provincia_localidad %]", | ||
[% ELSIF panel.packager_code_data.cc == 'uk' %] | ||
"subtitle": "[% panel.packager_code_data.district %]", | ||
[% END %] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that seems not very scalable ! Why do we have different fields name based on the country ?
We should at least hide it in a perl function…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it's complicated: each country has a registry of packaging codes to companies, but each of them has completely different fields for them. So far we chose to include all the fields they have, but we have not attempted to map fields to common names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here in fact I think it's more of a display choice: for each country, which field do we want to use as title, and which field do we want to use as subtitle. We can't always use the same field (e.g. the company name, or the city + the country) because those fields may not be available for that country.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
To test:
https://uk.openfoodfacts.dev/product/3266980033613/l-escalope-cordon-bleu-de-poulet-x2-le-gaulois
Knowledge panels JSON:
https://fr.openfoodfacts.dev/api/v2/produit/3266980033613/l-escalope-cordon-bleu-de-poulet-x2-le-gaulois?fields=knowledge_panels
To do: evaluate the manufacturing place good / average / bad with the distance to the user.