Skip to content

[Editor] Make sure editor session starts (with content_type=new) matches post_created event#10097

Merged
daniloercoli merged 2 commits intodevelopfrom
issue/10061-fix-editor-session-starts-match-post_created
Jun 25, 2019
Merged

[Editor] Make sure editor session starts (with content_type=new) matches post_created event#10097
daniloercoli merged 2 commits intodevelopfrom
issue/10061-fix-editor-session-starts-match-post_created

Conversation

@daniloercoli
Copy link
Copy Markdown
Contributor

@daniloercoli daniloercoli commented Jun 24, 2019

This PR fixes #10061 by making sure to re-use the same logic to track a post as new in both session_starts and post_created events.

Note: The logic is pretty simple, there is a boolean flag passed to the activity, that is true on new posts only. Previously, the session_starts event was checking if the post content was empty or not, and it failed on draft posts with empty content.

To test:

To test #2

  • Open a draft post in the app
  • Make sure trackEditorCreatedPost is not bumped
  • Make sure createPostEditorAnalyticsSessionTracker is called with isNewPost = false

Update release notes:

  • [ x ] If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

…es `post_created` event by re-using the same logic to track a post as new
@daniloercoli daniloercoli added this to the 12.8 milestone Jun 24, 2019
@daniloercoli daniloercoli requested review from mkevins and mzorz June 24, 2019 15:12
@daniloercoli daniloercoli changed the title Make sure editor session starts (with content_type=new) event match… Make sure editor session starts (with content_type=new) matches post_created event Jun 24, 2019
@daniloercoli daniloercoli changed the title Make sure editor session starts (with content_type=new) matches post_created event [Editor] Make sure editor session starts (with content_type=new) matches post_created event Jun 24, 2019
@daniloercoli daniloercoli requested review from etoledom and wpmobilebot and removed request for wpmobilebot June 24, 2019 15:46
@mkevins
Copy link
Copy Markdown
Contributor

mkevins commented Jun 25, 2019

I'm attempting to reproduce on WordPress-Android develop (47f36b2) and I'm unable to save an empty draft to test the behavior in the second test. Also, when I leave the editor, the empty post is not shown in drafts (I guess it never saves when exiting the editor, if the post is empty). Are there some special steps to save an empty post?

@daniloercoli
Copy link
Copy Markdown
Contributor Author

@mkevins Nope, you can't save a completely empty post|page. In order to test no.2 you need to insert a title, and keep the content blank. You can either do that on the web, or in the mobile app.

@mkevins
Copy link
Copy Markdown
Contributor

mkevins commented Jun 25, 2019

@daniloercoli not sure why, but when I test case 2 on develop, without these changes, I observe that mIsNewPost is false when I open the draft (which has a title, but no body content).

Edit:

I tried this from a post only drafted on web, as well as a post drafted on mobile. In both cases, I see mIsNewPost as false, and trackEditorCreatedPost does not get called.

@daniloercoli
Copy link
Copy Markdown
Contributor Author

@mkevins That's the default/correct behaviour.
When you open a draft (even if it does't contain any content in the "body") mIsNewPost should be false since it's not a new post.

Previously to this PR the session tracking logic was just checking the post content, instead it should rely on the mIsNewPost variable, and be in par with the other tracking logic.

Copy link
Copy Markdown
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

Previously to this PR the session tracking logic was just checking the post content, instead it should rely on the mIsNewPost variable, and be in par with the other tracking logic.

Ah, this logic is working correctly with this PR. I tested this on with a Pixel 3a (real device). In the testing steps, I first tested the behavior on develop, and was expecting both case 1 and case 2 to "fail", but observed that for case 2, the member variable mIsNewPost was already false when reaching the condition that skips the call to trackEditorCreatedPost. Specifically, the bullet point:

Make sure trackEditorCreatedPost is not bumped

Was behaving correctly even prior to these changes. I had misinterpreted the testing steps to expect differing behavior for that specific point on develop, so thanks for clarifying.

Indeed the salient bullet point:

Make sure createPostEditorAnalyticsSessionTracker is called with isNewPost = false

Is working correctly on this PR (and not working correctly on develop). This looks good, and fixes the issue as described. LGTM! 🎉

@daniloercoli daniloercoli merged commit fff2c46 into develop Jun 25, 2019
@daniloercoli daniloercoli deleted the issue/10061-fix-editor-session-starts-match-post_created branch June 25, 2019 12:31
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.

[Analytics] Session starts with content_type=new should match post_created event

3 participants