Fixes part of #8472: EdiatbleSkillBackendApiService migration to Angular 8 #8863

srijanreddy98 · 2020-03-17T18:11:57Z

Overview

This PR fixes part of Migrate AngularJS services to Angular 8 #8472.
This PR does the following: migrates EditableSkillBackendApiService to angular 8

Essential 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 linter/Karma presubmit checks have passed locally on your machine.
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
- This lets reviewers restart your CircleCI tests for you.
The PR is made from a branch that's not called "develop".

PR Pointers

Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
For what code owners will expect, see the Code Owner's wiki page.
Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays
Never force push. If you do, your PR will be closed.

…arJS to Angular8. Migrate AngularJS files to Angular 8 #8472

…ution-opportunities

This reverts commit b974e78.

Revert yarn lock changes

…ution-opportunities-spec-file

Remove extra space after the last function argument.

…ortunities-spec-file

….com/srijanreddy98/oppia into contribution-opportunities-spec-file

oppiabot · 2020-03-17T18:11:59Z

Hi, @srijanreddy98. This pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

srijanreddy98 · 2020-03-17T19:11:41Z

It doesn't pass the karma tests or e2e tests. I pushed it using --no-verify. The reason for the failing tests is that the Request Interceptor is not intercepting the request.
@bansalnitish need help here. Any changes I need to make so that request interceptor intercepts the request.

…ackend-api-migration

srijanreddy98 · 2020-04-09T07:04:05Z

@sagangwee @DubeySandeep could you ptal

DubeySandeep

@srijanreddy98, I've left a few comments, PTAL!

Also, can you please share the screenshot of a skill created and edited through this branch? (It would be helpful for reviewers)

DubeySandeep · 2020-04-09T14:39:41Z

.github/CODEOWNERS

@@ -330,6 +330,7 @@


 # New structures team.
+/core/templates/domain/skill/skill-domain.types.ts @bansalnitish


Move this inside skill project section (After line 360)

DubeySandeep · 2020-04-09T14:40:00Z

core/templates/domain/skill/skill-backend-api.service.spec.ts

@@ -15,62 +15,53 @@
 /**
 * @fileoverview Unit tests for SkillBackendApiService.
 */
+import { HttpClientTestingModule, HttpTestingController } from


Add a newline above

DubeySandeep · 2020-04-09T14:41:41Z

core/templates/domain/skill/skill-backend-api.service.spec.ts

-// ^^^ This block is to be removed.
-
+import { CsrfTokenService } from
+  'services/csrf-token.service';


Can we adjust this to the line above (line 22)? (Considering at max 80 chars in a line)

DubeySandeep · 2020-04-09T14:41:48Z

core/templates/domain/skill/skill-backend-api.service.spec.ts

+import { CsrfTokenService } from
+  'services/csrf-token.service';
+import { SkillBackendApiService } from
+  'domain/skill/skill-backend-api.service';


DubeySandeep · 2020-04-09T14:42:47Z

core/templates/domain/skill/skill-backend-api.service.spec.ts

-      var deferred = $q.defer();
-      deferred.resolve('sample-csrf-token');
-      return deferred.promise;
+  let skillBackendApiService = null;


This is written as the angular8 service, right? If yes can we add the type of these variables?

DubeySandeep · 2020-04-09T15:05:07Z

core/templates/domain/skill/skill-backend-api.service.ts

+      SkillDomainConstants.EDITABLE_SKILL_DATA_URL_TEMPLATE, {
+        skill_id: skillId
+      });
+    // eslint-disable-next-line dot-notation


Why is this needed? If it's for any specific reason, can we avoid using pragma?

DubeySandeep · 2020-04-09T15:06:21Z

core/templates/pages/skill-editor-page/skill-editor-page.module.ts

 import { RequestInterceptor } from 'services/request-interceptor.service';

+


is this expected change?

DubeySandeep · 2020-04-09T15:06:56Z

core/templates/pages/skill-editor-page/skill-editor-page.module.ts

@@ -22,10 +22,10 @@ import 'zone.js';
 import { Component, NgModule, StaticProvider } from '@angular/core';
 import { BrowserModule } from '@angular/platform-browser';
 import { downgradeComponent } from '@angular/upgrade/static';
-import { HttpClientModule } from '@angular/common/http';
-import { HTTP_INTERCEPTORS } from '@angular/common/http';
+import { HttpClientModule, HTTP_INTERCEPTORS } from '@angular/common/http';


Thanks for fixing this! :)

DubeySandeep · 2020-04-09T15:07:54Z

core/templates/domain/skill/skill-domain.types.ts

+// TODO(#7165): Replace any with exact type.
+export interface EditableSkillResponseConfig {
+  skill?: any;
+  skills?: any;


why both skill and skills, if required yo can create two interface skillResponse and skillsResponse

DubeySandeep · 2020-04-09T15:09:31Z

core/templates/domain/skill/skill-domain.types.ts

+  grouped_skill_summaries?: any;
+}
+
+export interface ChangeList {


ChangeList sounds like an array but it's a dict. don't use the List suffix

sagangwee

@srijanreddy98 LGTM once Sandeep's comments are addressed. Thanks!

sagangwee · 2020-04-10T05:37:13Z

core/templates/domain/skill/skill-backend-api.service.ts

+      }
+    );
+    this.http.get(skillDataUrl).toPromise().then(
+      (data: EditableSkillResponseConfig) => successCallback(data.skills),


I want to say this is a remnant of the renaming of the EditableSkillBackend service (the 'editable' was dropped).

oppiabot · 2020-04-18T20:45:51Z

Hi @srijanreddy98, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

bansalnitish · 2020-04-21T12:33:22Z

Ping @srijanreddy98

oppiabot · 2020-04-28T13:08:02Z

Hi @srijanreddy98, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

bansalnitish · 2020-04-28T17:24:13Z

Hey @srijanreddy98, could we get this in? (Let me know if you're busy I can take over this)

srijanreddy98 · 2020-04-28T17:43:51Z

@bansalnitish sorry for letting this fall this far... I'll finish it. Actually I have deleted the fork which had this branch, so I will shift this to a different pr.

kevinlee12 · 2020-05-06T03:36:23Z

@srijanreddy98 should this PR be closed? If so, please close it.

nithusha21 · 2020-05-09T06:17:42Z

Hi @srijanreddy98 could you please merge with develop as there were some new checks introduced, and a change to the development workflow which will help make sure develop will not break after merging this PR.

srijanreddy98 · 2020-05-09T06:23:15Z

@nithusha21 I am closing this pr, it needs some work with the typings and I had to delete this version of my fork because it kept on adding some files changes (made in my very first pr) to every pr I made. Please continue as if this didn't exist

srijanreddy98 added 20 commits February 22, 2020 13:37

Migrated contribution-opportunities-backend-api.service.ts from Angul…

a74892a

…arJS to Angular8. Migrate AngularJS files to Angular 8 #8472

Migrated contribution-opportunities-backend-api.service.ts from Angul…

145569e

…arJS to Angular8. Migrate AngularJS files to Angular 8 #8472

Migrated contribution-opportunities-backend-api.service.ts from Angul…

c31d4ae

…arJS to Angular8. Migrate AngularJS files to Angular 8 #8472

Migrated contribution-opportunities-backend-api.service.ts from Angul…

c7ef1fc

…arJS to Angular8. Migrate AngularJS files to Angular 8 #8472

Refactor some functions

af7219f

Update contribution-opportunities-backend-api.service.ts

81f6b5c

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

1b4a634

…ution-opportunities

Resolve Merge Conflicts

b974e78

Revert yarn.lock

51cf62d

Revert "Resolve Merge Conflicts"

5a65be2

This reverts commit b974e78.

Revert yarn lock changes

fbf9292

Revert yarn lock changes

Update contribution-opportunities-backend-api.service.ts

1590195

Added spec file for ContributionOpportunitiesBackendApi service

af355fb

Added tests for fetch tanslation and fetch voiceover opportunitites

d440096

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

b4141e2

…ution-opportunities-spec-file

Merge remote-tracking branch 'upstream/develop' into develop

ac303a9

Update contribution-opportunities-backend-api.service.ts

a226649

Remove extra space after the last function argument.

Merge remote-tracking branch 'upstream/develop' into contribution-opp…

57ee2be

…ortunities-spec-file

Merge branch 'contribution-opportunities-spec-file' of https://github…

90917ac

….com/srijanreddy98/oppia into contribution-opportunities-spec-file

Migrate editable-skill-backend-api-service to angular 8

2df2a67

srijanreddy98 requested review from aks681, bansalnitish, DubeySandeep and sagangwee as code owners March 17, 2020 18:11

Merge branch 'develop' into ediatble-skill-backend-api-migration

cd1c87c

srijanreddy98 changed the title ~~Fixes part of #8472: EdiatbleSkillBackendApiService migration to angular 8~~ Fixes part of #8472: EdiatbleSkillBackendApiService migration to Angular 8 Mar 17, 2020

Adding code to intercept request

9977414

srijanreddy98 requested a review from ankita240796 as a code owner March 17, 2020 20:33

kevinlee12 unassigned kevintab95 and DubeySandeep Apr 3, 2020

srijanreddy98 added 2 commits April 9, 2020 09:13

Merge remote-tracking branch 'upstream/develop' into ediatble-skill-b…

e960d32

…ackend-api-migration

Merge with develop

e1effde

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Apr 9, 2020

srijanreddy98 assigned sagangwee and DubeySandeep and unassigned sagangwee Apr 9, 2020

Code reviews

e82ea4e

srijanreddy98 assigned sagangwee and unassigned srijanreddy98 Apr 9, 2020

DubeySandeep requested changes Apr 9, 2020

View reviewed changes

sagangwee reviewed Apr 10, 2020

View reviewed changes

sagangwee removed their assignment Apr 10, 2020

DubeySandeep assigned srijanreddy98 and unassigned DubeySandeep Apr 11, 2020

oppiabot bot added the stale label Apr 18, 2020

oppiabot bot removed the stale label Apr 21, 2020

oppiabot bot added the stale label Apr 28, 2020

oppiabot bot removed the stale label Apr 28, 2020

srijanreddy98 closed this May 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes part of #8472: EdiatbleSkillBackendApiService migration to Angular 8 #8863

Fixes part of #8472: EdiatbleSkillBackendApiService migration to Angular 8 #8863

srijanreddy98 commented Mar 17, 2020 •

edited

Loading

oppiabot bot commented Mar 17, 2020

srijanreddy98 commented Mar 17, 2020

srijanreddy98 commented Apr 9, 2020

DubeySandeep left a comment

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

DubeySandeep Apr 9, 2020

sagangwee left a comment

sagangwee Apr 10, 2020

oppiabot bot commented Apr 18, 2020

bansalnitish commented Apr 21, 2020

oppiabot bot commented Apr 28, 2020

bansalnitish commented Apr 28, 2020

srijanreddy98 commented Apr 28, 2020

kevinlee12 commented May 6, 2020

nithusha21 commented May 9, 2020

srijanreddy98 commented May 9, 2020

		@@ -330,6 +330,7 @@


		# New structures team.
		/core/templates/domain/skill/skill-domain.types.ts @bansalnitish

		import { RequestInterceptor } from 'services/request-interceptor.service';

Fixes part of #8472: EdiatbleSkillBackendApiService migration to Angular 8 #8863

Fixes part of #8472: EdiatbleSkillBackendApiService migration to Angular 8 #8863

Conversation

srijanreddy98 commented Mar 17, 2020 • edited Loading

Overview

Essential Checklist

PR Pointers

oppiabot bot commented Mar 17, 2020

srijanreddy98 commented Mar 17, 2020

srijanreddy98 commented Apr 9, 2020

DubeySandeep 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagangwee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 18, 2020

bansalnitish commented Apr 21, 2020

oppiabot bot commented Apr 28, 2020

bansalnitish commented Apr 28, 2020

srijanreddy98 commented Apr 28, 2020

kevinlee12 commented May 6, 2020

nithusha21 commented May 9, 2020

srijanreddy98 commented May 9, 2020

srijanreddy98 commented Mar 17, 2020 •

edited

Loading