-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
server: fix bulk add preview caching #1787
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR!
It doesn't look like this accounts for partial successes. If the add preview only partially succeeds (eg. if 2 video links are requested, but only 1 video is returned), the request will still get cached in the user's browser.
} else if (videoKeys.length > 2) { | ||
return video; |
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.
What is this branch here for?
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.
You're right it's still broken. I don't think this approach is correct, I'll revise it.
The branch is to only return the video if the video is found in the cache, since it should contain more than the VideoId fields then.
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.
This is incorrect behavior then. We need to be able to return videos that were not already in the cache.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1787 +/- ##
================================================
+ Coverage 61.6161% 61.6170% +0.0008%
================================================
Files 169 169
Lines 25443 25454 +11
Branches 1447 1449 +2
================================================
+ Hits 15677 15684 +7
- Misses 9712 9716 +4
Partials 54 54 ☔ View full report in Codecov by Sentry. |
Fix #1289. Also filters videos that failed to fetch from being returned to the client.