-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Desktop app: Beta release for OAuth login #99742
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
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.
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
3bbab0b
to
95e3287
Compare
95e3287
to
724bc07
Compare
wp-desktop ci passing, closing review
5452390
to
2210ab0
Compare
2210ab0
to
62eb38f
Compare
d26bd2c
to
7a7ac3b
Compare
7a7ac3b
to
0585d89
Compare
So that the logic is more explicit
0585d89
to
3b3fce0
Compare
I've tested this on Mac and Windows, all good. Currently testing on Linux. |
Sadly, this doesn't seem to be working on Linux (debian 12.9). After installing the app from the
Then run: update-desktop-database
xdg-mime default wpdesktop.desktop x-scheme-handler/wpdesktop This makes the app open after the user clicks Authorize on their browser, but the app just hangs apparently indefinitely, until the system asks me if I want to force quit it. For now I will disable the OAuth flow on Linux, so that it uses the regular login flow, so that we can ship to Mac and Windows. |
@nsakaimbo Added you as reviewer since PCYsg-RR0-p2 mentions doing so. |
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.
Changes make sense to me 👍
I can't help too much with testing it, I wonder if we can borrow some help or post on the cft p2?
No worries @tyxla, thanks for reviewing! I tested on Mac and Windows in multiple browsers and everything appears operational. The idea would be to merge this PR so that a beta version of the app can be released. Once the beta is released, we can do further testing of this and some dependency updates we have done in other PRs that require using a signed version of the app, and only released versions are signed. Once we have the beta released, I will request help from cft P2 (thanks for the suggestion!) |
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.
👍 🚀
Related to #89736 #72754 #55611
Proposed Changes
In #98345 we introduced a new OAuth login flow for the desktop app, but kept it disabled. This PR enables that flow, and issues a new beta release so that these changes can be tested before releasing to customers. Please see the description of #98345 for details on the changes that are being introduced.
In addition to enabling the OAuth flow, this PR also fixes issues that were found during testing, namely preventing that the user ever sees the regular login page, and instead always sees the desktop login page.
Why are these changes being made?
Please see the description of #98345.
Testing Instructions
xattr -c /Applications/WordPress.com.app
Pre-merge Checklist