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

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

Merged
merged 16 commits into from
Nov 17, 2017
Merged

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

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
bf61e1f
Partial Fix #3826: extract from EditorServices.js
bching Oct 14, 2017
85388f9
Merge branch 'develop' into extract-editorservice
bching Oct 15, 2017
a05df4b
fix merge mistakes
bching Oct 15, 2017
675cf07
Merge remote-tracking branch 'origin/extract-editorservice' into extr…
bching Oct 15, 2017
e2c4ff3
fix e2e errors and linting errors
bching Oct 23, 2017
587a84f
remove new line linter error
bching Oct 27, 2017
55ad344
Merge branch 'develop' into extract-editorservice
bching Oct 28, 2017
84dcff2
Update StatisticsTab.js
bching Oct 28, 2017
106e41f
fix format and remove includes
bching Nov 4, 2017
d98998c
Merge remote-tracking branch 'origin/extract-editorservice' into extr…
bching Nov 4, 2017
6de9b5f
Revert "Merge remote-tracking branch 'origin/extract-editorservice' i…
bching Nov 7, 2017
ad62ead
move to previous line
bching Nov 11, 2017
8fae1c1
add periods
bching Nov 13, 2017
323e730
remove erroneous additions
bching Nov 14, 2017
31ef802
Merge branch 'develop' into extract-editorservice
bching Nov 15, 2017
2e614c0
remove line
bching Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/templates/dev/head/components/AnswerGroupEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ oppia.directive('answerGroupEditor', [
'answer_group_editor_directive.html'),
controller: [
'$scope', 'stateInteractionIdService', 'ResponsesService',
'editorContextService', 'alertsService', 'INTERACTION_SPECS',
'EditorStateService', 'alertsService', 'INTERACTION_SPECS',
'RULE_TYPE_CLASSIFIER', 'RuleObjectFactory',
function(
$scope, stateInteractionIdService, ResponsesService,
editorContextService, alertsService, INTERACTION_SPECS,
EditorStateService, alertsService, INTERACTION_SPECS,
RULE_TYPE_CLASSIFIER, RuleObjectFactory) {
$scope.rulesMemento = null;
$scope.activeRuleIndex = ResponsesService.getActiveRuleIndex();
Expand Down
6 changes: 3 additions & 3 deletions core/templates/dev/head/components/OutcomeDestinationEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ oppia.directive('outcomeDestinationEditor', [
'/components/' +
'outcome_destination_editor_directive.html'),
controller: [
'$scope', 'editorContextService', 'explorationStatesService',
'$scope', 'EditorStateService', 'explorationStatesService',
'StateGraphLayoutService', 'PLACEHOLDER_OUTCOME_DEST',
'FocusManagerService', 'editorFirstTimeEventsService',
function(
$scope, editorContextService, explorationStatesService,
$scope, EditorStateService, explorationStatesService,
StateGraphLayoutService, PLACEHOLDER_OUTCOME_DEST,
FocusManagerService, editorFirstTimeEventsService) {
$scope.$on('saveOutcomeDestDetails', function() {
Expand Down Expand Up @@ -62,7 +62,7 @@ oppia.directive('outcomeDestinationEditor', [
$scope.newStateNamePattern = /^[a-zA-Z0-9.\s-]+$/;
$scope.destChoices = [];
$scope.$watch(explorationStatesService.getStates, function() {
var currentStateName = editorContextService.getActiveStateName();
var currentStateName = EditorStateService.getActiveStateName();

// This is a list of objects, each with an ID and name. These
// represent all states, as well as an option to create a
Expand Down
6 changes: 3 additions & 3 deletions core/templates/dev/head/components/OutcomeEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ oppia.directive('outcomeEditor', [
'/components/' +
'outcome_editor_directive.html'),
controller: [
'$scope', 'editorContextService',
'$scope', 'EditorStateService',
'stateInteractionIdService',
function($scope, editorContextService,
function($scope, EditorStateService,
stateInteractionIdService) {
$scope.editOutcomeForm = {};
$scope.feedbackEditorIsOpen = false;
Expand Down Expand Up @@ -83,7 +83,7 @@ oppia.directive('outcomeEditor', [
$scope.isSelfLoop = function(outcome) {
return (
outcome &&
outcome.dest === editorContextService.getActiveStateName());
outcome.dest === EditorStateService.getActiveStateName());
};

$scope.getCurrentInteractionId = function() {
Expand Down
6 changes: 3 additions & 3 deletions core/templates/dev/head/components/ResponseHeaderDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,16 @@ oppia.directive('responseHeader', [
'/components/' +
'response_header_directive.html'),
controller: [
'$scope', 'editabilityService', 'editorContextService', 'RouterService',
'$scope', 'editabilityService', 'EditorStateService', 'RouterService',
'PLACEHOLDER_OUTCOME_DEST',
function(
$scope, editabilityService, editorContextService, RouterService,
$scope, editabilityService, EditorStateService, RouterService,
PLACEHOLDER_OUTCOME_DEST) {
$scope.editabilityService = editabilityService;

$scope.isOutcomeLooping = function() {
var outcome = $scope.getOutcome();
var activeStateName = editorContextService.getActiveStateName();
var activeStateName = EditorStateService.getActiveStateName();
return outcome && (outcome.dest === activeStateName);
};

Expand Down
4 changes: 2 additions & 2 deletions core/templates/dev/head/components/RuleEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ oppia.directive('ruleEditor', [
'/components/' +
'rule_editor_directive.html'),
controller: [
'$scope', '$timeout', 'editorContextService',
'$scope', '$timeout', 'EditorStateService',
'explorationStatesService', 'RouterService', 'ValidatorsService',
'ResponsesService', 'stateInteractionIdService', 'INTERACTION_SPECS',
'RULE_TYPE_CLASSIFIER', function(
$scope, $timeout, editorContextService,
$scope, $timeout, EditorStateService,
explorationStatesService, RouterService, ValidatorsService,
ResponsesService, stateInteractionIdService, INTERACTION_SPECS,
RULE_TYPE_CLASSIFIER) {
Expand Down
12 changes: 6 additions & 6 deletions core/templates/dev/head/components/SolutionEditorDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@

oppia.directive('solutionEditor', [
'$modal', 'UrlInterpolationService', 'stateSolutionService',
'editorContextService', 'explorationStatesService',
'EditorStateService', 'explorationStatesService',
'explorationWarningsService', 'alertsService',
'SolutionObjectFactory', 'SolutionVerificationService',
'explorationContextService', 'oppiaExplorationHtmlFormatterService',
'stateInteractionIdService', 'stateCustomizationArgsService',
'INFO_MESSAGE_SOLUTION_IS_INVALID',
function($modal, UrlInterpolationService, stateSolutionService,
editorContextService, explorationStatesService,
EditorStateService, explorationStatesService,
explorationWarningsService, alertsService,
SolutionObjectFactory, SolutionVerificationService,
explorationContextService, oppiaExplorationHtmlFormatterService,
Expand Down Expand Up @@ -64,11 +64,11 @@ oppia.directive('solutionEditor', [
backdrop: 'static',
controller: [
'$scope', '$modalInstance', 'stateInteractionIdService',
'stateSolutionService', 'editorContextService',
'stateSolutionService', 'EditorStateService',
'oppiaExplorationHtmlFormatterService',
'explorationStatesService',
function($scope, $modalInstance, stateInteractionIdService,
stateSolutionService, editorContextService,
stateSolutionService, EditorStateService,
oppiaExplorationHtmlFormatterService,
explorationStatesService) {
$scope.SOLUTION_EDITOR_FOCUS_LABEL = (
Expand All @@ -79,7 +79,7 @@ oppia.directive('solutionEditor', [
stateInteractionIdService.savedMemento,
/* eslint-disable max-len */
explorationStatesService.getInteractionCustomizationArgsMemento(
editorContextService.getActiveStateName()),
EditorStateService.getActiveStateName()),
/* eslint-enable max-len */
$scope.SOLUTION_EDITOR_FOCUS_LABEL));
$scope.EXPLANATION_FORM_SCHEMA = {
Expand Down Expand Up @@ -119,7 +119,7 @@ oppia.directive('solutionEditor', [
]
}).result.then(function(result) {
var correctAnswer = result.solution.correctAnswer;
var currentStateName = editorContextService.getActiveStateName();
var currentStateName = EditorStateService.getActiveStateName();
var state = explorationStatesService.getState(currentStateName);
SolutionVerificationService.verifySolution(
explorationContextService.getExplorationId(),
Expand Down
3 changes: 3 additions & 0 deletions core/templates/dev/head/pages/collection_editor/collection_editor.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these really needed here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@shubha1593 shubha1593 Nov 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

<script src="{{TEMPLATE_DIR_PREFIX}}/pages/exploration_editor/ExplorationEditor.js"></script>
<script src="{{TEMPLATE_DIR_PREFIX}}/pages/exploration_editor/RouterService.js"></script>

Expand Down
29 changes: 29 additions & 0 deletions core/templates/dev/head/pages/exploration_editor/AngularNameService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2014 The Oppia Authors. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS-IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
* @fileoverview A service that maps IDs to Angular names.
*/

oppia.factory('AngularNameService', [function() {
var angularName = null;

return {
getNameOfInteractionRulesService: function(interactionId) {
angularName = interactionId.charAt(0).toLowerCase() +
interactionId.slice(1) + 'RulesService';
return angularName
}
};
}]);
34 changes: 34 additions & 0 deletions core/templates/dev/head/pages/exploration_editor/AngularNameServiceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright 2014 The Oppia Authors. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS-IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
* @fileoverview Unit test for the Angular names service.
*/

describe('Angular names service', function() {
beforeEach(module('oppia'));

describe('angular name service', function() {
var ans = null;

beforeEach(inject(function($injector) {
ans = $injector.get('AngularNameService');
}));

it('should map interaction ID to correct RulesService', function() {
expect(ans.getNameOfInteractionRulesService('TextInput')).toEqual(
'textInputRulesService');
});
});
});
Loading