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

Issue/360 check caption language #434

Merged

Conversation

atarisafari
Copy link
Contributor

@atarisafari atarisafari commented Jul 17, 2019

Fixes #360

@atarisafari atarisafari force-pushed the issue/360-check-caption-language branch from 3de7cf1 to bda94dc Compare July 17, 2019 17:41
@atarisafari atarisafari reopened this Jul 17, 2019
@atarisafari atarisafari added this to the v2.6.0 milestone Jul 19, 2019
Copy link
Member

@bagofarms bagofarms left a comment

Choose a reason for hiding this comment

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

My manual tests did not completely pass. While it was successful in detecting a mismatch in language, there is a major edge case that needs to be addressed. YouTube seems to use non-standard language codes for dialects. For example, a video from Univision will have captions with the language code es-MX for "Spanish (Mexico)". Example: https://www.youtube.com/watch?v=FcnrUf35LmQ

Since there could be tons of dialects for languages, maybe the best approach would be to only look at the first two characters of the language that YouTube reports back.

lib/quail/quail/common/services/media/youtube.php Outdated Show resolved Hide resolved
config/herokuConfig.php Outdated Show resolved Hide resolved
lib/quail/quail/common/services/media/vimeo.php Outdated Show resolved Hide resolved
config/localConfig.template.php Outdated Show resolved Hide resolved
@atarisafari
Copy link
Contributor Author

Upgraded feature to not rely on config variables. Now just checks for the Canvas course language without any need for user setup.

accounting for dialects

lots of cleanup
@atarisafari atarisafari force-pushed the issue/360-check-caption-language branch 2 times, most recently from c31cf33 to d9ccbdc Compare August 9, 2019 14:48
@atarisafari
Copy link
Contributor Author

Note: Remind users in release notes that dialects will be treated as being the same language

@bagofarms
Copy link
Member

I did some testing and found that way more YouTube API requests were being made than expected. I also talked to Matt about how we might be able to eliminate the extra API call for language by combining the two errors together and recognizing which one is which during the report building stage.

added field limiting to vimeo checks

testing api cache with vimeo
@atarisafari atarisafari force-pushed the issue/360-check-caption-language branch from 84e5a0a to 3ccf4a3 Compare October 11, 2019 19:55
@bagofarms bagofarms merged commit 53e9d9c into ucfopen:dev/v2-6-0 Oct 22, 2019
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