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

Fix part of #8038: Remove http requests from the files that are not backend api services #9433

Merged
merged 39 commits into from Jun 25, 2020
Merged

Fix part of #8038: Remove http requests from the files that are not backend api services #9433

merged 39 commits into from Jun 25, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented Jun 4, 2020

Overview

  1. This PR fixes or fixes part of HTTPs calls in frontend should only happen through *backend-api.services #8038.
  2. This PR does the following: Remove http requests from the files that are not backend api services .

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.

@oppiabot
Copy link

oppiabot bot commented Jun 4, 2020

Hi, @nishantwrp. 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!

@oppiabot
Copy link

oppiabot bot commented Jun 4, 2020

Assigning @ankita240796 for the first-pass review of this pull request. Thanks!

@oppiabot
Copy link

oppiabot bot commented Jun 5, 2020

Hi @nishantwrp. 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!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Few comments. Can you use the new naming for the newly created files?

@@ -59,14 +58,14 @@ angular.module('oppia').directive('questionEditor', [
controllerAs: '$ctrl',
controller: [
'$scope', '$rootScope', '$uibModal',
'AlertsService', 'QuestionCreationService',
'AlertsService',
Copy link
Member

@vojtechjelinek vojtechjelinek Jun 5, 2020

Choose a reason for hiding this comment

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

Can you move some of the stuff from the next line onto this line?

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 5, 2020

Choose a reason for hiding this comment

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

done

'$scope', '$filter', '$http', '$q', '$timeout', '$uibModal', '$window',
'$location', 'AlertsService', 'QuestionCreationService', 'UrlService',
'$location', 'AlertsService', 'UrlService',
'NUM_QUESTIONS_PER_PAGE', 'EditableQuestionBackendApiService',
'SkillBackendApiService', 'MisconceptionObjectFactory',
'QuestionObjectFactory', 'SkillDifficultyObjectFactory',
Copy link
Member

@vojtechjelinek vojtechjelinek Jun 5, 2020

Choose a reason for hiding this comment

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

While you are modifying this, can you also sort these firstly by the $ services, regular services and constants at the end and then also alphabetically?

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 5, 2020

Choose a reason for hiding this comment

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

done

Copy link
Member

@vojtechjelinek vojtechjelinek Jun 6, 2020

Choose a reason for hiding this comment

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

Not done?

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 6, 2020

Choose a reason for hiding this comment

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

These must have got back to this when I fixed the merge conficts. done!

// limitations under the License.

/**
* @fileoverview Unit tests for AdminDataObjectFactorySpec.ts.
Copy link
Member

@vojtechjelinek vojtechjelinek Jun 5, 2020

Choose a reason for hiding this comment

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

Suggested change
* @fileoverview Unit tests for AdminDataObjectFactorySpec.ts.
* @fileoverview Unit tests for AdminDataObjectFactory.ts.

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 5, 2020

Choose a reason for hiding this comment

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

done

return this.http.get(
AdminPageConstants.ADMIN_HANDLER_URL).toPromise().then((
adminData: IAdminDataBackendDict) => {
return this.adminDataObjectFactory.createFromBackendDict(adminData);
});
Copy link
Member

@vojtechjelinek vojtechjelinek Jun 5, 2020

Choose a reason for hiding this comment

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

Could you cache the promise in a variable so that when getData is called twice only one request is produced?

Copy link
Member

@vojtechjelinek vojtechjelinek Jun 5, 2020

Choose a reason for hiding this comment

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

Ditto for all the other HTTP requests.

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 5, 2020

Choose a reason for hiding this comment

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

Should that be done for all of em? I'm not sure like if we expect the response of backend to be different we make the request?

Also for adminBackendApiService that's already done in AdminDataService

}
}).toPromise().then((data: any) => {
fetchQuery(queryId: string): Promise<EmailDashboardQuery> {
return this.emailDashboardBackendApiService.fetchQuery(queryId).then((
Copy link
Member

@vojtechjelinek vojtechjelinek Jun 5, 2020

Choose a reason for hiding this comment

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

break after fetchQuery(queryId)

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 5, 2020

Choose a reason for hiding this comment

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

done

// limitations under the License.

/**
* @fileoverview Backend api service for playthrough.
Copy link
Member

@vojtechjelinek vojtechjelinek Jun 5, 2020

Choose a reason for hiding this comment

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

Maybe use uppercased API.

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 5, 2020

Choose a reason for hiding this comment

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

done

return backendDict.summaries.map((
summaryDict: IExplorationSummaryBackendDict) => {
return this.explorationSummaryObjectFactory.createFromBackendDict(
summaryDict);
Copy link
Member

@vojtechjelinek vojtechjelinek Jun 5, 2020

Choose a reason for hiding this comment

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

Shouldn't this be indented by 2 spaces to the right?

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 5, 2020

Choose a reason for hiding this comment

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

That gives a lint error.

@oppiabot
Copy link

oppiabot bot commented Jun 5, 2020

Hi @nishantwrp. 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!

@oppiabot
Copy link

oppiabot bot commented Jun 22, 2020

Hi @nishantwrp. 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!

Hudda
Hudda approved these changes Jun 23, 2020
Copy link
Member

@Hudda Hudda left a comment

LGTM

Copy link
Member

@DubeySandeep DubeySandeep left a comment

LGTM!

Copy link
Member

@srijanreddy98 srijanreddy98 left a comment

Thanks @nishantwrp! A minor nit, otherwise LGTM!

@@ -26,6 +26,8 @@ import {
ɵangular_packages_common_http_http_d
} from '@angular/common/http';

import { AdminBackendApiService } from
Copy link
Member

@srijanreddy98 srijanreddy98 Jun 23, 2020

Choose a reason for hiding this comment

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

You need to add all of these new services to OppiaAngularRoot as well. You can take a look at step 12 here: https://github.com/oppia/oppia/wiki/Angular-Migration#implementation-details-to-migrate-services for further instructions.

Copy link
Contributor Author

@nishantwrp nishantwrp Jun 23, 2020

Choose a reason for hiding this comment

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

done

@nishantwrp nishantwrp requested a review from srijanreddy98 Jun 23, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jun 23, 2020

@srijanreddy98 ptal!

Copy link
Member

@srijanreddy98 srijanreddy98 left a comment

LGTM from the code-owner perspective. Thanks @nishantwrp!

@oppiabot
Copy link

oppiabot bot commented Jun 24, 2020

Hi @nishantwrp. 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!

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jun 25, 2020

Hi @kevinlee12 @vojtechjelinek, the CI checks have passed! Please merge this one before i get merge conflicts again :P.

@vojtechjelinek vojtechjelinek merged commit 3317ab5 into oppia:develop Jun 25, 2020
13 of 14 checks passed
@nishantwrp nishantwrp deleted the backend-api-service branch Jun 25, 2020
shavavo pushed a commit to shavavo/oppia that referenced this pull request Aug 20, 2020
…not backend api services (oppia#9433)

* Remove http requests from the files that are not backend api services

* Add CODEOWNERS

* Typo

* FE tests coverage

* Reviews

* Reviews

* Refactor Code

* FE Tests

* refactor code

* fe tests

* fe tests

* reviews

* Fix ts errors

* JobSpec -> JobStatusSummary

* Remove admin object factory

* FE Tests

* Fix all failing e2e tests

* Refactor Code

* Add a check and create backend api services for remaining files

* Reviews

* Reviews

* Reviews

* root component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment