Update: MySiteViewModel Architecture

[AjeshRPai]

Description

This PR updates the architecute in MySiteViewModel.

---

To Test:

---

Tasks: Update ViewModelSlice unit tests

• [ ] MySiteViewModelTest
• [x] AccountDataViewModelSliceTest @AjeshRPai
• [x] SiteInfoHeaderCardViewModelSliceTest @zwarm
• [ ] DashboardCardsViewModelSliceTest @zwarm
  • [x] BlazeCardViewModelSliceTest @zwarm
  • [x] PersonalizeCardViewModelSliceTest @zwarm
  • [ ] BloganuaryNudgeCardViewModelSliceTest @AjeshRPai
  • [ ] CardsViewModelSliceTest @AjeshRPai
    • [x] DynamicCardsViewModelSliceTest @AjeshRPai
    • [x] TodaysStatsViewModelSliceTest @AjeshRPai
    • [x] PagesCardViewModelSliceTest @AjeshRPai
    • [x] PostsCardViewModelSliceTest @AjeshRPai
    • [x] ActivityLogCardViewModelSliceTest @AjeshRPai
  • [x] JpMigrationSuccessCardViewModelSliceTest @zwarm
  • [x] JetpackInstallFullPluginCardViewModelSliceTest @zwarm
  • [x] BloggingPromptCardViewModelSliceTest @zwarm
  • [ ] QuickStartCardVewModelSliceTest @AjeshRPai
  • [x] PlansCardViewModelSliceTest @zwarm
  • [x] DomainRegistrationCardViewModelSliceTest @zwarm
• [x] DashboardItemsViewModelSliceTest @zwarm
  • [x] WpSotw2023NudgeCardViewModelSliceTest @zwarm
  • [x] JetpackBadgeViewModelSliceTest @AjeshRPai

WIP - this list will be updated

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

Regression Notes

1. Potential unintended areas of impact

   • TODO

2. What I did to test those areas of impact (or what existing automated tests I relied on)

   • TODO

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

   • TODO

---

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)

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr19869-8120e49
	Commit	8120e493f385a69bb685ea55f03c27392a781160
	Direct Download	wordpress-prototype-build-pr19869-8120e49.apk

Note: Google Login is not supported on these builds.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr19869-8120e49
	Commit	8120e493f385a69bb685ea55f03c27392a781160
	Direct Download	jetpack-prototype-build-pr19869-8120e49.apk

Note: Google Login is not supported on these builds.

[AjeshRPai]

Hey @zwarm

Has me thinking "what can we do so we didn't have to update MySiteViewModel each time a new vmslice is added?" Is there an abstract concept we can use instead? One that won't have us sacrificing composition for inheritance?

Maybe we can add a delegation pattern. I am not sure. I have something in mind, but as you said, the first step would be to get this sorted and then take it up.

Let's get these sorted first and then we can go back and further refactor. Yup, I agree 👍🏼

[dangermattic]

	2 Warnings
:warning:	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
:warning:	Class ElevenItemContainer is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by :no_entry_sign: Danger

[peril-wordpress-mobile[bot]]

	Fails
:no_entry_sign:	Danger failed to run /app/danger-0.tqibwxs4vtg.ts.

Error TypeError

Cannot read property 'diffForFile' of undefined
TypeError: Cannot read property 'diffForFile' of undefined
    at Object.exports.default (/app/danger-0.tqibwxs4vtg.ts:27:46)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

22|     // Calculate the changes to Test files because we don't want to limit those
23|     // Note that linesOfCode() function in the latest GitDSL can solve this problem in a more elegant way,
24|     // but the version of Danger currently used by Peril doesn't have it yet.
25|     const testFiles = [...modifiedFiles, ...createdFiles, ...deletedFiles].filter((path: string) => path.includes("/src/test"));
26|     let changesToTests = 0;
------------------------------------------------^
27|     for (let file of testFiles) {        
28|         const stringDiffs = await danger.git.diffForFile(file)
29|         const addedLines = stringDiffs.added.split("\n").filter(x => x.startsWith("+")).length;
30|         const removedLines = stringDiffs.removed.split("\n").filter(x => x.startsWith("-")).length;

Generated by :no_entry_sign: dangerJS

[pantstamp]

Hey @AjeshRPai ! This comment responds to your request for feedback regarding the communication between ViewModelSlice and its parent.

First of all, great job on the work so far! This PR represents a significant enhancement over the MySiteViewModel, and employing ViewModelSlices could greatly improve our management of complex screens.

I don't see any major issues with the current implementation that would prevent merging this PR. However, here are some suggestions for potential enhancements:

I've noticed that many of the Slices expose multiple LiveData objects. For instance, the AccountDataViewModelSlice exposes isRefreshing and uiModel separately. A possible simplification could involve each slice exposing a single "UiState" object representing its entire state. Additionally, we could have another LiveData for one-time events like navigation and snack bar handling.

Introducing an abstraction for the ViewModelSlice concept (via an interface or abstract class) could help standardize behaviors common across all slices. Such behaviors might include:

• Declaring child slices, as each slice can contain others.

• An init() function to initialize children slices and pass the viewModelScope to them.

• Exposing a specific type of UiModel (a data class representing the slice's various states).

• Declaring a "merger" function to define how the children's uiModels merge into a new uiModel.

• Respecting the lifecycle events of the initial ViewModel, like onClear().

These initial requirements are aimed at simplifying development, making it easier for developers to implement new slices without redundant code, thereby reducing future bugs. Do you think this approach would be a valuable future improvement? If so, I could attempt an implementation during this hack week. Do you foresee any issues that could undermine this idea?

We're currently using LiveData to merge different UiModels from the slices. I don't anticipate noticeable performance issues given the generally small number of slices. However, my concern lies with handling concurrent updates from two or more slices' UiModels. I haven't had the chance to investigate this, so I'm raising the question in case you have insights. It might be worth exploring, even if it seems like a remote possibility.

Looking ahead, we might consider migrating from LiveData to Kotlin flows. To clarify, I'm not proposing we make this change immediately. Sticking with LiveData for now seems wise, with a potential shift to flows as a secondary phase if we discern tangible benefits. In theory, flows offer certain advantages over LiveData, as detailed in this article: LiveData vs Flows: Navigating the Currents of Android Data Streams. I've prepared a brief table outlining their pros and cons for future reference.

Below is a comparison table outlining the pros and cons of using LiveData versus Kotlin Flow for the scenario involving multiple ViewModelSlices, each responsible for a portion of the screen, and the goal of combining their states into a general state for the whole screen.

Feature	LiveData	Kotlin Flow
Lifecycle Awareness	Pro: Inherently lifecycle-aware, automatically managing subscription and unsubscription based on the Android Lifecycle.	Con (turned Pro with extra work): Requires explicit handling with Lifecycle.repeatOnLifecycle for lifecycle-aware collections, adding slight complexity.
Concurrency	Con: Limited support for handling concurrent updates directly. Merging and handling concurrent data requires more manual management.	Pro: Better support for handling concurrent updates with operators like combine, ensuring atomic updates and consistency.
Operators for Data Transformation	Con: Offers fewer operators for data transformation, which may limit flexibility in handling complex transformations or combinations of data.	Pro: Provides a rich set of operators for data transformation, combination, filtering, and more, allowing for more elegant and concise solutions.
Backpressure Handling	Con: Does not support backpressure directly, which might be a concern with rapid data updates.	Pro: Supports backpressure, allowing for sophisticated handling of rapid emissions in complex data streams.
Cold Stream vs. Hot Stream	Con: Always hot, meaning it is always emitting data once it has observers, potentially leading to unnecessary data processing.	Pro: Cold streams by default, starting to emit data only when collected, which can improve performance and resource usage.
Asynchronous and Coroutine Support	Con: Requires additional work to integrate with coroutines for asynchronous operations.	Pro: Designed to work seamlessly with Kotlin coroutines, offering straightforward asynchronous data handling and manipulation.
Testing	Con: Testing LiveData with its lifecycle dependencies can be more cumbersome.	Pro: Easier to test due to its coroutine-based nature, especially with tools designed for coroutine testing.
Use Case Suitability	Pro: Simple and effective for straightforward data-binding use cases where lifecycle awareness is crucial.	Pro: More suited for complex data processing, multiple data sources combination, and when using coroutines extensively.

In summary:

As mentioned earlier, you've done excellent work, and we are in a good position to proceed with the current implementation. Should we find it beneficial, we can consider implementing improvements in the future to simplify the process of creating a ViewModelSlice for developers.

[AjeshRPai]

Hey @pantelis 👋🏼 First of all, thanks for the code review and providing your input. You are awesome 🙌🏼

I've noticed that many of the Slices expose multiple LiveData objects. For instance, the AccountDataViewModelSlice exposes isRefreshing and uiModel separately.

I agree, Pantelis. Exposing just one live data point for the current data state is better than multiple. I kept separate live data points for the loading/refreshing state because it becomes harder to update a live data point when there is already data in it, and we want to update that state to indicate that it's refreshing. So, suppose there is data in live data and the screen is refreshed, we will need to copy the current data and update a variable in that state class to indicate that the data is being refreshed. Something like this Check ScanViewModel → UiState is what you have in mind?.

Introducing an abstraction for the ViewModelSlice concept (via an interface or abstract class) could help standardize behaviors common across all slices. Such behaviors might include:

Yep, definitely, this would be a good way to move forward; I played with this idea but didn't pursue it further since there were a lot of changes in this PR already.

Do you think this approach would be a valuable future improvement? If so, I could attempt an implementation during this hack week. Do you foresee any issues that could undermine this idea?

Yep definitely 👍🏼 , I don't see any issues with the idea. I think it would be a valuable improvement.

We're currently using LiveData to merge different UiModels from the slices. I don't anticipate noticeable performance issues given the generally small number of slices. However, my concern lies with handling concurrent updates from two or more slices' UiModels. I haven't had the chance to investigate this, so I'm raising the question in case you have insights. It might be worth exploring, even if it seems like a remote possibility.

Good point, Pantelis. 👍🏼 I haven't seen noticeable performance issues either, and as you said, probably because there aren't many VM slices. To minimize the UI model updates, we have added the distinctUntillChanged so it might be helping as well. I think this issue is worth exploring.

Looking ahead, we might consider migrating from LiveData to Kotlin flows. To clarify, I'm not proposing we make this change immediately. Sticking with LiveData for now seems wise, with a potential shift to flows as a secondary phase if we discern tangible benefits. In theory, flows offer certain advantages over LiveData, as detailed in this article: LiveData vs Flows: Navigating the Currents of Android Data Streams. I've prepared a brief table outlining their pros and cons for future reference.

Thanks, Pantelis, for providing this blog and summarizing the pros and cons. Really helpful 🙌🏼 hands. I think we should move to the kotlin flows moving forward. As you pointed out, it has much better support for concurrent updates and has a rich set of operators for data transformation. The present live data merge logic is something I don't like, and there wasn't any alternative as well(let me know if I am wrong). But I think if we had used kotlin flows, we could have implemented the merging logic better. So yeah, in the future, let's move from live data to kotlin flows 👍🏼

Should we find it beneficial, we can consider implementing improvements in the future to simplify the process of creating a ViewModelSlice for developers.

Definitely 💯

[pantstamp]

Hey @AjeshRPai , this comment is in response to your feedback on unit testing.

I took a look at BlazeCardViewModelSliceTest. Great job as the unit tests provide comprehensive validation of BlazeCardViewModelSlice's functionality, covering state management, decision logic, user interaction handling, and more.

Regarding the @VisibleForTesting annotation, I think it can be seen both positively and negatively, depending on the context and how it's used.

On the pros, it allows unit tests to access and directly test the internal workings of the class. Sometimes, the easiest and fastest way to get something tested is to make it more accessible to the testing code.

On the cons, it breaks the encapsulation principle by exposing internal details of a class. Ideally, we should focus on the public interface to ensure that internal changes don't break the tests. Also, it might indicate a design smell. Needing to use @VisibleForTesting might give us a hint that the class is doing too much. And, of course, there's a risk it could be misused by other classes, not just test code.

I am sure that you already know all of this, so my opinion is that before using @VisibleForTesting, we should think of the trade-offs case by case.

For example, for BlazeCardViewModelSliceTest, one possible solution might be to extract a factory class for the BlazeCardParams and inject it into BlazeCardViewModelSlice.

class BlazeCardParamsFactory {
    fun getParams(campaign: BlazeCampaignModel? = null): BlazeCardBuilderParams {
        return campaign?.let {
            // Existing logic to create CampaignWithBlazeCardBuilderParams
        } ?: run {
            // Existing logic to create PromoteWithBlazeCardBuilderParams
        }
    }
}

In this way, we could test this logic separately and mock it for the BlazeCardViewModelSliceTest.

To sum up, what I suggest is not to remove all usages of @VisibleForTesting right away, but instead, each time we see that we have to use it, just spend some time to think if a small refactoring might improve our design per case.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 63.30435% with 422 lines in your changes are missing coverage. Please review.

Project coverage is 40.34%. Comparing base (2fcbb61) to head (8120e49). Report is 162 commits behind head on trunk.

:exclamation: Current head 8120e49 differs from pull request most recent head cbee896. Consider uploading reports for the commit cbee896 to get more accurate results

Files	Patch %	Lines
...id/ui/mysite/cards/DashboardCardsViewModelSlice.kt	58.57%	66 Missing and 4 partials :warning:
...rds/quicklinksitem/QuickLinksItemViewModelSlice.kt	0.00%	53 Missing :warning:
...n/java/org/wordpress/android/util/LiveDataUtils.kt	36.98%	46 Missing :warning:
...loggingprompts/BloggingPromptCardViewModelSlice.kt	24.52%	40 Missing :warning:
...id/ui/mysite/cards/dashboard/CardViewModelSlice.kt	72.91%	23 Missing and 16 partials :warning:
...e/cards/quickstart/QuickStartCardViewModelSlice.kt	58.90%	23 Missing and 7 partials :warning:
...id/ui/mysite/items/DashboardItemsViewModelSlice.kt	68.00%	14 Missing and 10 partials :warning:
...org/wordpress/android/ui/mysite/MySiteViewModel.kt	78.08%	12 Missing and 4 partials :warning:
...gistration/DomainRegistrationCardViewModelSlice.kt	76.66%	11 Missing and 3 partials :warning:
.../ui/mysite/cards/dashboard/plans/PlansCardUtils.kt	0.00%	13 Missing :warning:
... and 22 more

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #19869      +/-   ##
==========================================
- Coverage   40.44%   40.34%   -0.10%     
==========================================
  Files        1462     1459       -3     
  Lines       67241    67387     +146     
  Branches    11176    11164      -12     
==========================================
- Hits        27193    27185       -8     
- Misses      37573    37743     +170     
+ Partials     2475     2459      -16     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[AjeshRPai]

Hey @pantstamp 👋🏼 Once again, thanks for reviewing and sharing your thoughts on the visible for testing annotation

For example, for BlazeCardViewModelSliceTest, one possible solution might be to extract a factory class for the BlazeCardParams and inject it into BlazeCardViewModelSlice.

I am unsure if I understand this correctly, Pantelis, So if we extract a factory class for the params, the click functions in the param builder will be in the factory class or the VM Slice.

AFAIK, we introduced the visible for testing to test the click functions we pass inside the cards, such as more click or hide click. IMO, the click functions and logic should reside in the VM slice.

💡 Idea: While checking the vm slice, builder param, and visibleForTesting logic, I thought maybe we had to take this approach because we pass the click functions inside the params and to the data model for the view. This makes it harder to test the click functions as they are private. Also, the params data class and the data class of the card are huge because the click functions are a member variable of both. I am unsure about the performance implications of passing functions as a variable in the view data class where a diff runs on every rendering; maybe it's negligent, but I'm wondering whether it's a good practice. In the future, we should not pass the functions and only data. On click of the view in the recycler view, a call back should happen only to the parent VM(with some identifier for the card and the click type), and then this VM will call the child VM slices, this way,

• we will have more public interfaces in VM Slice to test in isolation
• will reduce the @visibleFoTesting as these click functions need not be passed in card data classes
• will reduce the card data class size
• will reduce the params data class size

Let me know what you think.

[antonis]

Awesome work team 🏅 I had a quick look at the parts that involve the Dynamic Dashboard cards and did not detect any issues.

I also tested by creating cards in my sandbox in various scenarios and appeared as expected.

Top card	Bottom card	Two cards

I will try to iterate with more feedback the following days 🙇

[AjeshRPai]

Hey @thomashorta I am adding you as a reviewer to this PR. Can you please review the following cards and related classes?

1. BloggingPromptCard
2. State of the word card
3. Jetpack install full plugin card
4. JpMigrationSuccessCard

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.

The Bloganuary card implementation will be removed in a future PR as the code changes in this PR are already many lines.

Please feel free to review any other code or architecture changes and give any feedback you have.

Thanks in advance. 🙌🏼

[AjeshRPai]

Hey @irfano I am adding you as a reviewer to this PR. Can you please review the following cards and related classes?

1. Plans Card
2. Today Stats Card

Please feel free to review any other code or architecture changes and give any feedback you have.

Thanks in advance. 🙌🏼

[AjeshRPai]

Hey @irfano Thanks for reviewing the code changes. 🙌🏼

"Register Domain" and "Prompts" cards are no longer visible.

I have fixed this issue with commit https://github.com/wordpress-mobile/WordPress-Android/pull/19869/commits/8f5322676f6c8c52c3562bcc46fb85e3f2430bfa and https://github.com/wordpress-mobile/WordPress-Android/pull/19869/commits/16fa6dd858e28b56297f3d3ce728e87205e22442

The card order was 1.Blaze 2.Free domain with annual plan 3.Stats 4.Draft post 5.Pages 6.Recent activity, but now it is 1.Blaze 2.Stats 3.Pages 4.Draft post 5.Recent activity 6.Free domain with annual plan. There are two unexpected changes: "the plans card moved to the end", and "the pages and draft post cards swapped places".

Fixed this order with https://github.com/wordpress-mobile/WordPress-Android/pull/19869/commits/20329d7bee65960091a959c4775e932574f44a86 and commit 79ffbb97a91b6e79cc58fc6bdb90897cd5534c53

🟡 My site icon has turned black. Could it have been caused by his PR?

I don't think this issue is caused due to the changes in this PR, I will add this issue to the known issues for now and fix it if necessary

Can you re-review the PR once you have bandwidth? 🤔 Thanks in Advance

[pantstamp]

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 (e634f02ea375f893696d7511fef886090e4e9cf4). 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.

[antonis]

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 https://github.com/wordpress-mobile/WordPress-Android/pull/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]

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

[AjeshRPai]

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.

https://github.com/wordpress-mobile/WordPress-Android/pull/19869#issuecomment-1999052649

[sonarqubecloud[bot]]

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]

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.

[RenanLukas]

@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.

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