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

Logout feature added #122

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

swissbyte
Copy link

Hi

This is the merged #106 feature which a branch already existed for. But the existing branch lacked the functionality of removing the tokens from the key vault. This branch removes the token from:

  • local in memory storage (also from the persistent one)
  • local cache if the key vault is used
  • key vault

Would be nice if you accept the request :)

@khaelys
Copy link
Collaborator

khaelys commented Dec 27, 2021

Thank you for your contribution! I'll check this when I return from vacation ☺️

@LorenDB
Copy link

LorenDB commented Jan 19, 2022

Is this coming anytime soon?

@swissbyte
Copy link
Author

@LorenDB you can check my repository... there you can find a version with the logout feature already integrated

@LorenDB
Copy link

LorenDB commented Jan 19, 2022

@LorenDB you can check my repository... there you can find a version with the logout feature already integrated

The problem is that the xstream-hosted instance doesn't have it merged. 😕

@swissbyte
Copy link
Author

I was not aware that there is a hosted version... and why dont you host it on your own azure instance?

@LorenDB
Copy link

LorenDB commented Jan 20, 2022

Because (a) I was not entirely aware that self-hosting was an option and (b) I was just testing out the system to see how I liked it.

@swissbyte
Copy link
Author

Self hosting is very easy. Check out my repo. I updated the readme.md with a lot of additional information. Also regarding self hosting

@khaelys
Copy link
Collaborator

khaelys commented Jan 24, 2022

Is this coming anytime soon?

@swissbyte I'll try to look at this PR this evening, but I can't give you any guarantee, we are busy in this period :)

@khaelys
Copy link
Collaborator

khaelys commented Jan 24, 2022

Hi @swissbyte I started reviewing your PR, but it looks like you added other features, like set working hours, an improvement on reminders and an improvement on the documentation.

In this PR I would like to review only the logout feature (I guess the last 3 commits). Can you cherry-pick them and resubmit the PR? The other features look stunning, but I would like to review and add them one at a time :)

@swissbyte
Copy link
Author

Hi @khaelys thanks for answering.
Sure, i thought it would only include the commits up to the date when k did the pull request 😅

Damn... so i have to get into cherry picking... i will have a look at it tomorrow

If you have a small step by step, i would be glad to get this...

K know basically what cherry picking is but not in conjunction with a PR

@khaelys
Copy link
Collaborator

khaelys commented Jan 25, 2022

@swissbyte I am not comfortable with it either, but I thought about two options:

  • cherry-pick the last 3 commits in a new local branch, point to the same remote branch, and force push.
  • cherry-pick the last 3 commits in a new local branch, push to a new or the same remote branch, but you open a new pull request

One more thing, I noted that there aren't tests, but It may be the case to add some, like a couple of tests for the InMemoryTokenRepository and TokenRepository, you'll find the suites under Bot.Tests/Data.

It will be nice to also start unit testing dialogs as we go. You can look at Bot.Tests/Data/Dialogs/ClockifySetupDialogTest for some ideas on how to do it.

If you need help let me know!

@swissbyte
Copy link
Author

Thanks for the answer. Yes, k defeniteley have to dive into tests. I know the theory behind and did some during studies but i have not weitten that much cases before...

@LorenDB
Copy link

LorenDB commented Apr 20, 2022

Ping!

I'm still hoping that this PR will get merged...

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.

3 participants