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

Block Editor Tracking: Add pattern_category property to wpcom_pattern_inserted event #53492

Merged

Conversation

david-szabo97
Copy link
Contributor

@david-szabo97 david-szabo97 commented Jun 8, 2021

Changes proposed in this Pull Request

  • Add pattern_category property to wpcom_pattern_inserted event
  • Add E2E test
    • Had to increase E2E events queue size. Inserting patterns fire a lot of block insertion events so this was necessary.
    • Also had to dismiss notices in some cases to avoid overlapping elements.

Testing instructions

  • Follow steps to enable tracking debugging at PCYsg-nrf-p2
  • Open Post Editor, and insert a pattern from the inserter sidebar. Make sure wpcom_pattern_inserted is fired and includes a pattern_category which is equal to the selected pattern category.
  • Now insert a pattern from the quick inserter. Make sure wpcom_pattern_inserted is fired and and pattern_category doesn't exist..

Related to #53410

@david-szabo97 david-szabo97 self-assigned this Jun 8, 2021
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 8, 2021
@matticbot
Copy link
Contributor

matticbot commented Jun 8, 2021

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

These questions are mainly for @ianstewart and @kurt213 regarding how we would want this to work.

Open Post Editor, and insert a pattern from the inserter sidebar. Make sure wpcom_pattern_inserted is fired and includes a pattern_category which is equal to the selected pattern category.

A pattern may have multiple categories, so I think either we need to:

  • track whichever category is selected (as is done here)
    OR
  • track the list of categories the pattern corresponds to?

However, when we select from the quick inserter where there are no categories to select.

Now insert a pattern from the quick inserter. Make sure wpcom_pattern_inserted is fired and includes a pattern_category which is equal to the first category of the pattern.

I would think instead of this we should either:

  • Populate the field as undefined since no category can be selected. (If we are going with the first option above)
    OR
  • once again track the list of categories the pattern corresponds to?

@david-szabo97 david-szabo97 force-pushed the add/tracking-pattern-insertion-pattern-category-name branch from 3e2a426 to 8ae94da Compare June 9, 2021 08:40
@ianstewart
Copy link
Contributor

track whichever category is selected (as is done here)
Populate the field as undefined since no category can be selected. (If we are going with the first option above)

This seems like it would be useful. cc @kurt213

@david-szabo97 david-szabo97 requested a review from Copons June 16, 2021 11:26
@Copons
Copy link
Contributor

Copons commented Jun 18, 2021

@david-szabo97 I think we can move ahead with the approaches confirmed by @ianstewart:

  • Insert pattern from the sidebar inserter.
    • Track the selected category.
  • Insert pattern from the quick inserter.
    • pattern_category: undefined.
      We should then clarify this in the event's property description in Tracks to make sure the undefined value is analyzed appropriately.

@kurt213
Copy link

kurt213 commented Jun 18, 2021

Insert pattern from the sidebar inserter.
Track the selected category.
Insert pattern from the quick inserter.
pattern_category: undefined.
We should then clarify this in the event's property description in Tracks to make sure the undefined value is analyzed appropriately.

This sounds great and this logic makes the most sense to me for understanding a user's 'intent'.

@david-szabo97 david-szabo97 force-pushed the add/tracking-pattern-insertion-pattern-category-name branch from a07aea5 to 45ce8e9 Compare June 18, 2021 16:04
@matticbot
Copy link
Contributor

This PR modifies the release build for wpcom-block-editor

To test your changes on WordPress.com, run install-plugin.sh wpcom-block-editor add/tracking-pattern-insertion-pattern-category-name on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-l4k-p2

@david-szabo97 david-szabo97 force-pushed the add/tracking-pattern-insertion-pattern-category-name branch from 45ce8e9 to 944ade1 Compare June 22, 2021 10:54
@david-szabo97 david-szabo97 requested review from scinos, WunderBart and a team as code owners June 22, 2021 13:55
@Addison-Stavlo
Copy link
Contributor

Now insert a pattern from the quick inserter. Make sure wpcom_pattern_inserted is fired and and pattern_category doesn't exist..

Im not seeing this at all. In both post editor and site editor inserting a pattern from the quick inserter doesn't seem to track any pattern_inserted event (on this PR or on trunk). Maybe I am missing something? 🤔

@david-szabo97
Copy link
Contributor Author

Now insert a pattern from the quick inserter. Make sure wpcom_pattern_inserted is fired and and pattern_category doesn't exist..

Im not seeing this at all. In both post editor and site editor inserting a pattern from the quick inserter doesn't seem to track any pattern_inserted event (on this PR or on trunk). Maybe I am missing something? 🤔

🤔 Looks like quick inserter isn't using an object as metadata:

image

Look how the pattern name is just a string in the array. Whereas inserting patterns from the inserter sidebar:

image

Is using an object with a patternName property, and this is what we use.

@david-szabo97
Copy link
Contributor Author

@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit add/tracking-pattern-insertion-pattern-category-name on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@Addison-Stavlo
Copy link
Contributor

Maybe this is part of the issue I was noticing before - but Im running into another oddity 😅 .

It seems that whether we are using the sidebar or the quick inserter, it may fire wpcom_block_inserted or wpcom_block_picker_block_inserted depending whether or not a block was selected (that is, it used insertBlocks vs. replaceBlocks.

In the case of inserting patterns via the sidebar, it seems we are getting the wpcom_pattern_inserted event in both cases! 🥳

However, with the quick inserter it seems that we are only getting the wpcom_pattern_inserted event to fire in the insertBlocks case. So when the quick inserter is used from inside the context of an empty paragraph block it doesn't seem to be firing the wpcom_pattern_inserted event. 🤔

Screen Shot 2021-06-23 at 10 50 06 AM

@david-szabo97
Copy link
Contributor Author

Ughhh!

The pattern name is on a different index so the actionData[4] failed there.

Replaced with a more robust logic.

@matticbot
Copy link
Contributor

This PR modifies the release build for notifications

To test your changes on WordPress.com, run install-plugin.sh notifications add/tracking-pattern-insertion-pattern-category-name on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-elI-p2

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

This is all working as expected now 😁

Copy link
Member

@WunderBart WunderBart left a comment

Choose a reason for hiding this comment

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

Just one comment!

test/e2e/lib/components/site-editor-component.js Outdated Show resolved Hide resolved
test/e2e/lib/components/site-editor-component.js Outdated Show resolved Hide resolved
test/e2e/lib/components/site-editor-component.js Outdated Show resolved Hide resolved
test/e2e/lib/gutenberg/gutenberg-editor-component.js Outdated Show resolved Hide resolved
test/e2e/lib/gutenberg/gutenberg-editor-component.js Outdated Show resolved Hide resolved
@david-szabo97 david-szabo97 force-pushed the add/tracking-pattern-insertion-pattern-category-name branch from 51fd119 to 35c6101 Compare June 29, 2021 17:27
Copy link
Member

@WunderBart WunderBart left a comment

Choose a reason for hiding this comment

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

Looks good from the E2E perspective, thanks! 🚀

@david-szabo97 david-szabo97 merged commit c0b4b91 into trunk Jun 30, 2021
@david-szabo97 david-szabo97 deleted the add/tracking-pattern-insertion-pattern-category-name branch June 30, 2021 15:25
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants