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

Laushinka/let people unsubscribe 4761 #5281

Merged
merged 4 commits into from
Aug 24, 2021

Conversation

laushinka
Copy link
Contributor

This PR created initially to run the following command and test that the new events are reflected on Segment.

/werft analytics=segment|TEZnsG4QbLSxLfHfNieLYGF4cDwyFWoe

Fixes #4761

@roboquat roboquat requested a review from geropl August 19, 2021 15:47
@laushinka laushinka removed the request for review from geropl August 19, 2021 15:48
@laushinka laushinka force-pushed the laushinka/let-people-unsubscribe-4761 branch from d4d51e3 to fa1448d Compare August 19, 2021 16:01
@JanKoehnlein
Copy link
Contributor

Product question: Should this be an Enterprise Edition feature?

@laushinka
Copy link
Contributor Author

Product question: Should this be an Enterprise Edition feature?

Who should answer this?

@JanKoehnlein
Copy link
Contributor

| Who should answer this?
Somebody from our product team. I put it in their inbox (Product Groundwork project) so they should answer fast

@laushinka laushinka force-pushed the laushinka/let-people-unsubscribe-4761 branch from c383328 to 0ab0839 Compare August 20, 2021 13:39
Copy link
Contributor

@jakobhero jakobhero left a comment

Choose a reason for hiding this comment

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

Please use identify instead of notification_change track call and - if ok to include in the scope of this PR - add a /subscribe endpoint that subscribes a user to the passed mailing list.

}
}
});
this.analytics.track({
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also change this to an identify call in order to be consistent with what 1) we are doing in the else branch of the conditional and 2) customer.io / other 3rd party software is expecting downstream (see here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

get apiRouter(): express.Router {
const router = express.Router();

router.get("/unsubscribe", async (req: express.Request, res: express.Response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we would like to use this API to subscribe newsletter users so that they are automatically attached to their user accounts if a the email for the newsletter signup matches any user's primary email. is it ok for you to extend the scope of this PR to include a /subscribe endpoint that signs up a known or unknown user for a specified subscription list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to keep the existing scope of this ticket for reasons of: 1) planning and 2) transparency :) When tickets are approved and put to in progress, I assume there are already plans and estimations of how much effort the current ticket/board would take. Changing the scope might change the planning for the rest of the board. Secondly, a changing scope almost at the end of the work might not be known to the stakeholders of the board.
However, I am still learning how the team usually manages their projects so I will tag @JanKoehnlein here for his thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's open another ticket for @jakobhero's request. I feel like it has to be discussed in terms of security and GDPR as well.

@laushinka laushinka force-pushed the laushinka/let-people-unsubscribe-4761 branch from 0ab0839 to 5e06952 Compare August 24, 2021 08:10
@svenefftinge
Copy link
Member

Product question: Should this be an Enterprise Edition feature?

It can stay in the open-source code base. We would only consider code that adds useful enterprise features to be moved to an enterprise-licensed directory, everything else goes open-source.

@svenefftinge svenefftinge removed this from Inbox in Product Groundwork Aug 24, 2021
get apiRouter(): express.Router {
const router = express.Router();

router.get("/unsubscribe", async (req: express.Request, res: express.Response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's open another ticket for @jakobhero's request. I feel like it has to be discussed in terms of security and GDPR as well.

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: fb94e55211835b3e7496627d88980e9742c4542a

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JanKoehnlein

Associated issue: #4761

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 03c996b into main Aug 24, 2021
@roboquat roboquat deleted the laushinka/let-people-unsubscribe-4761 branch August 24, 2021 09:07
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.

Let people unsubscribe from emails without logging in
5 participants