Fix part of #5002 remove GCS_RESOURCE_BUCKET_NAME from GLOBALS #7130

jameesjohn · 2019-07-12T10:36:20Z

Explanation

Fixes part of #5002 remove GCS_RESOURCE_BUCKET_NAME from GLOBALS

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

codecov · 2019-07-12T11:33:03Z

Codecov Report

Merging #7130 into develop will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #7130      +/-   ##
===========================================
+ Coverage    98.06%   98.06%   +<.01%     
===========================================
  Files          377      379       +2     
  Lines        63086    63116      +30     
===========================================
+ Hits         61864    61894      +30     
  Misses        1222     1222

Impacted Files	Coverage Δ
feconf.py	`99.46% <100%> (ø)`	⬆️
main.py	`87.73% <100%> (+0.11%)`	⬆️
core/controllers/app_identity_test.py	`100% <100%> (ø)`
core/controllers/app_identity.py	`100% <100%> (ø)`

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 f52947f...3c169e3. Read the comment docs.

codecov · 2019-07-12T11:33:04Z

Codecov Report

Merging #7130 into develop will decrease coverage by 9.28%.
The diff coverage is 64.58%.

@@             Coverage Diff             @@
##           develop    #7130      +/-   ##
===========================================
- Coverage    98.62%   89.34%   -9.29%     
===========================================
  Files          381     1198     +817     
  Lines        64057    96865   +32808     
  Branches         0     2469    +2469     
===========================================
+ Hits         63179    86544   +23365     
- Misses         878     9806    +8928     
- Partials         0      515     +515

Impacted Files	Coverage Δ
...ge/directives/OppiaNoninteractiveImageDirective.ts	`18.42% <ø> (ø)`
core/controllers/base.py	`100% <ø> (ø)`	⬆️
main.py	`87.61% <ø> (ø)`	⬆️
...jects/templates/ImageWithRegionsEditorDirective.ts	`0.76% <0%> (ø)`
...sions/objects/templates/FilepathEditorDirective.ts	`0.85% <0%> (ø)`
...ctives/OppiaInteractiveImageClickInputDirective.ts	`8.13% <0%> (ø)`
core/controllers/resources_test.py	`100% <100%> (ø)`	⬆️
core/controllers/resources.py	`100% <100%> (ø)`	⬆️
...s/dev/head/services/AssetsBackendApiServiceSpec.ts	`99.11% <100%> (ø)`
feconf.py	`99.45% <100%> (ø)`	⬆️
... and 818 more

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 50e8ae3...f6ee206. Read the comment docs.

vojtechjelinek

Thanks! Few comments.

core/controllers/app_identity.py

vojtechjelinek · 2019-07-13T11:25:27Z

core/templates/dev/head/services/AssetsBackendApiService.ts

-                         window.MSBlobBuilder;
+                          window.WebKitBlobBuilder ||
+                          window.MozBlobBuilder ||
+                          window.MSBlobBuilder;


Why this change?

Unnecessary. I think it is due to my editor formatting.
Removed.

vojtechjelinek · 2019-07-13T11:27:39Z

core/templates/dev/head/services/AssetsBackendApiService.ts

+        GCS_PREFIX = ('https://storage.googleapis.com/' +
+          GCS_RESOURCE_BUCKET_NAME + '/exploration');
+        AUDIO_DOWNLOAD_URL_TEMPLATE = (
+          (DEV_MODE ? '/assetsdevhandler' : GCS_PREFIX) +


Why do you need to check DEV_MODE here? Shouldn't this be handled on line 51?

Yes, removed.

vojtechjelinek · 2019-07-13T11:27:51Z

core/templates/dev/head/services/AssetsBackendApiService.ts

+    var _initialize = function() {
+      if (DEV_MODE) {
+        // GCS_PREFIX = ('https://storage.googleapis.com/' +
+        //   GCS_RESOURCE_BUCKET_NAME + '/exploration');


Why is this commented out?

The prefix is only used in PROD

…-removal2

vojtechjelinek

Thanks! Few comments.

vojtechjelinek · 2019-07-15T08:34:46Z

core/templates/dev/head/services/AssetsBackendApiService.ts

@@ -63,19 +61,17 @@ oppia.factory('AssetsBackendApiService', [
      }

      $http.get('/appidentityhandler').then(function(response) {
-        if (!DEV_MODE && !response.data.GCS_RESOURCE_BUCKET_NAME) {
+        if (!response.data.GCS_RESOURCE_BUCKET_NAME) {
          throw Error('GCS_RESOURCE_BUCKET_NAME is not set in prod.');
        }


Now that I think of it. Remove this if altogether.

I don't think this if should be removed since it validated the result gotten from the handler

vojtechjelinek · 2019-07-15T08:35:29Z

core/templates/dev/head/services/AssetsBackendApiService.ts

@@ -63,19 +61,17 @@ oppia.factory('AssetsBackendApiService', [
      }

      $http.get('/appidentityhandler').then(function(response) {
-        if (!DEV_MODE && !response.data.GCS_RESOURCE_BUCKET_NAME) {
+        if (!response.data.GCS_RESOURCE_BUCKET_NAME) {
          throw Error('GCS_RESOURCE_BUCKET_NAME is not set in prod.');
        }
        GCS_RESOURCE_BUCKET_NAME = response.data.GCS_RESOURCE_BUCKET_NAME;
      }).then(function() {


Do we need to chain the .then( here?

No
Removed. Thanks.

…-removal2

oppiabot · 2019-07-23T04:17:31Z

Hi @Jamesjay4199. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

seanlip

Took another pass, thanks @Jamesjay4199.

Could you please describe your manual testing procedure for this PR? I suggest doing that in general for PRs that you do from now onwards, in order to avoid cases like the recent regressions.

seanlip · 2019-07-23T07:35:39Z

core/templates/dev/head/services/AssetsBackendApiService.ts

-    var IMAGE_DOWNLOAD_URL_TEMPLATE = (
-      (DEV_MODE ? '/assetsdevhandler' : GCS_PREFIX) +
-      '/<exploration_id>/assets/image/<filename>');
+      if (_initialized) {


Perhaps this should come at the very beginning of the function?

seanlip · 2019-07-23T07:36:56Z

core/templates/dev/head/services/AssetsBackendApiService.ts


-    var AUDIO_UPLOAD_URL_TEMPLATE =
-      '/createhandler/audioupload/<exploration_id>';
+      $http.get('/gcs_resource_bucket_name_handler').then(function(response) {


Don't you need to return this?

True. Done.

seanlip · 2019-07-23T07:38:47Z

core/templates/dev/head/services/AssetsBackendApiService.ts

-          exploration_id: explorationId,
-          filename: filename
-        });
+      if (_initialized) {


Do you need this clause? I thought the "if _initialized" check in _initialize() should take care of it seamlessly. It would be better to drop lines 230-238 as long as you verify the code is still correct.

Done. Thanks.

seanlip · 2019-07-23T07:39:25Z

extensions/interactions/ImageClickInput/directives/OppiaInteractiveImageClickInputDirective.ts

@@ -96,8 +96,12 @@ oppia.directive('oppiaInteractiveImageClickInput', [
            // preview mode. We don't have loading indicator or try again for
            // showing images in the exploration editor or in preview mode. So
            // we directly assign the url to the imageUrl.
-            ctrl.imageUrl = AssetsBackendApiService.getImageUrlForPreview(
-              ContextService.getExplorationId(), ctrl.filepath);
+            AssetsBackendApiService.getImageUrlForPreview(


For async methods can you rename them to have async in the name? E.g. getImageUrlForPreviewAsync, get TrustedResourceUrlForImageFileNameAsync, etc.

Ditto elsewhere.

…-removal2

vojtechjelinek

LGTM!

seanlip · 2019-07-24T17:43:38Z

Backend tests are failing, @Jamesjay4199 .

seanlip · 2019-07-24T17:44:12Z

Also, did you see this comment? Please address it.

Could you please describe your manual testing procedure for this PR? I suggest doing that in general for PRs that you do from now onwards, in order to avoid cases like the recent regressions.

seanlip

Thanks, looks good @Jamesjay4199. Just one small comment.

That said, the bigger comment is the one about manual testing. Please address that. I will wait on that before giving LGTM.

seanlip · 2019-07-24T17:45:09Z

core/templates/dev/head/services/AssetsBackendApiService.ts

@@ -227,22 +227,13 @@ oppia.factory('AssetsBackendApiService', [
    };

    var _getDownloadUrl = function(explorationId, filename, assetType) {


Change to _getDownloadUrlAsync

jameesjohn · 2019-07-24T18:21:44Z

Could you please describe your manual testing procedure for this PR? I suggest doing that in general for PRs that you do from now onwards, in order to avoid cases like the recent regressions.

Sorry for the delays, I did the testing again to confirm.
For manual testing, I created an exploration, did a voice over, then played the exploration. While playing the exploration, I played the audio which I loaded.

However, I think there should be more testing since the bucket name is going to be null while testing it locally.

seanlip · 2019-07-25T07:16:14Z

Thanks for confirming @Jamesjay4199. I'll add a note to the August release notes doc to make sure to verify this when we push to the test server, though typically you should be testing this on your own deployment.

seanlip · 2019-07-25T07:20:20Z

core/templates/dev/head/services/AssetsBackendApiServiceSpec.ts

-    expect(
-      AssetsBackendApiService.getAudioDownloadUrl('expid12345', 'a.mp3')
-    ).toEqual('/assetsdevhandler/expid12345/assets/audio/a.mp3');
+    AssetsBackendApiService.getAudioDownloadUrlAsync('expid12345', 'a.mp3')


@Jamesjay4199 I just realized something (after I merged) about this test. Change the URL on line 70 to something random and run the frontend tests. Do they pass?

If so, please rework this test to call done() after the async call. See https://jasmine.github.io/2.0/introduction.html#section-Asynchronous_Support. Do this for other async calls.

Please address this in a separate follow-up PR.

Addressed in https://github.com/oppia/oppia/pull/7244/files

seanlip · 2019-07-25T07:21:19Z

Also, @Jamesjay4199, please do a test of this functionality on a prod server of your own. If you have trouble doing that please email me and Vojta and we can help you get it set up. If this causes problems during the release -- that would not be great.

/cc @vojtechjelinek

…LOBALS (oppia#7130)" This reverts commit a7bb506.

#7130)" (#7278) This reverts commit a7bb506.

remove GCS_RESOURCE_BUCKET_NAME from GLOBALS

7e7435b

jameesjohn added the CHANGELOG: Code health label Jul 12, 2019

jameesjohn requested a review from vojtechjelinek July 12, 2019 10:36

jameesjohn requested review from aks681, apb7, DubeySandeep, nithusha21, seanlip and vinitamurthi as code owners July 12, 2019 10:36

refactorings

3c169e3

jameesjohn requested a review from kevinlee12 as a code owner July 12, 2019 11:33

jameesjohn added 3 commits July 12, 2019 13:02

refactor test

a9b1b20

work on failing FE test

5469744

still on failing test

f9d53ea

kevinlee12 assigned jameesjohn Jul 12, 2019

vojtechjelinek suggested changes Jul 13, 2019

View reviewed changes

jameesjohn added 2 commits July 14, 2019 16:12

Merge branch 'develop' of https://github.com/oppia/oppia into globals…

435c25a

…-removal2

address review comments

5f95d2e

vojtechjelinek suggested changes Jul 15, 2019

View reviewed changes

address review comment

f4d9ae3

jameesjohn assigned kevinlee12, vojtechjelinek, seanlip, vinitamurthi, nithusha21, DubeySandeep and aks681 and unassigned jameesjohn Jul 16, 2019

jameesjohn reopened this Jul 22, 2019

jameesjohn added 2 commits July 23, 2019 00:12

Merge branch 'develop' of https://github.com/oppia/oppia into globals…

f02a37c

…-removal2

working on FE test

5028a22

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Jul 23, 2019

seanlip requested changes Jul 23, 2019

View reviewed changes

Merge branch 'develop' of https://github.com/oppia/oppia into globals…

25470cf

…-removal2

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Jul 23, 2019

jameesjohn added 2 commits July 24, 2019 18:05

address review comments

c832832

Merge branch 'develop' of https://github.com/oppia/oppia into globals…

f82ad3b

…-removal2

vojtechjelinek assigned seanlip Jul 24, 2019

vojtechjelinek approved these changes Jul 24, 2019

View reviewed changes

seanlip requested changes Jul 24, 2019

View reviewed changes

address review comments and work on failing backend test

f6ee206

jameesjohn requested review from seanlip and DubeySandeep July 24, 2019 20:05

seanlip approved these changes Jul 25, 2019

View reviewed changes

seanlip merged commit a7bb506 into oppia:develop Jul 25, 2019

seanlip reviewed Jul 25, 2019

View reviewed changes

seanlip unassigned vojtechjelinek and seanlip Jul 25, 2019

jameesjohn mentioned this pull request Aug 2, 2019

Address review comments on #7130 #7244

Closed

9 tasks

jameesjohn added a commit to jameesjohn/oppia that referenced this pull request Aug 2, 2019

Revert "Fix part of oppia#5002 remove GCS_RESOURCE_BUCKET_NAME from G…

3d35650

…LOBALS (oppia#7130)" This reverts commit a7bb506.

jameesjohn mentioned this pull request Aug 2, 2019

Revert "Fix part of #5002 remove GCS_RESOURCE_BUCKET_NAME from GLOBAL… #7278

Merged

9 tasks

seanlip pushed a commit that referenced this pull request Aug 3, 2019

Revert "Fix part of #5002 remove GCS_RESOURCE_BUCKET_NAME from GLOBALS (

2570a9e

#7130)" (#7278) This reverts commit a7bb506.

		@@ -227,22 +227,13 @@ oppia.factory('AssetsBackendApiService', [
		};

		var _getDownloadUrl = function(explorationId, filename, assetType) {

Fix part of #5002 remove GCS_RESOURCE_BUCKET_NAME from GLOBALS #7130

Fix part of #5002 remove GCS_RESOURCE_BUCKET_NAME from GLOBALS #7130

Conversation

jameesjohn commented Jul 12, 2019

Explanation

Checklist

codecov bot commented Jul 12, 2019

Codecov Report

codecov bot commented Jul 12, 2019 • edited Loading

Codecov Report

vojtechjelinek 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

vojtechjelinek 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

oppiabot bot commented Jul 23, 2019

seanlip 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

vojtechjelinek left a comment

Choose a reason for hiding this comment

seanlip commented Jul 24, 2019

seanlip commented Jul 24, 2019

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameesjohn commented Jul 24, 2019

seanlip commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jul 25, 2019

codecov bot commented Jul 12, 2019 •

edited

Loading