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

[db] Subscribe existing users to DevXMail #5442

Merged
merged 1 commit into from
Sep 2, 2021

Conversation

laushinka
Copy link
Contributor

@laushinka laushinka commented Aug 30, 2021

Users who were subscribed to Changelog will also be subscribed
to DevX.

Fixes #5315

@JanKoehnlein
Copy link
Contributor

/uncc @atduarte
/cc @jakobhero
/cc @AlexTugarev

Do we know how long such a migration would run? Would it make sense to merge that with #5439 ?

@roboquat roboquat requested review from AlexTugarev and jakobhero and removed request for atduarte August 31, 2021 07:00
@JanKoehnlein
Copy link
Contributor

/hold

Let's not jeopardize today's deployment. If reviewers are sure it is harmless, unhold.

@atduarte
Copy link
Contributor

atduarte commented Aug 31, 2021

According to the following query, 116 users unsubscribed from the DevX emails making it impractical to manually unsubscribe them:

SELECT user_id, 
  ARRAY_AGG(unsubscribed_devx ORDER BY uuid_ts DESC)[OFFSET(0)] AS unsubscribed_devx_
FROM `gitpod-growth.production_trusted.notification_change`
WHERE unsubscribed_devx IS NOT NULL
GROUP BY user_id
HAVING unsubscribed_devx_ = true;

Could we change the migration to take this into consideration?

Also, just as a reminder: synchronization with customer.io happens through signup/notification_change/deletion segment events. So this change won't be represented in customer.io until we manually import it.

@laushinka
Copy link
Contributor Author

laushinka commented Aug 31, 2021

According to the following query, 116 users unsubscribed from the DevX emails making it impractical to manually unsubscribe them:

@atduarte This is unfortunate. Did the 116 happen in these two weeks? If yes then it will be impractical to manually change them, and the DB doesn't reflect how the values changed - we can't know whether the unsubscribed_devx was due to a user unchecking the box. @ChristinFrohne what do you say?

@chrifro
Copy link

chrifro commented Aug 31, 2021

We have sent a DevX Digest on August 26. The newsletter had 1,092 total clicks on https://gitpod.io/notifications. So it seems a bit weird that "only" 116 users unsubscribed from the DevX email. But also possible as the link requires logging in and maybe most gave up on the way. Also, I don't know the number of unique clicks, might be that some users clicked several times on the link.

we can't know whether the unsubscribed_devx was due to a user unchecking the box. @ChristinFrohne what do you say?

Yes, that's very likely as we sent a DevX email within the last 14 days. Also what other way would be possible to unsubscribe? @laushinka

@atduarte
Copy link
Contributor

Users that unsubscribed from DevX will have the AllowDevXMail as false in the database. As such, I believe the easiest approach is to change this query to only update users where the AllowDevXMail property is not defined.

@atduarte atduarte self-requested a review August 31, 2021 13:16
@laushinka laushinka force-pushed the laushinka/automatically-enable-5315 branch from 3645e1c to 04b1f84 Compare August 31, 2021 15:06
@laushinka laushinka force-pushed the laushinka/automatically-enable-5315 branch from 04b1f84 to 6e1633e Compare August 31, 2021 16:18
@atduarte atduarte self-requested a review August 31, 2021 19:28
@atduarte
Copy link
Contributor

/approve

@JanKoehnlein
Copy link
Contributor

/unhold

@laushinka
Copy link
Contributor Author

@JanKoehnlein Apologies for the scope creep request, but @ChristinFrohne saw that this onboarding mail isn't active for existing users, therefore it should be activated for all - similar to what this PR is doing for another mail. Would it be okay that I add this change to this PR as well?

@JanKoehnlein
Copy link
Contributor

@laushinka sure. The less DB migrations we have to do the better. And AFAIKS it does neither add any completely different functionality nor raise other concerns.

Users who were subscribed to Changelog will also be subscribed
to Changelog.

Fixes #5315
@laushinka laushinka force-pushed the laushinka/automatically-enable-5315 branch from 6e1633e to cd52ba0 Compare September 1, 2021 10:28
@laushinka laushinka marked this pull request as ready for review September 1, 2021 12:33
@laushinka
Copy link
Contributor Author

Ready for re-review @atduarte @JanKoehnlein

@atduarte
Copy link
Contributor

atduarte commented Sep 1, 2021

/approve

@JanKoehnlein
Copy link
Contributor

/lgtm

@roboquat
Copy link
Contributor

roboquat commented Sep 2, 2021

LGTM label has been added.

Git tree hash: 813f78f4d3314d5084cd4bdc2a3ba66f3cd2bd6f

@roboquat
Copy link
Contributor

roboquat commented Sep 2, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atduarte, JanKoehnlein

Associated issue: #5315

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 56f3952 into main Sep 2, 2021
@roboquat roboquat deleted the laushinka/automatically-enable-5315 branch September 2, 2021 11:15
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.

automatically enable "product tips" notifications for existing users
5 participants