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.

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

[INV-3858] Batch download activities #3873

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

Conversation

LocalNewsTV
Copy link
Collaborator

@LocalNewsTV LocalNewsTV commented Feb 24, 2025

Overview

This PR includes the following proposed change(s):

Important

Batch Numbers are arbitrarily set because odd numbers updating on the screen appears faster than only ever seeing the 10's position update.
IAPP in its SQL has a hard limit of 20 which would require refactoring to push above it. So its limit is set at 19

  • Add new Auth endpoint for pulling multiple records by their ID

    • Records can be from Either IAPP or Activities
  • Implement the new endpoint in Caching.

  • Optimize Queries used for IAPP obtaining IAPP records.

    • Implement the stubbed point_of_interest_ids key (previously unused)
  • Cache Time Improvements (From Browser / LocalForage):

    • 1,000 IAPP Records can be cached in ~10 seconds
    • 1,450 Activity Records can be cached in ~15 seconds!
  • Modify getIappExtractFromDB to take a DB connection as an argument

  • Modify saveIapp as it doesn't need to tunnel the response to save objects.

  • Update point-of-interest model to use optional chaining

  • Correct some thrown errors to be Error objects (code smell)

  • in mapSitesRowToJSON:

    • Convert 9 array constants to one object
    • convert individual filters to condensed for loop of object keys
    • remove assigning filter results to 9 more variables. Replace existing arrays in place.
    • Convert remaining arrays to implicit return
  • Migrate getSelectColumnsByRecordSetType to sharedAPI

  • Closes Batch Processing Activity Records in Cache #3858

  • Closes Batch Processing IAPP records in Caching #3859

@LocalNewsTV LocalNewsTV force-pushed the 3858-batch-download-activities branch from 162dc25 to 76c3743 Compare February 25, 2025 00:09
@LocalNewsTV LocalNewsTV force-pushed the 3858-batch-download-activities branch from b3cf220 to 8ac6e97 Compare February 26, 2025 21:44
Copy link
Collaborator

@meghna0593 meghna0593 left a comment

Choose a reason for hiding this comment

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

Tested out the functionality, works significantly faster than before! Nice work.

The main change I'd recommend is improving the organization of the api. Currently, the record-caching api is handling both fetching multiple activities by IDs and fetching iapp records by IDs.

Consider splitting this into two separate apis—one dedicated to fetching multiple iapp records by IDs and another for activities. This will improve readability and make the purpose of each API clearer.

Also, I noticed that this is more of a batch request than a batch download, since we're still downloading the fetched records one at a time. A batch download would likely require more complex logic due to the pause/resume functionality. That said, I think sticking with the batch request approach makes sense since the download speed has improved significantly with these changes!

@LocalNewsTV
Copy link
Collaborator Author

Tested out the functionality, works significantly faster than before! Nice work.

The main change I'd recommend is improving the organization of the api. Currently, the record-caching api is handling both fetching multiple activities by IDs and fetching iapp records by IDs.

Consider splitting this into two separate apis—one dedicated to fetching multiple iapp records by IDs and another for activities. This will improve readability and make the purpose of each API clearer.

Also, I noticed that this is more of a batch request than a batch download, since we're still downloading the fetched records one at a time. A batch download would likely require more complex logic due to the pause/resume functionality. That said, I think sticking with the batch request approach makes sense since the download speed has improved significantly with these changes!

Ticket #3877 made to adjust further optimization efforts.
Other Feedback implemented

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
37.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@plasticviking
Copy link
Collaborator

Looks good to me. I wouldn't worry too much about optimization. An API overhaul and move to streaming is in order once we have time to tackle some debt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch Processing IAPP records in Caching Batch Processing Activity Records in Cache
3 participants