Add the built-in WP REST API validation to CountryCodeTrait to avoid errors when clearing all audience countries in the onboarding flow

[eason9487]

Changes proposed in this Pull Request:

This PR solves a task of 📌 Clarify API error messages in #1993

Closes #1855

Originally, clearing the recipient country did not cause API errors. After #1327, the POST /mc/target_audience API was also validated by this check, which causes the auto-saving behavior in the onboarding flow to get errors.

• Add the built-in validation of WP REST API for the country codes in CountryCodeTrait.
• Change to use the parameter's schema to validate if the country codes array is not empty for needed Controllers:
  • BudgetRecommendationController
  • CampaignController
• Add or adjust tests for the related Controllers:
  • Add basic tests for the TargetAudienceController and ShippingTimeBatchController.
  • Add the test case of empty country codes for the BudgetRecommendationController
  • Remove an unused variable from the test_create_campaign_empty_targeted_locations in CampaignControllerTest.php

📌 Please take a look at the Additional details for contexts and other solutions.

Screenshots:

Kapture.2023-06-28.at.15.09.40.mp4

Detailed test instructions:

1. Go to step 2 of the onboarding flow.
2. Remove all audience countries.
3. Check if there is no API error.

Additional details:

About the validation mechanism of WP REST API

When extending the WP REST API, the mechanism of validate_callback and sanitize_callback is somewhat surprising. In summary, the findings are:

1. If the sanitize_callback is not specified but type is specified, both built-in validate_callback and sanitize_callback will be applied no matter whether the validate_callback is a custom one.
2. If both validate_callback and sanitize_callback are specified with custom callbacks, the built-in schema validation won't be applied.
3. The built-in validate_callback may be applied two times if not specify both callbacks.
4. When the built-in validate_callback is skipped, the required specified in args will still work. This makes the validation options of args work or not, resulting in more unexpected inconsistencies.
5. Looks like there is no way to specify in args that when a custom validate_callback is specified, still uses the built-in schema validation.

This can be a bit confusing to use because the main goal of sanitize_callback doesn't seem to be data validation, but whether the built-in schema validation works in the end depends on sanitize_callback and not just validate_callback.

Why the current solution: Add the built-in validation to CountryCodeTrait

When dealing with this issue, I have thought about other possible solutions, and the reasons for choosing the current one so far are:

• The ability to use core validation that has been validated for a long time can prevent GLA from doing similar validation additionally.
• There are some GLA APIs that use validation schemas such as minItems and uniqueItems for country codes, but these have not really worked in the past. This solution will allow them to work together as well.
  • BatchSchemaTrait
  • ShippingRateSuggestionsController
• There are a few similar implementations in the WP core.
  • class-wp-rest-posts-controller.php
  • class-wp-rest-menus-controller.php
  • class-wp-rest-themes-controller.php

But I still have some doubts about whether doing so within CountryCodeTrait is too subtle and becomes inconsistent at a higher level concept.

Other possible solutions

1. Add the third parameter to CountryCodeTrait::validate_country_codes and to all related callers.
2. Similar to the first one but don't add the parameter to callers. Instead, add a new method such as get_country_code_and_allow_empty_validate_callback to CountryCodeTrait.
3. Wrap the validate_callback in TargetAudienceController with an additional process: When countries is found to be empty, skip calling to the validation function returned by get_country_code_validate_callback.
4. Remove the custom sanitize_callback from all CountryCodeTrait::validate_country_codes-related callers so that the built-in validate_callback will be applied.
5. Add a new API dedicated to removing the audience countries and change the original POST /mc/target_audience to create audience countries instead of a PUT like the one defined in the RESTful API.
6. Any others?

Changelog entry

Fix - Avoid errors when clearing all audience countries in the onboarding flow.

[codecov]

Codecov Report

Merging #2003 (4eeffe8) into develop (e434627) will increase coverage by 0.4%.
The diff coverage is 100.0%.

Additional details and impacted files

@@             Coverage Diff             @@
##             develop   #2003     +/-   ##
===========================================
+ Coverage       57.5%   57.9%   +0.4%     
  Complexity      4111    4111             
===========================================
  Files            456     456             
  Lines          17655   17658      +3     
===========================================
+ Hits           10156   10224     +68     
+ Misses          7499    7434     -65     

Flag	Coverage Δ
php-unit-tests	57.9% <100.0%> (+0.4%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...Controllers/Ads/BudgetRecommendationController.php	100.0% <100.0%> (ø)
...rc/API/Site/Controllers/Ads/CampaignController.php	100.0% <100.0%> (ø)
src/API/Site/Controllers/CountryCodeTrait.php	100.0% <100.0%> (ø)

... and 2 files with indirect coverage changes

[puntope]

I'd suggest renaming this as $validation_result, $validation or something like that. Since it can return a WP_Error object, hence is not "valid".

[eason9487]

Renamed in bdfb365.

[puntope]

Maybe we can add somewhere a short explanation of your findings of why rest_validate_request_arg is not being called automatically.

[eason9487]

Added in 4eeffe8.

[puntope]

Maybe We can add some tests for:

• Countries with wrong format return Error
• Countries not supported can be added without errors.

[puntope]

Same as https://github.com/woocommerce/google-listings-and-ads/pull/2003/files#r1245074785

[puntope]

Can we test if in ShippingRateController the validation for country also works?

Same for ShippingRateSuggestionsController, at least would be nice to include the country_codes test.

[puntope]

If the sanitize_callback is not specified but type is specified, both built-in validate_callback and sanitize_callback will be applied no matter whether the validate_callback is a custom one.

Checking here, seems like it runs rest_parse_request_arg as the default sanitize callback when a type is added.
https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/class-wp-rest-request.php#L821

That rest_validate_request_arg runs the validation.

When the built-in validate_callback is skipped, the required specified in args will still work. This makes the validation options of args work or not, resulting in more unexpected inconsistencies.

Seems like the dispatcher is calling has_valid() function which checks the required params.

[puntope]

Hi @eason9487 , the PR looks good to me. I tested it locally and I cannot see anymore the early error when no countries are selected.

I left some checks I did after your findings. And some suggestions in regards test and some variable naming. Since the variable naming and the tests are not blockers, I will approve this in advance.

6. Any others?

To say something one simple solution might be to just not call the endpoint if country codes are empty in the JS side and keep all of them as required (minItems 1) in the backend.

[eason9487]

@puntope, thanks for the review.

Can we test if in ShippingRateController the validation for country also works?
Same for ShippingRateSuggestionsController, at least would be nice to include the country_codes test.

ShippingRateController specifies the country as a string type and it only indicates the required option, so it's not affected by this PR and doesn't have a validation schema that will work. In addition, the POST mc/shipping/rates API and all mc/shipping/rates/{country_code} APIs have not been in use after #411.

About ShippingRateSuggestionsController, I just recalled that its API endpoint mc/shipping/rates/suggestions has not been in use for some time after the use was removed by #1616 Perhaps removing it would be a bit better than writing tests for it.

Maybe We can add some tests for: [...]
Quoted from #2003 (comment) and #2003 (comment)

Considering several controllers use CountryCodeTrait to get the validation callback for country codes, it would be a bit repetitive to add the relevant tests for each of them. So I'm going to add tests for CountryCodeTrait instead, but the code changes were already more than expected and beyond this PR scope. I will open a separate PR for it.

6. Any others?

To say something one simple solution might be to just not call the endpoint if country codes are empty in the JS side and keep all of them as required (minItems 1) in the backend.

The intention of calling this API even if the countries array is empty on the UI side is to always remember the entered data during the onboarding flow, so that the user can continue with the previously entered data if they interrupt or refresh the onboarding flow. So I would think this solution would conflict with the purpose.

[eason9487]

Sorry, @puntope. A thing I mentioned in #2003 (comment) is incorrect. The POST mc/shipping/rates API and most mc/shipping/rates/{country_code} APIs are still being used by ShippingRateBatchController internally. I have also added a strikethrough for the incorrect description.