-
Notifications
You must be signed in to change notification settings - Fork 50
Tally top 80 results exactly and fix "monday" timestamp calculation #1113
Tally top 80 results exactly and fix "monday" timestamp calculation #1113
Conversation
API Developer Docs Preview: Ready https://wordpress.github.io/openverse-api/_preview/1113 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, nice find! It looks like there's a failing test, I'm able to produce it locally too
results_to_tally = results | ||
elif max_result_depth - page_size < 80: | ||
# Applies when `page_size * page` could land beyond 80, but still | ||
# encompas on _this page_ some results that are below the 80th |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# encompas on _this page_ some results that are below the 80th | |
# encompass on _this page_ some results that are below the 80th |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test failure is reproducible for me as well :/
Testing manually the tallies LGTM, though. I tested:
- "normal" flow by not adjusting
page_size
and checking the tallies increase correctly after requesting page 1-4 but not after page 5 - using
page_size=7
and checked the tallies increase only for the first 80 values - using
page_size =100
(after updatingMAX_ANONYMOUS_PAGE_SIZE
locally 😄) and verified only first 80 on page 1 are counted
Thanks for the reviews. The failing test was due to the fact that I'd accidentally removed the falsey check on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect timing for deploy :)
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting problem and solution, thanks @sarayourfriend!
Fixes
Staci and I noticed two issues while we were deploying the API today:
Description
Applies fixes for the above described issues. There are (hopefully) sufficient comments to hopefully describe the approach and intentions as the logic is a bit convoluted.
Testing Instructions
Check out the much expanded unit tests. You can test locally following the process Staci describes here: #1088 (review). However, because local results for any given query do not really have 80 results, you'll need to test without a query or with a
*
query.Checklist
Update index.md
).main
) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin