feat: bypass data quality error for citrus #8444
Conversation
github-actions
bot
added
categories
🧽 Data quality
lib/ProductOpener/DataQualityFood.pm
Outdated
| # following error/warning should be ignored for some categories | ||
| # for example, lemon juices containing organic acid, it is forbidden to display organic acid in nutrition tables but | ||
| # organic acid contributes to the total energy calculation | ||
| my $skip_energy_error = 0; | ||
| if (defined $product_ref->{categories_tags}) { | ||
| # start from end, categories_tags has the most specific categories at the end | ||
| my $i = @{$product_ref->{categories_tags}} - 1; | ||
| while ( | ||
| ($i >= 0) | ||
| and ( | ||
| not( | ||
| # category - or a parent - with expected tag | ||
| defined get_inherited_property( | ||
| "categories", $product_ref->{categories_tags}[$i], | ||
| "ignore_energy_calculated_error:en" | ||
| ) | ||
| and ( | ||
| # we expect single letter a, b, c, d, e for nutriscore grade in the taxonomy. Case insensitive (/i). | ||
| get_inherited_property( | ||
| "categories", $product_ref->{categories_tags}[$i], | ||
| "ignore_energy_calculated_error:en" | ||
| ) =~ /yes$/i | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| { | ||
| $i--; | ||
| } | ||
|
|
||
| if ($i >= 0) { | ||
| $skip_energy_error = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we make it a separate method ? (easier to read code)
There was a problem hiding this comment.
There is a new function/subroutine in Tags.pm (because it is based on get_inherited_property subroutine).
That new subroutine is called from DataQualityFood.pm
The idea would be to reuse this function in the other PR (#8360)
(eventually) It could be improved, because we expect a "yes" and we could maybe add what is expected as input parameter in the function. Here we expect a "yes", in the other PR we expect a nutriscore. What do you think?
lib/ProductOpener/DataQualityFood.pm
Outdated
| # start from end, categories_tags has the most specific categories at the end | ||
| my $i = @{$product_ref->{categories_tags}} - 1; | ||
| while ( | ||
| ($i >= 0) |
There was a problem hiding this comment.
It seems to me you could rewrite this with a foreach and the reverse function + a break if you match condition to skip energy error. It would be more consistent with the rest of the codebase.
There was a problem hiding this comment.
Also, you make two calls to get_inherited_property while it's a costly method, you'd better store the result in a local variable.
There was a problem hiding this comment.
yes, thank you for the review and comments. Sorry, I am still learning. I will try to improve it.
I was inspired by the following snippet in ProducersFood.pm
my $i = @{$product_ref->{categories_tags}} - 1;
while (
($i >= 0)
and not((defined $categories_nutriments_ref->{$product_ref->{categories_tags}[$i]})
and (defined $categories_nutriments_ref->{$product_ref->{categories_tags}[$i]}{nutriments}))
)
{
$i--;
}
# categories_tags has the most specific categories at the end
if ($i >= 0) {
Now I reworked it based on compute_ecoscore_agribalyse found in Ecoscore.pm
Can you review this PR as well: #8360?
I did code something similar there too.
Codecov Report
@@ Coverage Diff @@
## main #8444 +/- ##
==========================================
+ Coverage 48.54% 48.57% +0.02%
==========================================
Files 114 114
Lines 21314 21327 +13
Branches 4776 4780 +4
==========================================
+ Hits 10347 10359 +12
Misses 9679 9679
- Partials 1288 1289 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| =cut | ||
|
|
||
| sub get_inherited_property_from_categories_tags ($product_ref, $property) { |
There was a problem hiding this comment.
Good idea to create this function!
lib/ProductOpener/Tags.pm
Outdated
| $category_match = lc(get_inherited_property("categories", $category, $property)); | ||
| last if $category_match; | ||
| } | ||
| if ($category_match) { |
There was a problem hiding this comment.
You could remove the lines 411 to 420 and just have a single "return $category_match" at the end (which would return undef if there was no match).
There was a problem hiding this comment.
Thanks for the suggestion!
Done! Please, check again before to merge
|
Kudos, SonarCloud Quality Gate passed!
|
benbenben2
deleted the
feat_exclude_categories_from_energy_error_from_other_nutrients
branch
* added feature to ignore energy does not match value computed from other nut * rework get_inherited_property * rework get_inherited_property * apply suggestions for return
Make sure you've done all the following (You can delete the checklist before submitting)
What
Added feature to bypass "Energy value in kJ does not correspond to the value calculated from the other nutrients"
Screenshot
Not sure what to screenshot because there is no error anymore :-D
Related issue(s) and discussion