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

MNTOR-2945: add del subscription api and unsub before deleting an user #4303

Merged
merged 13 commits into from
Mar 9, 2024

Conversation

mansaj
Copy link
Collaborator

@mansaj mansaj commented Mar 5, 2024

References:

Jira: MNTOR-2945

Description

Implement the unsubscribe feature correctly as a part of deletion, so the user

  1. won't be billed again when signing up again
  2. won't have any pre-existing records of Plus account

Screenshot (if applicable)

Not applicable.

How to test

In heroku, use a new account to subscribe to Plus. Delete that account. Watch for unsubscribe email to come through from FxA

Checklist (Definition of Done)

  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in readme and/or code comments
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

@mansaj mansaj marked this pull request as ready for review March 6, 2024 01:59
@mansaj mansaj requested a review from Vinnl March 6, 2024 02:32
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Unfortunately it appears like I'm getting an error when trying it locally - shouldn't the request to FxA be authenticated somehow?

And I think we also don't want to try unsubscribing if we just got a notification from FxA that the user has been unsubscribed?

(And a small question: won't this a bit too noisy for our logs?)

src/utils/fxa.js Outdated Show resolved Hide resolved
src/utils/fxa.js Outdated Show resolved Hide resolved
src/app/functions/server/deleteAccount.ts Show resolved Hide resolved
src/utils/fxa.js Fixed Show fixed Hide fixed
@mansaj mansaj requested review from pdehaan and Vinnl March 7, 2024 06:40
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Thanks, the code makes sense to me now. But when I try it out locally, I'm getting

deleteSubscriber error: delete from "subscribers" where "fxa_uid" = $1 returning "id" - update or delete on table "subscribers" violates foreign key constraint "onerep_scans_onerep_profile_id_foreign" on table "onerep_scans"

I wonder if we're missing a cascade somewhere to also delete scans and scan results when we delete a subscriber?

src/utils/fxa.js Outdated Show resolved Hide resolved
src/app/functions/server/deleteAccount.ts Show resolved Hide resolved
@mansaj
Copy link
Collaborator Author

mansaj commented Mar 7, 2024

Thanks, the code makes sense to me now. But when I try it out locally, I'm getting

deleteSubscriber error: delete from "subscribers" where "fxa_uid" = $1 returning "id" - update or delete on table "subscribers" violates foreign key constraint "onerep_scans_onerep_profile_id_foreign" on table "onerep_scans"

I wonder if we're missing a cascade somewhere to also delete scans and scan results when we delete a subscriber?

Hmm interesting.. Is the db migration up to date? npm run db:migrate?

*/
// Not covered by tests; mostly side-effects. See test-coverage.md#mock-heavy
/* c8 ignore start */
async function getSubscriptions(bearerToken) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Vinnl I also refactored the code a little bit so the GET call is now separate and reusable.. in case one day we do want to address the unsubscribe vs. resubscribe CTA in settings

@mansaj mansaj requested review from Vinnl and rhelmer March 7, 2024 17:25
Copy link
Collaborator

@rhelmer rhelmer left a comment

Choose a reason for hiding this comment

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

I don't understand how we're sure the subscription ID is for the monitor subscription. Have you tested this with an account that is subscribed to multiple services?

We should really convert this to typescript and use the newer formatting rules, is that already in our tech debt epic? That part isn't a blocker for this PR, but not being able to use structured logging here is annoying.

src/utils/fxa.js Show resolved Hide resolved
src/utils/fxa.js Outdated Show resolved Hide resolved
@mansaj
Copy link
Collaborator Author

mansaj commented Mar 9, 2024

@rhelmer Thanks for catching that, verified the fix in heroku just now. The unsub works

I don't understand how we're sure the subscription ID is for the monitor subscription. Have you tested this with an account that is subscribed to multiple services?

We should really convert this to typescript and use the newer formatting rules, is that already in our tech debt epic? That part isn't a blocker for this PR, but not being able to use structured logging here is annoying.

@mansaj mansaj merged commit 2590fdf into main Mar 9, 2024
15 checks passed
@mansaj mansaj deleted the MNTOR-2945 branch March 9, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants