[dashboard/notifications] improve email preference handling #5142
Conversation
roboquat
requested review from
AlexTugarev,
csweichel,
jankeromnes and
JanKoehnlein
laushinka
removed request for
AlexTugarev,
csweichel,
JanKoehnlein and
jankeromnes
|
/werft run 👍 started the job as gitpod-build-laushinka-update-notification-4793.16 |
laushinka
force-pushed
the
laushinka/update-notification-4793
branch
from
27ba2cc
to
9c63d71
Compare
|
/hold |
|
/cc |
|
/werft run 👍 started the job as gitpod-build-laushinka-update-notification-4793.22 |
|
@JanKoehnlein The DB queries have been tested, also with extra eyes from @jakobhero. This PR is now ready for review. |
|
/werft run 👍 started the job as gitpod-build-laushinka-update-notification-4793.25 |
laushinka
force-pushed
the
laushinka/update-notification-4793
branch
from
fef01cc
to
2fb5a61
Compare
|
/werft run 👍 started the job as gitpod-build-laushinka-update-notification-4793.27 |
|
Looks like the initial values on the page are not automatically refreshed:
|
| }); | ||
| await getGitpodService().server.trackEvent({ | ||
| event: "notification_change", | ||
| properties: { "unsubscribed_changelog": isChangelogMail } |
There was a problem hiding this comment.
Nit: I was at first glance confused why you're not using the !isChangelogMail here. Maybe it'd be better readable if we create a variable newIsChangelogMail = !isChangelogMail at the beginning of this block and use that instead of !isChangeLogMail in the following and write !newIsChangelogMail here. WDYT?
There was a problem hiding this comment.
Yes this part did get confusing to me too, because the event name is a negative word “unsubscribe”. But your suggestion makes it a bit more readable. Will do this change.
| }); | ||
| await getGitpodService().server.trackEvent({ | ||
| event: "notification_change", | ||
| properties: { "unsubscribed_devx": isDevXMail } |
Fixed by setting the user context with the new value. |
|
/werft run 👍 started the job as gitpod-build-laushinka-update-notification-4793.29 |
|
/werft run 👍 started the job as gitpod-build-laushinka-update-notification-4793.30 |
|
/lgtm |
|
LGTM label has been added. Git tree hash: c021eea2f62fe8b11af4fa01308c95cdcc094239
|
|
/approve no-issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JanKoehnlein Associated issue requirement bypassed by: JanKoehnlein 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 |
|
|
||
| public async up(queryRunner: QueryRunner): Promise<any> { | ||
| if (await columnExists(queryRunner, "d_b_user", "allowsMarketingCommunication")) { | ||
| await queryRunner.query("UPDATE d_b_user set additionalData = JSON_MERGE_PATCH(IFNULL(additionalData, '{}'), JSON_SET('{\"emailNotificationSettings\":{\"allowsChangelogMail\":true}}', '$.emailNotificationSettings.allowsChangelogMail', IF(allowsMarketingCommunication, 'true', 'false')))"); |
There was a problem hiding this comment.
@laushinka and @JanKoehnlein, please have a look at the db on gitpod-staging.com after this migration did run.
There was a problem hiding this comment.
Yes, will do this.
There was a problem hiding this comment.
I've checked and it works as expected for both existing and new users.
Creating this pull request firstly in order to run the following command and test that the new events are reflected on Segment.
/werft analytics=segment|TEZnsG4QbLSxLfHfNieLYGF4cDwyFWoe