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

Fix thumbs for direct uploads #2222

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Fix thumbs for direct uploads #2222

merged 4 commits into from
Jun 18, 2024

Conversation

mjh1
Copy link
Contributor

@mjh1 mjh1 commented Jun 18, 2024

No description provided.

@mjh1 mjh1 requested a review from a team as a code owner June 18, 2024 11:26
Copy link

vercel bot commented Jun 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
livepeer-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 18, 2024 1:00pm

@mjh1
Copy link
Contributor Author

mjh1 commented Jun 18, 2024

I'm thinking we may want to add coverage for this in MediaGuard? Unless @victorges you think it should be easy to add a unit test here, I couldn't see an existing one I could piggy back on.

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +983 to +986
thumbnails: !(await isExperimentSubject(
"vod-thumbs-off",
req.user?.id,
)),
Copy link
Member

Choose a reason for hiding this comment

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

If you want to have this logic for all assets, might wanna check usages of this createTask function. I know there's at least recordings tasks which are created from cannon.ts

@victorges
Copy link
Member

re tests: I think we are fine without a test here, it's a really simple logic. I also understood that MediaGuard was supposed to test only the basic stuff for a continuous test, while we test all features like this from E2E tests instead. So maybe those would be better? (probably on catalyst with the box?)

@mjh1 mjh1 merged commit 701ddd5 into master Jun 18, 2024
8 checks passed
@mjh1 mjh1 deleted the mh/thumbs branch June 18, 2024 14:01
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.

2 participants