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

Fix editor_session_start event custom property key #17130

Merged

Conversation

dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented Sep 7, 2021

Prior to this change, wpios_editor_session_start events were not sent to Tracks, as they were marked as invalid with the following error. This relates to changes made in #16848 and #16832.

Custom properties dictionary keys must contain alpha characters and underscores only.

To test:

  1. Install 18.2 beta build.
  2. Launch post editor.
  3. ⚠️ Verify wpios_editor_session_start event does not arrive to Tracks.
  4. Install build from this PR.
  5. Launch post editor.
  6. ✅ Verify wpios_editor_session_start event arrives to Tracks with expected properties.

Regression Notes

  1. Potential unintended areas of impact
    Custom properties for the event are missing.
  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Verified custom properties arrived with the event in Tracks.
  3. What automated tests I added (or what prevented me from doing so)
    None. We would need to either write an integration test to verify the session event property keys are "valid" with Automattic-Tracks-iOS or iterate over them manually asserting their validity.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Prior to this change, `wpios_editor_session_start` events were not sent
to Tracks, as they were marked as invalid with the following error.

```
Custom properties dictionary keys must contain alpha characters and underscores only.
```
@dcalhoun dcalhoun added this to the 18.2 ❄️ milestone Sep 7, 2021
@dcalhoun dcalhoun self-assigned this Sep 7, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 7, 2021

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 56210. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@dcalhoun dcalhoun requested a review from chipsnyder September 7, 2021 18:55
@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for fixing this @dcalhoun

@chipsnyder
Copy link
Contributor

cc @antonis

@dcalhoun
Copy link
Member Author

dcalhoun commented Sep 8, 2021

Hey @mokagio. 👋🏻 This is my first time targeting a beta release. Is it alright to merge this regression fix into release/18.2 now? Are there any other steps I need to complete? Thanks! 🙇🏻

@mokagio
Copy link
Contributor

mokagio commented Sep 9, 2021

Hey @dcalhoun!

This is my first time targeting a beta release

achievement-unlocked

😄 👏

Sorry about missing this ping yesterday. I did see the PR in the list, but just assumed it was waiting to be merged by the author 😞

Anyways... Yes the only thing you need to do is merge it. The release train conductor (currently me) will take it from there and generate a new beta build.

We don't automatically generate new betas when a merge on the branch happens because there are times when multiple PRs get merged in a single day and we don't want to overload the comms channel with consecutive betas.

@mokagio
Copy link
Contributor

mokagio commented Sep 9, 2021

I'm going to merge this myself now to make up for letting it sit here for so long 😳

@mokagio mokagio merged commit 0c3331e into release/18.2 Sep 9, 2021
@mokagio mokagio deleted the bug/fix-editor-session-start-event-properties branch September 9, 2021 01:22
@mokagio
Copy link
Contributor

mokagio commented Sep 9, 2021

@dcalhoun this has been bundled as part of 18.2 beta 1 (18.2.0.1).

Thanks for your work 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants