WordPress-FluxC-Android icon indicating copy to clipboard operation
WordPress-FluxC-Android copied to clipboard

Querying Activity Log events returns only a subset of Site's events

Open malinajirka opened this issue 4 years ago • 1 comments

Current behavior

This is a pretty tricky issue. Sometimes Activity Log screen in WPAndroid displays only a subset of available events for the site. The app fetches first 100 events from the server, however, sometimes the app displays only eg. 4 events even though the server response contained 100 events.

What we found

  1. The server returns the correct set of data so this is a bug in the WPAndroid app (FluxC)
  2. All fetched data are correctly passed to insertOrUpdateActivities
  3. ActivityLogTable contains events with identical REMOTE_SITE_ID but different LOCAL_SITE_ID. Only one of those LOCAL_SITE_ID is present in SiteModelTable.
  4. Even items which are referencing the invalid/out-dated LOCAL_SITE_ID are stored in activitiesToUpdate.
  5. updateQueries filters out entries with invalid LOCAL_SITE_ID which leads to this issue.

Example

Imagine your local database looks like the below table and "fetchActivities" returns activities with activity_id 1-8.

This line returns events with activity_id 1-7. Event with activity_id = 8 is inserted into database on https://github.com/wordpress-mobile/WordPress-FluxC-Android/blob/7ba5682b47cfd0a0bcb43aed045d5b0d4c9a984f/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/ActivityLogSqlUtils.kt#L36 as it doesn't exist in the local db. Items with activity_id 1-7 should be inserted on this line but only item with activity_id = 7 gets actually inserted/updated since the rest doesn't match this condition. The UI will show only activity_id 7-8 as the remaining items are referencing invalid local_site_id.

SiteModelTable
id (aka local site id) = 10 siteId(aka remote site id) = 101010
ActivityLogTable
local_site_id = 7 remote_site_id = 101010 activity_id = 1
local_site_id = 7 remote_site_id = 101010 activity_id = 2
local_site_id = 7 remote_site_id = 101010 activity_id = 3
local_site_id = 7 remote_site_id = 101010 activity_id = 4
local_site_id = 7 remote_site_id = 101010 activity_id = 5
local_site_id = 7 remote_site_id = 101010 activity_id = 6
local_site_id = 10 remote_site_id = 101010 activity_id = 7

We are not sure how to get into the state where the ActivityLogTable contains items with an invalid local_site_id (or not invalid, but referencing an id which doesn't exist anymore in the SiteModel table).

Fix proposals: Since we are not able to get the app into this invalid state again, the following proposals might be not be the best possible solutions. However, we have verified that at least fix proposal 1 fixed the issue.

  1. Add .equals(ActivityLogTable.LOCAL_SITE_ID, it.localSiteId) below this line to make sure we are querying only events which are referencing the corresponding site (existing local_site_id).
  2. Make sure that we purge ActivityLogTable when we are removing an entry from SiteModelTable -> for our example, we'd delete events in ActivityLogTable when we were deleting site with local_site_id = 7 from the SiteModelTable. I'm not sure where we should put this code since I'm not sure from when/who deleted the site with local_site_id = 7 from the SiteModelTable.

Props to @ParaskP7 for reporting this issue and jumping on a call and helping to investigate what is going on. Props to @ashiagr If I recall correctly, you encountered this issue a few weeks/months ago and your findings were similar to ours (it helped us quickly find and verify the behavior)

cc @planarvoid Do you by any chance remember why this line contains the "local_site_id" check but this line doesn't? My guess is that it was a mistake and the same check should have been added even to the second expressions. But I thought I'd better ping you just in case I'm missing something.

malinajirka avatar Feb 18 '21 15:02 malinajirka

Do you by any chance remember why this line contains the "local_site_id" check but this line doesn't? My guess is that it was a mistake and the same check should have been added even to the second expressions. But I thought I'd better ping you just in case I'm missing something.

I have no idea @malinajirka but it looks like a mistake :-)

planarvoid avatar Feb 25 '21 15:02 planarvoid