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

DRAFT: Turn off push notifications on logout #2644

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danieldaquino
Copy link
Contributor

DRAFT!

This is just a draft for whoever works on #1707, I am not actively working on it, and this has not been tested AT ALL.

TODO:

  • Handle edge cases (what if revoking the token fails? What if the user did not have push notifications to start? etc)
  • Improve error handling (If things fails, what do we show the user?)
  • What if the user already logged out before this change and their npub is still registered on our servers? How can we detect that and revoke?

Summary

[Please provide a summary of the changes in this PR.]

Checklist

  • I have read (or I am familiar with) the Contribution Guidelines
  • I have tested the changes in this PR
  • My PR is either small, or I have split it into smaller logical commits that are easier to review
  • I have added the signoff line to all my commits. See Signing off your work
  • I have added appropriate changelog entries for the changes in this PR. See Adding changelog entries
    • I do not need to add a changelog entry. Reason: [Please provide a reason]
  • I have added appropriate Closes: or Fixes: tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patches

Test report

Please provide a test report for the changes in this PR. You can use the template below, but feel free to modify it as needed.

Device: [Please specify the device you used for testing]

iOS: [Please specify the iOS version you used for testing]

Damus: [Please specify the Damus version or commit hash you used for testing]

Setup: [Please provide a brief description of the setup you used for testing, if applicable]

Steps: [Please provide a list of steps you took to test the changes in this PR]

Results:

  • PASS
  • Partial PASS
    • Details: [Please provide details of the partial pass]

Other notes

[Please provide any other information that you think is relevant to this PR.]

Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
print("txn: damus close")
try await self.push_notification_client.revoke_token()
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggest: fire and forget instead of delaying close

Copy link
Collaborator

@jb55 jb55 left a comment

Choose a reason for hiding this comment

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

one suggestion

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.

2 participants