Add exit survey #3729

yscik · 2020-10-21T13:04:24Z

Fixes #2374

Changes proposed in this Pull Request

When clicking Deactivate on the plugins screen, open up a modal with an exit survey form
Add AJAX action to save the submitted feedback and send track event
Send the event anonymously, with a hashed site ID, when usage tracking is disabled

Testing instructions

Go to the Plugins screen, and click Deactivate on the Sensei LMS plugin
A modal should open up
Clicking Skip Feedback should continue with deactivating plugin
--
Do it again, select a reason, type in details and submit
A sensei_plugin_deactivate track event should be sent, with the selected reason and details

Screenshot / Video

renatho

Looks good and works well!

Just when we're submitting the survey, if the usage tracking is disabled, it doesn't send. Maybe should we send it anyway? Or maybe don't show the modal in this case?

assets/admin/exit-survey/form-item.js

renatho · 2020-10-21T14:48:54Z

assets/admin/exit-survey/form.js

+						'sensei-lms'
+					) }
+				</p>
+				{ reasons.map( ExitSurveyFormItem ) }


Interesting! I have never seen this approach to map directly in the component instead of the instance. I didn't know it worked!
But while debugging, I noticed that React dev tools don't recognize the mapped components. Do you know a way to fix that? Or maybe we should map it using the component instance?

Yeah, this is just a function, so it works. But updated it to component instance to make it clearer.

I think with this change we can also remove the key={ label } from the ExitSurveyFormItem render, right?

Yup, updated in 180ed8b

assets/admin/exit-survey/form.js

renatho · 2020-10-21T14:59:51Z

Hum, I think we also need to update some e2e tests that are failing. ;)

renatho · 2020-10-21T15:57:08Z

Ah, sorry! Another thing I remembered only now. I think we should use the release/3.5.3 branch as source and target for this feature because our master is with the code to the 3.6.0.

Co-authored-by: Renatho De Carli Rosa <[email protected]>

yscik · 2020-10-21T16:35:23Z

Rebased to release/3.5.3 and updated the E2E tests this affects.

yscik · 2020-10-21T16:39:53Z

if the usage tracking is disabled, it doesn't send. Maybe should we send it anyway? Or maybe don't show the modal in this case?

Right, sending it anyway was the plan I think. That also means though that we are sending along all the base fields for the site. What should we do here @donnapep?

Send the full event when the user submits this feedback
Send a partial event with only these fields (eg. no site url, admin email)
Do not show the exit survey

donnapep · 2020-10-21T17:34:53Z

Send a partial event with only these fields (eg. no site url, admin email)

This would be the safest thing to do from a privacy perspective. We really shouldn't submit any data that they haven't opted in to sending.

renatho

I tested the d44d4fa and it worked well! Just adding some comments about that.

renatho · 2020-10-22T17:26:35Z

includes/admin/class-sensei-exit-survey.php

+			'details' => isset( $_POST['details'] ) ? sanitize_text_field( wp_unslash( $_POST['details'] ) ) : null,
+		];
+
+		update_option( 'sensei_exit_survey_data', $feedback );


Now I noticed that. More for documentation purposes, what's the idea of this option?

If this is a new option, we should also add it to class-sensei-data-cleaner.php.

I recall talking about storing this, but we can get rid of it if we don't want to. Not used for anything right now.

renatho · 2020-10-22T17:41:32Z

includes/lib/usage-tracking/class-usage-tracking-base.php

+	 * @param null|int $event_timestamp When the event occurred.
+	 *
+	 * @return bool
+	 **/


I see that the other functions in this file close the PHPDoc with **/, but considering it's a new function, maybe we could add it correctly */. WDYT?
Or we could also take the opportunity to replace all of them in this file! =)

Cleaned them up in a54b8b9

renatho · 2020-10-22T17:46:51Z

includes/lib/usage-tracking/class-usage-tracking-base.php

+	 *
+	 * @return bool
+	 **/
+	public function send_anonymous_event( $event, $properties = array(), $event_timestamp = null ) {


For the normal events, sounds a little weird to me going through the send_anonymous_event. Maybe we could call it as create_pixel, or something like that, and also have send_anonymous_event to be called in the save_exit_survey. The send_anonymous_event would also call the create_pixel (and it wouldn't do anything else for now). Does it make sense to you?

Changed this up in bda3c37, as the anonymous method has it's own logic now.

donnapep · 2020-10-23T12:33:08Z

Could the background be made a bit darker when the modal is displayed? I think it would help the modal stand out a bit more.

This event is being rejected by Tracks:

Looks like we are going to need to send userid and useridtype at a minimum. If usage tracking is enabled though, we may as well send everything as per usual. That will be especially important for us to be able to determine if they are also using WCPC.

Also, @gkaragia will want this new event to be registered in Tracks. 🙂

yscik · 2020-10-27T11:51:41Z

Also had to update the event name to match the <source>_<context>_<action> pattern required by tracks. Changed it to sensei_plugin_deactivate.

When usage tracking is enabled, we send the full event. When it's not, we send the hash of the site url as the user ID. This way it's always the same for the same site, but still anonymous.

donnapep · 2020-10-27T12:27:27Z

Event is being logged now. 🙌

With usage tracking enabled, details and reason are logged, but I'm not seeing data like admin_email, courses, paid etc.

yscik · 2020-10-27T12:31:49Z

Sorry about that, must have removed that logic at some point — f587121 now sends the full event if tracking is enabled.

donnapep · 2020-10-27T13:26:21Z

Something is still a wee bit off. admin_email is there now, but courses, learners, paid and source are not.

donnapep

Approving based on Renatho's previous code review and the fact that logging is working well. 👍

yscik added this to the 3.5.3 milestone Oct 21, 2020

yscik requested a review from a team October 21, 2020 13:04

renatho reviewed Oct 21, 2020

View reviewed changes

yscik added 6 commits October 21, 2020 18:14

Add exit survey class with ajax handler

bd64e06

Add exit survey frontend JS and form component

2dcc4af

Add jest-dom for easier test queries

51b71bf

Add unit test to form component

732ea53

Fix plugin deactivation in E2E test

44c7571

Create component instance for ExitSurveyFormItem

570815f

yscik force-pushed the add/exit-survey branch from c487c76 to 570815f Compare October 21, 2020 16:29

yscik changed the base branch from master to release/3.5.3 October 21, 2020 16:29

Fix spacing

5ef5ca5

Co-authored-by: Renatho De Carli Rosa <[email protected]>

Add testing helper not on this branch yet

84755fb

yscik added 2 commits October 21, 2020 21:43

Fix E2E tests?

ed9d4bd

Separate tracking event sending with and without user info

d44d4fa

renatho reviewed Oct 22, 2020

View reviewed changes

yscik added 4 commits October 27, 2020 12:36

Add user id and userid type properties to anonymous event

bda3c37

Clean up comments

a54b8b9

Add option to data cleaner

3caa822

Update modal background opacity

b8a726c

yscik added 2 commits October 27, 2020 12:53

Change event name to fit tracks format

79bac1e

Remove unneeded key attribute

180ed8b

Send full event if tracking is enabled

f587121

Use log function that adds base fields

1e0c34b

donnapep approved these changes Oct 28, 2020

View reviewed changes

yscik merged commit e9affe2 into release/3.5.3 Oct 28, 2020

yscik deleted the add/exit-survey branch October 28, 2020 12:28

yscik linked an issue Oct 28, 2020 that may be closed by this pull request

Add an exit survey when the plugin is deactivated #2374

Closed

donnapep changed the title ~~Add Exit Survey~~ Add exit survey Oct 28, 2020

alexsanford mentioned this pull request Oct 29, 2020

Release 3.5.3 #3742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exit survey #3729

Add exit survey #3729

yscik commented Oct 21, 2020 •

edited

Loading

renatho left a comment

renatho Oct 21, 2020

yscik Oct 21, 2020

renatho Oct 21, 2020

yscik Oct 27, 2020

renatho commented Oct 21, 2020

renatho commented Oct 21, 2020

yscik commented Oct 21, 2020

yscik commented Oct 21, 2020

donnapep commented Oct 21, 2020

renatho left a comment

renatho Oct 22, 2020

donnapep Oct 23, 2020

yscik Oct 27, 2020

renatho Oct 22, 2020

yscik Oct 27, 2020

renatho Oct 22, 2020

yscik Oct 27, 2020

donnapep commented Oct 23, 2020 •

edited

Loading

yscik commented Oct 27, 2020

donnapep commented Oct 27, 2020

yscik commented Oct 27, 2020

donnapep commented Oct 27, 2020

donnapep left a comment

Add exit survey #3729

Add exit survey #3729

Conversation

yscik commented Oct 21, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

Screenshot / Video

renatho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho commented Oct 21, 2020

renatho commented Oct 21, 2020

yscik commented Oct 21, 2020

yscik commented Oct 21, 2020

donnapep commented Oct 21, 2020

renatho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnapep commented Oct 23, 2020 • edited Loading

yscik commented Oct 27, 2020

donnapep commented Oct 27, 2020

yscik commented Oct 27, 2020

donnapep commented Oct 27, 2020

donnapep left a comment

Choose a reason for hiding this comment

yscik commented Oct 21, 2020 •

edited

Loading

donnapep commented Oct 23, 2020 •

edited

Loading