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.

Sign up for GitHub

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

Jump to bottom

Fix part of #3704: make audio download URL safe; don't show flagging modal if all translations are flagged; use URL interpolation for modals. #3709

Merged
merged 1 commit into from
Aug 5, 2017
Merged

Fix part of #3704: make audio download URL safe; don't show flagging modal if all translations are flagged; use URL interpolation for modals. #3709

merged 1 commit into from
Aug 5, 2017

Conversation

seanlip
Copy link
Member

@seanlip seanlip commented Aug 5, 2017

This PR has a bunch of miscellaneous audio (and other) fixes:

  • Add more folders to the skip list.
  • Fix audio files not uploading in Firefox (it detects a different file type than Chrome when an MP3 file is uploaded).
  • Mark audio files using $sce so that they are loaded correctly in the editor.
  • Shorten tooltip wording for flag.
  • Do not show confirmation modal when all translations are already flagged.
  • Use UrlInterpolationService for some learner view modal directives so that they are loaded asynchronously when they are needed.

@seanlip
Audio fixes: make audio download URL safe; don't show flagging modal …
41459a6 
…if all translations are flagged; use templateUrl for modals.
@seanlip seanlip requested a review from tjiang11 August 5, 2017 09:43
@seanlip seanlip changed the title Fix part of #3704: make audio download URL safe; don't show flagging modal if all translations are flagged; use templateUrl for modals. Fix part of #3704: make audio download URL safe; don't show flagging modal if all translations are flagged; use URL interpolation for modals. Aug 5, 2017
tjiang11
tjiang11 approved these changes Aug 5, 2017
View reviewed changes
Copy link
Contributor

@tjiang11 tjiang11 left a comment

Choose a reason for hiding this comment

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

I just realized we're imposing a 1MB file size limit on the backend... can we get rid of that or raise to maybe something like 10MB?

Audio playback works now! Both from the creator view and the player view.

I did notice that if playing audio in the preview tab and then switching back to the creator, the audio still plays. We should probably stop it? Same with when switching from the creator view to learner view.

The creator can also play/stop multiple audio's in the creator view at once, but I don't think that matters too much. A better behavior might be to pause/stop other audio's when starting a new one. But this seems like it might not be worth the trouble.

Another random note is that it'd be nice to have more tests on uploading files since I've pretty much just been uploading files recorded on my linux audio recorder application... I still don't have that great of a feel of how picky mutagen is about things.

Code lgtm. I don't see anything wrong in this PR itself--will let you decide if you want to add more to it. Thanks!

@seanlip
Copy link
Member Author

seanlip commented Aug 5, 2017

Thanks @tjiang11!

I just realized we're imposing a 1MB file size limit on the backend... can we get rid of that or raise to maybe something like 10MB?

I think this is imposed only in the dev server by App Engine, since in that case we store files in the datastore. So probably a no-op. I couldn't find an explicit 1MB check in the code we've written.

I did notice that if playing audio in the preview tab and then switching back to the creator, the audio still plays. We should probably stop it? Same with when switching from the creator view to learner view.

The creator can also play/stop multiple audio's in the creator view at once, but I don't think that matters too much. A better behavior might be to pause/stop other audio's when starting a new one. But this seems like it might not be worth the trouble.

Yeah, I saw this as well. I don't think we need to fix it urgently but I'll file an issue.

Another random note is that it'd be nice to have more tests on uploading files since I've pretty much just been uploading files recorded on my linux audio recorder application... I still don't have that great of a feel of how picky mutagen is about things.

I think we'll learn with experience! If we can update the test server with the new functionality then perhaps we can test this during release-testing time.

@codecov-io
Copy link

Codecov Report

Merging #3709 into develop will increase coverage by 0.01%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3709      +/-   ##
===========================================
+ Coverage    45.07%   45.09%   +0.01%     
===========================================
  Files          264      264              
  Lines        20408    20414       +6     
  Branches      3173     3174       +1     
===========================================
+ Hits          9199     9205       +6     
  Misses       11209    11209
Impacted Files Coverage Δ
...v/head/pages/exploration_player/LearnerLocalNav.js 4% <ø> (ø) ⬆️
...v/head/pages/exploration_player/LearnerViewInfo.js 2% <ø> (ø) ⬆️
...d/domain/exploration/SubtitledHtmlObjectFactory.js 100% <100%> (ø) ⬆️
...n_editor/editor_tab/StateContentEditorDirective.js 67.34% <100%> (+0.68%) ⬆️
...ages/exploration_player/FatigueDetectionService.js 4.54% <33.33%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efd0d63...41459a6. Read the comment docs.

@seanlip seanlip merged commit 71b630f into develop Aug 5, 2017
@seanlip seanlip deleted the more-audio-fixes branch August 5, 2017 22:36
@seanlip seanlip mentioned this pull request Aug 6, 2017
Closed
5 tasks
giritheja added a commit to giritheja/oppia that referenced this pull request Aug 8, 2017
@giritheja
Merge remote-tracking branch 'upstream/develop' into next-job-controller
2f53eb9 
* upstream/develop: (39 commits)
  Add a one-off job to list which explorations use gadgets; remove old fallbacks job. (oppia#3717)
  Fix oppia#2998 implement collection skills update commands (oppia#3710)
  Improvements and bug fixes to changes introduced by PR oppia#3671 (oppia#3684)
  Fix oppia#3688: refactoring ExplorationStatusHandler to remove unpublish and publicize functionality as it is not called from frontend. (oppia#3715)
  fix oppia#3698: Shift exception handling for NotLoggedInException so that it does not generate log errors. (oppia#3714)
  Revert "Fix oppia#3707 Change text of collection learner view "start here"" (oppia#3713)
  Fix oppia#3701 Fixed Continue button name issue (oppia#3711)
  Fix oppia#3467: Introduce re-training and add_to_training_queue methods (oppia#3650)
  Fix oppia#3707 Change text of collection learner view "start here" (oppia#3700)
  Audio fixes: make audio download URL safe; don't show flagging modal if all translations are flagged; use templateUrl for modals. (oppia#3709)
  Learner audio updates (oppia#3705)
  Move hint button to left corner for supplemental cards (oppia#3702)
  Make the representation of the learner's 'Continue' answer more conversational. (oppia#3675)
  Add editor for audio translations (oppia#3692)
  Fix oppia#3686 fixed text overflow for contributors list (oppia#3697)
  Fix oppia#3551: Manually set cookie value during language change (oppia#3622)
  Learner view audio translations (oppia#3681)
  The sixth milestone of the learner dashboard. (oppia#3680)
  Routine update of translations. (oppia#3685)
  SiteWide ACL Refactor: Milestone 3.1 (oppia#3682)
  ...
giritheja added a commit to giritheja/oppia that referenced this pull request Aug 11, 2017
@giritheja
Merge remote-tracking branch 'upstream/develop' into publish-modal
00556d2 
* upstream/develop: (28 commits)
  Preloading of audio and bandwidth confirmation (oppia#3727)
  Fix oppia#3693: Deprecate ClassifierDataModel and update ClassifierTrainingJobModel (oppia#3734)
  Fix part of oppia#3453: Removed jinja template in editor_navigation_directive (oppia#3732)
  Eliminates stray tick mark (oppia#3731)
  Fix oppia#3453: Removed jinja template in collection editor navigation bar directive (oppia#3723)
  Fix oppia#3726: prevent graph SVG from overlapping modal footer buttons. (oppia#3728)
  address sean review comments
  Add a one-off job to list which explorations use gadgets; remove old fallbacks job. (oppia#3717)
  fix linting
  remove jinja template in create_activity_modal
  Fix oppia#2998 implement collection skills update commands (oppia#3710)
  Improvements and bug fixes to changes introduced by PR oppia#3671 (oppia#3684)
  Fix oppia#3688: refactoring ExplorationStatusHandler to remove unpublish and publicize functionality as it is not called from frontend. (oppia#3715)
  fix oppia#3698: Shift exception handling for NotLoggedInException so that it does not generate log errors. (oppia#3714)
  Revert "Fix oppia#3707 Change text of collection learner view "start here"" (oppia#3713)
  Fix oppia#3701 Fixed Continue button name issue (oppia#3711)
  Fix oppia#3467: Introduce re-training and add_to_training_queue methods (oppia#3650)
  Fix oppia#3707 Change text of collection learner view "start here" (oppia#3700)
  Audio fixes: make audio download URL safe; don't show flagging modal if all translations are flagged; use templateUrl for modals. (oppia#3709)
  Learner audio updates (oppia#3705)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjiang11 tjiang11 tjiang11 approved these changes

Assignees

@tjiang11 tjiang11

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seanlip @codecov-io @tjiang11