Skip to content

Fix non-ASCII coupon search #56319

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

Merged
merged 29 commits into from
Mar 19, 2025
Merged

Fix non-ASCII coupon search #56319

merged 29 commits into from
Mar 19, 2025

Conversation

timur27
Copy link
Contributor

@timur27 timur27 commented Mar 12, 2025

Submission Review Guidelines:

Changes proposed in this Pull Request:

Closes #32536

When PHP attempts to strtolower() for non-ASCII chars, the converted value is corrupted which is why the search for coupons with titles like e.g. VÅR20 failed. This PR removes the lowercase, similarly to how it was done in #25314.

How to test the changes in this Pull Request:

0️⃣ Preparation steps

  • Change the collation for your DB and all the tables in it with the following steps

Once inside the database, click on the "Operations" tab in the top menu.
Change Database Collation:
Scroll down to the "Collation" section.
From the dropdown menu, select latin1_swedish_ci.
Check the boxes for "Change all tables collations" and "Change all tables columns collations".
Click Go to apply changes.

  • Use the following SQL to check whether the table has the latin1_swedish_ci collation set
SELECT TABLE_NAME, TABLE_COLLATION 
FROM information_schema.TABLES 
WHERE TABLE_SCHEMA = 'wordpress'
AND TABLE_NAME = 'wp_posts';

Also define the collation and charset constants in docker/wordpress/wp-config.php

define('DB_CHARSET', 'latin1');
define('DB_COLLATE', 'latin1_swedish_ci');

1️⃣ Adding coupons

  • Go to WooCommerce → Coupons → Add New
  • Enter a coupon code with mixed case (e.g., SummerSale2025)
  • Create the coupon with the following title: VÅR20

2️⃣ Applying coupons

On the shortcode

  • Apply the VÅR20 coupon and confirm it's applied
  • Remove the VÅR20 and confirm it's removed
  • Apply the vÅr20 coupon and confirm case-insensitivity as the coupon should be applied too
  • Apply the SummerSale2025 coupon and confirm it's applied
  • Apply the SUMMERSALE2025 coupon and confirm that you see the "Coupon code already applied!" message under the input field

3️⃣ Removing coupons

Staying on the shortcode

  • remove all the currently applied coupons and confirm they are successfully removed

4️⃣ Other pages

  • Repeat steps 2️⃣ and 3️⃣ on
    • Blocks checkout page
    • Cart block page
    • Shortcode cart page

5️⃣ Manual order creation & coupon management

  • Create a new order in WooCommerce admin (WooCommerce → Orders → Add New)
  • Add some products to the order
  • Apply the VÅR20 coupon and confirm it's applied
  • Remove the VÅR20 and confirm it's removed
  • Apply the vÅr20 coupon and confirm case-insensitivity as the coupon should be applied too
  • Apply the SummerSale2025 coupon and confirm it's applied
  • Apply the SUMMERSALE2025 coupon and confirm that you see the "Coupon code already applied!" message under the input field
  • Remove the coupons by clicking on the small "x" on the coupon icons and confirm the coupons are successfully removed

6️⃣ APIv1 tests

  • Perform a POST /wp-json/wc/v1/coupons
Here's the request body to take as an example
{
  "code": "MÅQ25",
  "discount_type": "percent",
  "amount": "10",
  "individual_use": true,
  "exclude_sale_items": false,
  "minimum_amount": "100.00"
}
  • Once the POST request is successful, please verify if you can use this coupon on any of the checkout or cart pages in different forms
    • MÅQ25
    • mÅq25

7️⃣ APIv2 tests

  • Perform a POST /wp-json/wc/v2/coupons
Here's the request body to take as an example
{
  "code": "DÅQ25",
  "discount_type": "percent",
  "amount": "10",
  "individual_use": true,
  "exclude_sale_items": false,
  "minimum_amount": "100.00"
}
  • Once the POST request is successful, please verify if you can use this coupon on any of the checkout or cart pages in different forms
    • DÅQ25
    • dÅq25

8️⃣ APIv3 order test

  • perform aPOST /wp-json/wc/v3/orders request with a different case of a coupon included in the request body and confirm, that for all of them the request is successful
Example (additionally try with other combinations of existing coupon code e.g. `vÅR20` and `VÅR20` or `vÅr20`
{
  "payment_method": "bacs",
  "payment_method_title": "Direct Bank Transfer",
  "set_paid": false,
  "billing": {
    "first_name": "John",
    "last_name": "Doe",
    "email": "admin@example.com"
  },
  "line_items": [
    {
      "product_id": 123,
      "quantity": 2
    }
  ],
  "coupon_lines": [
    {
      "code": "vÅR20"
    }
  ]
}
  • Once the POST request is successful, check that the coupon is attached to the order in the order details

9️⃣ WooCommerce Analytics

  • Navigate to Analytics -> Orders and confirm that the coupon codes are displayed for the orders they were used at
  • Navigate to Analytics -> Coupons and confirm that the filtering/search of coupons works and is case-insensitive

Testing that has already taken place:

Changelog entry

  • Automatically create a changelog entry from the details below.
  • This Pull Request does not require a changelog entry. (Comment required below)
Changelog Entry Details

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Changelog Entry Comment

Comment

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 12, 2025
@timur27 timur27 changed the title remove strtolower operation that caused corrupted transformation Fix non-ASCII coupon search Mar 12, 2025
@timur27 timur27 requested review from a team and frosso and removed request for a team March 12, 2025 16:14
@timur27
Copy link
Contributor Author

timur27 commented Mar 12, 2025

I checked the failing e2e tests and they occur on trunk too.

@timur27 timur27 marked this pull request as ready for review March 12, 2025 16:14
Copy link
Contributor

github-actions bot commented Mar 12, 2025

Testing Guidelines

Hi @frosso ,

Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed.

Reminder: PR reviewers are required to document testing performed. This includes:

  • 🖼️ Screenshots or screen recordings.
  • 📝 List of functionality tested / steps followed.
  • 🌐 Site details (environment attributes such as hosting type, plugins, theme, store size, store age, and relevant settings).
  • 🔍 Any analysis performed, such as assessing potential impacts on environment attributes and other plugins, conducting performance profiling, or using LLM/AI-based analysis.

⚠️ Within the testing details you provide, please ensure that no sensitive information (such as API keys, passwords, user data, etc.) is included in this public issue.

Copy link
Contributor

github-actions bot commented Mar 12, 2025

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

@timur27 timur27 force-pushed the fix/non-ascii-coupon-search branch from e1cad97 to ae4cdbf Compare March 13, 2025 07:40
@timur27
Copy link
Contributor Author

timur27 commented Mar 13, 2025

I think I switched the context to something else yesterday and forgot to push ae4cdbf as well. Now it's here.

@timur27 timur27 force-pushed the fix/non-ascii-coupon-search branch from 0697e22 to 6122a36 Compare March 13, 2025 07:47
@timur27 timur27 force-pushed the fix/non-ascii-coupon-search branch from 6122a36 to eb006c9 Compare March 13, 2025 16:09
@timur27 timur27 force-pushed the fix/non-ascii-coupon-search branch from f6c90dd to b3a1814 Compare March 14, 2025 07:37
@timur27 timur27 requested a review from frosso March 14, 2025 07:42
Copy link
Contributor

@frosso frosso left a comment

Choose a reason for hiding this comment

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

I left a few more comments about the new implementation, but the main one is about the possibility that filters applied onto woocommerce_coupon_code wouldn't be applied to woocommerce_coupon_code_without_lower - I'd like to hear your thoughts on that point.

@timur27
Copy link
Contributor Author

timur27 commented Mar 14, 2025

After discussing in p1741951466805729-slack-C8X6Q7XQU and f2f, we picked the initial version of the implementation, which removes the wc_strtolower from the formatting filter chain. I also added the helper comparison function to encapsulate the lowercasing to simplify it a bit.

@frosso thanks for the reviews so far, may I ask you to review & test it?

@timur27 timur27 requested a review from frosso March 14, 2025 15:32
@timur27
Copy link
Contributor Author

timur27 commented Mar 17, 2025

@frosso after #56319 (comment) on Friday, I also added 3f2624c today in the morning because I realized that it wouldn't hurt to have an extra sanitization when comparing values too. I hope it didn't interfere with your review process. Thanks!

Copy link
Contributor

@frosso frosso left a comment

Choose a reason for hiding this comment

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

Tested all checkout flavors, API, coupon management with same coupon code, etc.
Here's a non-comprehensive dump of test scenarios I performed:
Screenshot 2025-03-17 at 10 16 10 PM
Screenshot 2025-03-17 at 9 58 15 PM
Screenshot 2025-03-17 at 9 58 05 PM
Screenshot 2025-03-17 at 9 57 54 PM
Screenshot 2025-03-17 at 9 57 03 PM
Screenshot 2025-03-17 at 9 54 28 PM
2025-03-17 21 53 52
Screenshot 2025-03-17 at 9 52 06 PM

My comment is non-blocking. Just trying to think of different scenarios. Thank you for all the discussion(s) so far!

* @return bool
*/
function wc_is_same_coupon( $coupon_1, $coupon_2 ) {
return wc_strtolower( $coupon_1 ) === wc_strtolower( $coupon_2 );
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) I wonder about the utility of a function that says it's comparing coupons, when in reality it's just comparing two strings converted to lowercase. Are you just trying to get a consistent comparison without the overhead of WC_Coupon?

I was thinking that one alternative could be to use the WC_Coupon core class - something like this:

function wc_is_same_coupon( $coupon_1, $coupon_2 ) {
    $coupon_obj_1 = new WC_Coupon( $coupon_1 );
    $coupon_obj_2 = new WC_Coupon( $coupon_2 );
    
    return $coupon_obj_1->get_code() === $coupon_obj_2->get_code();
}

That way, you don't need to call wc_strtolower each time (and you could possibly keep the filter).

But there might be some overhead with the usage of WC_Coupon 🤷

In terms of naming convention, I noticed that there seem to be a slight preference towards the wc_[object]_is* convention - so maybe consider the naming wc_coupon_is_equal()? But again, up for discussion, I'd like to hear your thoughts 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the thoughts!

The coupon class is slightly more preferable to me too, and we could move to using the state of the class and doing something like

public function is_coupon_code_equal_to( $coupon_code ) {
    return wc_strtolower( $this->get_code() ) === wc_strtolower( $coupon_code );
}

in WC_Coupon, but I haven't done so because there's also WC_Order_Item_Coupon class for order flows and I didn't want to repeat the logic when there's an utility file. I acknowledge that there are existing places that are duplicated but I didn't want to follow this behavior.


In terms of naming convention, I noticed that there seem to be a slight preference towards the wc_[object]_is* convention

I was considering that too, but I found helper functions following the wc_is_* pattern to be more prevalent:

wc_is_order_status()
wc_is_valid_url()
wc_is_attribute_in_product_name()
wc_is_webhook_valid_topic()
wc_is_webhook_valid_status()
wc_is_current_account_menu_item()
wc_is_external_resource()
wc_is_active_theme()
wc_is_wp_default_theme_active()
wc_is_file_valid_csv()

many of wc_is_* are also helper functions performing some basic operations of objects/collections/types.

I think having said that plus the fact that new changes will require additional round of testing, I think keeping things as they are would be my favorite option, but I will keep the PR open for a bit in case you will want to chat more. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

The coupon class is slightly more preferable to me too, and we could move to using the state of the class and doing something like [...] in WC_Coupon, but I haven't done so because there's also WC_Order_Item_Coupon class for order flows and I didn't want to repeat the logic when there's an utility file. I acknowledge that there are existing places that are duplicated but I didn't want to follow this behavior.

That's also a great suggestion - unfortunate about WC_Order_Item_Coupon.

I was considering that too, but I found helper functions following the wc_is_* pattern to be more prevalent

I was basing my observation off of this:
(wc|woocommerce)_(is|has)_[a-z]*: 97 results, 29 unique instances
(wc|woocommerce)_[a-z]*_(is|has)_[a-z_-]*: 110 results, 37 unique instances

This includes both functions and filters, of course.

But either way - not a massive difference in terms of results :) Thank you for the consideration!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: analysis Indicates if the PR requires a PR testing scrub session. plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coupons: Issues with latin1 and non-latin1 characters
2 participants