Remove woocommerce_loop_add_to_cart_link filter

[MiguelAxcar]

All Submissions:

• Does your code follow the WooCommerce Sniffs variant of WordPress coding standards?
• Have you written new tests for your changes, as applicable?
• Have you successfully run tests with your changes locally?
• Will this change require new documentation or changes to existing documentation?

---

Changes proposed in this Pull Request:

This PR removes the woocommerce_loop_add_to_cart_link filter and its associated functionality. The goal is to ensure consistent styling between the "Add to Cart" and "Buy Gift Card" buttons for all themes.

I found that the button "Buy Gift Card" was being rendered by this piece of code called by woocommerce_loop_add_to_cart_link filter. This function was added on 2023 Jul 25 by this PR and the main idea was:

• Replacing "Add to Cart" button with "Buy Gift Card".
• Changing the link of the button to the product page instead of adding it to the cart, so users could fill in gift card.

Upon inspecting the rendered content, the "Buy Gift Card" button lacked several styling classes present on the "Add to Cart" button. Additionally, the "Add to Cart" button contained a nested <span> for the text, while the "Buy Gift Card" button did not. This would explain the "Buy Gift Card" button not matching the "Add to Cart" button styling. The first idea here was using the WP_HTML_Tag_Processor class to only change button text and link, trying a less visually impactful approach as all the parameters would be kept, instead of recreating the button as it's being done.

After researching the code, I found that when attending to this issue, on 2024 Jun 24 this PR added support for the product blocks, changing specifically button "Add to Cart" text and URL, targeting to the product page. A very clean approach, changing only what is necessary in the button.

Then I removed the filter, tested the following features on Block/FSE Themes TT5, TT4 and TT3 and it worked as expected, added by

• The "Add to Cart" button is correctly replaced by "Buy Gift Card" for Square Gift Card products.
• The "Buy Gift Card" button redirects users to the product page instead of directly adding it to the cart, ensuring that necessary input fields are filled on the product page.

I also tested on the TT1 Classic Theme and, although it isn't visually appealing, I confirm that main functionality is also working.

Given that, I understand that the filter woocommerce_loop_add_to_cart_link can be removed, as the main functionality is being covered by the code added by this PR .

Closes #140.

Steps to test the changes in this Pull Request

1. Activate TT4 (Twenty Twenty-Four) theme.
2. Switch to this branch on your local environment
3. Create a Square Gift Card product.
   
4. Visit the Shop page and confirm the following:
   • A "Buy Gift Card" is being displayed for such product instead of "Add to Cart" button.
   • The "Buy Gift Card" button visually matches the "Add to Cart" button.
   • Clicking "Buy Gift Card" redirects to the product page where users can complete necessary input fields for the gift card.

Additional tests

1. Activate TT5, TT3, or TT1 Classic theme and repeat steps to confirm that functionality remains intact across Block/FSE and Classic themes.

Changelog entry

Fix - Remove woocommerce_loop_add_to_cart_link filter to standardize "Buy Gift Card" and "Add to Cart" button styles across themes.

[dkotter]

Code here looks fine but I'm seeing some E2E test failures. At a glance, doesn't seem like those are related to this PR but would be ideal to investigate those before we proceed.

[MiguelAxcar]

Hello @dkotter .

I dived on E2E tests today, and I have some findings to share.

Broader issue

I found that some recent pull requests stumbled on E2E failures.

• fix/202: make "You're connected to Square" page entire UI to be visible #224

  2 failed
    [chromium] › d6.woo-sor.spec-with-inventory-new-editor.spec.js:25:5 › OnePlus 8 pushed to Square with inventory 
    [chromium] › d6.woo-sor.spec-with-inventory-new-editor.spec.js:105:5 › Update inventory from Woo to Square 
  1 flaky
    [chromium] › b7.gift-card-partial-payment-partial-refund.spec.js:43:5 › Gift card - Partial payment 

• Remove woocommerce_loop_add_to_cart_link filter #237

  5 failed
    [chromium] › d6.woo-sor.spec-with-inventory-new-editor.spec.js:25:5 › OnePlus 8 pushed to Square with inventory 
    [chromium] › d6.woo-sor.spec-with-inventory-new-editor.spec.js:105:5 › Update inventory from Woo to Square 
    [chromium] › f1.pre-orders.spec.js:53:2 › Pre-Orders Tests › [Charge upon release] Square Credit Card should work with Pre-Orders 
    [chromium] › f1.pre-orders.spec.js:106:2 › Pre-Orders Tests › [Upfront Charge] Square Credit Card should work with Pre-Orders 
    [chromium] › f1.pre-orders.spec.js:152:2 › Pre-Orders Tests › [Upfront Charge] Square Cash App Pay should work with Pre-Orders 
  1 flaky
    [chromium] › c3.woo-sor.spec-with-inventory.spec.js:53:5 › OnePlus 8 pushed to Square with inventory 

• Bump WooCommerce "tested up to" version 9.5  #239

  4 failed
    [chromium] › b8.gift-card-product.spec.js:128:5 › Gift card recipient email ────────────────────
    [chromium] › f1.pre-orders.spec.js:53:2 › Pre-Orders Tests › [Charge upon release] Square Credit Card should work with Pre-Orders 
    [chromium] › f1.pre-orders.spec.js:106:2 › Pre-Orders Tests › [Upfront Charge] Square Credit Card should work with Pre-Orders 
    [chromium] › f1.pre-orders.spec.js:152:2 › Pre-Orders Tests › [Upfront Charge] Square Cash App Pay should work with Pre-Orders 
  1 flaky
    [chromium] › d1.cash-app-pay.spec.js:328:6 › Cash App Pay Tests › [Block]: Customers can pay using Cash App Pay after decline transcation once - @foundational 

• On my local environment, against trunk
  The Gift Card test fails intermittently on my local env, the others happened on all the ~5 full e2e tests I performed.

8 failed
    [chromium] › b7.gift-card-partial-payment-partial-refund.spec.js:43:5 › Gift card - Partial payment 
    [chromium] › b8.gift-card-product.spec.js:53:5 › Purchase Gift card product ────────────────────
    [chromium] › b8.gift-card-product.spec.js:128:5 › Gift card recipient email ────────────────────
    [chromium] › c3.woo-sor.spec-with-inventory.spec.js:53:5 › OnePlus 8 pushed to Square with inventory 
    [chromium] › d6.woo-sor.spec-with-inventory-new-editor.spec.js:25:5 › OnePlus 8 pushed to Square with inventory 
    [chromium] › f1.pre-orders.spec.js:53:2 › Pre-Orders Tests › [Charge upon release] Square Credit Card should work with Pre-Orders 
    [chromium] › f1.pre-orders.spec.js:106:2 › Pre-Orders Tests › [Upfront Charge] Square Credit Card should work with Pre-Orders 
    [chromium] › f1.pre-orders.spec.js:152:2 › Pre-Orders Tests › [Upfront Charge] Square Cash App Pay should work with Pre-Orders 

Starting point

I had chosen the tests c3.woo-sor.spec-with-inventory.spec.js and d6.woo-sor.spec-with-inventory-new-editor.spec.js as a start, as they were related to same process and they are happening more often, expecting to fix them, but after investing some initial time, I'm delivering partial results and aborting this investigation until further notice as I expect that this is a bigger issue which will need dedicated time allocation to be completely fixed.

Specifically about the "OnePlus 8 pushed to Square with inventory" test, I found challenging to guarantee that a product added to Square catalog could be properly retrieved.

The common initial error was regarding to page.goto being called after the test has ended, as test flow doesn't wait for the async interval logic in the code, causing the page.goto call to occur outside the lifecycle of the test.

Error: page.goto: Test ended.
    Call log:
      - navigating to "http://localhost:8889/wp-admin/admin.php?page=wc-settings&tab=square&section=update", waiting until "load"


      58 |  const result = await new Promise( ( resolve ) => {
      59 |      let intervalId = setInterval( async () => {
    > 60 |          await page.goto( '/wp-admin/admin.php?page=wc-settings&tab=square&section=update' );
         |                     ^
      61 |          const __result = await listCatalog();
      62 |          if ( __result.objects ) {
      63 |              clearInterval( intervalId );

        at Timeout._onTimeout (/app/public/wp-content/plugins/woocommerce-square/tests/e2e/specs/c3.woo-sor.spec-with-inventory.spec.js:60:15)

Failing code

const result = await new Promise( ( resolve ) => {
  let intervalId = setInterval( async () => {
    await page.goto( '/wp-admin/admin.php?page=wc-settings&tab=square&section=update' );
    const __result = await listCatalog();
    if ( __result.objects ) {
      clearInterval( intervalId );
      resolve( __result );
    }
  }, 3000 );
} );

Issues regarding to this approach

• Using setInterval with async can lead to unexpected behavior as setInterval is unable to handle promises properly.
• The test may finish before the interval completes, leading to such errors Error: page.goto: Test ended

I initially replaced the setInterval in the promise with a polling loop using a recursive setTimeout, preventing race conditions.

const result = await new Promise((resolve, reject) => {
    let attempts = 0;
    const maxAttempts = 10;

    const checkCatalog = async () => {
        attempts++;
        console.log(`attempt ${attempts}: checking Square catalog...`);

        try {
            await page.goto('/wp-admin/admin.php?page=wc-settings&tab=square&section=update');
            const __result = await listCatalog();

            if (__result.objects) {
                console.log('items found!');
                resolve(__result);
                return;
            }

            console.log('not yet found.');
        } catch (error) {
            console.error(`error on attempt ${attempts}: `, error);
        }

        if (attempts < maxAttempts) {
            setTimeout(checkCatalog, 5000); // 5 seconds between each retrying
        } else {
            console.error('Catalog objects not found at all.');
        }
    };

    // Start the first attempt
    checkCatalog();
});

That made the test to work, but it gave me some insights.

Testing observations

• The test durations vary significantly between runs - from over a minute and 10 retries to under 10 seconds, with a couple of requests.
• The number of attempts required to find the catalog objects decreases over successive runs (from 7 attempts down to 2). It may indicate caching or residual data from previous runs potentially affecting the test results.
• A fixed delay given by setTimeout between attempts can be inefficient and lead to unnecessary waiting. We aren't actually waiting for the catalog objects to become available; instead, it retries after a fixed delay.
• The use of setTimeout within an async function complicates the control flow and may lead to unpredictable behavior.

Last working code

Test "OnePlus 8 pushed to Square with inventory" from c3.woo-sor.spec-with-inventory.spec.js

I might add that I ended up unable to gather reliable data to determine a reasonable maximum number of attempts based on expected response times. This may be the next step for this test.

But this code below is working and may be used as a future reference.

test( 'OnePlus 8 pushed to Square with inventory', async ( { page } ) => {
    test.slow();

    await page.goto( '/wp-admin/admin.php?page=wc-settings&tab=square&section=update' );

    console.log('getting catalog...');
    const result = await new Promise((resolve) => {
        let intervalId = setInterval(async () => {
            const __result = await listCatalog();
            if (__result.objects) {
                console.log('catalog retrieved from Square:', __result);
                clearInterval(intervalId);
                resolve(__result);
            } else {
                console.log('catalog not yet available, retrying in 3 seconds...');
            }
        }, 3000);

        setTimeout(() => {
            clearInterval(intervalId);
            throw new Error('timeout waiting for catalog');
        }, 90000);
    });

    console.log('extracting catalog info...');
    const { name, variations } = extractCatalogInfo( result.objects[0] );
    console.log('catalog data:', { name, variations });

    console.log('catalog assertions...');
    expect( name ).toEqual( 'OnePlus 8' );
    expect( variations[ 0 ].sku ).toEqual( 'oneplus-8' );
    expect( variations[ 0 ].price ).toEqual( 29900 );

    console.log('initial inventory count...');
    let inventory = await retrieveInventoryCount( variations[ 0 ].id );
    console.log('Initial inventory:', inventory);

    if ( ! inventory.counts ) {
        console.log('initial counts unavailable, polling...');
        inventory = await expect.poll(
            async () => {
                const __inventory = await retrieveInventoryCount( variations[ 0 ].id );
                console.log('inventory poll result:', __inventory);
                return __inventory.counts ? __inventory : null;
            },
            {
                message: 'waiting for inventory counts...',
                timeout: 90000, // max timeout
                interval: 4000, // time between retries
            }
        );
    }

    console.log('inventory assertions...');
    expect( inventory ).toHaveProperty( 'counts' );
    expect( inventory ).toHaveProperty( 'counts[0].quantity', '56' );
} );

Suggestion

Moving forward with this pull request, ignoring the E2E failing tests as they are not related to the fix, and creating a specific issue to investigate and fix all the problems regarding to E2E tests, allocating around one week of effort to it. Or maybe one issue per failing test, as you prefer.

I can create this issue ticket (or tickets) if you find this a good idea, adding failing tests as ticket tasks.

The code I added above could be used as a reference, an starting point.

Thanks!

[dkotter]

@MiguelAxcar Thanks for the very detailed explanation! I'm fine with moving this PR forward and we can investigate tests in separate issues. I know @iamdharmesh has worked a lot on tests (see #238 as one example of trying to make tests run faster) and he may have thoughts on the best approach here. For now, I'd suggest creating a new issue with the things you've found (could be mostly a copy/paste of your comment) and we can continue discussion there.

[dkotter]

@qasumitbagthariya Note this can go to UAT once QA passes

[qasumitbagthariya]

QA update: ✅

I tested the following on this branch:

• verify with different theme

TT4 Theme

Storefront

TT5

Responsive

Testing Environment

• WordPress: 6.7
• Theme: Storefront 4.6.0
• Theme: Twenty Twenty-Four 1.3
• Theme: Twenty Twenty-Four 1.0
• WooCommerce - 9.4.1
• PHP: 8.0.30
• Web Server: Nginx 1.20.2
• Browser: Chrome
• OS: macOS Ventura 13.3
• Branch: fix/140-match-buy-gift-card-button-style-tt4

Steps to Test- As mentioned in the PR description.
Test Results - It is working as expected.
Functional Demo / Screencast -
Special Notes - Ready for UAT
Testing Document status:
Cases related to this Issue/PR are added to the Critical Flow Wiki pages:

• Yes
• Not Required/Applicable for this PR

---