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

Delete subscription upon disconnecting user #42

Merged
merged 10 commits into from
Mar 31, 2020

Conversation

MatthewDorner
Copy link
Collaborator

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 from mscalendar/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

@hanzei hanzei requested a review from mickmister March 4, 2020 14:39
@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 4, 2020
@mickmister mickmister requested a review from levb March 4, 2020 23:51
Copy link
Contributor

@mickmister mickmister left a 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.

server/mscalendar/user.go Outdated Show resolved Hide resolved
levb
levb previously approved these changes Mar 6, 2020
@hanzei hanzei requested a review from mickmister March 7, 2020 14:54
mickmister
mickmister previously approved these changes Mar 9, 2020
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @MatthewDorner!

@mickmister mickmister removed the 2: Dev Review Requires review by a core committer label Mar 9, 2020
@mickmister mickmister requested a review from DHaussermann March 11, 2020 03:22
@mickmister
Copy link
Contributor

/update-branch

@mickmister
Copy link
Contributor

@DHaussermann I'll defer to you whether you want to test this before master

@mattermod
Copy link
Contributor

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

@hanzei
Copy link
Collaborator

hanzei commented Mar 29, 2020

@DHaussermann Gentle reminder to review this PR 😉

@hanzei
Copy link
Collaborator

hanzei commented Mar 29, 2020

@MatthewDorner Heads up that there is a merge conflict to resolve

@MatthewDorner
Copy link
Collaborator Author

Oh, I didn't notice the merge conflict. Will fix.

@MatthewDorner MatthewDorner dismissed stale reviews from mickmister and levb via 75b88c6 March 29, 2020 14:59
@MatthewDorner
Copy link
Collaborator Author

I'm guessing tests are failing until #63 gets merged in.

@hanzei
Copy link
Collaborator

hanzei commented Mar 29, 2020

That is correct @MatthewDorner. Sorry for the inconvenience.

@DHaussermann
Copy link

@MatthewDorner I'm not seeing this work as expected. I wonder if the way I'm trying to validate this is correct?
Steps:

  • Query PluginKeyValueStore and see how many MS Calendar records have a PKey value of sub_...
  • In a browser session, connect test user with the plugin
  • Note that one additional sub record is visible in the ``PluginKeyValueStore` table (as expected)
  • Return to browser session and disconnect test user
  • Return to DB browser and query table again
    Expected: Row count is reduced by 1
    Observed: Row count is unchanged. Nothing written to the log by the plugin.

Is this maybe dependant on another change in master? Thoughts?

@MatthewDorner
Copy link
Collaborator Author

KV Store

What I'm seeing is that when you run /mscalendar connect it creates row 8 in this screenshot. Then when you run /mscalendar subscribe, it creates row 7, and when you run /mscalendar disconnect with the changes in this PR, it deletes row 7. So it creates two records but deletes one.

I'm not sure what the value in row 8 is supposed to be.

@codecov-io
Copy link

codecov-io commented Mar 30, 2020

Codecov Report

Merging #42 into master will decrease coverage by 0.21%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
server/mscalendar/user.go 16.83% <0.00%> (-3.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdde4b5...1656872. Read the comment docs.

@hanzei
Copy link
Collaborator

hanzei commented Mar 31, 2020

@MatthewDorner Build is green now

@MatthewDorner
Copy link
Collaborator Author

I think the record that's not getting deleted is the OAuth state being saved here:

return s.subscriptionKV.Store(state, []byte(state))

Maybe it shouldn't be saved with the sub_ prefix in the first place, since it's not a subscription? And I'm not sure how to delete it, although it should probably be deleted upon /mscalendar disconnect since it gets created upon /mscalendar connect. Should this be a separate issue?

@DHaussermann
Copy link

Thanks @MatthewDorner you're correct. The record I was seeing was caused by the oAuth connection.
I was not seeing the additional record as I was not re-subscribing.

Copy link

@DHaussermann DHaussermann left a 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!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Mar 31, 2020
@levb levb merged commit 6d19b8a into mattermost:master Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Lifecycle/1:stale QA Review Done PR has been approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete subscriptions upon user disconnect
7 participants