Add editor for audio translations #3692

seanlip · 2017-08-02T09:23:16Z

Hi @tjiang11 -- unfortunately, this is all I was able to do tonight, but I think it's not bad :) Could you please take it for a spin and also do a preliminary code review? Note that this has quite a few dependencies on #3681 (which you'll notice when reviewing) -- so if you could get that into develop asap, that would be great, since there's a fair bit of merge work needed!

I haven't yet written tests. I'll do that tomorrow, though I'd appreciate a first pass first (both code and UI) to make sure things aren't too amiss.

Tests for newly-added functionality

Also, the following stuff has not been done. I'd welcome advice on whether you think either or both of them are necessary for v1?

"Deprecate audio" functionality
~~"Edit audio" modal~~ (Tony and I decided that this wasn't really necessary)

Thanks!

Edit: Additional TODO --

Reinstate resources_test from 3681

seanlip · 2017-08-02T09:28:35Z

Btw, I'm slightly worried about the GCS part of this, since I wasn't able to test it when working on this PR. We should make sure to do some robust tests of the end-to-end audio upload/play functionality during the release prep week. (I'd appreciate a slightly more detailed review on that part of the code.)

tjiang11

This looks good! Couldn't find any major flaws, perhaps the biggest thing being that errors are hidden behind the add translation modal.

I also tried deploying to test and successfully uploaded audio to GCS via the creator view, so as far as uploading goes I think it's pretty set. We just need to make sure that we can also reliably download the data directly from GCS. As you said, we should do more e2e testing on this. Also, the bucket should be set to be publicly accessible if it hasn't already yet.

Thanks Sean!

Also, I barely got under the 10,000 file limit after a couple of tries... here is the skip_files I used (including the defaults). I'm bad at regex so it could probably be cleaned up.

skip_files:

^(./)?#.#$
^(./)?.~$
^(./)?..py[co]$
^(./)?./RCS/.*$
^(./)?..$
^(.*)Spec.js$
tests/.*$
data/.*$
scripts/.*$

tjiang11 · 2017-08-04T00:23:26Z

core/domain/fs_domain.py

        raise NotImplementedError

+    def commit(self, unused_user_id, filepath, raw_bytes, mimetype):
+        bucket_name = constants.GCS_RESOURCE_BUCKET_NAME


Just a note that this will probably have to change to feconf when merging in the learner portion.

Done -- thanks for the heads-up!

tjiang11 · 2017-08-04T00:36:28Z

core/templates/dev/head/components/forms/AudioTranslationsEditorDirective.js

+              templateUrl: UrlInterpolationService.getDirectiveTemplateUrl(
+                '/components/forms/' +
+                'add_audio_translation_modal_directive.html'),
+              backdrop: true,


I think use 'static' backdrop to prevent clicking outside for closing the modal

tjiang11 · 2017-08-04T00:39:39Z

core/templates/dev/head/components/forms/AudioTranslationsEditorDirective.js

+                        var parsedResponse = angular.fromJson(transformedData);
+                        alertsService.addWarning(
+                          parsedResponse.error ||
+                          'Error communicating with server.');


Hmm it seems like the alert is hidden since a modal is being used :\ Maybe just display the error in the modal?

tjiang11 · 2017-08-04T01:03:40Z

core/templates/dev/head/components/forms/AudioTranslationsEditorDirective.js

+          $scope.getAudioLanguageDescription = (
+            LanguageUtilService.getAudioLanguageDescription);
+
+          $scope.getAudioLanguageFullUrl = function(filename) {


Naming seems a bit off, getAudioTranslationFullUrl?

tjiang11 · 2017-08-04T01:08:28Z

core/templates/dev/head/components/forms/AudioTranslationsEditorDirective.js

+
+            if (allowedAudioLanguageCodes.length === 0) {
+              alertsService.addWarning(
+                'Sorry, all audio translation slots are full.');


Maybe change the wording to something like "there are no more available languages to translate into"

Much better, thanks! Done.

tjiang11 · 2017-08-04T01:21:06Z

core/templates/dev/head/components/forms/audio_translations_editor_directive.html

+          <[getAudioLanguageDescription(languageCode)]>
+        </div>
+        <div class="col-lg-9 col-md-9 col-sm-9">
+          <audio controls preload="none" controlsList="nodownload">


Hm, do we need the volume control here? It might be misleading in that it will control the actual volume playback for the learner.

Unfortunately I don't think we can remove it. See https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/controlsList -- adjusting controls of HTML5 audio elements is fiddly, and I don't think there's full support in all browsers yet.

tjiang11 · 2017-08-04T01:24:10Z

core/templates/dev/head/components/forms/AudioTranslationsEditorDirective.js

+            });
+          };
+
+          $scope.deleteAudioTranslation = function(languageCode) {


Not essential, but after trying it out it I kind of feel that a confirmation dialog should appear here for deletion.

codecov-io · 2017-08-04T05:43:31Z

Codecov Report

Merging #3692 into develop will increase coverage by 0.07%.
The diff coverage is 64.86%.

@@             Coverage Diff             @@
##           develop    #3692      +/-   ##
===========================================
+ Coverage    45.01%   45.08%   +0.07%     
===========================================
  Files          264      264              
  Lines        20344    20403      +59     
  Branches      3168     3172       +4     
===========================================
+ Hits          9158     9199      +41     
- Misses       11186    11204      +18

Impacted Files	Coverage Δ
...ploration_player/AudioTranslationManagerService.js	`2.77% <ø> (ø)`	⬆️
...lates/dev/head/services/AssetsBackendApiService.js	`48.93% <0%> (-10.04%)`	⬇️
...ges/exploration_editor/feedback_tab/FeedbackTab.js	`0.72% <0%> (ø)`	⬆️
...s/dev/head/domain/utilities/LanguageUtilService.js	`100% <100%> (ø)`	⬆️
...omain/exploration/AudioTranslationObjectFactory.js	`100% <100%> (+8.33%)`	⬆️
...d/domain/exploration/SubtitledHtmlObjectFactory.js	`100% <100%> (+9.37%)`	⬆️
...n_editor/editor_tab/StateContentEditorDirective.js	`66.66% <26.31%> (-26.67%)`	⬇️

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 0ea0c85...3c5c3b5. Read the comment docs.

seanlip · 2017-08-04T08:05:49Z

app.yaml

+# Karma test files
+- ^(.*/)Spec.js$
+# Other folders to ignore
+- tests/


Note: I can't omit data/, since the dev server actually needs it. The dev server obeys this skip_files attribute.

I think we should investigate if we can skip more files, but probably outside the scope of this PR.

seanlip · 2017-08-04T08:40:50Z

Hi @tjiang11 -- I've addressed the comments, and I think this PR should be complete and ready to merge. PTAL!

tjiang11 · 2017-08-04T23:30:11Z

Thanks @seanlip ! I think you need to put LanguageUtilService in the collection player (failing test)

seanlip · 2017-08-05T00:00:33Z

Done, thanks @tjiang11! Do you have other comments?

tjiang11 · 2017-08-05T00:04:57Z

Hm wait, actually--can we only show the modal that asks whether they want to flag audio translations if it will actually make a difference? e.g. don't show the modal if all the audio translations are already flagged.

seanlip · 2017-08-05T00:52:07Z

Definitely! I'll do that in a separate PR tonight, as we discussed.

* 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) ...

seanlip added 2 commits August 1, 2017 03:25

Add editor for audio translations.

f10bcb1

Complete functionality for adding and deleting audio files.

028a254

seanlip assigned tjiang11 Aug 2, 2017

seanlip requested a review from tjiang11 August 2, 2017 09:23

Remove console logs.

87f94b0

tjiang11 suggested changes Aug 4, 2017

View reviewed changes

seanlip added 3 commits August 3, 2017 21:48

Merge branch 'develop' into subtitled-html-editor

cbbbe98

More minor updates from the merge. Reinstate resources_test.

986d577

Address most review comments.

8d7d711

seanlip added 2 commits August 3, 2017 23:32

Add modal for audio deletion. Fix backend tests.

22f603f

Add functionality for 'mark audio as needing update'.

6a3e4db

seanlip commented Aug 4, 2017

View reviewed changes

Add Karma tests.

e4741c3

Add missing dependency.

3c5c3b5

tjiang11 approved these changes Aug 5, 2017

View reviewed changes

seanlip merged commit 597ce33 into develop Aug 5, 2017

seanlip deleted the subtitled-html-editor branch August 5, 2017 00:56

tjiang11 mentioned this pull request Aug 8, 2017

Allow audio components in different languages to be added to an exploration. #3408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add editor for audio translations #3692

Add editor for audio translations #3692

seanlip commented Aug 2, 2017 •

edited

Loading

seanlip commented Aug 2, 2017 •

edited

Loading

tjiang11 left a comment

tjiang11 Aug 4, 2017

seanlip Aug 4, 2017

tjiang11 Aug 4, 2017

seanlip Aug 4, 2017

tjiang11 Aug 4, 2017

seanlip Aug 4, 2017

tjiang11 Aug 4, 2017

seanlip Aug 4, 2017

tjiang11 Aug 4, 2017

seanlip Aug 4, 2017

tjiang11 Aug 4, 2017

seanlip Aug 4, 2017

tjiang11 Aug 4, 2017

seanlip Aug 4, 2017

codecov-io commented Aug 4, 2017 •

edited

Loading

seanlip Aug 4, 2017 •

edited

Loading

seanlip commented Aug 4, 2017

tjiang11 commented Aug 4, 2017

seanlip commented Aug 5, 2017

tjiang11 commented Aug 5, 2017

seanlip commented Aug 5, 2017

Add editor for audio translations #3692

Add editor for audio translations #3692

Conversation

seanlip commented Aug 2, 2017 • edited Loading

seanlip commented Aug 2, 2017 • edited Loading

tjiang11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 4, 2017 • edited Loading

Codecov Report

seanlip Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

seanlip commented Aug 4, 2017

tjiang11 commented Aug 4, 2017

seanlip commented Aug 5, 2017

tjiang11 commented Aug 5, 2017

seanlip commented Aug 5, 2017

seanlip commented Aug 2, 2017 •

edited

Loading

seanlip commented Aug 2, 2017 •

edited

Loading

codecov-io commented Aug 4, 2017 •

edited

Loading

seanlip Aug 4, 2017 •

edited

Loading