-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Offer the application password login inside a card in MySite screen #21878
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
Offer the application password login inside a card in MySite screen #21878
Conversation
Generated by 🚫 Danger |
|
App Name | ![]() |
|
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr21878-5552813 | |
Commit | 5552813 | |
Direct Download | wordpress-prototype-build-pr21878-5552813.apk |
|
App Name | ![]() |
|
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr21878-5552813 | |
Commit | 5552813 | |
Direct Download | jetpack-prototype-build-pr21878-5552813.apk |
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.
Pull Request Overview
This pull request introduces a new Application Password login card on the MySite screen for self-hosted sites. Key changes include the addition of an ApplicationPasswordViewModelSlice with local caching, updates to integrate this card in MySiteViewModel and MySiteFragment, and corresponding test adjustments.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt | New tests to verify the behavior of the application password card. |
WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt | Removed redundant test cases and references for application password. |
WordPress/src/main/res/values/strings.xml | Added a new localized string for the application password title. |
WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt | Implements the business logic for discovering and showing the auth URL. |
WordPress/src/main/java/org/wordpress/android/ui/mysite/SiteNavigationAction.kt | Added a new navigation action for application password authentication. |
WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt | Integrated the new slice into the overall UI model composition. |
WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteFragment.kt | Updated fragment to trigger the new application password flow. |
Comments suppressed due to low confidence (1)
WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt:151
- Rename 'applicationPAsswordModel' to 'applicationPasswordModel' for consistency and clarity.
val headerList = listOfNotNull(nonNullSiteInfoHeaderCard, applicationPAsswordModel)
...rdpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt
Outdated
Show resolved
Hide resolved
...rdpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt
Outdated
Show resolved
Hide resolved
…applicationpassword/ApplicationPasswordViewModelSliceTest.kt Co-authored-by: Copilot <[email protected]>
…applicationpassword/ApplicationPasswordViewModelSliceTest.kt Co-authored-by: Copilot <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #21878 +/- ##
=======================================
Coverage 39.63% 39.63%
=======================================
Files 2124 2124
Lines 99218 99218
Branches 15243 15243
=======================================
Hits 39330 39330
Misses 56427 56427
Partials 3461 3461 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...g/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt
Outdated
Show resolved
Hide resolved
...g/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt
Outdated
Show resolved
Hide resolved
This card doesn't show up in the "Personalize your home tab" screen. I assume we don't want it there, but just wanted to let you know this feature exists. personliaze.mp4 |
Good point. I don't think this is a personalizable card sicne it's an encouraged one-time action. |
How did you logged in for |
…-login-inside-a-card
@adalpari I logged in using the WP.com site url and credentials. Here's the relevant logging:
Note in the log it's attempting to log into Odd that the site can not be discovered but the card is shown anyway.. Maybe a problem with the local cache...
About the used URL, I am using the one stored in the DB. So, probably we are storing ´nbradbury10.wordpress.com´ directly instead of |
@adalpari Possibly related to this, I noticed that WPMainActivity.setSelectedSite was being called multiple times after logging in, and not always with the site that was selected after logging in. This is a pre-existing issue, but I'm wondering if it might be causing the problem I mentioned? |
…he-Application-Password-login-inside-a-card
…e-a-card' of https://github.com/wordpress-mobile/WordPress-Android into feat/CMM-335-Offer-the-Application-Password-login-inside-a-card
It could certainly be... But if te other cards are correctly built I am wondering why the Application Password card is not 🤔 |
Yes, this happens every time. The steps are I simply login using a site address.
Tapping the card opens the site I expected. |
ok, it looks like the login discovery is returning a valid URL for , when probably it shouldn't...
|
* Adding analytic event * Minor refactor * Fixing test * Adding event properties * Detekt * Fixing test
@nbradbury could you run the PR again, and send me the logs with the tag |
@adalpari Here's what I see in the log:
|
Hey @nbradbury! |
|
@adalpari Thanks for the clarification! I'll approve this PR. |
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.
Looks good!
Description
This PR is adding a new card at the top of MySite screen. That card should be only visible for self-hosted sited, and clicking on it should open the Application Password authentication flow.
Most of the logic has been moved from the VM to a Slide class to follow how other cards are constructed. In addition, a local memory cache has been included to avoid calling the api unnecessarily.
Testing instructions
Screen.Recording.2025-05-14.at.16.50.50.mov
Screen.Recording.2025-05-14.at.16.58.03.mov