Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[INV-3630] cache download stops in progress, start emancipating from Redux State #3782

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from
Open

[INV-3630] cache download stops in progress, start emancipating from Redux State #3782

wants to merge 19 commits into from

Conversation

LocalNewsTV
Copy link
Collaborator

@LocalNewsTV LocalNewsTV commented Jan 8, 2025 •
edited
Loading

Details

This PR Includes the following changes:

  • Users can cancel a Record cache download that is in progress.
    • Users are notified with a success alert upon termination
  • Record cache metadata is now persisted in the local DB (Previously just Redux)
  • Method names changed to align more consistently with maptile (and future) methods
  • If user deletes a cache that is in progress of downloading, the cache deletes
  • Fix issue where a Download stops in progress immediately
    • This was caused by there being no filter object, causing the bbox functionality to throw an error and crash. This is now fixed.
  • Migrate business logic in async thunks, to appropriate Database methods, decoupling the two.
  • Added stub placement for adding ProgressCallback functionality

Important

This PR Starts the process of moving Redux Recordset metadata into DB storage. In the meantime both states (Redux and DB) have to be stored. Next iteration will be removing the redux data, keeping one source of truth. You may find weird consistencies and Error checks while both states are being managed.

Testing

Tested the Following

Tip

Additional Context: We don't download duplicate records, so when deleting we use a frequency map to ensure we don't delete Records used in multiple places

  • Caching a recordset
  • Caching two recordsets with the same filters then deleting one to make sure the second cache/records remains in tact
  • Caching two recordsets with different filters then deleting one making sure there was no cross-repo delete
  • Deleting a cached recordset to check cleanup
  • Deleting a recordset in progress to check cleanup
  • Cancelling a download that is in progress

Testing was done in both Firefox and the Simulator
Sum of records and Cache metadata was manually verified and counted through queries in their respective storages.

@LocalNewsTV
Correct type mismatch for cached Geojson that didn't throw error
0429729
@LocalNewsTV
re-introduce Cache metadata in [localForage] Database
61a9f31
@LocalNewsTV
Add cancel prompt for when Downloads in Progress
cf8bdf3
@LocalNewsTV
Unhook deleteCache from Redux State idLists to Db stored cachedIds
92ad8ef
@LocalNewsTV
set 'cachedSet' in top level download function
1fe7446
@LocalNewsTV
more tightly assign types in RecordSet.requestCaching
f7277f4
@LocalNewsTV
Modify getBoundingBoxFromRecordsetFilters to work when filters is null
a115d85
@LocalNewsTV
Do not fire Error alert if we terminated early
e2d10ee
@LocalNewsTV
remove Destructuring of recordset, add cache abort flow (else condition)
a27faf7
@LocalNewsTV
consume errors in deleteCachedRecordsFromId function, add boolean ret…
e499b30 
…urn to download functions
@LocalNewsTV
make function names 'repository' centric for consistency, add delete …
24171ff 
…method
@LocalNewsTV
Modify downloads to handle early exits
8b869e5
@plasticviking
Copy link
Collaborator

Looks reasonable to me

@LocalNewsTV LocalNewsTV force-pushed the 3630-cache-download-stops-in-progress branch from 084d8a0 to b96c14d Compare January 9, 2025 16:06
@LocalNewsTV LocalNewsTV changed the title WIP - [INV-3630] cache download stops in progress, start emancipating from Redux State [INV-3630] cache download stops in progress, start emancipating from Redux State Jan 9, 2025
@LocalNewsTV LocalNewsTV force-pushed the 3630-cache-download-stops-in-progress branch from 3a35fb3 to c702f4d Compare January 9, 2025 22:45
LocalNewsTV
LocalNewsTV commented Jan 9, 2025
View reviewed changes
Copy link
Collaborator Author

@LocalNewsTV LocalNewsTV left a comment

Choose a reason for hiding this comment

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

Highlighted some of the weird code that is there until the second pass is complete.

This is done in two passes to

  • Avoid a massive PR with differing goals
  • Keep focus on the original goal here:
    • Caching to Db
    • removing a bug in the downloads
    • being able to stop a download in progress

app/src/state/reducers/userSettings.ts
Comment on lines +204 to +207
if (action.error.message === 'Early Exit') {
draftState.recordSets[action.meta.arg.setId].cacheMetadata = {
status: UserRecordCacheStatus.NOT_CACHED
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checks like these are for while we are managing the Two states [Redux / DB] And will be removed in next iteration

app/src/utils/record-cache/index.ts
Comment on lines +122 to +129
if (spec.recordSetType === RecordSetType.Activity && (await this.downloadActivity(args))) {
Object.assign(responseData, await this.loadRecordsetSourceMetadata(spec.idsToCache));
} else if (spec.recordSetType === RecordSetType.IAPP && (await this.downloadIapp(args))) {
Object.assign(responseData, await this.loadIappRecordsetSourceMetadata(spec.idsToCache));
} else {
this.deleteRepository(spec.setId);
throw Error('Early Exit');
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The thrown error is temporary until Second pass is completed

Download functions return a true on success (Early exit is considered a failure)

We intentionally throw an Error in the false since the cache was abandoned and we don't want to update the Redux state. Now the Thunk will fail, and this specific error gets consumed so the user is none the wiser

@LocalNewsTV LocalNewsTV marked this pull request as ready for review January 9, 2025 23:31
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jan 9, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mobile: Cache download stops while in progress
2 participants
@LocalNewsTV @plasticviking