Jetpack Section: Backup Screen - Introduce Backup Screen by ParaskP7 · Pull Request #13762 · wordpress-mobile/WordPress-Android

ParaskP7 · 2021-01-13T15:36:08Z

This PR introduces the Backups screen, which is actually an Activity Log screen in disguise. As such, this PR reused the Activity Log screen to show the Backups screen. The below list of items change when launching an the Activity Log for the Backups screen:

The screen title changes to Backup.
The Activity Type chip is being hidden.
The list of activities only contain Rewindable items.
The empty screen state without filter changes to Your first backup will be ready soon.
The empty screen state with filter changes to No matching backups found.

To test:

Launch app and go to My Site tab.
Go to My Site -> Toolbar Avatar -> App Settings -> Test feature configuration.
Enable BackupsFeatureConfig, scroll down and click the RESTART THE APP button. DO NOT confuse this feature flag with the BackupFeatureConfig (singular). Make sure to enable the right feature flag.
Go back to My Site tab and notice Backup menu is shown.
Select the Activity menu and verify UI correctness (see above) and that all features work as expected. This is includes but is not limited to:
- Scrolling infinitely, from top to bottom and vice-versa.
- Triggering the Date Range and Activity Type filters.
- Selecting an activity and navigating to its detail page.
- Triggering Restore to this point and Download backup from the menu item.
- While restoring to a point observe the progress item.
- Offline support.
- Configuration changes (rotation, language, etc).
Select the Backup menu and verify UI correctness (see above) and that all features work as expected. This is includes but is not limited to:
- Scrolling infinitely, from top to bottom and vice-versa. ⚠️ - Scrolling is not optimized yet, try an break it... more on that later
- Triggering the Date Range filter.
- Selecting an activity and navigating to its detail page.
- Triggering Restore to this point and Download backup from the menu item.
- While restoring to a point observe the progress item.
- Offline support.
- Configuration changes (rotation, language, etc).

This solution is based on the investigation notes here. Below is a summary of the decisions that was made based on that:

Filter the list of activities after storing them in db. The necessary API enhancement is not available yet and as such filtering it before would create unnecessary overhead and problems later on.
Do not enhance the scrolling behavior since there will be anyway cases where filtering after storing in db approach chosen above would decrease user experience (ie. 1 rewindable item per 20 items per page) or even worse break it altogether (ie. 0 rewindable items per 20 items per page). As such, the API enhancement is unavoidable and it is already being discussed.
Hide the Activity Type filter since the API for this filter is only available for all items. An intermediate solution is still viable, but it has been decided to just hide this filter for now.
Reuse Activity Log screen since by not doing so this will create lots of duplicate code.

Future:

Add tracking.
Polish UI.
Accessibility.
Design review.

PR submission checklist:

I have considered adding unit tests where possible.
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.

This 'FluxC' hash updates 'FluxC' to that 'branch' version where 'ActivityLogStore.getActivityLogForSite(...)' utilises the 'rewindableOnly' to retrieve only the rewindable activities from the database. This step is required in order to build the 'Backups' screen. During code review and before merging to 'develop' this hash will be replaced with the appropriate tag.

This commit adds a new launcher method, the 'viewBackupList', which launches an 'Activity Log' screen, just like the 'viewActivityLogList(...)'. The only difference is the additional extra boolean, the 'ACTIVITY_LOG_REWINDABLE_ONLY_KEY', which is added prior to launching the screen and will become the differentiator between presenting the 'Activity Log' or 'Backups' screen.

If the rewindable only flag is present and true then the title of the activity becomes 'Backup', else it remains as 'Activity Log'. This way both screens are visually differentiated by title.

If the rewindable only flag is present and true then the 'Activity Type' chip is hidden, else it remains visible. As such, on the 'Activity Log' screen the 'Activity Type' chip and filtering capability will be available. But, on the 'Backups' screen the same 'Activity Type' chip and filtering capability will not be available. This way both screens are visually differentiated by 'Activity Type'. Note: This commit also update the synthetic import, from 'toolbar_main' to 'activity_log_list_activity'. It seems that the previous import was incorrect.

This commit adds the following: 1) Parses the 'ACTIVITY_LOG_REWINDABLE_ONLY_KEY' as passed by the 'Activity', and 2) Saves the 'ACTIVITY_LOG_REWINDABLE_ONLY_KEY' during 'onSaveInstanceState' for configuration change purposes. 3) Passes the rewindable only flag to the fragment's 'ViewModel'.

As part of this commit the below occurred: 1) The 'rewindableOnly' flag was added on the test suite. But, no additional tests or updates occurred on the test suite itself. This is by design since the store is mocked. 2) An enhancement to the test suite's 'initializeActivityList()' function was made to replicate a list that contains both rewindable, and non-rewindable items.

These tests will fail as the implementation is not ready yet. The next commit will make the tests pass (TDD).

This commit makes the previous failing test pass (TDD).

This will then match the way 'EmptyUiState.Backup' level is structured and increase readability while using this sealed class.

This behavior was not intended and has been confirmed with design that it should be removed. Also, on the iOS platform, this extra behavior was never implemented. As such, this commit also increases feature parity between the two platforms.

Also, as part of this extraction a accompanying Javadoc was added to explain the reasoning for such code branching and for preventing any further encouragement to do the same in the future (should the need arises and the complexity needs to further increase).

This Javadoc tries to do two things: 1) Explain the reasoning for such code branching, and 2) Prevent any further encouragement to do the same in the future (should the need arises and the complexity needs to further increase).

This 'FluxC' tag updates FluxC to that 'develop' version where 'ActivityLogStore.getActivityLogForSite(...)' utilises the 'rewindableOnly' to retrieve only the rewindable activities from the database. This step is required in order to build the 'Backups' screen.

peril-wordpress-mobile · 2021-01-13T15:36:20Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-01-13T15:45:59Z

You can test the changes on this Pull Request by downloading the APK here.

malinajirka

Really great job @ParaskP7! The differences between Activity Log and Backup Screen are clearly separated ❤️.

Enable BackupsFeatureConfig, scroll down and click the RESTART THE APP button. DO NOT confuse this feature flag with the BackupFeatureConfig (singular). Make sure to enable the right feature flag.

Tbh I didn't realize these two flags are so similar. I think it might be worth renaming them (eg. BackupScreenFeatureFlag and BackupDownloadFeatureFlag). It'd be quite unfortunate if there was a bug in production and we disabled the wrong flag. Wdyt?

WordPress/src/main/java/org/wordpress/android/ui/activitylog/list/ActivityLogListFragment.kt

WordPress/src/main/res/values/strings.xml

ParaskP7 · 2021-01-14T08:44:46Z

Tbh I didn't realize these two flags are so similar. I think it might be worth renaming them (eg. BackupScreenFeatureFlag and BackupDownloadFeatureFlag). It'd be quite unfortunate if there was a bug in production and we disabled the wrong flag. Wdyt?

I agree! 🌟

I'll create a PR after this PR is merged which will do exactly that. @zwarm are you okay Jirka's suggestion?

The system recreates the activity, as such the extras will still be available in activity's intent. As such, retrieving the 'rewindableOnly' flag from there is a much better and robust alternative. Note: The 'SiteModel' is a bit different since it can change. Ideally only the 'siteId' would be stored to avoid that as well. As such, those two cases are not comparable.

This is a much better alternative with less boilerplate code.

malinajirka · 2021-01-14T14:29:31Z

Feel free to merge this when the conflict is resolved ;)!

…f github.com:wordpress-mobile/WordPress-Android into issue/13629-introduce-backups-screen � Conflicts: � WordPress/src/main/java/org/wordpress/android/ui/ActivityLauncher.java � WordPress/src/main/java/org/wordpress/android/ui/activitylog/list/ActivityLogListFragment.kt � WordPress/src/test/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModelTest.kt � build.gradle

ParaskP7 · 2021-01-15T14:29:51Z

I'll create a PR after this PR is merged which will do exactly that. @zwarm are you okay Jirka's suggestion?

FYI: This PR is now ready @malinajirka : #13785

ParaskP7 added 19 commits January 13, 2021 16:26

Refactor: Remove unnecessary non-null assertion from my site fragment

2dad830

Feature: Change activity log title based on the rewindable only flag

24aa915

If the rewindable only flag is present and true then the title of the activity becomes 'Backup', else it remains as 'Activity Log'. This way both screens are visually differentiated by title.

Refactor: Move the is started view model flag closer to if/return logic

568c441

Feature: Add backup related empty ui states for activity log view model

e336e42

Test: Add unit tests for backup related empty ui states

4b8911f

These tests will fail as the implementation is not ready yet. The next commit will make the tests pass (TDD).

Feature: Implement backup related empty ui states

3e847c6

This commit makes the previous failing test pass (TDD).

Refactor: Extract refresh empty ui state function

8a0d6a8

Refactor: Extract refresh backup/activity log empty ui state functions

b3b88d1

Refactor: Add activity new log level to empty ui state sealed class

c72990f

This will then match the way 'EmptyUiState.Backup' level is structured and increase readability while using this sealed class.

Test: Optimize imports for assertions on activity log view model test

4eda186

Build: Update fluxc tag

9d8aa71

This 'FluxC' tag updates FluxC to that 'develop' version where 'ActivityLogStore.getActivityLogForSite(...)' utilises the 'rewindableOnly' to retrieve only the rewindable activities from the database. This step is required in order to build the 'Backups' screen.

ParaskP7 added [Type] Task Jetpack labels Jan 13, 2021

ParaskP7 added this to the 16.6 milestone Jan 13, 2021

ParaskP7 requested a review from malinajirka January 13, 2021 15:36

ParaskP7 self-assigned this Jan 13, 2021

ParaskP7 mentioned this pull request Jan 13, 2021

Jetpack Section: Backup Screen #13629

Closed

40 tasks

malinajirka approved these changes Jan 14, 2021

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/activitylog/list/ActivityLogListFragment.kt Outdated Show resolved Hide resolved

WordPress/src/main/res/values/strings.xml Show resolved Hide resolved

Refactor: Update retrieval of the rewindable only flag for activity

4594b31

This is a much better alternative with less boilerplate code.

malinajirka approved these changes Jan 14, 2021

View reviewed changes

ParaskP7 merged commit b18c11a into develop Jan 14, 2021

ParaskP7 deleted the issue/13629-introduce-backups-screen branch January 14, 2021 15:18

ParaskP7 changed the title ~~Jetpack Mobile: Backups - Introduce Backups Screen~~ Jetpack Mobile: Backup Screen - Introduce Backup Screen Jan 15, 2021

ParaskP7 changed the title ~~Jetpack Mobile: Backup Screen - Introduce Backup Screen~~ Jetpack Section: Backup Screen - Introduce Backup Screen Jan 28, 2021

ParaskP7 added the Jetpack Mobile label Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetpack Section: Backup Screen - Introduce Backup Screen#13762

Jetpack Section: Backup Screen - Introduce Backup Screen#13762
ParaskP7 merged 22 commits intodevelopfrom
issue/13629-introduce-backups-screen

ParaskP7 commented Jan 13, 2021 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Jan 13, 2021 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Jan 13, 2021 •

edited

Loading

Uh oh!

malinajirka left a comment

Uh oh!

Uh oh!

Uh oh!

ParaskP7 commented Jan 14, 2021

Uh oh!

malinajirka commented Jan 14, 2021

Uh oh!

ParaskP7 commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ParaskP7 commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ParaskP7 commented Jan 14, 2021

Uh oh!

malinajirka commented Jan 14, 2021

Uh oh!

ParaskP7 commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ParaskP7 commented Jan 13, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 13, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 13, 2021 •

edited

Loading