Skip to content

Issue/13268 data filtering#13647

Merged
malinajirka merged 12 commits intodevelopfrom
issue/13268-data-filtering
Dec 23, 2020
Merged

Issue/13268 data filtering#13647
malinajirka merged 12 commits intodevelopfrom
issue/13268-data-filtering

Conversation

@malinajirka
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka commented Dec 21, 2020

Parent issue #13268

Merge instructions

  1. Make sure Issue/13268 add clear cache method WordPress-FluxC-Android#1815 is merged
  2. Replace FluxC hash with a tag
  3. Remove "Not ready for merge" label
  4. Merge this PR
  • e40023d - start passing currently set filters to the fetchActivities request
  • 82b49ca - renames parameter name from isLoadingMore to loadMore. Since the parameter says that the caller wants to load more items, not that loading more items is in progress.
  • 01fa658 - previously the app would ignore requests to fetch data when there was an ongoing request. This commit changes this behavior so that any previous request gets canceled. The only exception to this behavior is when the app is loading more items and the caller wants to load more items again. (*)
  • 6fe25a2 - fixes a bug where the app wouldn't refresh the content when no items were found for the currently selected filters. I reverted this fix in cc39f74 and fixed it on the FluxC's side in here (*)
  • 35430ab - clears cache when the user leaves the screen and the filters are not empty. Since filters are not retained across sessions, the data in cache need to be cleared so they are not displayed the next time user accesses this screen. This is a naive implementation but I think it is good enough for now. We considered implementing a more robust solution which would allow us to save different data sets (based on the filters) into the cache, but decided against it for now (it's not worth the effort).

Known issues which will be fixed in upcoming PRs

  1. When there are no data for a certain filter combination, "No Activity Yet" empty screen is shown. This will be fix in an upcoming PR.
  2. When a date range with no data available is selected and the user opens Activity Type filter, an empty list is shown. This will be fix in an upcoming PR.
  3. The filters are accessible even on sites which don't support filtering. This will be fix in an upcoming PR.
  4. Filters are shown for all site types, however, they don't work everywhere. I'm not 100% sure yet, what are the conditions. But I tested it on an atomic site and a self-hosted site with Jetpack Premium plan and it worked there. Use one of these site types for the tests

To test:

Test filters 1

  • Enable ActivityLogFiltersFeatureConfig
  1. Open Activity Log
  2. Verify data get loaded
  3. Set date range to today
  4. Verify data get filtered
  5. Set an Activity Type filter
  6. Verify data get filtered
  7. Clear Date range filter
  8. Notice data get refreshed and verify only data for activity type filter are shown
  9. Continue playing with the filters and verify everything works

Test filters 2

  • Enable ActivityLogFiltersFeatureConfig
  1. Open Activity Log
  2. Verify data get loaded
  3. Set date range filter 4 months ago (any date range for which there aren't any activities available)
  4. Notice an empty screen is shown - before 6fe25a2 the app would keep showing the previous content

Test Clear cache action

  • Enable ActivityLogFiltersFeatureConfig
  1. Open Activity Log
  2. Wait until the data are shown
  3. Click on back
  4. Turn on Airplane mode
  5. Open Activity Log - verify cached data are shown (the cache didn't get cleared)
  6. Turn off airplane mode
  7. Set an Activity Type filter
  8. Wait until the data get filtered
  9. Click on back
  10. Turn on airplane mode
  11. Open Activity log - verify no data are shown ( the cache got cleared)

Test cancelation of an ongoing request

  • Enable ActivityLogFiltersFeatureConfig
  1. Throttle your network speed/quality (or use a proxy and pause the request)
  2. Open Activity Log
  3. Pull to refresh and really quickly open the date range picker, select a date and click on Save
  4. Verify data for the correct date range are shown (the request didn't get canceled because of the ongoing "load more" request)
  5. Clear the filter
  6. Scroll to bottom and notice the app is loading more items -> before the request completes scroll a bit to top (few pixels) and scroll to bottom again -> keep doing this until more data get loaded (the load more request doesn't get cancelled even)

  • Disable ActivityLogFiltersFeatureConfig and make sure the activity log still works as expected. Commits marked with * affect even the activity log without filters and these changes will go live with the next version of the app.

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.

@malinajirka malinajirka added this to the 16.5 milestone Dec 21, 2020
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Dec 21, 2020

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

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Dec 21, 2020

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

@malinajirka malinajirka requested a review from ashiagr December 21, 2020 17:18
@malinajirka malinajirka marked this pull request as ready for review December 21, 2020 17:18
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clear instructions @malinajirka! I tested the scenarios (after changing fluxc version to the latest commit hash from the linked fluxc PR) and mostly everything worked as described:

Test filters 2 ✅
Test clear cache action ✅
Test cancelation of an ongoing request ✅
Disable ActivityLogFiltersFeatureConfig and make sure the activity log still works ✅

Good job!

Few observations when playing with the filters in "Test filters 1":

  1. Selecting same day in the start and end date doesn't seem to filter the logs correctly:

    Dec 21-22 -> log appears
    Dec 21 -> empty screen
    Dec 22 -> empty screen

    Log that appeared in Dec 21-22 should appear either in Dec 21 or Dec 22 but empty screen is shown for both the dates.
    Video

  2. "Activity type name shown on the chip is not the same as the one selected, is that expected?
    E.g. Choosing "Backup and Restore" displays "rewind"
    Video

  3. Different no. of logs appear on the web and mobile for the same date range (timezone effect?):
    Web: https://cldup.com/sxBP-XAmkH.png
    Mobile: https://cldup.com/oWowe3YDDd.png

Used below sites for testing:
1, 2: pressable-jetpack-daily-backup
3: pressable-jetpack-complete

@malinajirka
Copy link
Copy Markdown
Contributor Author

Thanks so much for the thorough review @ashiagr!

Issues 1 and 3 are both related to the timezone issues. Fixes are coming soon in another PRs.

Issue 2 has been fixed in 28ad91f.

It's ready for another round, thank you!

@malinajirka malinajirka requested a review from ashiagr December 22, 2020 12:58
@malinajirka malinajirka mentioned this pull request Dec 22, 2020
3 tasks
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍. Thank you!

@malinajirka malinajirka merged commit c5fbb7c into develop Dec 23, 2020
@malinajirka malinajirka deleted the issue/13268-data-filtering branch December 23, 2020 11:13
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.

2 participants