fix: refactor + ignore 'null' product_type sent by app (#11032)

I'm hoping this PR will fix openfoodfacts/smooth-app#5864 If we receive product_type=null, we just ignore it. Also refactored the code to remove some duplication.
openfoodfacts · Nov 21, 2024 · 48a5322 · 48a5322
1 parent d51b6f0
commit 48a5322
Show file tree

Hide file tree

Showing 12 changed files with 182 additions and 99 deletions.
diff --git a/cgi/product_jqm_multilingual.pl b/cgi/product_jqm_multilingual.pl
@@ -57,7 +57,8 @@ =head1 DESCRIPTION
 use ProductOpener::ForestFootprint qw/:all/;
 use ProductOpener::Text qw/remove_tags_and_quote/;
 use ProductOpener::API qw/get_initialized_response check_user_permission/;
-use ProductOpener::APIProductWrite qw/skip_protected_field/;
+use ProductOpener::APIProductWrite
+	qw/process_change_product_type_request_if_we_have_one process_change_product_code_request_if_we_have_one skip_protected_field/;
 
 use Apache2::RequestRec ();
 use Apache2::Const ();
@@ -230,30 +231,14 @@ =head1 DESCRIPTION
 
 	# Change code or product type
 
-	if (defined single_param('new_code')) {
+	push @errors,
+		process_change_product_code_request_if_we_have_one($request_ref, $response_ref, $product_ref,
+		single_param("new_code"));
+	$code = $product_ref->{code};
 
-		if (check_user_permission($request_ref, $response_ref, "product_change_code")) {
-
-			push @errors, change_product_code($product_ref, single_param('new_code'));
-			$code = $product_ref->{code};
-		}
-		else {
-			push @errors, "No permission: product_change_code";
-		}
-	}
-
-	if (    (defined single_param("product_type"))
-		and ($product_ref->{product_type} ne single_param("product_type")))
-	{
-
-		if (check_user_permission($request_ref, $response_ref, "product_change_product_type")) {
-
-			push @errors, change_product_type($product_ref, single_param("product_type"));
-		}
-		else {
-			push @errors, "No permission: product_change_product_type";
-		}
-	}
+	push @errors,
+		process_change_product_type_request_if_we_have_one($request_ref, $response_ref, $product_ref,
+		single_param("product_type"));
 
 	# Display an error message and exit if we have a fatal error (no permission to change barcode or product type, or invalid barcode or product type)
 	if ($#errors >= 0) {

diff --git a/cgi/product_multilingual.pl b/cgi/product_multilingual.pl
@@ -56,6 +56,8 @@
 use ProductOpener::APIProductWrite qw/skip_protected_field/;
 use ProductOpener::ProductsFeatures qw/feature_enabled/;
 use ProductOpener::Orgs qw/update_import_date update_last_import_type/;
+use ProductOpener::APIProductWrite
+	qw/process_change_product_type_request_if_we_have_one process_change_product_code_request_if_we_have_one/;
 
 use Apache2::RequestRec ();
 use Apache2::Const ();
@@ -408,18 +410,14 @@ ($product_ref)
 
 	exists $product_ref->{new_server} and delete $product_ref->{new_server};
 
-	if ($request_ref->{admin} or $User{moderator}) {
-		if ((defined single_param("new_code")) and (single_param("new_code") ne "")) {
-			change_product_code($product_ref, single_param("new_code"));
-			$code = $product_ref->{code};
-		}
-		if (    (defined single_param("product_type"))
-			and (single_param("product_type") ne "")
-			and ($product_ref->{product_type} ne single_param("product_type")))
-		{
-			change_product_type($product_ref, single_param("product_type"));
-		}
-	}
+	# Check if the request is for changing the product code or the product type, if so process it
+
+	process_change_product_code_request_if_we_have_one($request_ref, $response_ref, $product_ref,
+		single_param("new_code"));
+	$code = $product_ref->{code};
+
+	process_change_product_type_request_if_we_have_one($request_ref, $response_ref, $product_ref,
+		single_param("product_type"));
 
 	my @param_fields = ();
 

diff --git a/lib/ProductOpener/APIProductWrite.pm b/lib/ProductOpener/APIProductWrite.pm
@@ -37,6 +37,8 @@ BEGIN {
 	use vars qw(@ISA @EXPORT_OK %EXPORT_TAGS);
 	@EXPORT_OK = qw(
 		&write_product_api
+		&process_change_product_code_request_if_we_have_one
+		&process_change_product_type_request_if_we_have_one
 		&skip_protected_field
 	);    # symbols to export on request
 	%EXPORT_TAGS = (all => [@EXPORT_OK]);
@@ -361,6 +363,113 @@ sub update_product_fields ($request_ref, $product_ref, $response_ref) {
 	return;
 }
 
+=head2 process_change_product_code_request_if_we_have_one($request_ref, $response_ref, $product_ref, $new_code)
+
+Process a change of code request if we have one.
+
+=head3 Parameters
+
+=head4 $request_ref (input)
+
+Reference to the request object.
+
+=head4 $response_ref (input)
+
+Reference to the response object.
+
+=head4 $product_ref (input)
+
+Reference to the product object.
+
+=head4 $new_code (input)
+
+New code.
+
+=head3 Return value
+
+undef if we don't have a change product code request, or if it was processed correctly.
+an error id if there was an error (e.g. no_permisssion or invalid_product_type).
+
+=cut
+
+sub process_change_product_code_request_if_we_have_one($request_ref, $response_ref, $product_ref, $new_code) {
+
+	my $error;
+	# Change of code
+	if (    (defined $new_code)
+		and ($new_code ne $product_ref->{code}))
+	{
+
+		if (check_user_permission($request_ref, $response_ref, "product_change_code")) {
+
+			$error = change_product_code($product_ref, $new_code);
+			if ($error) {
+				add_error(
+					$response_ref,
+					{
+						message => {id => $error},
+						field => {id => "new_code"},
+						impact => {id => "failure"},
+					}
+				);
+			}
+		}
+		else {
+			$error = "no_permission: product_change_code";
+		}
+	}
+	# If we have an error, we return it, otherwise we just use "return;"
+	# so that the function can be used in list context: push @errors, process_change_product_code_request_if_we_have_one(...)
+	if ($error) {
+		return $error;
+	}
+	return;
+}
+
+sub process_change_product_type_request_if_we_have_one($request_ref, $response_ref, $product_ref, $new_product_type) {
+
+	my $error;
+
+	# Change of product type
+	if (
+			(defined $new_product_type)
+		and ($new_product_type ne "")
+		and ($new_product_type ne
+			"null")    # 2024/11/21: OFF app sends "null" as a string, ignore it as it is not a valid product type
+		and ($new_product_type ne $product_ref->{product_type})
+		)
+	{
+
+		if (check_user_permission($request_ref, $response_ref, "product_change_product_type")) {
+
+			$error = change_product_type($product_ref, $new_product_type);
+			if ($error) {
+				add_error(
+					$response_ref,
+					{
+						message => {id => $error},
+						field => {id => "product_type"},
+						impact => {id => "failure"},
+					}
+				);
+			}
+			else {
+				$request_ref->{updated_product_fields}{product_type} = 1;
+			}
+		}
+		else {
+			$error = "no_permission: product_change_product_type";
+		}
+	}
+
+	# If we have an error, we return it, otherwise we just use "return;"
+	# so that the function can be used in list context: push @errors, process_change_product_code_request_if_we_have_one(...)
+	if ($error) {
+		return $error;
+	}
+	return;
+}
+
 =head2 write_product_api($request_ref)
 
 Process API v3 WRITE product requests.
@@ -483,62 +592,22 @@ sub write_product_api ($request_ref) {
 		}
 		else {
 
-			# Change of code
-			if (    (defined $request_body_ref->{product}{code})
-				and ($product_ref->{code} ne $request_body_ref->{product}{code}))
+			if (
+				process_change_product_code_request_if_we_have_one(
+					$request_ref, $response_ref, $product_ref, $request_body_ref->{product}{code}
+				)
+				)
 			{
-
-				if (check_user_permission($request_ref, $response_ref, "product_change_code")) {
-
-					my $change_product_code_error
-						= change_product_code($product_ref, $request_body_ref->{product}{code});
-					if ($change_product_code_error) {
-						add_error(
-							$response_ref,
-							{
-								message => {id => $error},
-								field => {id => "new_code"},
-								impact => {id => "failure"},
-							}
-						);
-						$error = 1;
-					}
-					else {
-						$code = $product_ref->{code};
-					}
-				}
-				else {
-					$error = 1;
-				}
+				$error = 1;
 			}
 
-			# Change of product type
-			if (    (defined $request_body_ref->{product}{product_type})
-				and ($product_ref->{product_type} ne $request_body_ref->{product}{product_type}))
+			if (
+				process_change_product_type_request_if_we_have_one(
+					$request_ref, $response_ref, $product_ref, $request_body_ref->{product}{product_type}
+				)
+				)
 			{
-
-				if (check_user_permission($request_ref, $response_ref, "product_change_product_type")) {
-
-					my $change_product_type_error
-						= change_product_type($product_ref, $request_body_ref->{product}{product_type});
-					if ($change_product_type_error) {
-						add_error(
-							$response_ref,
-							{
-								message => {id => $error},
-								field => {id => "product_type"},
-								impact => {id => "failure"},
-							}
-						);
-						$error = 1;
-					}
-					else {
-						$request_ref->{updated_product_fields}{product_type} = 1;
-					}
-				}
-				else {
-					$error = 1;
-				}
+				$error = 1;
 			}
 
 			if (not $error) {

diff --git a/tests/integration/change_product_code_and_product_type.t b/tests/integration/change_product_code_and_product_type.t
@@ -220,6 +220,29 @@ my $tests_ref = [
 		},
 		ua => $moderator_ua,
 	},
+	# Send null product type
+	# 2024/11/21: the OFF app is sending product_type=null for new products
+	{
+		test_case => 'change-product-type-to-null-api-v2-moderator',
+		method => 'POST',
+		path => '/cgi/product_jqm_multilingual.pl',
+		form => {
+			code => "1234567890102",
+			product_type => "null",
+		},
+		ua => $moderator_ua,
+	},
+	# Send empty string product type
+	{
+		test_case => 'change-product-type-to-empty-string-api-v2-moderator',
+		method => 'POST',
+		path => '/cgi/product_jqm_multilingual.pl',
+		form => {
+			code => "1234567890102",
+			product_type => "",
+		},
+		ua => $moderator_ua,
+	},
 	# Send product_type=beauty to move product to Open Beauty Facts
 	{
 		test_case => 'change-product-type-to-beauty-api-v2-moderator',
@@ -390,7 +413,7 @@ my $tests_ref = [
 		}',
 		ua => $moderator_ua,
 	},
-	# Change the product_type field to petfood, with API v3
+	# Change the product_type field to invalid product type, with API v3
 	{
 		test_case => 'change-product-type-to-opff-api-v3-invalid-product-type',
 		method => 'PATCH',

diff --git a/...sults/change_product_code_and_product_type/change-product-code-api-v2-normal-account.json b/...sults/change_product_code_and_product_type/change-product-code-api-v2-normal-account.json
@@ -1,4 +1,4 @@
 {
    "status" : 0,
-   "status_verbose" : "No permission: product_change_code"
+   "status_verbose" : "no_permission: product_change_code"
 }
diff --git a/...esults/change_product_code_and_product_type/change-product-code-api-v2-not-logged-in.json b/...esults/change_product_code_and_product_type/change-product-code-api-v2-not-logged-in.json
@@ -1,4 +1,4 @@
 {
    "status" : 0,
-   "status_verbose" : "No permission: product_change_code"
+   "status_verbose" : "no_permission: product_change_code"
 }
diff --git a/...nge_product_code_and_product_type/change-product-code-api-v3-moderator-existing-code.json b/...nge_product_code_and_product_type/change-product-code-api-v3-moderator-existing-code.json
@@ -11,7 +11,7 @@
             "name" : "Failure"
          },
          "message" : {
-            "id" : 0,
+            "id" : "error_new_code_already_exists",
             "lc_name" : "",
             "name" : ""
          }

diff --git a/...ange_product_code_and_product_type/change-product-code-api-v3-moderator-invalid-code.json b/...ange_product_code_and_product_type/change-product-code-api-v3-moderator-invalid-code.json
@@ -11,9 +11,9 @@
             "name" : "Failure"
          },
          "message" : {
-            "id" : 0,
-            "lc_name" : "",
-            "name" : ""
+            "id" : "invalid_code",
+            "lc_name" : "Invalid code",
+            "name" : "Invalid code"
          }
       }
    ],

diff --git a/...ge_product_code_and_product_type/change-product-type-to-beauty-api-v2-normal-account.json b/...ge_product_code_and_product_type/change-product-type-to-beauty-api-v2-normal-account.json
@@ -1,4 +1,4 @@
 {
    "status" : 0,
-   "status_verbose" : "No permission: product_change_product_type"
+   "status_verbose" : "no_permission: product_change_product_type"
 }
diff --git a/...e_product_code_and_product_type/change-product-type-to-empty-string-api-v2-moderator.json b/...e_product_code_and_product_type/change-product-type-to-empty-string-api-v2-moderator.json
@@ -0,0 +1,4 @@
+{
+   "status" : 1,
+   "status_verbose" : "fields saved"
+}
diff --git a/...ts/change_product_code_and_product_type/change-product-type-to-null-api-v2-moderator.json b/...ts/change_product_code_and_product_type/change-product-type-to-null-api-v2-moderator.json
@@ -0,0 +1,4 @@
+{
+   "status" : 1,
+   "status_verbose" : "fields saved"
+}
diff --git a/...roduct_code_and_product_type/change-product-type-to-opff-api-v3-invalid-product-type.json b/...roduct_code_and_product_type/change-product-type-to-opff-api-v3-invalid-product-type.json
@@ -11,9 +11,9 @@
             "name" : "Failure"
          },
          "message" : {
-            "id" : 0,
-            "lc_name" : "",
-            "name" : ""
+            "id" : "invalid_product_type",
+            "lc_name" : "Invalid product type",
+            "name" : "Invalid product type"
          }
       }
    ],