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

oyenuga17 · 2023-03-28T19:09:41Z

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

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

oyenuga17 · 2023-03-28T19:10:57Z

@stephanegigandet please help review

stephanegigandet · 2023-03-29T08:05:38Z

lib/ProductOpener/Products.pm

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

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


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?

stephanegigandet · 2023-03-29T08:06:45Z

lib/ProductOpener/Products.pm

+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);


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

Okay thank you @stephanegigandet

stephanegigandet · 2023-03-29T08:14:54Z

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 · 2023-03-29T09:05:41Z

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

oyenuga17 · 2023-04-01T16:42:39Z

@stephanegigand i have made changes please review

stephanegigandet · 2023-04-03T08:11:15Z

lib/ProductOpener/Products.pm

+Product object reference.
+
+=head4 $products_collection
+=head4 $data_root


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

stephanegigandet · 2023-04-03T08:12:48Z

scripts/update_all_products.pl

@@ -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);


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 · 2023-04-03T08:30:12Z

Okay will see to it

…

On Mon, 3 Apr 2023 at 09:13, Stéphane Gigandet ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In scripts/update_all_products.pl <#8263 (comment)> : > @@ -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); 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. — Reply to this email directly, view it on GitHub <#8263 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APKMDCRIHDSQYD5KOADHTOTW7KBA5ANCNFSM6AAAAAAWK7KBY4> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

jnsereko · 2023-04-03T10:48:24Z

lib/ProductOpener/Products.pm

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


Suggested change

# make sure nutrient values are numbers

ProductOpener::Products::make_sure_numbers_are_stored_as_numbers($product_ref);

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

Suggested change

# make sure nutrient values are numbers

make_sure_numbers_are_stored_as_numbers($product_ref);

oyenuga17 · 2023-04-03T10:57:30Z

@stephanegigandet please review ..

stephanegigandet

Thank you!

alexgarel · 2023-04-27T16:31:38Z

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

sonarcloud · 2023-04-27T21:50:59Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

oyenuga17 · 2023-04-27T21:57:40Z

@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 · 2023-05-19T08:16:15Z

@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 · 2023-06-16T09:44:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

teolemon · 2024-04-30T14:36:19Z

@alexgarel @stephanegigandet is it possible/worth salvaging ?

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

0808a8a

oyenuga17 requested a review from a team as a code owner March 28, 2023 19:09

github-actions bot added Products update all products labels Mar 28, 2023

github-actions bot assigned oyenuga17 Mar 28, 2023

stephanegigandet reviewed Mar 29, 2023

View reviewed changes

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

oyenuga17 added 2 commits April 1, 2023 15:34

Merge branch 'main' into refactor-update-function

3666f12

made requested changes

8539668

stephanegigandet reviewed Apr 3, 2023

View reviewed changes

jnsereko suggested changes Apr 3, 2023

View reviewed changes

fix: modified code

996506c

github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Apr 7, 2023

Merge branch 'main' into refactor-update-function

d163a53

stephanegigandet approved these changes Apr 17, 2023

View reviewed changes

github-actions bot removed the 💥 Merge Conflicts 💥 Merge Conflicts label Apr 20, 2023

oyenuga17 added 3 commits April 27, 2023 22:23

Merge branch 'main' into refactor-update-function

b53c9e2

ran make lint

ba0cb9d

run make lint

18ec94e

openfoodfacts deleted a comment from github-advanced-security bot May 19, 2023

Merge branch 'main' into refactor-update-function

994d2bd

github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Aug 8, 2023

teolemon mentioned this pull request Oct 2, 2023

Refactor Product Opener #5527

Open

github-actions bot added the Stale label Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oyenuga17 commented Mar 28, 2023 •

edited by stephanegigandet

Loading

oyenuga17 commented Mar 28, 2023

stephanegigandet Mar 29, 2023

stephanegigandet Mar 29, 2023

oyenuga17 Mar 29, 2023

stephanegigandet commented Mar 29, 2023

oyenuga17 commented Mar 29, 2023

oyenuga17 commented Apr 1, 2023

stephanegigandet Apr 3, 2023

stephanegigandet Apr 3, 2023

oyenuga17 commented Apr 3, 2023 via email

jnsereko Apr 3, 2023

alexgarel May 11, 2023

alexgarel May 11, 2023

oyenuga17 commented Apr 3, 2023

stephanegigandet left a comment

alexgarel commented Apr 27, 2023

sonarcloud bot commented Apr 27, 2023

oyenuga17 commented Apr 27, 2023

VaiTon commented May 19, 2023

sonarcloud bot commented Jun 16, 2023

teolemon commented Apr 30, 2024


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


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

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

Are you sure you want to change the base?

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

Conversation

oyenuga17 commented Mar 28, 2023 • edited by stephanegigandet Loading

What

Related issue(s) and discussion

oyenuga17 commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanegigandet commented Mar 29, 2023

oyenuga17 commented Mar 29, 2023

oyenuga17 commented Apr 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyenuga17 commented Apr 3, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyenuga17 commented Apr 3, 2023

stephanegigandet left a comment

Choose a reason for hiding this comment

alexgarel commented Apr 27, 2023

sonarcloud bot commented Apr 27, 2023

oyenuga17 commented Apr 27, 2023

VaiTon commented May 19, 2023

sonarcloud bot commented Jun 16, 2023

teolemon commented Apr 30, 2024

oyenuga17 commented Mar 28, 2023 •

edited by stephanegigandet

Loading