Premium Analytics: swap the ease question for a readiness one in the feedback modal - #52109
Premium Analytics: swap the ease question for a readiness one in the feedback modal#52109chihsuan wants to merge 1 commit into
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Premium Analytics plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 2 files.
|
Nikschavan
left a comment
There was a problem hiding this comment.
Thank you, the changes look good. I left two comments inline: one on the separator in the Happiness message, and one question about keeping the event name.
| ); | ||
| // The endpoint has no readiness field, so the answer rides along in the message. | ||
| submitStatsUserFeedback( { | ||
| comment: `${ readinessSummary( readiness ) }\n\n${ message }`, |
There was a problem hiding this comment.
The \n\n between the summary and the comment does not survive the endpoint: the wpcom handler for jetpack-stats/user-feedback runs feedback through sanitize_text_field, which collapses every run of newlines and spaces to a single space before the email body is built. Happiness reads Message: Ready to replace the old Traffic tab? Not yet Charts load slowly, so the boundary between the answer and the comment is gone, and the it.each at dashboard-options-menu.test.tsx#L285 asserts a separator nothing downstream keeps. Ending the summary with a full stop, or wrapping the answer in brackets, would keep the two readable on one line.
| const message = comment.trim(); | ||
|
|
||
| trackEvent( 'jetpack_premium_analytics_feedback_submit', { rating, comment: message } ); | ||
| trackEvent( 'jetpack_premium_analytics_feedback_submit', { readiness, comment: message } ); |
There was a problem hiding this comment.
The tracking section of the description points at an "Open questions" section for the event-name decision, but there is no such section, so the question is only implied. Keeping jetpack_premium_analytics_feedback_submit means rows carry rating up to this deploy and readiness after it, and a query on rating goes quiet without an error. The event is six days old (#51870), so a rename is the cheapest it will ever be. Is keeping the name the intended choice?
dognose24
left a comment
There was a problem hiding this comment.
Reviewed the diff and ran it locally on the branch: the options-menu, feedback and data/api suites (91 tests), tsc and ESLint all pass, and the branch is level with trunk.
Dropping rating from the Happiness copy is safe: StatsUserFeedback.rating is already optional, the stats-admin proxy validates nothing on that route, and Calypso's own Stats feedback modal sends only product_name and feedback to the same endpoint. The split into ComparisonFields / ReadinessFields with a shared CommentField keeps the modal change small, and the new test pins the switch off dialog to its five point scale. Changelog coverage matches the plugins that bundle the package (mu-wpcom-plugin rightly skipped).
Approving, with two things worth tidying before merge:
-
The description points at a section that isn't there. The data paragraph says "See 'Open questions' below", but the body has no such section. Either add it (the event name decision below is presumably what it was going to hold) or drop the sentence.
-
Keeping the event name while swapping its main property. From the merge date,
jetpack_premium_analytics_feedback_submitsilently stops carryingratingwhilepreview_disablestill does, so anything queryingratingon the feedback event goes quietly empty rather than failing. I'd keep the name (the event still means "the reader sent feedback") but note the cutover date wherever the analysis lives, STATS-461 or the tracking doc, so nobody reads the gap as a drop in submissions.
One optional product nit, not for this PR: after "Yes, I'd be happy to switch now", the follow up "What's missing?" reads oddly. That's what UNI-757 specifies, so it belongs with Gary rather than here, but it might be worth asking whether the open question should follow the answer.
Fixes UNI-757
Proposed changes
jetpack_premium_analytics_feedback_submitnow carriesreadiness(ready|almost|not_yet) instead ofrating.rating.FeedbackFieldssplits intoComparisonFieldsandReadinessFields; the switch off dialog keeps the comparison scale and its numericrating, unchanged.The switch off dialog was deliberately left alone: pressing "Switch it off" already answers the readiness question by action, so asking it again there would be redundant. That dialog shares the comment field but now supplies its own question above it.
Related product discussion/links
Does this pull request change what data or activity we track or use?
Yes, for one existing event.
jetpack_premium_analytics_feedback_submitkeeps its name but swaps its main property:rating(an integer 1 to 5) is replaced byreadiness(one of three fixed strings). No new category of data is collected; the free textcommentproperty is unchanged, and the same answer also reaches the existing Stats feedback endpoint as message text rather than as a numeric field. See "Open questions" below, since keeping the event name is a decision worth a second opinion.Testing instructions
pixel.wp.com/t.gifrequest should carryreadiness=<answer>and norating, and there should be aPOSTto.../jetpack-stats/user-feedback.user-feedbackat all.Before / after
Same site, same session, captured at the moment of the switch.
An answer picked and a comment typed: