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.

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

Upgrade more services to Angular 8 #7145

Merged
merged 38 commits into from
Jul 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
13424bf
Untested work on PlaythroughIssueObjectFactory.ts
YashJipkate Jul 8, 2019
5c8bec3
Revert "Untested work on PlaythroughIssueObjectFactory.ts"
YashJipkate Jul 8, 2019
3b51282
Merge branch 'develop' into ser-up-more
YashJipkate Jul 8, 2019
3ae97e8
TopicRightsObjectFactory
YashJipkate Jul 8, 2019
7f22747
Lint fix
YashJipkate Jul 8, 2019
ffa8a7e
Removed unnecessary constructor statement
YashJipkate Jul 8, 2019
952d5c0
ClassifierObjectFactory
YashJipkate Jul 8, 2019
41987d5
AnswerClassificationResultObjectFactory
YashJipkate Jul 9, 2019
9990fe5
PredictionResultObjectFactory
YashJipkate Jul 9, 2019
027340d
WrittenTranslationObjectFactory
YashJipkate Jul 9, 2019
cc0ec34
RuleObjectFactory
YashJipkate Jul 9, 2019
906fd76
AudioTranslationObjectFactory
YashJipkate Jul 9, 2019
5b56005
ExplorationDraftObjectFactory
YashJipkate Jul 9, 2019
1502ad2
Fix failing FE in TopicRightsObjectFactory
YashJipkate Jul 10, 2019
f3a0997
Merge branch 'develop' into ser-up-more
YashJipkate Jul 10, 2019
5e96b0f
Add force bootstrap to all pages
YashJipkate Jul 10, 2019
6f906ef
function() -> () =>
YashJipkate Jul 10, 2019
feb8a4c
Merge
YashJipkate Jul 11, 2019
56f61f0
Merge proper
YashJipkate Jul 11, 2019
64f9fea
Merge branch 'develop' into ser-up-more
YashJipkate Jul 14, 2019
69681da
Update mocking method
YashJipkate Jul 14, 2019
04d7091
Merge branch 'develop' into ser-up-more
YashJipkate Jul 15, 2019
727a6bf
More typing
YashJipkate Jul 16, 2019
abb470f
Audit and newline
YashJipkate Jul 16, 2019
d8e573b
Merge branch 'develop' into ser-up-more
YashJipkate Jul 16, 2019
6fa6c50
Merge develop
YashJipkate Jul 16, 2019
0ca2eae
Merge branch 'develop' into ser-up-more
YashJipkate Jul 17, 2019
5c75b38
Stronger typing
YashJipkate Jul 17, 2019
0e557bf
Fix FE
YashJipkate Jul 17, 2019
dff56fd
Address comments
YashJipkate Jul 17, 2019
e26e464
Remove newlines, TODO for imports, modify MockOutcome
YashJipkate Jul 18, 2019
aa5133d
Fix matcher case, modify TODO for imports
YashJipkate Jul 18, 2019
951982b
Stronger typing in RuleObjectFactory
YashJipkate Jul 18, 2019
de7d2ad
Fix e2e
YashJipkate Jul 19, 2019
ce5cec8
Fix AJAX errors in Karma
YashJipkate Jul 19, 2019
b5c2ef9
Merge develop
YashJipkate Jul 19, 2019
b6f243f
Fix FE
YashJipkate Jul 19, 2019
3df1a3d
Add comment in csrftoken
YashJipkate Jul 19, 2019
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
13 changes: 13 additions & 0 deletions core/templates/dev/head/AppSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,21 @@
* @fileoverview Unit tests for generic services.
*/

// TODO(YashJipkate) Remove the following block of unnnecessary imports once
// App.ts is upgraded to Angular 8.
import { RuleObjectFactory } from 'domain/exploration/RuleObjectFactory.ts';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why these need to be added here. They don't seem to be used in the test?

There should probably be documentation somewhere to explain -- either here if it's specific to this, or in our wiki if it's a general principle to follow. In any case I think it would be nice to add a comment above line 25 explaining why lines 26-29 are needed. Do the same in all other similar files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not specific to this file, this is the case in many files. What's happening is that since we do not use isolated tests, these spec files depended upon the upgraded services and therefore angular mock could not interpret them.
This would be more suited to the wiki than a comment. I'll add this to the Angular wiki.

Copy link
Member

Choose a reason for hiding this comment

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

Is this something that would be removed after the full migration to Angular 8?

If so, we should add a TODO to remove them in the future, like Vojta did for some of the webpack requires. I realize that that might be repetitive, but we should do that anyway to make clear that this isn't a long-term practice to be followed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The statements can be removed once the corresponding non-spec file is upgraded.
Added the TODOs!

import { WrittenTranslationObjectFactory } from
'domain/exploration/WrittenTranslationObjectFactory.ts';
// ^^^ This block is to be removed.

describe('Constants Generating', function() {
beforeEach(angular.mock.module('oppia'));
beforeEach(angular.mock.module('oppia', function($provide) {
$provide.value(
'WrittenTranslationObjectFactory',
new WrittenTranslationObjectFactory());
$provide.value('RuleObjectFactory', new RuleObjectFactory());
}));

var $injector = null;
beforeEach(angular.mock.inject(function(_$injector_) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@
* @fileoverview Unit tests for the question player state service.
*/

// TODO(YashJipkate) Remove the following block of unnnecessary imports once
// question-player-state.service.ts is upgraded to Angular 8.
import { RuleObjectFactory } from 'domain/exploration/RuleObjectFactory.ts';
import { WrittenTranslationObjectFactory } from
'domain/exploration/WrittenTranslationObjectFactory.ts';
// ^^^ This block is to be removed.

require(
'components/question-directives/question-player/services/' +
'question-player-state.service.ts');
Expand All @@ -28,7 +35,12 @@ describe('Question player state service', function() {
var question;

beforeEach(angular.mock.module('oppia'));

beforeEach(angular.mock.module('oppia', function($provide) {
$provide.value('RuleObjectFactory', new RuleObjectFactory());
$provide.value(
'WrittenTranslationObjectFactory',
new WrittenTranslationObjectFactory());
}));
beforeEach(angular.mock.inject(function($injector) {
qpservice = $injector.get('QuestionPlayerStateService');
QuestionObjectFactory = $injector.get('QuestionObjectFactory');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,21 @@
* editor page.
*/

// TODO(YashJipkate) Remove the following block of unnnecessary imports once
// state-property.service.ts is upgraded to Angular 8.
import { ExplorationDraftObjectFactory } from
'domain/exploration/ExplorationDraftObjectFactory.ts';
// ^^^ This block is to be removed.

require('pages/exploration-editor-page/services/change-list.service.ts');
require('pages/exploration-editor-page/services/exploration-title.service.ts');

describe('Change list service', function() {
beforeEach(angular.mock.module('oppia'));
beforeEach(angular.mock.module('oppia', function($provide) {
$provide.value(
'ExplorationDraftObjectFactory', new ExplorationDraftObjectFactory());
}));

describe('change list service', function() {
var cls = null;
Expand Down Expand Up @@ -205,6 +215,10 @@ describe('Change list service', function() {

describe('Exploration title service', function() {
beforeEach(angular.mock.module('oppia'));
beforeEach(angular.mock.module('oppia', function($provide) {
$provide.value(
'ExplorationDraftObjectFactory', new ExplorationDraftObjectFactory());
}));

describe('exploration title service', function() {
var ets = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@
* @fileoverview Unit tests for the controller of 'State Interactions'.
*/

// TODO(YashJipkate) Remove the following block of unnnecessary imports once
// state-interaction-editor.directive.ts is upgraded to Angular 8.
import { ExplorationFeaturesService } from
'services/ExplorationFeaturesService.ts';
// ^^^ This block is to be removed.

require(
'pages/exploration-editor-page/editor-tab/' +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,39 @@
* Classification Result domain objects.
*/

var oppia = require('AppInit.ts').module;
import { downgradeInjectable } from '@angular/upgrade/static';
import { Injectable } from '@angular/core';

export class AnswerClassificationResult {
outcome: any;
answerGroupIndex: number;
ruleIndex: number;
classificationCategorization: string;

oppia.factory('AnswerClassificationResultObjectFactory', [function() {
var AnswerClassificationResult = function(
outcome, answerGroupIndex, ruleIndex, classificationCategorization) {
constructor(
outcome: any, answerGroupIndex: number, ruleIndex: number,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't outcome be an Outcome domain object? I thought we were veering away from using "any", in general.

Please try to replace "any" with something more specific in all other files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The complete elimination of any would require extensive examination of the parameters, therefore I and @kevinlee12 decided that we would have specific types for the primitive ones and any for the complex ones such as the objects.
I have replaced any with specific types wherever they are primitive and are visible directly (like in their corresponding spec).
Should we track this by filing an issue to completely remove any from our codebase? I will try to do it if time permits since complete elimination would require a considerable amount of of effort.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's file an issue. But I want to dig deeper into this particular case. What would 'outcome' end up being typed as? Let's at least establish a pattern for getting the right types for objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #7165.

I think if outcome is a Outcome domain object (as you said), then that class could be imported and then the outcome can be declared as Outcome type.

Copy link
Member

@seanlip seanlip Jul 18, 2019

Choose a reason for hiding this comment

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

Try doing that here (in this PR), just for one case. Then link to this in the issue you filed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Outcome object is defined in OutcomeObjectFactory.ts which is still in AngularJS, therefore it does not have any Outcome class which could be imported, once that class gets created, this can be done.

Copy link
Member

@seanlip seanlip Jul 18, 2019

Choose a reason for hiding this comment

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

OK, here's what I suggest then. Can we type everything, except for things that can't be typed because they are still in AngularJS? I.e., instead of "complex objects" as the criterion, it should be specifically "objects that are still in AngularJS"?

In general, typing things should not be hard. The specific issue you mention is a fair one but what we should be doing, e.g., is to update the typing for "outcomes" throughout the codebase in the same PR that migrates OutcomeObjectFactory to Angular. This is fine to do on a best-effort basis and we can do a final round of cleanup after everything is migrated (per #7165), but we shouldn't wilfully ignore it.

Does this sound OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that sounds pretty good and I am currently also trying to eliminate as much any types as I can and will also keep that in mind for the future migrations.

But the problem of determining the correct type of some complex types (like arrays) and types of those with no spec files from which I could determine the types is super difficult, especially because we use an elaborate network of services so the parameters to a service in a non-spec file is always a reference to some other service and the loop goes on, sometimes even reaching the backend. This would take a great amount of time. That's why I came up with the 'complex' basis.

Copy link
Member

Choose a reason for hiding this comment

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

Understood. Thanks @YashJipkate! I do get the point about complex types (though arrays of primitives ought to be doable...) and I am fine with leaving actually-complex types till a final pass. I just think that currently the bar of "complex" == "not primitive" is set a little too low.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that can be definitely done and I am trying to do as much as possible! Please let me know if I missed out something.

classificationCategorization: string) {
this.outcome = outcome;
this.answerGroupIndex = answerGroupIndex;
this.ruleIndex = ruleIndex;
this.classificationCategorization = classificationCategorization;
};
}
}

// TODO (ankita240796) Remove the bracket notation once Angular2 gets in.
/* eslint-disable dot-notation */
AnswerClassificationResult['createNew'] = function(
/* eslint-enable dot-notation */
outcome, answerGroupIndex, ruleIndex, classificationCategorization) {
@Injectable({
providedIn: 'root'
})
export class AnswerClassificationResultObjectFactory {
createNew(
outcome: any, answerGroupIndex: number, ruleIndex: number,
classificationCategorization: string) {
return new AnswerClassificationResult(
outcome, answerGroupIndex, ruleIndex, classificationCategorization);
};
return AnswerClassificationResult;
}]);
}
}

var oppia = require('AppInit.ts').module;

oppia.factory(
'AnswerClassificationResultObjectFactory',
downgradeInjectable(AnswerClassificationResultObjectFactory));
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,74 @@
* @fileoverview Unit tests for the AnswerClassificationResultObjectFactory.
*/

require('domain/classifier/AnswerClassificationResultObjectFactory.ts');
import { AnswerClassificationResultObjectFactory } from
'domain/classifier/AnswerClassificationResultObjectFactory.ts';

require('domain/exploration/OutcomeObjectFactory.ts');
require(
'pages/exploration-player-page/services/answer-classification.service.ts');

describe('Answer classification result object factory', function() {
var oof, acrof;
var DEFAULT_OUTCOME_CLASSIFICATION;
class MockSubtitledHtml {
_html: string;
_contentId: string;
constructor(html: string, contentId: string) {
this._html = html;
this._contentId = contentId;
}
}

class MockOutcome {
kevinlee12 marked this conversation as resolved.
Show resolved Hide resolved
dest: string;
feedback: MockSubtitledHtml;
labelledAsCorrect: boolean;
paramChanges: any;
refresherExplorationId: any;
missingPrerequisiteSkillId: any;
constructor(
dest: string, feedback: MockSubtitledHtml, labelledAsCorrect: boolean,
paramChanges: any, refresherExplorationId: any,
missingPrerequisiteSkillId: any) {
this.dest = dest;
this.feedback = feedback;
this.labelledAsCorrect = labelledAsCorrect;
this.paramChanges = paramChanges;
this.refresherExplorationId = refresherExplorationId;
this.missingPrerequisiteSkillId = missingPrerequisiteSkillId;
}
}

beforeEach(angular.mock.module('oppia'));
class MockOutcomeObjectFactory {
kevinlee12 marked this conversation as resolved.
Show resolved Hide resolved
createNew(
dest: string, feedbackTextId: string, feedbackText: string,
paramChanges: any) {
return new MockOutcome(
dest,
new MockSubtitledHtml(feedbackText, feedbackTextId),
false,
paramChanges,
null,
null);
}
}

beforeEach(angular.mock.inject(function($injector) {
acrof = $injector.get('AnswerClassificationResultObjectFactory');
oof = $injector.get('OutcomeObjectFactory');
DEFAULT_OUTCOME_CLASSIFICATION = $injector.get(
'DEFAULT_OUTCOME_CLASSIFICATION');
}));
describe('Answer classification result object factory', () => {
let acrof: AnswerClassificationResultObjectFactory;
let oof: MockOutcomeObjectFactory;
let DEFAULT_OUTCOME_CLASSIFICATION: string;

beforeEach(() => {
acrof = new AnswerClassificationResultObjectFactory();
oof = new MockOutcomeObjectFactory();
DEFAULT_OUTCOME_CLASSIFICATION = 'default_outcome';
});

it('should create a new result', function() {
it('should create a new result', () => {
var answerClassificationResult = acrof.createNew(
oof.createNew('default', '', []), 1, 0, DEFAULT_OUTCOME_CLASSIFICATION
oof.createNew('default', '', '', []), 1, 0, DEFAULT_OUTCOME_CLASSIFICATION
);

expect(answerClassificationResult.outcome).toEqual(
oof.createNew('default', '', []));
oof.createNew('default', '', '', []));
expect(answerClassificationResult.answerGroupIndex).toEqual(1);
expect(answerClassificationResult.ruleIndex).toEqual(0);
expect(answerClassificationResult.classificationCategorization).toEqual(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,33 @@
* domain objects.
*/

var oppia = require('AppInit.ts').module;
import { downgradeInjectable } from '@angular/upgrade/static';
import { Injectable } from '@angular/core';

export class Classifier {
algorithmId: string;
classifierData: any;
dataSchemaVersion: number;

oppia.factory('ClassifierObjectFactory', [function() {
var Classifier = function(algorithmId, classifierData, dataSchemaVersion) {
constructor(
algorithmId: string, classifierData: any, dataSchemaVersion: number) {
this.algorithmId = algorithmId;
this.classifierData = classifierData;
this.dataSchemaVersion = dataSchemaVersion;
};
}
}

// TODO (ankita240796) Remove the bracket notation once Angular2 gets in.
/* eslint-disable dot-notation */
Classifier['create'] = function(
/* eslint-enable dot-notation */
algorithmId, classifierData, dataSchemaVersion) {
@Injectable({
providedIn: 'root'
})
export class ClassifierObjectFactory {
create(algorithmId: string, classifierData: any, dataSchemaVersion: number) {
return new Classifier(algorithmId, classifierData, dataSchemaVersion);
};
}
}

var oppia = require('AppInit.ts').module;

return Classifier;
}]);
oppia.factory(
'ClassifierObjectFactory',
downgradeInjectable(ClassifierObjectFactory));
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,19 @@
* @fileoverview Unit tests for the ClassifierObjectFactory.
*/

require('domain/classifier/ClassifierObjectFactory.ts');
import { ClassifierObjectFactory } from
'domain/classifier/ClassifierObjectFactory.ts';

describe('Classifier Object Factory', function() {
var ClassifierObjectFactory;
describe('Classifier Object Factory', () => {
let classifierObjectFactory: ClassifierObjectFactory;

beforeEach(angular.mock.module('oppia'));

beforeEach(angular.mock.inject(function($injector) {
ClassifierObjectFactory = $injector.get('ClassifierObjectFactory');
}));
beforeEach(() => {
classifierObjectFactory = new ClassifierObjectFactory();
});

it('should create a new classifier', function() {
it('should create a new classifier', () => {
var classifierObject = (
ClassifierObjectFactory.create('TestClassifier', {}, 1));
classifierObjectFactory.create('TestClassifier', {}, 1));

expect(classifierObject.algorithmId).toEqual('TestClassifier');
expect(classifierObject.classifierData).toEqual({});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
* result domain objects.
*/

var oppia = require('AppInit.ts').module;
import { downgradeInjectable } from '@angular/upgrade/static';
import { Injectable } from '@angular/core';

oppia.factory('PredictionResultObjectFactory', [function() {
export class PredictionResult {
/**
* Stores the prediction result for an answer as returned by the
* various prediction services used in Oppia for Machine Learning based
Expand All @@ -31,31 +32,31 @@ oppia.factory('PredictionResultObjectFactory', [function() {
* its prediction label. The value is probability (between 0 and 1) that
* answer belongs to predicted answer group.
*/
var predictionResult = function(label, confidence) {
predictionLabel: number;
predictionConfidence: number;
constructor(label: number, confidence: number) {
this.predictionLabel = label;
this.predictionConfidence = confidence;
};

// TODO (ankita240796) Remove the bracket notation once Angular2 gets in.
/* eslint-disable dot-notation */
predictionResult['createNew'] = function(label, confidence) {
/* eslint-enable dot-notation */
return new predictionResult(label, confidence);
};
}
}

// TODO (ankita240796) Remove the bracket notation once Angular2 gets in.
/* eslint-disable dot-notation */
predictionResult['getLabel'] = function() {
/* eslint-enable dot-notation */
@Injectable({
kevinlee12 marked this conversation as resolved.
Show resolved Hide resolved
providedIn: 'root'
})
export class PredictionResultObjectFactory extends PredictionResult {
createNew(label: number, confidence: number) {
return new PredictionResult(label, confidence);
}
getLabel() {
return this.predictionLabel;
};

// TODO (ankita240796) Remove the bracket notation once Angular2 gets in.
/* eslint-disable dot-notation */
predictionResult['getConfidence'] = function() {
/* eslint-enable dot-notation */
}
getConfidence() {
return this.predictionConfidence;
};
}
}

var oppia = require('AppInit.ts').module;

return predictionResult;
}]);
oppia.factory(
'PredictionResultObjectFactory',
downgradeInjectable(PredictionResultObjectFactory));
Loading