Upgrade more services to Angular 8 #7145

YashJipkate · 2019-07-15T06:33:13Z

Explanation

Upgrades:

AnswerClassificationResultObjectFactory
ClassifierObjectFactory
ExplorationDraftObjectFactory
PredictionResultObjectFactory
RuleObjectFactory
TopicRightsObjectFactory
WrittenTranslationObjectFactory

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.

This reverts commit 13424bf.

kevinlee12 · 2019-07-18T23:12:18Z

@YashJipkate, ptal at the failing tests! (I've restarted them after Sean restarted them and it still fails.)

oppiabot · 2019-07-19T05:32:33Z

Hi @YashJipkate. 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 · 2019-07-19T05:45:09Z

core/templates/dev/head/domain/topic/TopicRightsObjectFactory.ts

-        canPublishTopicBool: boolean;
-        canEditTopicBool: boolean;
-      }) {
+      topicRightsBackendObject: any) {


This concerns me. Why does typing raise errors?

This is because if I type it then can_publish_topic would raise a lint error, so I changed it to camelCase. But this also needs to changed everywhere it is being referenced and most importantly in all of the backend since it is passed as a dict with these keys. So, this comes under 'complex' criteria and I decided to type it as any

I think we should file an issue to overcome this. Note that this might be tricky because I think pylint has a rule against naming in camelCase and favors underscore_case.

What do you think?

Yes, I understand the complexity. One thing I'd say is -- for anything like this, please file an issue proactively (and leave a comment here pointing to it), don't wait for a reviewer to point it out. Thanks!

Okay I will keep that in mind in the future.
Raised #7176,
Thanks!

seanlip · 2019-07-19T05:45:25Z

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

@@ -15,6 +15,9 @@
 /**
 * @fileoverview Service for managing CSRF tokens.
 */
+
+import * as $ from 'jquery';


This is a bit concerning. What's going on here?

This is actually a fix to something not going well with Karma. After the introduction of Angular and upgrading some services, this service started to come under Angular zone due to it being dependency, now the zone was not able to find AJAX directly as a global like we do in AngularJS (the same reason we cannot upgrade the constants like INTERACTION_SPECS which are defined in a global variable).

And hence we need to import it specifically, like we do for all the Angular things (like the imports in AppInit.ts are explicit and do not depend on global definitions)

There needs to be a comment for things like this, which explains what is going on and that points to an authoritative link explaining the context behind such a solution. Developers won't understand it.

Okay I will add a comment to it.
But about a link, I do not have one because I discovered this fix myself. There is one open question in which the problem of OP is the same as mine but he also did not get a correct answer, I myself have posted an answer on it which was this very fix. I don't know which link to provide.

Could you please give some suggestions?

You can link to that thread. Also, explain in the comment what this syntax is actually doing.

(Thanks for helping other folks on StackOverflow, btw!)

YashJipkate · 2019-07-19T08:22:10Z

@seanlip @kevinlee12 The tests pass now. PTAL!

seanlip

OK, lgtm!

YashJipkate added 21 commits July 8, 2019 09:46

Untested work on PlaythroughIssueObjectFactory.ts

13424bf

Revert "Untested work on PlaythroughIssueObjectFactory.ts"

5c8bec3

This reverts commit 13424bf.

Merge branch 'develop' into ser-up-more

3b51282

TopicRightsObjectFactory

3ae97e8

Lint fix

7f22747

Removed unnecessary constructor statement

ffa8a7e

ClassifierObjectFactory

952d5c0

AnswerClassificationResultObjectFactory

41987d5

PredictionResultObjectFactory

9990fe5

WrittenTranslationObjectFactory

027340d

RuleObjectFactory

cc0ec34

AudioTranslationObjectFactory

906fd76

ExplorationDraftObjectFactory

5b56005

Fix failing FE in TopicRightsObjectFactory

1502ad2

Merge branch 'develop' into ser-up-more

f3a0997

Add force bootstrap to all pages

5e96b0f

function() -> () =>

6f906ef

Merge

feb8a4c

Merge proper

56f61f0

Merge branch 'develop' into ser-up-more

64f9fea

Update mocking method

69681da

YashJipkate added the CHANGELOG: Infrastructural changes label Jul 15, 2019

YashJipkate requested review from aks681, ankita240796, brianrodri, DubeySandeep, kevinlee12, nithusha21, prasanna08 and seanlip as code owners July 15, 2019 06:33

YashJipkate closed this Jul 18, 2019

YashJipkate reopened this Jul 18, 2019

YashJipkate closed this Jul 18, 2019

YashJipkate reopened this Jul 18, 2019

kevinlee12 assigned YashJipkate and unassigned seanlip Jul 18, 2019

Fix e2e

de7d2ad

YashJipkate closed this Jul 19, 2019

YashJipkate reopened this Jul 19, 2019

Fix AJAX errors in Karma

ce5cec8

YashJipkate requested a review from jameesjohn as a code owner July 19, 2019 05:32

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

YashJipkate added 2 commits July 19, 2019 11:04

Merge develop

b5c2ef9

Fix FE

b6f243f

seanlip reviewed Jul 19, 2019

View reviewed changes

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

Add comment in csrftoken

3df1a3d

YashJipkate closed this Jul 19, 2019

YashJipkate reopened this Jul 19, 2019

YashJipkate assigned seanlip and unassigned YashJipkate Jul 19, 2019

seanlip approved these changes Jul 19, 2019

View reviewed changes

seanlip merged commit f88216a into oppia:develop Jul 19, 2019

This was referenced Jul 22, 2019

Upgrade more services to Angular 8 - II #7178

Merged

Add comments, types and TODOs in #7145. #7212

Closed

DubeySandeep mentioned this pull request Jul 25, 2019

[Test server] Unable to create a question for skill #7210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade more services to Angular 8 #7145

Upgrade more services to Angular 8 #7145

YashJipkate commented Jul 15, 2019 •

edited by brianrodri

Loading

kevinlee12 commented Jul 18, 2019 •

edited

Loading

oppiabot bot commented Jul 19, 2019

seanlip Jul 19, 2019

YashJipkate Jul 19, 2019

seanlip Jul 19, 2019 •

edited

Loading

YashJipkate Jul 19, 2019

seanlip Jul 19, 2019

YashJipkate Jul 19, 2019

YashJipkate Jul 19, 2019

seanlip Jul 19, 2019 •

edited

Loading

YashJipkate Jul 19, 2019

seanlip Jul 19, 2019

YashJipkate Jul 19, 2019

YashJipkate commented Jul 19, 2019

seanlip left a comment

Upgrade more services to Angular 8 #7145

Upgrade more services to Angular 8 #7145

Conversation

YashJipkate commented Jul 15, 2019 • edited by brianrodri Loading

Explanation

Checklist

kevinlee12 commented Jul 18, 2019 • edited Loading

oppiabot bot commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Jul 19, 2019 • 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

seanlip Jul 19, 2019 • 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

YashJipkate commented Jul 19, 2019

seanlip left a comment

Choose a reason for hiding this comment

YashJipkate commented Jul 15, 2019 •

edited by brianrodri

Loading

kevinlee12 commented Jul 18, 2019 •

edited

Loading

seanlip Jul 19, 2019 •

edited

Loading

seanlip Jul 19, 2019 •

edited

Loading