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

Update: MySiteViewModel Architecture #19869

Merged
merged 264 commits into from
Mar 25, 2024

Conversation

AjeshRPai
Copy link
Contributor

@AjeshRPai AjeshRPai commented Jan 2, 2024

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

  1. Account Settings View
    1. Visibility Rule: Shown only on WP app + feature removal phase in New users phase | Phase 4 or Self-hosted users phase
    2. Testing Instructions
      1. Verify Visibility Rule
      2. Verify all the click functionality is working as before
      3. Verify that the tests on AccountDataViewModelSliceTest are correct and cover all the scenarios.
      4. Review the code on AccountDataViewModelSlice
  1. SiteInfoHeader
    1. Visibility Rule: The user should have a site, shown on both the JP and WP apps for all sites
    2. Testing Instructions
      1. Verify Visibility Rule
      2. Verify all the click functionality is working as before
      3. Verify that the tests on SiteInfoHeaderCardViewModelSliceTest are correct and cover all the scenarios.
      4. Review the code on 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.

  1. Jetpack Migration Success Card
    1. Visibility Rule: Shown only when
      1. Jetpack migration is done from the WP app - checked using a pref key.
      2. When the WP app is installed on the device
      3. When the App is Jetpack app
    2. Testing Instructions
      1. Check visibility Rule
      2. Check on clicking the card, Deleting the WP app is prompted.
      3. Verify that the tests on JpMigrationSuccessCardViewModelSliceTest are correct and cover all the scenarios.
      4. Code review JpMigrationSuccessCardViewModelSlice
  1. QuickLinks Card
    1. Visibility Rule: Jetpack app and site is WP com.
    2. Testing Instructions
      1. Verify Visibility Rule
      2. The items are built according to personalization.
        1. Go to MySiteDashboard → Go to bottom → Personalize card
        2. Add/Remove some quick links item.
        3. Verify that the items shown in the dashboard are as expected.
      3. Verify whether or not the backup and Scan items are shown/requested for eligible sites.
        1. Login to the Jetpack app with a site having Scan and Backup capabilities
        2. Go to the Personalization screen → Add Scan and Back to the Shortcuts.
        3. Go back to Dashboard.
        4. Verify that the items are shown in the dashboard.
      4. Code Review QuickLinkCardViewModelSlice
  1. QuickStartCard
    1. Visibility Rule: Shown when a new site is created or when Quick Start is in progress
    2. Testing Instructions
      1. Shown when the new site is created
        1. Create a new site.
        2. Click on Show me around when the dialog is shown.
        3. Verify that the Quick Start Card is shown.
      2. Shown when the user comes back to the site
        1. After following steps a, Go to another site.
        2. And switch to the Previous site, Check if the quick start card is shown
      3. Check if the card works as intended
        1. When each quick start task is completed, the quick start card should be updated
        2. When clicked on any tasks
      4. Code Review QuickStartCardVewModelSliceTest and QuickStartCardVewModelSlice
  1. Domain Registration Card
    1. Visibility Rule
      1. The user is on the free plan
      2. Domain credits are available for the user
    2. Testing Instructions
      1. Verify the visibility Rule
      2. Code review DomainRegistrationCardViewModelSlice
      3. Verify that the tests on DomainRegistrationCardViewModelSliceTest are correct and cover all the scenarios.
  1. Blogging Prompt Card
    1. Visibility Rule
      1. Blogging prompt feature flag is enabled on the app
      2. The site is using wpcom rest api
      3. Is prompt card enabled by the user (can be disabled in personalization)
      4. Is prompt skipped for today
    2. Testing Instructions
      1. Verify Visibility Rule
      2. Verify all the click functionality and tracking is working as expected
      3. Code review BloggingPromptCardViewModelSlice, BloggingPromptsCardTrackHelper
      4. Verify that the tests on BloggingPromptCardViewModelSliceTest are correct and cover all the scenarios.
  1. Today Stats Card
    1. Visibility Rule:
      1. Depends on the Dashboard endpoint +
      2. Not requested when the user hides the card
    2. Testing Instructions
      1. Verify Visibility Rule
      2. Verify all the click functionality and tracking is working as expected
      3. Code review TodaysStatsViewModelSlice
      4. Verify that the tests on TodaysStatsViewModelSliceTest are correct and cover all the scenarios.
  1. Post Card
    1. Visibility Rule - Shown if the user enables the card on personalization and the user has draft or scheduled posts
    2. Types of Card
      • Draft Post Card
      • Scheduled Post Card
    3. Testing Instructions
      1. Verify Visibility Rule
      2. Verify all the click functionality and tracking is working as expected
      3. Code review PostsCardViewModelSlice
      4. Verify that the tests on PostsCardViewModelSliceTest are correct and cover all the scenarios.
  1. Pages Card
    1. Visibility Rule
      1. WPcom - User has edit pages capabilities
      2. Self hosted site - is admin
      3. User has enabled the pages card in personalization
    2. Testing Instructions
      1. Verify Visibility Rule
      2. Verify all the click functionality and tracking is working as expected
      3. Code review PagesCardViewModelSllice
      4. Verify that the tests on PagesCardViewModelSlliceTest are correct and cover all the scenarios.
  1. Activity Log Card
    1. Visibility Rule
      1. User has enabled the activity log card in personalization
      2. Site is jetpack connected
      3. Site is WPCOm
      4. Site has manage options capability
      5. Site is not for wp teams
    2. Testing Instructions
      1. Verify Visibility Rule
      2. Verify all the click functionality and tracking is working as expected
      3. Code review ActivityLogCardViewModelSlice
      4. Verify that the tests on ActivityLogCardViewModelSliceTest are correct and cover all the scenarios.
  1. Blaze Card
    • Promote With Blaze Card

      1. Visibility Rule:
        1. Whether the user is admin or not
        2. Is the Jetpack app
        3. Whether the blaze feature is enabled for the app
        4. Whether the card is enabled on personalization
      2. Testing Instructions
        1. Verify Visibility Rule
        2. Verify all the click functionality and tracking is working as expected
        3. Code review BlazeCardViewModelSlice
        4. Verify that the tests on BlazeCardViewModelSliceTest are correct and cover all the scenarios.
    • Blaze Campaigns Card

      1. Visibility Rule:
        1. If user is admin
        2. Is Jetpack app
        3. If the blaze feature is enabled for the app
        4. If the blaze campaigns feature is enabled for the app
        5. If the blaze card is not hidden by user through personalization
      2. Testing Instructions
        1. Verify Visibility Rule
        2. Verify all the click functionality and tracking is working as expected
        3. Code review BlazeCardViewModelSlice
        4. Verify that the tests on BlazeCardViewModelSliceTest are correct and cover all the scenarios.
  1. PlansCard
    1. Visibility Rule
      1. Is Jetpack app
      2. Site is WP com or Atomic Site
      3. Has free plan
      4. User is an admin
      5. Is not WPForTeams Site
      6. The card is not hidden
    2. Testing Instructions
      1. Verify Visibility Rule
      2. Verify all the click functionality and tracking is working as expected
      3. Code review PlansCardViewModelSlice
      4. Verify that the tests on PlansCardViewModelSliceTest are correct and cover all the scenarios.
  1. Jetpack Plugins Card
    1. Visibility Rule
      1. Plugin card is enabled on the personalization.
      2. Jetpack plugin is installed without full plugin
    2. Testing Instructions
      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.
  1. Dynamic Dashboard Cards

    Follow the testing instructions in the internal document - pc8HXX-1sr-p2

  1. No Cards Message - Shown If all the cards are hidden
  2. Personalize Card - Shown on the dashboard, always

Site Items Section

These cards are shown when the site is self-hosted or when the app is WP.

  1. Site Items Menu
    1. Testing Instructions
    2. Go through all the items in the site items - themes, plugin etc and verify that everything is working and tracking events is fired as before
    3. Code Review SiteItemsViewModelSlice
    4. Verify that the tests in SiteItemsViewModelSliceTest is correct and cover all the scenarios
  1. Jetpack feature card
    1. Shown in WP and when the phase is jetpack removal
    2. Verify the clicks and tracking
    3. Code review JetpackFeatureCardViewModelSlice
    4. Verify that the tests in JetpackFeatureCardViewModelSliceTest.kt cover all the scenarios
  1. Jetpack Switch Menu
    1. Shown in WP and when the phase is jetpack removal
    2. Verify the clicks and tracking
    3. Code review JetpackSwitchMenuViewModelSlice
    4. Verify that the tests in JetpackSwitchMenuViewModelSliceTest.kt cover all the scenarios
  1. Jetpack Badge
    1. Shown in WP and when the phase is jetpack removal
    2. Verify the clicks and tracking
    3. Code review JetpackBadgeViewModelSlice
    4. Verify that the tests in JetpackBadgeViewModelSliceTest.kt cover all the scenarios
  1. State of the word card
    1. Shown in WP
    2. Verify the clicks and tracking
    3. Code review WpSotw2023NudgeCardViewModelSlice
    4. Verify that the tests in WpSotw2023NudgeCardViewModelSliceTest.kt cover all the scenarios

Code Review the following affected classes

  1. MySiteViewModel and MySiteViewModelTest
  2. DashboardCardViewModel and DashboardCardViewModelTest
  3. DasbhoardItemsViewModel and DashboardItemsViewModelTest
  4. LiveDataUtils.ktElevenItemContainer

Tasks

Update ViewModelSlice unit tests
  • MySiteViewModelTest @AjeshRPai
  • AccountDataViewModelSliceTest @AjeshRPai
  • SiteInfoHeaderCardViewModelSliceTest @zwarm
  • DashboardCardsViewModelSliceTest @zwarm
    • BlazeCardViewModelSliceTest @zwarm
    • PersonalizeCardViewModelSliceTest @zwarm
    • BloganuaryNudgeCardViewModelSliceTest @AjeshRPai
    • CardsViewModelSliceTest @AjeshRPai
    • JpMigrationSuccessCardViewModelSliceTest @zwarm
    • JetpackInstallFullPluginCardViewModelSliceTest @zwarm
    • BloggingPromptCardViewModelSliceTest @zwarm
    • QuickStartCardVewModelSliceTest @AjeshRPai
    • PlansCardViewModelSliceTest @zwarm
    • DomainRegistrationCardViewModelSliceTest @zwarm
  • DashboardItemsViewModelSliceTest @zwarm
    • WpSotw2023NudgeCardViewModelSliceTest @zwarm
    • JetpackBadgeViewModelSliceTest @AjeshRPai
    • JetpackSwitchMenuViewModelSlice
    • JetpackFeatureCardViewModelSlice
Known issues 🐛
  • NoCardsMessageViewModelSlice is not being used
  • DomainRegistrationViewModelSlice is not being used
  • Site Icon is not immediately refreshed after it is uploaded
  • After creating a site, Quick start notice is not shown, although quick start card is shown
  • When switching sites
    • Between self hosted and wp com sites, the dashboard items and site items are not switched properly
    • Between WP com sites, cards - pages, activity log and posts are not shown correct
  • Tracking is not implemented yet
  • Default site icon has a darker appearance

⚠️ Merge Instructions

Since there are a lot of code changes and the impact is huge, wait until all the reviewers approve the PR

Regression Notes

  1. Potential unintended areas of impact
    Dashboard is not working as expected

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Smoke testing the dashboard and unit testing

  3. What automated tests I added (or what prevented me from doing so)
    Unit tests


PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

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
@pantstamp
Copy link
Contributor

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.

Copy link
Member

@irfano irfano left a 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! 👍🏻

Copy link
Contributor

@antonis antonis left a 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 ❌
Screenshot_1710942773 Screenshot_1710942764

@antonis
Copy link
Contributor

antonis commented Mar 20, 2024

I would expect a top card to be just below the menu (like in pc8HXX-1sr-p2) and is below the blogging prompts.

I drafted a fix for this in #20512
The approach chosen is a bit hacky 😅 I'll be glad to iterate with any suggestions or help with testing and reviewing another solution to this 🙇

@AjeshRPai
Copy link
Contributor Author

Hey @antonis

The approach chosen is a bit hacky 😅 I'll be glad to iterate with any suggestions or help with testing and reviewing another solution to this 🙇

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
Copy link
Contributor

@antonis antonis left a 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 🎉

Copy link
Contributor

@thomashorta thomashorta left a 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! 😆

@AjeshRPai
Copy link
Contributor Author

Hey @thomashorta 👋🏼
Thanks for the review and the suggestions.

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! 😆

Yup definitely 💯

Also @thomashorta
Can you confirm whether the state of the word card is needed or not? I will remove it if it's not needed in a future PR.

#19869 (comment)

@AjeshRPai AjeshRPai removed the Do Not Merge In PRs with this label, our automation will fail a require check, preventing accidental merging label Mar 22, 2024
Copy link
Contributor

@thomashorta thomashorta left a 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! 🙇🏼

Copy link

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.9% Duplication on New Code

See analysis details on SonarCloud

@AjeshRPai
Copy link
Contributor Author

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.

@AjeshRPai AjeshRPai merged commit 93d9440 into trunk Mar 25, 2024
12 of 20 checks passed
@AjeshRPai AjeshRPai deleted the move-site-menu-items-to-viewmodel-slice branch March 25, 2024 08:09
@RenanLukas
Copy link
Contributor

@AjeshRPai 👋

Jetpack install full plugin card

I'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.

JpMigrationSuccessCard

This seems to be the card when the user moves to JP in the context of Jetpack Focus feature. I haven't worked on it, but I had a look and the card is being shown as expected.

image

The "Learn more" action is also opening the post-migration screen:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants