Skip to content
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

refactor: Refactor the update_all_products.pl in Products.pm Fixes#7271 #8263

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

oyenuga17
Copy link
Contributor

@oyenuga17 oyenuga17 commented Mar 28, 2023

What

Refactor the update_all_products.pl script to use a new function update_existing_product_without_creating_a_new_revision()

Related issue(s) and discussion

@oyenuga17
Copy link
Contributor Author

@stephanegigandet please help review

@@ -1001,6 +1001,25 @@ sub compute_sort_keys ($product_ref) {
return;
}

sub created_function($mongodb_to_mongodb, $product_ref, $products_collection, $data_root, $path) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename the function "update_existing_product_without_creating_a_new_revision()" instead of "created_function()", so that it's easier to understand what it does?

sub created_function($mongodb_to_mongodb, $product_ref, $products_collection, $data_root, $path) {

# make sure nutrient values are numbers
ProductOpener::Products::make_sure_numbers_are_stored_as_numbers($product_ref);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as this code is now in Products.pm, you can remove the "ProductOpener::Products::" part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thank you @stephanegigandet

@stephanegigandet stephanegigandet changed the title fix:Refactor the update_all_products.pl in Products.pm Fixes#7271 refactor: Refactor the update_all_products.pl in Products.pm Fixes#7271 Mar 29, 2023
@stephanegigandet
Copy link
Contributor

Thanks for the PR @oyenuga17 !

Overall it looks good, there are just a few things to address:

  • The "Check perl" test is failing. You can see why by clicking on "Details" at the right of the failing test. In this case it is just about the linting, so you just need to run "make lint" and commit the updated code.

  • When we create new functions, especially functions that can be called from other scripts, modules etc. It's best to add a brief description on top, for instance in Products.pm we have comments like this one:

=head2 product_url ( $code_or_ref )

Returns a relative URL for a product on the website.

=head3 Parameters

=head4 Product code or reference to product object $code_or_ref

=cut
  • Have you tried running the script you modified, to check that it works? You can do so like this:

~/openfoodfacts-server/tests$ docker exec -it po-backend-1 bash
www-data@25e43e0a50f5:/opt/product-opener$ ./scripts/update_all_products.pl --fields categories

The reason I'm asking is that I think you will need to declare your new function at the top of Products.pm so that other scripts can use it.

Thank you!

@oyenuga17
Copy link
Contributor Author

@stephanegigandet Thank you so much i will put everything to use

@oyenuga17
Copy link
Contributor Author

@stephanegigand i have made changes please review

Product object reference.

=head4 $products_collection
=head4 $data_root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$data_root is a global variable, you can use it without passing it as an argument to the function

@@ -1323,22 +1323,8 @@

# Otherwise, we silently update the .sto file of the last version
else {

# make sure nutrient values are numbers
ProductOpener::Products::make_sure_numbers_are_stored_as_numbers($product_ref);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you removed ProductOpener::Products::make_sure_numbers_are_stored_as_numbers($product_ref); from the script, but did not add it to the function.

@oyenuga17
Copy link
Contributor Author

oyenuga17 commented Apr 3, 2023 via email

sub update_existing_product_without_creating_a_new_revision ($mongodb_to_mongodb, $product_ref, $products_collection,
$data_root, $path)
{

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# make sure nutrient values are numbers
ProductOpener::Products::make_sure_numbers_are_stored_as_numbers($product_ref);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jnsereko if the sub is in same package I don't think you need the ProductOpener::Products:: prefix !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# make sure nutrient values are numbers
make_sure_numbers_are_stored_as_numbers($product_ref);

@oyenuga17
Copy link
Contributor Author

@stephanegigandet please review ..

@github-actions github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Apr 7, 2023
Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@github-actions github-actions bot removed the 💥 Merge Conflicts 💥 Merge Conflicts label Apr 20, 2023
@alexgarel
Copy link
Member

@oyenuga17 can you fix the perl check ? It might be as easy as make lint and then commit / push the changes.

@sonarcloud
Copy link

sonarcloud bot commented Apr 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@oyenuga17
Copy link
Contributor Author

@oyenuga17 can you fix the perl check ? It might be as easy as make lint and then commit / push the changes.

please merge .. i just ran the test

@VaiTon
Copy link
Member

VaiTon commented May 19, 2023

@oyenuga17
From Check Perl:

lib/ProductOpener/Products.pm: Subroutine "update_existing_product_without_creating_a_new_revision" does not end with "return" at line 1022, column 1.  See page 197 of PBP.  (Severity: 4)

From Unit tests:

tests/integration/export.t ......................... ok

#   Failed test at tests/integration/fix_non_normalized_codes.t line 102.
#     Structures begin differing at:
#          $got->[1] = 'Moved 0000012345678 to 12345678'
#     $expected->[1] = 'Removed 0000012345670 as duplicate of 12345670'
# Looks like you failed 1 test of 27.

@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Aug 8, 2023
@github-actions github-actions bot added the Stale label Dec 19, 2023
@teolemon
Copy link
Member

@alexgarel @stephanegigandet is it possible/worth salvaging ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants