Skip to content

feat(lib/vscode): add log out to application menu - #2922

Merged
jsjoeio merged 4 commits into
mainfrom
jsjoeio/add-logout
Mar 23, 2021
Merged

feat(lib/vscode): add log out to application menu#2922
jsjoeio merged 4 commits into
mainfrom
jsjoeio/add-logout

Conversation

@jsjoeio

@jsjoeiojsjoeio commented Mar 17, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

This adds a new option to the Application Menu called Log out. It deletes the code-server cookie and logs a user out.

Screenshot

2021-03-17 15 31 46

TODOs

  • use vscode logger
  • refactor Cookie to be in lib/vscode so we can use it
  • write e2e test
  • refactor Cookie and don't share between lib/vscode

Fixes#778

@jsjoeiojsjoeio added this to the v3.9.2 milestone Mar 17, 2021
@jsjoeiojsjoeio self-assigned this Mar 17, 2021
@jsjoeio
jsjoeioforce-pushed the jsjoeio/add-logout branch from 3f5b16c to 5274816CompareMarch 17, 2021 23:08
@jsjoeio
jsjoeio marked this pull request as ready for review March 17, 2021 23:23
@jsjoeio
jsjoeio requested a review from a team as a code ownerMarch 17, 2021 23:23
@jsjoeio
jsjoeioforce-pushed the jsjoeio/add-logout branch 2 times, most recently from d77243a to 6a05889CompareMarch 17, 2021 23:39
oxy
oxy suggested changes Mar 18, 2021
View reviewed changes

@oxyoxy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM! Just a newline at the end of lib/vscode/src/vs/server/common/cookie.ts should be all that's needed.

Comment threadlib/vscode/src/vs/server/common/cookie.ts Outdated
@oxy

oxy commented Mar 18, 2021•
edited
Loading

Copy link
Copy Markdown

So I was wondering why tsc was "failing" to use outDir...

Turns out it was, but since we import from lib/vscode in src now, what used to be out/node/entry.js has now become out/src/node/entry.js (and there's an out/lib/vscode/.../cookie.js).

Since our build script, ./ci/steps/build-code-server.sh looks for out/node/entry.js, it fails.

I think it might be best to just duplicate the key cookie.

oxy
oxy reviewed Mar 18, 2021
View reviewed changes
Comment threadtest/unit/util.test.ts Outdated
@jsjoeio

jsjoeio commented Mar 18, 2021

Copy link
Copy Markdown
ContributorAuthor

I think it might be best to just duplicate the key cookie.

Ah okay. Yeah it doesn't seem worth it to have to fix everything else for one thing to not be duplicated 😂 thanks for looking into this! I'll convert to draft and fix!

@jsjoeio
jsjoeio marked this pull request as draft March 18, 2021 15:16
@jsjoeio
jsjoeioforce-pushed the jsjoeio/add-logout branch from 6a05889 to c23215fCompareMarch 22, 2021 18:38
@jsjoeio
jsjoeio marked this pull request as ready for review March 22, 2021 18:40
@jsjoeio
jsjoeio requested a review from oxyMarch 22, 2021 18:40
oxy
oxy approved these changes Mar 22, 2021
View reviewed changes

@oxyoxy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM!

@jsjoeio
jsjoeioforce-pushed the jsjoeio/add-logout branch 2 times, most recently from d80bcdf to 7958d52CompareMarch 22, 2021 21:10
@jsjoeio
jsjoeio enabled auto-merge March 22, 2021 21:12
@jsjoeio
jsjoeioforce-pushed the jsjoeio/add-logout branch from 1697e49 to 5596f97CompareMarch 22, 2021 21:28
@bpmct

bpmct commented Mar 22, 2021

Copy link
Copy Markdown
Member

Love this!

@jsjoeio
feat(lib/vscode): add log out to application menu
9f790fd
This adds a new option to the Application Menu called Log out.
It deletes the code-server cookie and logs a user out.
@jsjoeio
refactor: add cookie.ts to lib/vscode
5cec620
@jsjoeio
feat: add e2e test for logout
090687d
@jsjoeio
jsjoeioforce-pushed the jsjoeio/add-logout branch from 806022a to 090687dCompareMarch 23, 2021 17:02
@jsjoeio
jsjoeio merged commit 947dd85 into mainMar 23, 2021
@jsjoeio
jsjoeio deleted the jsjoeio/add-logout branch March 23, 2021 19:44
@binaryfire

binaryfire commented Mar 24, 2021

Copy link
Copy Markdown

Nice! Just a thought - can you also add it to the profile icon at the bottom of the left sidebar? That seems like the most intuitive place to look for it.

@jsjoeio

jsjoeio commented Mar 24, 2021

Copy link
Copy Markdown
ContributorAuthor

@binaryfire thanks for the suggestion! There's some issues with the profile icon (see #2913) but when we work on that, we can consider it! Feel free to leave more thoughts there

@jsjoeiojsjoeio mentioned this pull request Mar 24, 2021
Closed
code-asher added a commit to code-asher/code-server that referenced this pull request Apr 16, 2021
@code-asher
Revert "Merge pull requestcoder#2922from cdr/jsjoeio/add-logout"
eb1bc16
The tests have been kept.
This reverts commit 947dd85, reversing
changes made to 24dc208.
@jsjoeiojsjoeio mentioned this pull request Apr 27, 2021
Closed
@Dual-0

Dual-0 commented May 10, 2021

Copy link
Copy Markdown

nice feature! Hope it is okay to put this in here...
At my new installed code-server instance this button isn't working right.
When I click on the button the site is reloading but I'm still logged in.
Cookie is still present.

thanks

@jsjoeio

jsjoeio commented May 10, 2021

Copy link
Copy Markdown
ContributorAuthor

@dual-oo there were a couple bugs fixed in #3277

We're cutting a new release this week so make sure to upgrade and if you have issues, please open a bug report!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@oxyoxyoxy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jsjoeiojsjoeio

Labels

None yet

Projects

None yet

Milestone

v3.9.2

Development

Successfully merging this pull request may close these issues.

Log out button

5 participants

@jsjoeio@oxy@bpmct@binaryfire@Dual-0