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

Pagination for area and climb media #449

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

glassbead0
Copy link
Contributor

@glassbead0 glassbead0 commented Feb 17, 2025

Add query endpoints for areaMediaPagination and climbMediaPagination

For each query, the uuid is required (climbUuid or areaUuid), and the other pagination options after, first, and maxFiles

Returns the uuid (climb or area), and a mediaConnection, with the same structure as the getUserMediaPagination endpoint, including totalItems.

Technical details:
For queuing areas/climbs with lots of media (eg, the entire US), this should be efficient and fairly fast.
We only query the number of media objects + 1 requested with first (defaults to 6), so we aren't loading all the media objects into memory. We use countDocuments to efficiently get the totalItems.

Testing performance locally is difficult as i don't currently have an efficient way to add lots and lots of media.

will it be possible to deploy this to staging and performance test there before pushing to production?
@vnugent @Vichy97

Screenshot 2025-02-17 at 2 08 28 PM
Screenshot 2025-02-17 at 2 09 00 PM

@glassbead0 glassbead0 force-pushed the pagination_for_area_and_climb_media branch 3 times, most recently from ee137c9 to 20bde8b Compare February 17, 2025 21:07
@glassbead0 glassbead0 requested a review from vnugent February 17, 2025 21:11
@Vichy97
Copy link
Collaborator

Vichy97 commented Feb 17, 2025

Add query endpoints for areaMediaPagination and climbMediaPagination

For each query, the uuid is required (climbUuid or areaUuid), and the other pagination options after, first, and maxFiles

Returns the uuid (climb or area), and a mediaConnection, with the same structure as the getUserMediaPagination endpoint, including totalItems.

Technical details:
For queuing areas/climbs with lots of media (eg, the entire US), this should be efficient and fairly fast.
We only query the number of media objects + 1 requested with first (defaults to 6), so we aren't loading all the media objects into memory. We use countDocuments to efficiently get the totalItems.

Testing performance locally is difficult as i don't currently have an efficient way to add lots and lots of media.

will it be possible to deploy this to staging and performance test there before pushing to production?
@vnugent @Vichy97

#TODO:
Write unit tests.

I wanted to start the draft and get any feedback on the code for now.

Screenshot 2025-02-17 at 2 08 28 PM
Screenshot 2025-02-17 at 2 09 00 PM

The problem is that staging doesn't have images. I think deploying to prod is fine though since nobody will be using the new endpoints yet

@glassbead0 glassbead0 force-pushed the pagination_for_area_and_climb_media branch from 20bde8b to 9324990 Compare February 17, 2025 22:59
@glassbead0 glassbead0 marked this pull request as ready for review February 17, 2025 22:59
@glassbead0
Copy link
Contributor Author

I finished writing tests for the new endpoints.

I haven't modified the existing area and climb endpoints yes to allow paginated media within the existing queries. I don't remember why we needed that. @vnugent Can you remind me? I'm happy to do it, but want to make sure its a useful implementation before i put work into it.

@vnugent
Copy link
Contributor

vnugent commented Feb 18, 2025

@glassbead0 we talked about creating a new GraphQL field, eg mediaPagination. Once added I'd be happy to merge and deploy to staging. USA on staging does have 181 photos (I just added a bunch of test photos and more if needed).

@glassbead0
Copy link
Contributor Author

glassbead0 commented Feb 18, 2025

@glassbead0 we talked about creating a new GraphQL field, eg mediaPagination. Once added I'd be happy to merge and deploy to staging. USA on staging does have 181 photos (I just added a bunch of test photos and more if needed).

I added new fields to the area and climb API's. There is now a new field under climb and area called mediaPagination, which has the same interface as the top level areaMediaPagination and climbMediaPagination, except they dont' require a UUID (since that is specified at the top level of the climb/area api and would be redundant)

I see an areas test under the integration tests for testing the area api, but currently it doesn't test anything around media, and there doesn't appear to be a climb integration test at all. I will take a look tomorrow to see how feasible it is to add tests for these new mediaPagination fields, and also welcome input on whether/how those tests should be implemented.
Screenshot 2025-02-18 at 4 29 49 PM
Screenshot 2025-02-18 at 4 29 56 PM

@glassbead0
Copy link
Contributor Author

I've added a test for the mediaPagination within the existing area query. I won't have time to add a full new test for the climb query (since there isn't an existing one) until mid-late next week.

I'm happy to let this sit until then if consensus is that we should add a test before merging, but seems like adding a more comprehensive test of the climb endpoint (along with several other endpoints that dont' have integration tests), should maybe be separate tasks?

What are your thoughts @vnugent

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.

3 participants