-
Notifications
You must be signed in to change notification settings - Fork 25
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
Delete subscription upon disconnecting user #42
Delete subscription upon disconnecting user #42
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatthewDorner Great job on the implementation! Just one suggestion on logging an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @MatthewDorner!
/update-branch |
@DHaussermann I'll defer to you whether you want to test this before master |
This issue has been automatically labelled "stale" because it hasn't had recent activity. /cc @jasonblais @hanzei |
@DHaussermann Gentle reminder to review this PR 😉 |
@MatthewDorner Heads up that there is a merge conflict to resolve |
Oh, I didn't notice the merge conflict. Will fix. |
I'm guessing tests are failing until #63 gets merged in. |
That is correct @MatthewDorner. Sorry for the inconvenience. |
@MatthewDorner I'm not seeing this work as expected. I wonder if the way I'm trying to validate this is correct?
Is this maybe dependant on another change in master? Thoughts? |
What I'm seeing is that when you run I'm not sure what the value in row 8 is supposed to be. |
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
==========================================
- Coverage 26.77% 26.56% -0.22%
==========================================
Files 57 57
Lines 2009 2025 +16
==========================================
Hits 538 538
- Misses 1416 1432 +16
Partials 55 55
Continue to review full report at Codecov.
|
@MatthewDorner Build is green now |
I think the record that's not getting deleted is the OAuth state being saved here:
Maybe it shouldn't be saved with the |
Thanks @MatthewDorner you're correct. The record I was seeing was caused by the oAuth connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and passed.
Confirmed that subscription record is in fact removed from KV store when user is disconnected.
LGTM! Thanks @MatthewDorner for the PR!
Summary
Add code in
DisconnectUser
to delete the user's event subscription from the KV store and also from the MS Graph API. Let me know if I should change the error handling or messages. I tried to imitate the error handling frommscalendar/subscription.go
where subscriptions are deleted, and I decided not to return error if the MS Graph API deletion fails since the subscription may have expired.Ticket Link
Fixes #34