Skip to content
This repository has been archived by the owner on Jan 4, 2024. It is now read-only.

fix(lambda): make the FxA lambda not throw on not found errors #796

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

bassrock
Copy link
Member

Goal

There are a bunch of users that deleted their Pocket account, but retained their FxA account, however we still receive FxA webhook callbacks for the deleted users.

In this event user-api will return a NotFound error and we should mark the SQS message as successful, before we were marking this as a failure.

This should stop the errors like https://pocket.sentry.io/issues/4224409867/?project=6055107&query=is%3Aunresolved&referrer=issue-stream&stream_index=0

I'd love feedback/perspectives on:

  • Is this the right way to check for errors?

Implementation Decisions

Deployment steps

  • Database migrations?
  • Secrets?
  • FOR POCKET DEVELOPERS ONLY - Post in #changelog:

Write a #changelog message using our best practices if this PR (potentially) impacts users. Describe the 'why' and 'what'. @mention relevant teams. Link to this PR.

References

JIRA ticket:

  • Link to JIRA ticket

Issue:

  • Link to GitHub issue

Documentation:

  • Project doc

@bassrock bassrock requested a review from a team as a code owner July 17, 2023 22:59
@bassrock bassrock requested review from efixler and Herraj July 17, 2023 22:59
@@ -161,6 +161,22 @@ describe('SQS Event Handler', () => {
expect(scope.isDone()).toBeTruthy();
});

it('returns success if a NotFoundError is returned from client-api for profile update event', async () => {
Copy link

Choose a reason for hiding this comment

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

Nice tests!

I understand the implementation but don't we need to tell FxA to stop pinging for this user somehow? Or do we just expect to get these forever, if there was ever a pocket-FxA user?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going with the latter here, but I will raise it with the FxA team today.

Copy link

@efixler efixler left a comment

Choose a reason for hiding this comment

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

lgtm but question about the implementation strategy.

@bassrock bassrock merged commit 6615472 into main Jul 18, 2023
2 checks passed
@bassrock bassrock deleted the cleanup-fxa-webhook branch July 18, 2023 13:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants