-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update: MySiteViewModel Architecture #19869
Conversation
Removes the site source dependency Adds the logic to build the card in the viewmodel slice
Updates to add the logic of building domain registration card
Updates the CardsSource.kt to viewmodel slice and adds logic for building cards
Updates: the BloggingPromptCardViewModelSlice to add card building logic and expose blogging prompt card
+ Adds: the logic to build quick start card + Adds: live data to expose quick start card
Hey @AjeshRPai ! I checked the site icon issue that is mentioned by @irfano. It appears that icon colour was turned to black by this commit (e634f02). I pushed a new commit reverting the colour to white to match trunk but feel free to change it back if I am missing any context here. |
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.
Can you re-review the PR once you have bandwidth? 🤔 Thanks in Advance
Thanks for addressing my comments. LGTM! 👍🏻
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.
Awesome work @AjeshRPai ⭐
The code looks good and the dynamic cards worked as expected retesting with the latest changes.
I noticed a minor issue with the order (that I missed in my previous test #19869 (comment)). I would expect a top card to be just below the menu (like in pc8HXX-1sr-p2) and is below the blogging prompts.
Bottom card ✅ | Top card ❌ |
---|---|
…p and bottom of the dashboard
I drafted a fix for this in #20512 |
Hey @antonis
Thanks for the thorough review and fixing this Issue 🙌🏼 . I was thinking of the same solution as you have done on the PR. I wouldn’t say this is hacky, IMO this is the only solution. As long as the endpoint is the same as other cards, there is no other solution |
…to-viewmodel-slice-dynamic-order Fixes dynamic dashboard cards ordering in the new MySiteViewModel Architecture
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.
I was thinking of the same solution as you have done on the PR. I wouldn’t say this is hacky, IMO this is the only solution. As long as the endpoint is the same as other cards, there is no other solution
Thank you for confirming and helping out with #20512 @AjeshRPai 🙇
I noticed a minor issue with the order (that I missed in my previous test #19869 (comment)). I would expect a top card to be just below the menu (like in pc8HXX-1sr-p2) and is below the blogging prompts.
I retested via #20512 (comment) and the order is the expected one 🎉
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.
I mainly tested and reviewed the code for Bloganuary and Blogging Prompts cards and it overall looks good to me and matches the existing behavior. I just added some minor questions and suggestions in my comments.
Thank you a lot for refactoring this part of the code and removing the CardSources, making dashboard cards implementation a bit easier! 🙇🏼
It would also be nice to get rid of the Builder + BuilderParams architecture in the future since it just feels like a lot of boilerplate without many advantages. But one step at a time! 😆
...dpress/android/ui/mysite/cards/dashboard/bloggingprompts/BloggingPromptCardViewModelSlice.kt
Outdated
Show resolved
Hide resolved
...dpress/android/ui/mysite/cards/dashboard/bloggingprompts/BloggingPromptCardViewModelSlice.kt
Outdated
Show resolved
Hide resolved
...dpress/android/ui/mysite/cards/dashboard/bloggingprompts/BloggingPromptCardViewModelSlice.kt
Outdated
Show resolved
Hide resolved
.../wordpress/android/ui/mysite/cards/dashboard/bloganuary/BloganuaryNudgeCardViewModelSlice.kt
Outdated
Show resolved
Hide resolved
…l-slice' into move-site-menu-items-to-viewmodel-slice
Hey @thomashorta 👋🏼
Yup definitely 💯 Also @thomashorta |
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.
Can you please review the following cards and related classes?
BloggingPromptCard
State of the word card
Jetpack install full plugin card
JpMigrationSuccessCard
This PR has so many hidden items that I completely missed #19869 (comment) 😓 Sorry about that!
Blogging Prompt Card
Reviewed, added a few comments, and saw your latest commits solving those comments, thanks! Looks good to me!
State of the word card
Also, can we remove the State of the word card from the code base, or do we need to keep it 🤔? I don't think we can show this using dynamic dashboard cards, as they are only shown in WP(Dynamic dashboard cards are only shown in JP), so I'm not sure.
Good question, in theory we have State of the Word every year and having the code present means we only need to make small changes next time, so I would keep the code for it, especially since you said dynamic cards cannot be used for WP. An alternative would be making dynamic card compatible with WP or create an equivalent feature specifically for WP (maybe based on the SotW code). Wdyt?
Jetpack install full plugin card
I added a small comment but overall the code looks good to me. 🙇🏼
Upon testing, though I noticed that a lot of things changed since this card has been implemented, and this card is not showing up anymore. Note that this card is also not showing in builds before these changes, so it's not an issue introduced by this PR!
I found this old GH issue with some specs: #17836, but I don't remember the specific logic and expected behavior. @RenanLukas @develric, should we revisit this with the current dashboard?
JpMigrationSuccessCard
I took a quick look at the code and looks good but I am not familiar with this card and its behavior nor how to properly test it. Maybe @RenanLukas can help double checking this one?
Thanks @AjeshRPai for the changes! I will leave it approved since I just added a minor suggestions, and the code I saw looks good to me! 🙇🏼
...ss/android/ui/mysite/cards/jpfullplugininstall/JetpackInstallFullPluginCardViewModelSlice.kt
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
Hey @RenanLukas Keeping this PR up to date with trunk is getting a little harder, I am going ahead and merging this PR. Feel free to test the Jetpack install full plugin card and JpMigrationSuccessCard and let us know if this PR has introduced any issues. Will take up a fix and solve it in a new PR. Thanks In advance. |
Jetpack install full plugin cardI've tried following the testing instructions from this Android PR but the card didn't appear. Must be a bug, but as pointed out by Thomás it wasn't introduced by this PR. JpMigrationSuccessCardThis seems to be the card when the user moves to JP in the context of The "Learn more" action is also opening the post-migration screen: |
Description
This PR updates the architecute in MySiteViewModel. The changes in the architecture is described in this internal document - pcdRpT-68h-p2
To Test:
Smoke test the following affected views and related code
Account Settings View
AccountDataViewModelSliceTest
are correct and cover all the scenarios.AccountDataViewModelSlice
SiteInfoHeader
SiteInfoHeaderCardViewModelSliceTest
are correct and cover all the scenarios.SiteInfoHeaderCardViewModelSlice
Dashboard Section
These cards are shown only in the Jetpack app, and the site is a WP.com site. The visibility of some cards depends on other factors, which are described in each card section.
Jetpack Migration Success Card
JpMigrationSuccessCardViewModelSliceTest
are correct and cover all the scenarios.JpMigrationSuccessCardViewModelSlice
QuickLinks Card
QuickLinkCardViewModelSlice
QuickStartCard
QuickStartCardVewModelSliceTest
andQuickStartCardVewModelSlice
Domain Registration Card
DomainRegistrationCardViewModelSlice
DomainRegistrationCardViewModelSliceTest
are correct and cover all the scenarios.Blogging Prompt Card
BloggingPromptCardViewModelSlice
,BloggingPromptsCardTrackHelper
BloggingPromptCardViewModelSliceTest
are correct and cover all the scenarios.Today Stats Card
TodaysStatsViewModelSlice
TodaysStatsViewModelSliceTest
are correct and cover all the scenarios.Post Card
PostsCardViewModelSlice
PostsCardViewModelSliceTest
are correct and cover all the scenarios.Pages Card
PagesCardViewModelSllice
PagesCardViewModelSlliceTest
are correct and cover all the scenarios.Activity Log Card
ActivityLogCardViewModelSlice
ActivityLogCardViewModelSliceTest
are correct and cover all the scenarios.Blaze Card
Promote With Blaze Card
BlazeCardViewModelSlice
BlazeCardViewModelSliceTest
are correct and cover all the scenarios.Blaze Campaigns Card
BlazeCardViewModelSlice
BlazeCardViewModelSliceTest
are correct and cover all the scenarios.PlansCard
PlansCardViewModelSlice
PlansCardViewModelSliceTest
are correct and cover all the scenarios.Jetpack Plugins Card
1. Verify Visibility Rule
2. Verify all the click functionality and tracking is working as expected
3. Code review
JetpackInstallFullPluginCardViewModelSlice
4. Verify that the tests on
JetpackInstallFullPluginCardViewModelSliceTest
are correct and cover all the scenarios.Dynamic Dashboard Cards
Follow the testing instructions in the internal document - pc8HXX-1sr-p2
Site Items Section
These cards are shown when the site is self-hosted or when the app is WP.
Site Items Menu
SiteItemsViewModelSlice
SiteItemsViewModelSliceTest
is correct and cover all the scenariosJetpack feature card
JetpackFeatureCardViewModelSlice
JetpackFeatureCardViewModelSliceTest.kt
cover all the scenariosJetpack Switch Menu
JetpackSwitchMenuViewModelSlice
JetpackSwitchMenuViewModelSliceTest.kt
cover all the scenariosJetpack Badge
JetpackBadgeViewModelSlice
JetpackBadgeViewModelSliceTest.kt
cover all the scenariosState of the word card
WpSotw2023NudgeCardViewModelSlice
WpSotw2023NudgeCardViewModelSliceTest.kt
cover all the scenariosCode Review the following affected classes
MySiteViewModel
andMySiteViewModelTest
DashboardCardViewModel
andDashboardCardViewModelTest
DasbhoardItemsViewModel
andDashboardItemsViewModelTest
LiveDataUtils.kt
→ElevenItemContainer
Tasks
Update ViewModelSlice unit tests
Known issues 🐛
NoCardsMessageViewModelSlice is not being usedDomainRegistrationViewModelSlice is not being usedSite Icon is not immediately refreshed after it is uploadedAfter creating a site, Quick start notice is not shown, although quick start card is shownBetween self hosted and wp com sites, the dashboard items and site items are not switched properlyBetween WP com sites, cards - pages, activity log and posts are not shown correctTracking is not implemented yetSince there are a lot of code changes and the impact is huge, wait until all the reviewers approve the PR
Regression Notes
Potential unintended areas of impact
Dashboard is not working as expected
What I did to test those areas of impact (or what existing automated tests I relied on)
Smoke testing the dashboard and unit testing
What automated tests I added (or what prevented me from doing so)
Unit tests
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: