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

Create caption files during transcoding #6145

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

Conversation

masaball
Copy link
Contributor

Related issue: #5702

@masaball masaball marked this pull request as ready for review December 12, 2024 14:23
@masaball masaball force-pushed the caption_extraction branch 2 times, most recently from 7e20070 to 56f3507 Compare January 14, 2025 20:55
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

Waiting to review this once ActiveEncode PR gets merged.

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

This is looking good overall! Exciting to see these changes in avalon to handle additional non-Derivative outputs from ActiveEncode. I'm hopeful the approach in this PR can be easy to extend if/when we add additional outputs other than captions.

Comment on lines +127 to +130
it 'uploads to Minio' do
encode.create!
expect(master_file).to have_received(:update_progress_on_success!)
end
Copy link
Member

Choose a reason for hiding this comment

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

Is there a check you can add here to ensure the file actually got uploaded? Maybe something along the lines of inspecting the supplemental file to make sure it is present, in minio, and non-zero length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. The Active Storage service is set at application boot and so the test suite is set to DiskService and I have been struggling to find a way to override that or mock it to test the Supplemental File.

Maybe we just remove this test and trust that Active Storage will properly do its thing?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to keep this test as a check that the supplemental file gets attached in the after_completed callback. So maybe ActiveStorage service independent would be best. Do you think this tests enough?

supplemental_file = master_file.supplemental_files.last
expect(supplemental_file.file.present?).to eq true
expect(supplemental_file.file.byte_size > 0).to eq true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That set of tests is probably sufficient, though Active Storage has an attached? method which may be better/more appropriate to check than present?. Will make the changes on Monday and see how it goes.

spec/models/watched_encode_spec.rb Show resolved Hide resolved
app/models/watched_encode.rb Show resolved Hide resolved
app/models/watched_encode.rb Show resolved Hide resolved
app/models/supplemental_file.rb Show resolved Hide resolved
app/models/master_file.rb Show resolved Hide resolved
app/models/watched_encode.rb Show resolved Hide resolved
app/models/master_file.rb Show resolved Hide resolved
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