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

Make OSA fields extendable, simplify naming #41690

Merged
merged 14 commits into from
Jan 8, 2024

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Nov 25, 2023

This is an add-on to #39701

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Smoke test Add order attribution feature #39701 for regression
  2. Add PHP snippet
add_filter(
    'wc_order_attribution_tracking_fields',
    function( $fields ) {
        $fields['new_field'] = 'first_add.fd';
        return $fields;
    }
);
  1. Visit classic checkout,
    1. check that <input type="hidden" name="wc_order_attribution_new_field" value="" /> is in the source code.
    2. Check its value is correctly set document.querySelector('input[name="wc_order_attribution_new_field"]').value === sbjs.get.first_add.fd
  2. Visit Block checkout, and open dev tools
    1. click "Place order"
    2. Check /checkout ajax request payload contains the new field
      image
  3. Check that order metadata contains the new property.
  4. Review the code and check if all field-related names are understandable

Changelog entry

  • Automatically create a changelog entry from the details below.

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

Comment

@tomalec tomalec requested review from layoutd and a team November 25, 2023 20:42
@tomalec tomalec self-assigned this Nov 25, 2023
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Nov 25, 2023
Copy link
Contributor

github-actions bot commented Nov 25, 2023

Hi @message-dimke,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Contributor

Hi @layoutd, @woocommerce/ventures

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@tomalec tomalec mentioned this pull request Nov 25, 2023
15 tasks
Copy link
Contributor

github-actions bot commented Nov 25, 2023

Test Results Summary

Commit SHA: c53cc88

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 37s
E2E Tests258003026114m 47s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@tomalec tomalec force-pushed the feature/order_source_attribution-extendable-fields branch from 8cfa6a2 to 88e7e3c Compare November 25, 2023 21:41
@layoutd layoutd force-pushed the feature/order_source_attribution branch from cb733d7 to 19bb2a3 Compare November 27, 2023 17:59
Remove the need for translating it back and forth.
using `wc_order_attribution_tracking_fields`.
Propagate the fileds config to the client-side as well.

Disambiguate `fields` in variables and functions.
@tomalec tomalec force-pushed the feature/order_source_attribution-extendable-fields branch from 88e7e3c to 9fb2291 Compare November 29, 2023 16:49
Base automatically changed from feature/order_source_attribution to trunk November 30, 2023 11:54
@tomalec tomalec mentioned this pull request Dec 4, 2023
11 tasks
@message-dimke message-dimke requested review from message-dimke and removed request for a team and layoutd December 9, 2023 13:47
@message-dimke
Copy link
Contributor

Hey, @tomalec !

I am curious what should the value be, first_add.fd or sbjs.get.first_add.fd, or a date time?
The filter sets first_add.fd but in your test instructions for blocks the image shows date time and the actual tests show date time.

I am getting date time for both classical checkout and the block one.

Block:

Fullscreen_09_12_2023__18_19

Classic:

Fullscreen_09_12_2023__18_22

@tomalec
Copy link
Member Author

tomalec commented Dec 10, 2023

I am curious what should the value be, first_add.fd or sbjs.get.first_add.fd, or a date time?

The value of the wc_order_attribution_tracking_fields item should be the property accessor within sbjs.get object: first_add.fd

then when processed in browser's runtime it should be resolved to the value of "sbjs.get.the.accessor.you.have.given". Which in the example should be the value of sbjs.get.first_add.fd. Which per sourcebuster docs is

Date and time of a visit. Format: yyyy-mm-dd hh:mm:ss...

So, the results you see are correct.

Copy link
Contributor

@message-dimke message-dimke left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, @tomalec !

@tomalec tomalec added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Dec 19, 2023
Copy link
Contributor

@layoutd layoutd left a comment

Choose a reason for hiding this comment

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

These looks like they're working well! I tested with block and shortcode checkouts, and with several types of source types. The template version needs to be updated to match the current trunk version.

I was going to suggest we add metadata migration in another PR to allow sites that already have OA data work with these changes, but I realized that the OA feature already maps the type and url to source_type and referrer meta keys:

/**
* Get the field name with the meta prefix.
*
* @param string $field The field name.
*
* @return string The prefixed field name.
*/
private function get_meta_prefixed_field( string $field ): string {
// Map some of the fields to the correct meta name.
if ( 'type' === $field ) {
$field = 'source_type';
} elseif ( 'url' === $field ) {
$field = 'referrer';
}
return "_{$this->get_prefixed_field( $field )}";
}

So that's one fewer thing to do!

@tomalec tomalec added the impact: high To be added by the PR author/reviewer if the PR could very likely cause SEV-1 or SEV-2 issues label Dec 28, 2023
@tomalec
Copy link
Member Author

tomalec commented Dec 28, 2023

I'm marking it as high-impact as:

  • It's a big PR (i.e. adds several changes in many files).
  • Modifies critical functionality: Shopper | Checkout | Existing customer can place order
  • Changes to WooCommerce hooks/actions/filters.

Co-authored-by: Justin Palmer <228780+layoutd@users.noreply.github.com>
@alopezari
Copy link
Contributor

alopezari commented Jan 3, 2024

Hi there! The E2E tests are failing here, I triggered a re-run just in case the failure was introduced by these changes. Will keep an eye on this.

Update: They passed 👍

…-fields

Resolve conflicts, redo some work in `plugins/woocommerce/client/legacy/js/frontend/order-attribution.js`,.
@tomalec tomalec force-pushed the feature/order_source_attribution-extendable-fields branch from 2fdfb6d to e887ea3 Compare January 3, 2024 22:12
@tomalec
Copy link
Member Author

tomalec commented Jan 3, 2024

This PR is mostly with developer-facing tweaks. Also, may require some follow-up action to double-check if tracking- and meta- data migrate well.

As #43012 got full approval and analysis, while #41856 (its version based on this PR) didn't, I merged #43012, to deliver the customer-facing fix ASAP.

The above means, trunk received changes that conflicts with this branch, and requires some adjustments. I tried to cover them in the merge commit, non-trivial changes were made to e887ea3#diff-21bb08f041552efddbc9e2515315dddcedadedef055476d4fe4dd35962d5c25f

Maybe another pair of eyes could check if those changes didn't break anything?

@tomalec tomalec merged commit fa62e63 into trunk Jan 8, 2024
30 checks passed
@tomalec tomalec deleted the feature/order_source_attribution-extendable-fields branch January 8, 2024 15:17
@github-actions github-actions bot added this to the 8.6.0 milestone Jan 8, 2024
@nigeljamesstevenson nigeljamesstevenson added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Jan 9, 2024
samueljseay pushed a commit that referenced this pull request Jan 9, 2024
- Use `referrer` & `source_type` field names consistently
   Remove the need to translate it back and forth.
- Make fields actually extendable using `wc_order_attribution_tracking_fields`.
   Propagate the field configuration to the client side as well.
   Disambiguate fields in variables and functions.

Co-authored-by: Justin Palmer <228780+layoutd@users.noreply.github.com>
tomalec added a commit that referenced this pull request Jan 10, 2024
layoutd pushed a commit that referenced this pull request Jan 22, 2024
layoutd added a commit that referenced this pull request Jan 23, 2024
* Rename order attribution template file
* Update OA field name to `source_type` to keep up with #41690
* Discard conditional UTM labels altogether

---------

Co-authored-by: Tomek Wytrębowicz <tomalecpub@gmail.com>
opr pushed a commit that referenced this pull request Jan 25, 2024
* Rename order attribution template file
* Update OA field name to `source_type` to keep up with #41690
* Discard conditional UTM labels altogether

---------

Co-authored-by: Tomek Wytrębowicz <tomalecpub@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: high To be added by the PR author/reviewer if the PR could very likely cause SEV-1 or SEV-2 issues needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants