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: Track convert to template part and detach blocks from template part actions #53592

Merged
merged 14 commits into from
Jul 8, 2021

Conversation

david-szabo97
Copy link
Contributor

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

Changes proposed in this Pull Request

  • Track "Template Part" Block -> Toolbar more menu -> "Detach blocks from template part" action
  • Track any block -> Toolbar more menu -> "Make template part" action
  • Make sure replaceBlocks isn't tracked after "Make template part"

NOTE: The #53598 PR is a prerequisite for this PR.

We are firing both events twice. One with block_names property and one without it. We do this to ensure the event is tracked all the time. The event fired with block_names property will fail if there are way too many blocks involved. URL length of the request becomes too long and the browser fails to deliver it.

Screen Shot 2021-07-09 at 12 16 04 PM

Testing instructions

  1. Follow steps to enable tracking debugging at PCYsg-nrf-p2
  2. Open Site Editor
  3. Select a Paragraph block
  4. Click on 3 dots (Options) in the toolbar menu
  5. Click on "Make Template Part"
  6. Confirm wpcom_block_editor_convert_to_template_part event is fired twice. One without block_names and one with block_names property.
  7. Confirm replaceBlocks is triggered, but no events are tracked
  8. Select the template part you just created
  9. Click on 3 dots (Options) in the toolbar menu
  10. Click on "Detach blocks from template part"
  11. Confim wpcom_block_editor_template_part_detach_blocks event is fired twice. One without block_names and one with block_names property.
  12. Confirm replaceBlocks is triggered, but no events are tracked (subscriber system utilized here)

Related to #53410

@david-szabo97 david-szabo97 self-assigned this Jun 10, 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.

@david-szabo97 david-szabo97 force-pushed the add/track-template-part-convert-detach-blocks branch from 9f50a6d to 2bcf92f Compare June 10, 2021 17:22
@david-szabo97 david-szabo97 changed the base branch from trunk to add/tracking-delegate-event-tracking-subscribers June 10, 2021 17:23
@matticbot
Copy link
Contributor

matticbot commented Jun 10, 2021

@david-szabo97 david-szabo97 marked this pull request as ready for review June 10, 2021 17:35
@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 10, 2021
@david-szabo97 david-szabo97 force-pushed the add/track-template-part-convert-detach-blocks branch from bfcb32f to b8b5d4a Compare June 10, 2021 18:49
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 working great in testing.

I wonder if we should add any extra properties to this event. 🤔

  • variation_slug probably makes sense to determine if they are creating/detaching a header, footer, or other in this way.
  • Im not sure if any information about the blocks being converted or detached might make sense here. Number of blocks? a flattened list of block names? Im not certain but maybe @kurt213 and/or @ianstewart have ideas here?


registerDelegateEventSubscriber(
'wpcom-block-editor-template-part-detach-blocks',
'before',
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting it seems that for this case it doesn't really matter if this is 'before' or 'after', although it seems good to have this flexibility in general! 🎉 Its great that we can add extra handlers when we want, and of course the big thing here is being able to track on capture as well!

@david-szabo97 david-szabo97 force-pushed the add/track-template-part-convert-detach-blocks branch from b8b5d4a to ec5c8b2 Compare June 14, 2021 08:45
@ianstewart
Copy link
Contributor

a flattened list of block names?

I could see this being useful in the future for design purposes if it could be pulled with a data request. Could be used to design better patterns.

@david-szabo97 david-szabo97 requested a review from Copons June 16, 2021 11:27
@david-szabo97 david-szabo97 force-pushed the add/tracking-delegate-event-tracking-subscribers branch from 6d9ca0a to 6165158 Compare June 16, 2021 12:21
Base automatically changed from add/tracking-delegate-event-tracking-subscribers to trunk June 16, 2021 13:57
@kurt213
Copy link

kurt213 commented Jun 18, 2021

variation_slug

👍 . I'm not too familiar with all the possible variations for this, but this level of detail would be helpful.

a flattened list of block names?

👍 This would be helpful. It wouldn't be something that can easily be analysed in something like Tracks, but as @ianstewart mentioned, a data request, custom SQL query or a transformation pipeline into Looker could make use of this level of granularity.

@Addison-Stavlo
Copy link
Contributor

I'm not too familiar with all the possible variations for this, but this level of detail would be helpful.

Basically just "header", "footer", or "general" at this point in time.

@david-szabo97 david-szabo97 force-pushed the add/track-template-part-convert-detach-blocks branch from ec5c8b2 to 340b641 Compare June 18, 2021 18:07
@david-szabo97
Copy link
Contributor Author

a flattened list of block names?

👍 This would be helpful. It wouldn't be something that can easily be analysed in something like Tracks, but as @ianstewart mentioned, a data request, custom SQL query or a transformation pipeline into Looker could make use of this level of granularity.

@kurt213 AFAIK, arrays aren't supported by Tracks. Should join the list of block names using comma (comma separated list) or turn it into a JSON?

@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/track-template-part-convert-detach-blocks on your sandbox.

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

@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/track-template-part-convert-detach-blocks on your sandbox.

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

@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/track-template-part-convert-detach-blocks on your sandbox.

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

@david-szabo97 david-szabo97 force-pushed the add/track-template-part-convert-detach-blocks branch from 30364fc to 0123066 Compare July 5, 2021 14:47
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 working well! I think we will needs some tests here or in a follow up?

Comment on lines 420 to 429
// We fire the event with and without the block names. We do this to
// make sure the event is tracked all the time. The block names
// might become a string that's too long and as a result it will
// fail because of URL length browser limitations.
tracksRecordEvent( 'wpcom_block_editor_convert_to_template_part', {
variation_slug: record.area,
} );
tracksRecordEvent( 'wpcom_block_editor_convert_to_template_part', {
variation_slug: record.area,
block_names: getFlattenedBlockNames( convertedParentBlocks ).join( ',' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not in love with the idea of sending two events... but given the slack discussions that seems like it makes sense for now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... sending two events is a bit gross. But I'm not sure how we could limit the length of the block_names property. Since the URL length will vary based on other properties too.

@david-szabo97 david-szabo97 requested review from scinos, WunderBart and a team as code owners July 6, 2021 16:30
@david-szabo97 david-szabo97 force-pushed the add/track-template-part-convert-detach-blocks branch from 21e71a2 to 79c9033 Compare July 7, 2021 07:35
@david-szabo97 david-szabo97 force-pushed the add/track-template-part-convert-detach-blocks branch from f05363f to 1b869c6 Compare July 8, 2021 10:30
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.

Functionality and tests work great!

@david-szabo97 david-szabo97 merged commit 8fc73cf into trunk Jul 8, 2021
@david-szabo97 david-szabo97 deleted the add/track-template-part-convert-detach-blocks branch July 8, 2021 13:29
@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 Jul 8, 2021
@a8ci18n
Copy link

a8ci18n commented Jul 8, 2021

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/6157542

Thank you @david-szabo97 for including a screenshot in the description! This is really helpful for our translators.

Comment on lines +655 to +658
() => {
ignoreNextReplaceBlocksAction = true;
}
);
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Jul 9, 2021

Choose a reason for hiding this comment

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

It seems this is bleeding into other events that may be triggered by the same selector, as we did not check the conditions here as we did for firing the event.

#54415 should fix

@a8ci18n
Copy link

a8ci18n commented Jul 15, 2021

Translation for this Pull Request has now been finished.

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.

6 participants