-
-
Notifications
You must be signed in to change notification settings - Fork 399
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: packaging stats per parent materials #8594
Changes from 6 commits
e202ff5
ce9608b
48a8a81
668a080
b9f5ca8
5a13414
2e9e174
a289d61
3fd19b5
eb1ce31
f7f9cdc
f4b5def
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// Create a structure so that we can display the packaging components for each recycling type | ||
[% SET recycling_types = {} %] | ||
[% SET unknown = "en:unknown" %] | ||
[% IF product.packagings %] | ||
[% FOREACH packaging IN product.packagings %] | ||
[% IF packaging.recycling.defined AND ((packaging.recycling == "en:discard") OR (packaging.recycling == "en:recycle")) %] | ||
[% SET recycling_types.${packaging.recycling} = 1 %] | ||
[% ELSE %] | ||
[% SET recycling_types.$unknown = 1 %] | ||
[% END %] | ||
[% END %] | ||
stephanegigandet marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[% END %] | ||
|
||
{ | ||
"level" :"info", | ||
"topics": [ | ||
"environment" | ||
], | ||
"title_element": { | ||
"title": "[% lang('packaging_parts') %]", | ||
}, | ||
"expanded": true, | ||
"elements": [ | ||
[% FOREACH recycling_type IN ["en:recycle", "en:discard", "en:unknown"] %] | ||
[% IF recycling_types.$recycling_type.defined %] | ||
{ | ||
"element_type": "text", | ||
"text_element": { | ||
"type": "summary", | ||
"icon_color_from_evaluation": true, | ||
[% IF recycling_type == "en:recycle" %] | ||
"evaluation": "good", | ||
"icon_url": "[% static_subdomain %]/images/icons/dist/recycle-variant.svg", | ||
"icon_alt": "[% display_taxonomy_tag_name("packaging_recycling",recycling_type) %]", | ||
[% ELSIF recycling_type == "en:discard" %] | ||
"evaluation": "bad", | ||
"icon_url": "[% static_subdomain %]/images/icons/dist/delete.svg", | ||
"icon_alt": "[% display_taxonomy_tag_name("packaging_recycling",recycling_type) %]", | ||
[% ELSE %] | ||
"evaluation": "neutral", | ||
"icon_url": "[% static_subdomain %]/images/icons/dist/help.svg", | ||
"icon_alt": "[% lang('unknown') %]", | ||
[% END %] | ||
"html": ` | ||
[% FOREACH packaging IN product.packagings %] | ||
[% IF packaging.recycling == recycling_type OR (recycling_type == "en:unknown" AND ((NOT packaging.recycling.defined) OR ((packaging.recycling != "en:discard") AND (packaging.recycling != "en:recycle")))) %] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A better way to do that, would have been to dispatch packagings in arrays in recycling_types instead of putting Because this condition is not trivial to read ! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, but it's likely that we will change how we display packaging components in the future. This code did not change, I just renamed the template for clarity. |
||
[% IF packaging.number_of_units %][% packaging.number_of_units %] x [% END %] | ||
<strong> | ||
[% display_taxonomy_tag_name('packaging_shapes',packaging.shape) %] | ||
[% IF packaging.quantity_per_unit %][% packaging.quantity_per_unit %] [% END %] | ||
</strong> | ||
[% IF packaging.material %] | ||
([% display_taxonomy_tag_name('packaging_materials',packaging.material) %][% IF packaging.weight_specified %][% sep %]: [% packaging.weight_specified %] g[% ELSIF packaging.weight_measured %][% sep %]: [% packaging.weight_measured %] g[% END %]) | ||
[% END %] | ||
<br> | ||
[% END %] | ||
[% END %] | ||
` | ||
} | ||
}, | ||
[% END %] | ||
[% END %] | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
{ | ||
"level" :"info", | ||
"topics": [ | ||
"environment" | ||
], | ||
"title_element": { | ||
"title": "[% lang('packaging_materials') %]", | ||
}, | ||
"expanded": true, | ||
"elements": [ | ||
{ | ||
"element_type": "table", | ||
"table_element": { | ||
"id": "packaging_materials", | ||
"title": "[% lang('packaging_materials') %]", | ||
"columns": [ | ||
{ | ||
"text": "[% lang('packaging_material') %]", | ||
"type": "text", | ||
}, | ||
{ | ||
"text": "%", | ||
"type": "text", | ||
}, | ||
{ | ||
"text": "[% lang('packaging_weight_total') %]", | ||
"type": "text", | ||
}, | ||
// packaging weight per 100g of product is computed only if we have a quantity | ||
[% IF product.product_quantity %] | ||
{ | ||
"text": "[% lang('packaging_weight_100g') %]", | ||
"type": "text", | ||
}, | ||
[% END %] | ||
], | ||
"rows": [ | ||
// keep a counter of materials so that we don't display the "all" raw if there's only one material | ||
[% SET materials = 0 %] | ||
[% FOREACH parent_material IN ["en:paper-or-cardboard", "en:plastic", "en:glass", "en:metal", "all"] %] | ||
[% IF product.packagings_materials.$parent_material.defined %] | ||
[% IF parent_material != 'all' OR materials > 1 %] | ||
[% SET materials = materials + 1 %] | ||
[% SET parent_material_data = product.packagings_materials.$parent_material %] | ||
{ | ||
[% IF parent_material == 'all' %] | ||
"style": "font-weight: bold", | ||
[% END %] | ||
"values": [ | ||
{ | ||
[% IF parent_material == 'all' %] | ||
"text": "[% lang('total') %]", | ||
[% ELSE %] | ||
"text": "[% display_taxonomy_tag_name('packaging_materials',parent_material) %]" | ||
[% END %] | ||
}, | ||
{ | ||
"text": "[% IF parent_material_data.weight_percent %][% round(parent_material_data.weight_percent) %]%[% END %]", | ||
}, | ||
{ | ||
"text": "[% IF parent_material_data.weight %][% parent_material_data.weight %] g[% END %]" | ||
}, | ||
[% IF product.product_quantity %] | ||
{ | ||
"text": "[% IF parent_material_data.weight_100g %][% round(parent_material_data.weight_100g) %] g[% END %]" | ||
}, | ||
[% END %] | ||
] | ||
}, | ||
[% END %] | ||
[% END %] | ||
[% 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.
It's okay, but don't we have a more efficient way to do this ? (like fetching all parents and then computing intersection with our values ?). Maybe with something like Set::Tiny.
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.
I don't know enough of the source code to suggest an entirely different approach to the logic itself, but a common pattern for getting the first item in a list that matches a certain criteria is using List::Util's
first
:but if this code is hot (i.e. running several times per call) maybe it could be worth turning
is_a()
into a hash lookup.Either way, the current implementation is okay and I wouldn't change it unless it was a bottleneck.
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.
Thanks for the suggestions. I changed to @garu 's suggestion as I find it more readable with first indeed.
It certainly could be done more efficiently, but this code is executed only when writing products, and given the shallowness of the packaging materials taxonomy, it's just a dozen operations, so I'd prefer not to spend time on it. If we want to optimize things, there are targets that are much more likely to make a difference (e.g. for read operations of products, where we compute attributes and knowledge panels for 100 products...)