Fix part of #3826: extract from EditorServices.js #3970

bching · 2017-10-14T11:28:47Z

explorationData to ExplorationDataService
editorContextService to EditorStateService
angularNameService to AngularNameService

* explorationData to ExplorationDataService * editorContextService to EditorStateService * angularNameService to AngularNameService

shubha1593 · 2017-10-15T08:52:23Z

Hi @bching, Travis check is failing!

…act-editorservice

shubha1593 · 2017-10-15T17:28:22Z

Hi @bching, thanks. I guess there are linting errors still present and e2etest is failing !

codecov-io · 2017-10-23T13:17:07Z

Codecov Report

Merging #3970 into develop will increase coverage by 0.01%.
The diff coverage is 26.4%.

@@             Coverage Diff             @@
##           develop    #3970      +/-   ##
===========================================
+ Coverage    43.92%   43.94%   +0.01%     
===========================================
  Files          352      355       +3     
  Lines        22299    22298       -1     
  Branches      3540     3540              
===========================================
+ Hits          9795     9798       +3     
+ Misses       12504    12500       -4

Impacted Files	Coverage Δ
...ation_editor/history_tab/CompareVersionsService.js	`96.66% <ø> (ø)`	⬆️
...loration_editor/editor_tab/TrainingModalService.js	`2.94% <ø> (ø)`	⬆️
.../exploration_editor/UserEmailPreferencesService.js	`3.7% <ø> (ø)`	⬆️
.../dev/head/components/AnswerGroupEditorDirective.js	`0.87% <ø> (ø)`	⬆️
...mplates/dev/head/components/RuleEditorDirective.js	`1.16% <ø> (ø)`	⬆️
...n_editor/editor_tab/StateContentEditorDirective.js	`67.34% <ø> (ø)`	⬆️
...ges/exploration_editor/feedback_tab/FeedbackTab.js	`0.72% <0%> (ø)`	⬆️
...ad/components/OutcomeDestinationEditorDirective.js	`2.27% <0%> (ø)`	⬆️
...es/exploration_editor/editor_tab/StateResponses.js	`2.6% <0%> (ø)`	⬆️
...s/exploration_editor/editor_tab/StateStatistics.js	`4.54% <0%> (ø)`	⬆️
... and 28 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 d9e1592...2e614c0. Read the comment docs.

shubha1593 · 2017-10-25T02:49:31Z

Hi @seanlip, Travis seems to fail on the RUN_LINT test, but seeing the logs I think it's an issue with travis. I tried restarting the job but it's failing again!

seanlip · 2017-10-25T03:14:02Z

I think it's a legitimate issue -- see https://travis-ci.org/oppia/oppia/jobs/291538603#L686

shubha1593 · 2017-10-25T03:20:50Z

Oops sorry, missed that!

bching · 2017-10-27T00:04:28Z

Hmm... so the Travis linter is at fault? However I am receiving these errors from running e2e tests on my local. I am not certain which or if all of them are the fault of my code.

               Failures                    *

Exploration history should display the history

Failed: unknown error: Element ... is not clickable at point (1061, 361). Other element would receive the click:
...

(Session info: chrome=61.0.3163.100)
(Driver info: chromedriver=2.33.506106 (8a06c39c4582fbfbab6966dbb1c38a9173bfb1a2),platform=Mac OS X 10.12.5 x86_64)

Site language should not change in an exploration

Expected 'Type a number' to equal 'Ingresa un número'.

Learner dashboard functionality displays incomplete and completed collections

Failed: Index out of bound. Trying to access element at index: 0, but there are only 0 elements that match locator By(css selector, .protractor-test-collection-node)
Expected $.length = 4 to equal 0.
Expected $[0] = Entry({ level: SEVERE, message: 'http://localhost:9001/collection_handler/data/egFzpYfn3nE9 - Failed to load resource: the server responded with a status of 404 (Not Found)', timestamp: 1509018948099, type: '' }) to equal undefined.
Expected $[1] = Entry({ level: SEVERE, message: 'http://localhost:9001/third_party/static/angularjs-1.5.8/angular.min.js 117:9 Object', timestamp: 1509018948099, type: '' }) to equal undefined.
Expected $[2] = Entry({ level: SEVERE, message: 'http://localhost:9001/third_party/static/angularjs-1.5.8/angular.min.js 117:9 "Could not find the page http://localhost:9001/collection_handler/data/egFzpYfn3nE9."', timestamp: 1509018948100, type: '' }) to equal undefined.
Expected $[3] = Entry({ level: SEVERE, message: 'http://localhost:9001/third_party/static/angularjs-1.5.8/angular.min.js 117:9 "There was an error loading the collection."', timestamp: 1509018948100, type: '' }) to equal undefined.

Parameters should navigate multiple states correctly, with parameters

Failed: No element found using locator: By(css selector, .protractor-test-add-param-button)

rich-text components should display correctly

Expected 'oppia-noninteractive-tabs' to be 'oppia-noninteractive-link'.
Expected '' to be 'http://google.com/'.
Expected '' to be '_blank'.
Expected 'oppia-noninteractive-collapsible' to be 'oppia-noninteractive-math'.
Expected null to match 'abc'.
Expected 'oppia-noninteractive-math' to be 'oppia-noninteractive-collapsible'.
Failed: No element found using locator: By(css selector, .protractor-test-collapsible-heading)

State editor should open appropriate modal on re-clicking an interaction to customize it

Failed: unknown error: Element ... is not clickable at point (755, 281). Other element would receive the click:
...

(Session info: chrome=61.0.3163.100)
(Driver info: chromedriver=2.33.506106 (8a06c39c4582fbfbab6966dbb1c38a9173bfb1a2),platform=Mac OS X 10.12.5 x86_64)

State editor should add a solution

Failed: unknown error: Element ... is not clickable at point (428, 841). Other element would receive the click:

(Session info: chrome=61.0.3163.100)
(Driver info: chromedriver=2.33.506106 (8a06c39c4582fbfbab6966dbb1c38a9173bfb1a2),platform=Mac OS X 10.12.5 x86_64)

Oppia static pages tour visits the donate link

Failed: No element found using locator: By(css selector, .protractor-test-donate-link)

Oppia static pages tour visits the terms page

Failed: No element found using locator: By(css selector, .protractor-test-terms-link)

Oppia static pages tour visits the privacy page

Failed: No element found using locator: By(css selector, .protractor-test-privacy-policy-link)

Suggestions on Explorations accepts a suggestion on a published exploration

Expected 0 to equal 1.
Expected undefined to match 'Uppercased the first letter'.
Failed: Cannot read property 'click' of undefined

Executed 53 of 53 specs (11 FAILED) in 1 hour 13 mins 16 secs.

seanlip · 2017-10-27T00:15:59Z

Hi @bching -- actually, the e2e/protractor tests are passing fine on Travis, so it's possible that this is something to do with your local machine. What @shubha1593 was referring to was the linting check -- see 7663.9 in this link. One of your files has a small lint error due to a newline character check failing. If you could fix that, then this PR is probably good to go (at least from Travis's perspective)!

bching · 2017-10-27T09:06:50Z

@seanlip Ahhh sorry I misunderstood the conversation exchange. I removed the new line now.

seanlip · 2017-10-27T09:45:38Z

Thanks, @bching! I'm going to defer to @shubha1593 for review on this.

fix line length

shubha1593 · 2017-11-01T11:19:13Z

core/templates/dev/head/pages/collection_editor/collection_editor.html

@@ -117,6 +117,9 @@

  {# TODO(bhenning): Remove these once RouterServices is decoupled from the exploration editor #}
  <script src="{{TEMPLATE_DIR_PREFIX}}/pages/exploration_editor/EditorServices.js"></script>
+  <script src="{{TEMPLATE_DIR_PREFIX}}/pages/exploration_editor/ExplorationDataService.js"></script>
+  <script src="{{TEMPLATE_DIR_PREFIX}}/pages/exploration_editor/EditorStateService.js"></script>
+  <script src="{{TEMPLATE_DIR_PREFIX}}/pages/exploration_editor/AngularNameService.js"></script>


Are these really needed here ?

Done. They are removed now.

If you are checking if these services are dependent on another, should I generally just do a grep on the file names and see if they are referenced by files within another directory i.e. collection_editor?

Oh, I meant you had to analyse it before removing! I don't know for sure whether all would be removed or not.
Check out this, here I have tried to explain how I would go about analysing it. (See the comments for file admin.html). Let me know if you have any questions :)

Thanks for the link with the great explanation.
So ExplorationDataService is a dependency within EditorServices; EditorStateService is a dependency within RouterService; and AngularNameService is a dependency within EditorServices. EditorServices and RouterService are imported in collection_editor, so I believe they are needed here if my thinking is correct?

Hi @bching, yup as much as I've checked these are needed here.
In general, you had to see, for example, if we need to include AngularNameService.js:
-AngularNameService is being directly used anywhere in collection_editor (say, there was a direct mention of AngularNameService in any of the services defined in collection_editor)
or
-Any other service which depends on AngularNameService (directly or indirectly) is being used in collection_editor.

We have AngularNameService defined in EditorServices.js and it is a dependency in ExplorationStateService (also defined in EditorServices.js). Now it might not be enough to say that as EditorServices.js is imported in collection_editor, so will also need AngularNameService.js. As it can be the case,(just for example sake consider), that in collection_editor we were using the changeListService only, because of which we needed to import EditorServices but then there would have been no need of importing AngularNameService too as changeListService does not depend on it.
In short you had to see the subset of services defined in EditorServices.js that depend on AngularNameService and whether those services were being used in collection_editor.

The reason why we need AngularNameService is because explorationStatesService has it as a dependency, and RouterService (defined in RouterService.js) has explorationStatesService as a dependency (so indirect dependency on AngularNameService). And in collection_editor we need RouterService.js, so we check which service defined in RouterService.js is actually needed by collection_editor. But as there is just one service present in RouterService.js we know for sure that RouterService is the one needed by collection_editor. So we can now say for sure that AngularNameService.js is also required by collection_editor.

Hope I haven't confused you :)

Then, one possible outcome of extracting these services into discrete files is a reduction in the number of imports in a page?

That was a good explanation. Thank you for clearing things up further.

Well yup, reduction in some pages and increase for some. I think the advantage of having separate file for each service, in terms of import would be that now each page would be including only the services really needed by it.

shubha1593 · 2017-11-01T11:20:13Z

core/templates/dev/head/pages/exploration_editor/EditorServices.js

@@ -578,11 +382,12 @@ oppia.factory('explorationRightsService', [
        var that = this;

        var explorationModeratorRightsUrl = (
-          '/createhandler/moderatorrights/' + explorationData.explorationId);
+          '/createhandler/moderatorrights/' + 
+            ExplorationDataService.explorationId);


Please indent it exactly below the above line ?

I'm sorry if I misunderstood the request, but I removed the indent below line 385. Or should I increase the indent?

Yup, right had to indent line 386 to be exactly under line 385 like

shubha1593 · 2017-11-01T11:21:33Z

core/templates/dev/head/pages/exploration_editor/statistics_tab/StatisticsTab.js

      } else {
        $scope.explorationStatisticsUrl = (
-          '/createhandler/statistics_old/' + explorationData.explorationId +
+          '/createhandler/statistics_old/' + 
+          ExplorationDataService.explorationId +
          '/' + version);


This can be moved to the previous line.

@bching, why am I not able to see this change ? Are you sure you committed it ?

@shubha1593 I'm sorry I rolled back some changes because of the Travis errors. I moved it to the previous line now.

@shubha1593 I restarted the linter job on Travis but it appears it failed on the new commit. Perhaps it is a bit too long for the linter to pass?

@bching Oops sorry, my bad! You're right.

Oh no wait, you moved ExplorationDataService.explorationId + to previous line. I meant move '/' + version); to previous line like

@shubha1593 Ahhh, all right I have done so now.

shubha1593

Thanks @bching and so sorry for being late! I have left a few inline comments.

…act-editorservice

pranavsid98 · 2017-11-07T03:50:08Z

Hi @shubha1593 . I think this PR is up for a review

bching · 2017-11-07T09:53:02Z

@pranavsid98 Ah not quite yet, I trying to resolve the Travis errors from the last merge.

…nto extract-editorservice" This reverts commit d98998c, reversing changes made to 84dcff2.

bching · 2017-11-08T11:30:26Z

@shubha1593 This PR is up for review now.

seanlip · 2017-11-12T01:10:10Z

I think there's a line-length error, @bching? See https://travis-ci.org/oppia/oppia/jobs/300599652#L1256

bching · 2017-11-12T14:33:41Z

@seanlip Yes, I was fiddling with the wrong line in a previous commit, it should be resolved now.

shubha1593 · 2017-11-12T19:24:17Z

core/templates/dev/head/pages/exploration_editor/AngularNameService.js

+// limitations under the License.
+
+/**
+ * @fileoverview A service that maps IDs to Angular names


Please add a period(.) at the end.

shubha1593 · 2017-11-12T19:24:38Z

core/templates/dev/head/pages/exploration_editor/EditorStateService.js

+
+/**
+ * @fileoverview A service that maintains a record of which state 
+ * in the exploration is currently active


Same, add period (.) at the end.

shubha1593

Hi @bching ,
I have left some inline comments. Sorry for not catching them!

shubha1593 · 2017-11-14T02:11:32Z

core/templates/dev/head/pages/exploration_editor/editor_tab/StateEditor.js

@@ -184,3 +184,100 @@ oppia.directive('trainingPanel', [
    };
  }
 ]);
+
+oppia.directive('trainingPanel', [function() {


@bching trainingPanel directive is already there right? I guess you added a second copy here.

@shubha1593 Sorry, it looks like I somehow did. Removed now.

shubha1593 · 2017-11-14T02:11:57Z

core/templates/dev/head/pages/exploration_editor/editor_tab/StateEditorSpec.js

@@ -124,4 +124,336 @@ describe('State Editor controller', function() {
      expect(scs.savedMemento.getHtml()).toEqual('This is some content.');
    });
  });
+
+  describe('TrainingDataService', function() {


@bching Why have you added this here? We have a separate Spec file for it TrainingDataServiceSpec.js, right?

@shubha1593 I'm sorry, it wasn't intentional. I'm not sure how I ended up inserting it there... Removed it now.

shubha1593

Great, thanks @bching, LGTM!

shubha1593 · 2017-11-15T16:16:18Z

core/templates/dev/head/pages/exploration_editor/ExplorationDataServiceSpec.js

@@ -0,0 +1,116 @@
+


@bching Please remove this line.

@shubha1593 Done.

shubha1593 · 2017-11-15T16:18:03Z

Hey @bching, just one more minor change required, then it's good to merge!

…t-p2 * upstream/develop: (129 commits) Fix 3946: Add visualization for Item selection inputs (oppia#4080) Remove feedback button from mobile learner view. (oppia#4093) Fix oppia#3289: Introduce domain objects for answer calculation output (oppia#4036) Renamed RTC_specs to RTC_definitions and removed useless __init__.py file. (oppia#4101) Fix exploration graph directive URL. (oppia#4098) Fix error in one off stats migration job. (oppia#4068) Add state id mapping mapreduce job to the job registry (oppia#4099) Added underline to 'Back to collection' link (oppia#4096) Minor updates to CHANGELOG v2.5.6 after proofreading (oppia#4097) Update Authors, Contributors, Changelog, Credits for Release 2.5.6. (oppia#4095) Prevent hint tooltip from going offscreen on mobile (oppia#4087) Implement one off MapRreduce job to generate state id mapping model for explorations. (oppia#4088) Routine update of translations. (oppia#4082) Fix part of oppia#3954: Created a Page Object for Thanks Page (oppia#4085) Rename CSS property oppia-learner-continue-button to oppia-learner-confirm-button; Make the Submit button in ItemSelectionInput the same style as the Continue button; Move informational text in ItemSelectionInput from bottom-right to top of the interaction so that it is more visible; Add extra text to ItemSelectionInput indicating to the learner that they may select more than one choice. (oppia#4083) Stop cards from going offscreen in mobile learner view. (oppia#4084) Upgrade SSL library version; remove obsolete challenge-response handler. (oppia#4081) Fix part of oppia#3826: extract from EditorServices.js (oppia#3970) Fix part of oppia#3826: Extract services from app.js (oppia#4079) Fix oppia#4055: Replaced redundant CSS selectors. (oppia#4059) ...

Partial Fix oppia#3826: extract from EditorServices.js

bf61e1f

* explorationData to ExplorationDataService * editorContextService to EditorStateService * angularNameService to AngularNameService

bching requested a review from shubha1593 October 14, 2017 11:28

bching added 3 commits October 15, 2017 20:16

Merge branch 'develop' into extract-editorservice

85388f9

fix merge mistakes

a05df4b

Merge remote-tracking branch 'origin/extract-editorservice' into extr…

675cf07

…act-editorservice

fix e2e errors and linting errors

e2c4ff3

seanlip assigned shubha1593 Oct 23, 2017

anthkris assigned bching and unassigned shubha1593 Oct 25, 2017

remove new line linter error

587a84f

bching added 2 commits October 28, 2017 16:23

Merge branch 'develop' into extract-editorservice

55ad344

Update StatisticsTab.js

84dcff2

fix line length

shubha1593 reviewed Nov 1, 2017

View reviewed changes

shubha1593 suggested changes Nov 1, 2017

View reviewed changes

bching added 2 commits November 4, 2017 22:06

fix format and remove includes

106e41f

Merge remote-tracking branch 'origin/extract-editorservice' into extr…

d98998c

…act-editorservice

pranavsid98 assigned shubha1593 and unassigned bching Nov 7, 2017

Revert "Merge remote-tracking branch 'origin/extract-editorservice' i…

6de9b5f

…nto extract-editorservice" This reverts commit d98998c, reversing changes made to 84dcff2.

bching force-pushed the extract-editorservice branch from 7059120 to 6de9b5f Compare November 7, 2017 12:30

shubha1593 assigned bching Nov 11, 2017

move to previous line

ad62ead

bching force-pushed the extract-editorservice branch from dc3a817 to ad62ead Compare November 12, 2017 14:30

shubha1593 reviewed Nov 12, 2017

View reviewed changes

shubha1593 suggested changes Nov 12, 2017

View reviewed changes

add periods

8fae1c1

shubha1593 reviewed Nov 14, 2017

View reviewed changes

shubha1593 changed the title ~~Partial Fix #3826: extract from EditorServices.js~~ Fix part of #3826: extract from EditorServices.js Nov 14, 2017

remove erroneous additions

323e730

shubha1593 approved these changes Nov 14, 2017

View reviewed changes

Merge branch 'develop' into extract-editorservice

31ef802

shubha1593 reviewed Nov 15, 2017

View reviewed changes

remove line

2e614c0

shubha1593 merged commit c3457f6 into oppia:develop Nov 17, 2017

seanlip mentioned this pull request Dec 30, 2017

Extract Angular services in the frontend into individual files. #3826

Closed

57 tasks

U8NWXD mentioned this pull request Apr 1, 2020

Protractor Flakes with Error Other element would receive the click #8962

Closed

Fix part of #3826: extract from EditorServices.js #3970

Fix part of #3826: extract from EditorServices.js #3970

Conversation

bching commented Oct 14, 2017

shubha1593 commented Oct 15, 2017

shubha1593 commented Oct 15, 2017

codecov-io commented Oct 23, 2017 • edited Loading

Codecov Report

shubha1593 commented Oct 25, 2017

seanlip commented Oct 25, 2017

shubha1593 commented Oct 25, 2017

bching commented Oct 27, 2017

seanlip commented Oct 27, 2017

bching commented Oct 27, 2017

seanlip commented Oct 27, 2017

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

shubha1593 Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubha1593 Nov 1, 2017 • edited Loading

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

shubha1593 Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubha1593 left a comment

Choose a reason for hiding this comment

pranavsid98 commented Nov 7, 2017

bching commented Nov 7, 2017

bching commented Nov 8, 2017

seanlip commented Nov 12, 2017

bching commented Nov 12, 2017

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

shubha1593 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubha1593 Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubha1593 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubha1593 commented Nov 15, 2017

codecov-io commented Oct 23, 2017 •

edited

Loading

shubha1593 Nov 10, 2017 •

edited

Loading

shubha1593 Nov 1, 2017 •

edited

Loading

shubha1593 Nov 12, 2017 •

edited

Loading

shubha1593 Nov 14, 2017 •

edited

Loading