Fixes: site monitor prevent webview reload #20077

AjeshRPai · 2024-01-30T09:56:18Z

Fixes

#20067

Description

This PR fixes the issue of reloading of the webviews in site monitoring activity when the tabs are selected again.

Screen_recording_20240131_151636.webm

Pending issues

[Fixed] On Loading of the Webview first time - there is a glitch happening (not sure if its because of the emulator or something else)

To Test:

Prerequisite

◐ Toggle the site_monitoring flag

Go to Me → App Settings → Debug settings → Remote Feature Flags
Tap on the item corresponding to the flag - site_monitoring
Tap RESTART THE APP button

Site monitor webviews are not re-loaded when selected again

Go to More → Site Monitoring
✅ Verify that the logs include tracking 🔵 Tracked: site_monitoring_screen_shown
✅ Verify that for each tab - Metrics, PHP logs and Web Server logs, the webview loads from the network first time
✅ Verify that on changing tab, the logs include tracking - `🔵 Tracked: site_monitoring_tab_shown, Properties: {"tab":"php_logs"}
✅ Verify that when the tabs are selected again, the webviews are not reloaded
`

Regression Notes

Potential unintended areas of impact

Site monitor webview screens are loaded on selecting tabs after the first time
Site monitor screens are not loaded in the first time

What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
N/A

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)

dangermattic · 2024-01-30T09:57:06Z

	1 Warning
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

wpmobilebot · 2024-01-30T10:16:06Z

📲 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	pr20077-82d7b92
	Commit	`82d7b92`
	Direct Download	`wordpress-prototype-build-pr20077-82d7b92.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-01-30T10:16:55Z

📲 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	pr20077-82d7b92
	Commit	`82d7b92`
	Direct Download	`jetpack-prototype-build-pr20077-82d7b92.apk`

Note: Google Login is not supported on these builds.

…load # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/sitemonitor/SiteMonitorParentActivity.kt # WordPress/src/main/java/org/wordpress/android/ui/sitemonitor/SiteMonitorTabViewModel.kt

codecov · 2024-01-31T10:24:57Z

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (69fe5f0) 40.16% compared to head (6a7a13d) 40.26%.
Report is 2 commits behind head on trunk.

❗ Current head 6a7a13d differs from pull request most recent head b697e79. Consider uploading reports for the commit b697e79 to get more accurate results

Files	Patch %	Lines
...droid/ui/sitemonitor/SiteMonitorParentViewModel.kt	0.00%	34 Missing ⚠️
...oid/ui/sitemonitor/SiteMonitorTabViewModelSlice.kt	0.00%	11 Missing ⚠️
...android/ui/sitemonitor/SiteMonitorWebViewClient.kt	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #20077      +/-   ##
==========================================
+ Coverage   40.16%   40.26%   +0.10%     
==========================================
  Files        1457     1455       -2     
  Lines       66980    66950      -30     
  Branches    11065    11051      -14     
==========================================
+ Hits        26904    26959      +55     
+ Misses      37587    37501      -86     
- Partials     2489     2490       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pantstamp · 2024-01-31T10:47:31Z

WordPress/src/main/java/org/wordpress/android/ui/sitemonitor/SiteMonitorParentViewModel.kt

+        webServerViewModel.start(SiteMonitorType.WEB_SERVER_LOGS, SiteMonitorTabItem.WebServerLogs.urlTemplate, site)
+    }
+
+    fun getUiState(siteMonitorType: SiteMonitorType): MutableState<SiteMonitorUiState> {


Would it be better to return here State instead of MutableState? I guess the consume is not going to modify the received state. WDYT @AjeshRPai ?

Nice catch @pantstamp, You are right. Its better to return state here. Done in f6633cf

pantstamp · 2024-01-31T10:57:49Z

WordPress/src/main/java/org/wordpress/android/ui/sitemonitor/SiteMonitorTabViewModel.kt

-    private val _uiState = MutableStateFlow<SiteMonitorUiState>(SiteMonitorUiState.Preparing)
-    val uiState: StateFlow<SiteMonitorUiState> = _uiState
+    private val _uiState = mutableStateOf<SiteMonitorUiState>(SiteMonitorUiState.Preparing)
+    val uiState = _uiState


Is there a reason why we do not return here state instead of mutable state like this?
val uiState: State<SiteMonitorUiState> = _uiState

You are right @pantstamp , I have updated the variable type as suggested in this commit fb3c6b9

pantstamp · 2024-01-31T11:34:30Z

Great work @AjeshRPai ! I really like the fact that you got rid of the fragment. I will take a look at the glitch to see what I can find.

pantstamp · 2024-01-31T11:40:49Z

@AjeshRPai the first time that we launch the site monitoring screen, I can see the Tracked: site_monitoring_screen_shown but not the Tracked: site_monitoring_tab_shown, Properties: {"tab":"metrics"} even though the metrics tab is shown.

Is this what we want? I think that we should track this event even the first time.

zwarm

@AjeshRPai - Thanks for finding a solution to the reload issue. 👏
With the exception of the few comment @pantstamp left, I am ready to give this a 👍 . In addition, I added a fix for the glitch. Please let me know what you think.

zwarm · 2024-01-31T13:21:59Z

@AjeshRPai the first time that we launch the site monitoring screen, I can see the Tracked: site_monitoring_screen_shown but not the Tracked: site_monitoring_tab_shown, Properties: {"tab":"metrics"} even though the metrics tab is shown.

Is this what we want? I think that we should track this event even the first time.

@AjeshRPai @pantstamp - 🤔 We already capture Tracked: more_menu_item_tapped, Properties: {"item":"site_monitoring"}, so let's make stick with including "tab" property whenever we track site_monitoring_tab_shown

…Utils

peril-wordpress-mobile · 2024-01-31T16:01:46Z

	Warnings
⚠️	PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

AjeshRPai added 6 commits January 30, 2024 15:02

+ Adds: model to loaded state

964fd10

↑ Updates: the webview client to use tab type

ae9ca81

+ Adds: SiteMonitorParentViewModel

236b450

↑ Updates: the logic in activity to use tabs

586e3ed

↑ Updates: the site monitor tab view model to use state flow

84d25bf

↑ Updates: the fragment to use loaded state

d73c942

AjeshRPai linked an issue Jan 30, 2024 that may be closed by this pull request

Site Monitor: Prevent webview reload #20067

Closed

AjeshRPai requested review from pantstamp and zwarm January 30, 2024 09:56

AjeshRPai changed the title ~~Fixes: site monitor prevent webview reload~~ WIP Fixes: site monitor prevent webview reload Jan 30, 2024

AjeshRPai added 12 commits January 31, 2024 11:46

↑ Updates: the logic to use three web views for each tab

47471c7

+ Adds: top bar view to the activity

5ada1fa

Merge branch 'trunk' into issue/20067-site-monitor-prevent-webview-re…

24d8aa9

…load # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/sitemonitor/SiteMonitorParentActivity.kt # WordPress/src/main/java/org/wordpress/android/ui/sitemonitor/SiteMonitorTabViewModel.kt

+ Adds: the logic to handle initial tab logic

554b42d

* Fixes: the incorrect passing of modifier and formatting

5444240

* Fixes: Incorrect passing of modifier instance

32877e7

* Fixes: Incorrect tab tracking and activity tracking

d532763

- Removes: Redundant logs from SiteMonitorWebViewClient

89f9895

→ Moves: Site Monitor Tab header logic to activity

7bf963a

- Removes: Site Monitor Fragment and Fragment container

dc1a6ba

- Removes: Redundant SiteMonitorTabNavigation.kt

8a6984a

* Reverts: the change for loaded state object as its no longer needed

adf4177

AjeshRPai added [Type] Task unit-tests-exemption labels Jan 31, 2024

* Renames: SiteMonitorTabViewModel.kt to SiteMonitorTabViewModelSlice

da6711c

AjeshRPai added this to the 24.2 milestone Jan 31, 2024

AjeshRPai marked this pull request as ready for review January 31, 2024 10:22

pantstamp reviewed Jan 31, 2024

View reviewed changes

AjeshRPai added 2 commits January 31, 2024 16:20

↑ Updates: the state returned from mutable to immutable

f6633cf

* Fixes: data not loaded in orientation change

cfb9132

pantstamp reviewed Jan 31, 2024

View reviewed changes

Refactor: wrap the content in it's own column to prevent tab jumping

8aa801b

zwarm approved these changes Jan 31, 2024

View reviewed changes

This was referenced Jan 31, 2024

[WIP] Restore state of webview #20084

Closed

Site Monitor: Refactors and Unit Tests #20083

Closed

zwarm and others added 4 commits January 31, 2024 08:41

Add unit tests for Site Monitor mapper and utils

c53f6ff

Add tracking for initial site monitor tab

6a7a13d

Add tracking for site monitor webview error

b697e79

Add capitalisation for tab names

5479d36

pantstamp approved these changes Jan 31, 2024

View reviewed changes

zwarm and others added 2 commits January 31, 2024 09:57

Add unit tests for Site Monitor web view client

29fa1f5

* Updates: the _uiState to use immutable state variable

fb3c6b9

AjeshRPai changed the title ~~WIP Fixes: site monitor prevent webview reload~~ Fixes: site monitor prevent webview reload Jan 31, 2024

zwarm added 3 commits January 31, 2024 11:01

Refactor: replace direct access to analytics tracker with siteMonitor…

6f0c48e

…Utils

Add unit tests for SiteMonitorParentViewModel

1cf5193

Fix detekt issue

c21915f

Add unit tests for SiteMonitorTabViewModelSlice

e8da1d8

zwarm enabled auto-merge January 31, 2024 17:16

Fix checkstyle empty line

82d7b92

zwarm merged commit aadadca into trunk Jan 31, 2024
19 of 20 checks passed

zwarm deleted the issue/20067-site-monitor-prevent-webview-reload branch January 31, 2024 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes: site monitor prevent webview reload #20077

Fixes: site monitor prevent webview reload #20077

AjeshRPai commented Jan 30, 2024 •

edited by zwarm

Loading

dangermattic commented Jan 30, 2024 •

edited

Loading

wpmobilebot commented Jan 30, 2024 •

edited

Loading

wpmobilebot commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

pantstamp Jan 31, 2024

AjeshRPai Jan 31, 2024

pantstamp Jan 31, 2024

AjeshRPai Jan 31, 2024

pantstamp commented Jan 31, 2024

pantstamp commented Jan 31, 2024

zwarm left a comment

zwarm commented Jan 31, 2024

peril-wordpress-mobile bot commented Jan 31, 2024 •

edited

Loading

Fixes: site monitor prevent webview reload #20077

Fixes: site monitor prevent webview reload #20077

Conversation

AjeshRPai commented Jan 30, 2024 • edited by zwarm Loading

Fixes

Description

Pending issues

To Test:

Prerequisite

Site monitor webviews are not re-loaded when selected again

Regression Notes

dangermattic commented Jan 30, 2024 • edited Loading

wpmobilebot commented Jan 30, 2024 • edited Loading

wpmobilebot commented Jan 30, 2024 • edited Loading

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

pantstamp Jan 31, 2024

Choose a reason for hiding this comment

AjeshRPai Jan 31, 2024

Choose a reason for hiding this comment

pantstamp Jan 31, 2024

Choose a reason for hiding this comment

AjeshRPai Jan 31, 2024

Choose a reason for hiding this comment

pantstamp commented Jan 31, 2024

pantstamp commented Jan 31, 2024

zwarm left a comment

Choose a reason for hiding this comment

zwarm commented Jan 31, 2024

peril-wordpress-mobile bot commented Jan 31, 2024 • edited Loading

AjeshRPai commented Jan 30, 2024 •

edited by zwarm

Loading

dangermattic commented Jan 30, 2024 •

edited

Loading

wpmobilebot commented Jan 30, 2024 •

edited

Loading

wpmobilebot commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

peril-wordpress-mobile bot commented Jan 31, 2024 •

edited

Loading