fix: Added remove tags from field function #9960

anaritadauane · 2024-03-19T15:07:30Z

What

Added a function to remove a tag/tags from taxonomized fields

I achieved this by:

Added a function remove_tags_from_fields, a function that removes tags from fields
Updated the update_tags_fieds function, now a field can be updated by removing or adding tags
Creating a test case to remove tags from a field

Screenshot

Related issue(s) and discussion

Fixes Add the possibility to remove a tag from taxonomized fields #9453

… fields function

alexgarel · 2024-03-21T10:43:15Z

lib/ProductOpener/Tags.pm

@@ -4613,6 +4613,49 @@ sub add_tags_to_field ($product_ref, $tag_lc, $field, $additional_fields) {
 	return;
 }

+sub remove_tag_from_field($product_ref, $tag_lc, $field, $tags_to_remove) {


You except a list of tag to remove so:

use plural in function name (remove_tags_from_field)

~~your last parameter should be a reference to that list~~

Here again I misread… you have a string which is a comma separated list of values (maybe it's a good indication that you should document the function ;-) )

So apart from the function name, it's all ok :-)

Suggested change

sub remove_tag_from_field($product_ref, $tag_lc, $field, $tags_to_remove) {

sub remove_tags_from_field($product_ref, $tag_lc, $field, $tags_to_remove) {

alexgarel

You did quite well for an issue that is not the easiest !

Here are some inputs to let you continue.

alexgarel · 2024-03-21T10:43:43Z

lib/ProductOpener/APIProductWrite.pm

+			foreach my $tag (@$remove_tags) {
+				remove_tags_from_field($product_ref, $tags_lc, $field, $tag);
+			}


There is a mistmacht between what you do here and you implementation which expect a list reference (which is the right way I think).

I don't understand, are you saying this because of the for loop or because the $tag variable is an element of the reference list?

Your code is working but you are doing one call per field, while it could be grouped (or am I missing something ?)

Why not:

Suggested change

foreach my $tag (@$remove_tags) {

remove_tags_from_field($product_ref, $tags_lc, $field, $tag);

}

remove_tags_from_field($product_ref, $tags_lc, $field, join(',', @$remove_tags));

Yes, you are right. I did that because it is already handled by the remove_tags_from_field method

It is one call per tag to be removed

lib/ProductOpener/Tags.pm

tests/integration/api_v3_product_write.t

codecov-commenter · 2024-03-22T09:15:49Z

Codecov Report

Attention: Patch coverage is 76.19048% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 49.61%. Comparing base (dc04d18) to head (0fb1de2).
Report is 198 commits behind head on main.

Files	Patch %	Lines
lib/ProductOpener/APIProductWrite.pm	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9960      +/-   ##
==========================================
+ Coverage   49.54%   49.61%   +0.06%     
==========================================
  Files          67       70       +3     
  Lines       20650    20952     +302     
  Branches     4980     5022      +42     
==========================================
+ Hits        10231    10395     +164     
- Misses       9131     9267     +136     
- Partials     1288     1290       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alexgarel

Some more comments !

tests/integration/api_v3_product_write.t

alexgarel · 2024-03-22T10:34:44Z

tests/unit/tags.t

+	categories => "pommes, bananes, en:pears, fr:fraises, es:limones",
+};
+
+remove_tag_from_field($product_ref, "fr", "categories", "bananes, fraises");


you forgot to change the name here.

Better run your tests locally before committing:

make test-unit test=tags.t

(and make test-int test=youfile.t for the integration)
See https://openfoodfacts.github.io/openfoodfacts-server/dev/how-to-write-and-run-tests/

tests/unit/tags.t

alexgarel · 2024-03-22T10:52:58Z

lib/ProductOpener/APIProductWrite.pm

+			foreach my $tag (@$remove_tags) {
+				remove_tags_from_field($product_ref, $tags_lc, $field, $tag);
+			}


Your code is working but you are doing one call per field, while it could be grouped (or am I missing something ?)

Why not:

Suggested change

foreach my $tag (@$remove_tags) {

remove_tags_from_field($product_ref, $tags_lc, $field, $tag);

}

remove_tags_from_field($product_ref, $tags_lc, $field, join(',', @$remove_tags));

alexgarel · 2024-03-22T10:54:44Z

tests/integration/api_v3_product_write.t

+	{
+		test_case => 'patch-tags-fields-remove',
+		method => 'PATCH',
+		path => '/api/v3/product/1234567890100',
+		body => '{
+			"fields" : "updated",
+			"product": { 
+				"categories_tags_remove": ["en:tea"],
+				"stores_tags_remove": ["Carrefour", "Mon Ptit magasin"],
+				"countries_tags_fr_remove": ["Italie", "en:spain"],
+				"labels_tags_remove": ["végétarien", "Something unrecognized in French"]
+			}
+		}',
+	},


also you should have a json file resulting from this test. I let you see the documentation https://openfoodfacts.github.io/openfoodfacts-server/dev/how-to-use-gitpod/

I have made a few changes to the code and want to test it, but I'm having trouble getting a json file with the expected results. I'm running this command: make test-unit="api_v3_product_write.t --update-expected-results"

It might be because your backend is crashing.

Are you able to run the website locally ?

Yes I can run the website locally

stephanegigandet · 2024-03-23T16:03:19Z

lib/ProductOpener/APIProductWrite.pm

@@ -196,7 +196,7 @@ Update packagings.

 =cut

-sub update_tags_fields ($request_ref, $product_ref, $field, $add_to_existing_tags, $tags_lc, $value) {
+sub update_tags_fields ($request_ref, $product_ref, $field, $add_to_existing_tags, $remove_tags, $tags_lc, $value) {


If you change the parameters of a n existing function, you need to look for all the uses of that function to update the function calls to send the right parameters.

Thanks, I made the changes but I'm having trouble generating a json file with the expected results. I'm running this command make test-unit="api_v3_product_write.t --update-expected-results"

oooh, this is make test-int test="api_v3_product_write.t :: --update-expected-results"

Yes, I corrected the code but still
I am going to try one more time

@anaritadauane changed a bit the command (we have to add :: now because of yath the new test runner)

…nction

alexgarel · 2024-03-26T08:53:19Z

lib/ProductOpener/APIProductWrite.pm

-			foreach my $tag (@$remove_tags) {
-				remove_tags_from_field($product_ref, $tags_lc, $field, $tag);
-			}
+			remove_tags_from_field($product_ref, $tags_lc, $field, $remove_tags);    #join(',', @$remove_tags)


you have a ref to an array but then in remove_tags_from_field you treat it like a string… it wont work ! Why don't you use a join ?

Because I'm calling the join function inside the remove_tags_from_field function

alexgarel · 2024-03-26T08:54:05Z

lib/ProductOpener/Tags.pm

-	my @removed_tags = ();
-	my @tags_to_remove = split(/,/, $tags_to_remove);
+	# Split the tags to remove string into separate tags
+	my @excluded_tags = split(/,\s*/, $tags_to_remove);


if you specify possible spaces after the comma, also add them before maybe ?

Suggested change

my @excluded_tags = split(/,\s*/, $tags_to_remove);

my @excluded_tags = split(/\s*,\s*/, $tags_to_remove);

alexgarel · 2024-03-26T08:55:04Z

tests/integration/api_v3_product_write.t

@@ -673,7 +673,7 @@ my $tests_ref = [
 				"categories_tags_remove": ["en:tea"],
 				"stores_tags_remove": ["Carrefour", "Mon Ptit magasin"],
 				"countries_tags_fr_remove": ["Italie", "en:spain"],
-				"labels_tags_remove": ["végétarien", "Something unrecognized in French"]


aren't you testing tags removal ?

tests/unit/tags.t

alexgarel · 2024-03-29T13:28:59Z

tests/integration/expected_test_results/api_v3_product_write/patch-tags-fields-remove.json

I pushed it, but as you see it's not what we expect.

It's important that you are able to run tests locally otherwise it will be very complicated to fix your PR.

sonarcloud · 2024-03-29T13:29:34Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

hangy · 2024-03-29T19:50:05Z

.DS_Store

I think this is some Apple-specific file? Should this be in .gitignore or is it actually useful?

Yes that is an apple file related to the directory.

Added remove tags from field function and the updated the update tags…

773f8f1

… fields function

github-actions bot added the API WRITE WRITE API to allow sending product info and image label Mar 19, 2024

github-actions bot assigned anaritadauane Mar 19, 2024

github-actions bot added 🧪 tests Tags API v3 labels Mar 19, 2024

anaritadauane added 2 commits March 19, 2024 17:08

fixed formatting

22bc15f

fixed some typos

4af4437

alexgarel reviewed Mar 21, 2024

View reviewed changes

anaritadauane added 2 commits March 22, 2024 10:42

added unit tests

677bba7

fixed formatting

fd3ee25

alexgarel reviewed Mar 22, 2024

View reviewed changes

stephanegigandet reviewed Mar 23, 2024

View reviewed changes

made some changes with my remove tags function and the update tags fu…

9ff4926

…nction

github-actions bot added Display 🧪 unit tests labels Mar 25, 2024

Merge branch 'main' into fix-issue-openfoodfacts#9453

a478ad7

alexgarel reviewed Mar 26, 2024

View reviewed changes

tests/unit/tags.t Outdated Show resolved Hide resolved

anaritadauane and others added 3 commits March 26, 2024 14:45

updated the files Tags.pm and APIProductWrite.pm

ec8dec5

Merge branch 'main' into fix-issue-openfoodfacts#9453

a036215

test: adding integration test result for tags-fields-remove

0fb1de2

alexgarel reviewed Mar 29, 2024

View reviewed changes

hangy reviewed Mar 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Added remove tags from field function #9960

fix: Added remove tags from field function #9960

anaritadauane commented Mar 19, 2024

alexgarel Mar 21, 2024 •

edited

Loading

alexgarel Mar 21, 2024 •

edited

Loading

alexgarel left a comment

alexgarel Mar 21, 2024

anaritadauane Mar 22, 2024

alexgarel Mar 22, 2024

anaritadauane Mar 22, 2024

anaritadauane Mar 22, 2024

codecov-commenter commented Mar 22, 2024 •

edited

Loading

alexgarel left a comment

alexgarel Mar 22, 2024

alexgarel Mar 22, 2024

alexgarel Mar 22, 2024

anaritadauane Mar 25, 2024

anaritadauane Mar 25, 2024 •

edited

Loading

alexgarel Mar 26, 2024

anaritadauane Mar 26, 2024

stephanegigandet Mar 23, 2024

anaritadauane Mar 25, 2024

alexgarel Mar 29, 2024 •

edited

Loading

anaritadauane Mar 29, 2024

alexgarel Mar 29, 2024

alexgarel Mar 26, 2024

anaritadauane Mar 26, 2024

alexgarel Mar 26, 2024

alexgarel Mar 26, 2024

anaritadauane Mar 26, 2024

alexgarel Mar 29, 2024

sonarcloud bot commented Mar 29, 2024

hangy Mar 29, 2024

aleene Mar 29, 2024

	sub remove_tag_from_field($product_ref, $tag_lc, $field, $tags_to_remove) {
	sub remove_tags_from_field($product_ref, $tag_lc, $field, $tags_to_remove) {

	my @excluded_tags = split(/,\s*/, $tags_to_remove);
	my @excluded_tags = split(/\s,\s/, $tags_to_remove);

fix: Added remove tags from field function #9960

Are you sure you want to change the base?

fix: Added remove tags from field function #9960

Conversation

anaritadauane commented Mar 19, 2024

What

Screenshot

Related issue(s) and discussion

alexgarel Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

alexgarel Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

alexgarel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 22, 2024 • edited Loading

Codecov Report

alexgarel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anaritadauane Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgarel Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 29, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgarel Mar 21, 2024 •

edited

Loading

alexgarel Mar 21, 2024 •

edited

Loading

codecov-commenter commented Mar 22, 2024 •

edited

Loading

anaritadauane Mar 25, 2024 •

edited

Loading

alexgarel Mar 29, 2024 •

edited

Loading