Skip to content
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

Use get_title() for content_name to match catalog name. #2781

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Conversation

rawdreeg
Copy link
Contributor

@rawdreeg rawdreeg commented Aug 8, 2024

Changes proposed in this Pull Request:

We received a report ( #2774 ) pointing out inconsistencies in populating the content_name property in events we send to Meta, sometimes using get_title() vs get_name(). I have looked into this and found that get_title() applies the woocommerce_product_title filter to the product name prop. This may result in different values being sent to Facebook. I also find that we use get_title for the catalog data, so I propose we use the get_title method

Closes #2774.

  • Do the changed files pass phpcs checks? Please remove phpcs:ignore comments in changed files and fix any issues, or delete if not practical.

Detailed test instructions:

  1. Use the woocommerce_product_title filter. E.g.:
function custom_woocommerce_product_title_prefix( $title, $product ) {
    // Add the prefix "Special: " to the product title
    $title = 'Special: ' . $title;

    return $title;
}
add_filter( 'woocommerce_product_title', 'custom_woocommerce_product_title_prefix', 10, 2 );

  1. Using the FB pixel helper browser extension, visit a product and confirm that the ViewContent event fires and all the details are confirmed.
  2. Repeat this with the addToCart, Checkout and Purchase Events

Additional details:

Changelog entry

Update - Use get_title() for content_name to match catalog name.

@github-actions github-actions bot added changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug. labels Aug 8, 2024
@rawdreeg rawdreeg self-assigned this Aug 8, 2024
@rawdreeg rawdreeg requested a review from a team August 8, 2024 12:22
Copy link
Contributor

@nima-karimi nima-karimi left a comment

Choose a reason for hiding this comment

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

LGTM ✅ All events include the correct title with the added prefix.

@rawdreeg
Copy link
Contributor Author

Thanks @nima-karimi for the review.

@rawdreeg rawdreeg merged commit 9ea6079 into develop Aug 14, 2024
4 checks passed
@rawdreeg rawdreeg deleted the fix/2774 branch August 14, 2024 08:18
@Dekadinious
Copy link

Not to be a party pooper here, but given that this is in a WooCommerce context and that WooCommerce also has filters for $product->get_name(); one would think that using that everywhere would be more future proof as WooCommerce might move products to custom tables just as they did with orders (HPOS).

Using native getters and setters on objects is the preferable way to code for WooCommerce anyway, so people using the post title filter might be considered to be "doing_it_wrong".

@rawdreeg
Copy link
Contributor Author

Thanks @Dekadinious,

I picked get_title() over get_name(). This is used for the catalogue product as well:

'title' => WC_Facebookcommerce_Utils::clean_string( $this->get_title() ),

This aligns the events sent with whatever name is set to Catalogue, reducing the risk of mismatch.

@Dekadinious
Copy link

Sorry, had a brain fart there for a second. Move on, nothing to see here! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent use of get_title vs. get_name on product objects
3 participants