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 #8015 and Fix #9914: Refactor backend-api services to return domain objects. #9917

Merged
merged 15 commits into from Jul 20, 2020
Merged

Fix part of #8015 and Fix #9914: Refactor backend-api services to return domain objects. #9917

merged 15 commits into from Jul 20, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented Jul 15, 2020

Overview

  1. This PR fixes or fixes part of Backend api service in frontend should return domain object instead of dict #8015.
    Fixes Profile page doesn't sort the explorations correctly #9914
  2. This PR does the following: Refactor backend-api services to return domain objects.

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 Jul 15, 2020

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

categorized_skills_dict['untriaged_skills'].append({
'skill_id': skill_id,
'skill_description': skill_summary_dict['description']
})
Copy link
Contributor Author

@nishantwrp nishantwrp Jul 15, 2020

Choose a reason for hiding this comment

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

I removed this because of two reasons

  • categorized_skills_dict is a dict with topic names as keys. In typescript if there's a dict all the values should be of same type.
  • The same information was already being returned in untriaged_skill_summary_dicts.

Copy link
Member

@seanlip seanlip Jul 15, 2020

Choose a reason for hiding this comment

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

Thanks @nishantwrp, adding @Showtim3 for review since he can advise on whether this change makes sense.

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 15, 2020

Choose a reason for hiding this comment

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

Ok @seanlip, Please note that to declare a type for this in the backend-api service. This is necessary.

Copy link
Member

@seanlip seanlip Jul 15, 2020

Choose a reason for hiding this comment

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

Yup sorry -- I don't disagree. I think @Showtim3 and you would need to collaborate to make sure that the changes to the data structure look good and are propagated correctly. Thanks!

Copy link
Contributor

@Showtim3 Showtim3 Jul 18, 2020

Choose a reason for hiding this comment

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

Yeah the changes look okay. Tested them locally also.

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 15, 2020

What did I test?

  • subtopic-viewer-backend-api.service.ts - Didn't change the response of the service. Just the types.
  • topic-viewer-backend-api.service.ts - Didn't change the response of the service. Just the types.
  • topic-creation-backend-api.service.ts - Didn't change the response of the service. Just the types.
  • topics-and-skills-dashboard-backend-api.service.ts - Loaded the dummy data. The topic and skill dashboard page looked fine. The skill merge modal was fine!, the filters were working. Assign to topic was working. Remove skills from topic, subtopic! Works fine!
  • contribution-opportunities-backend-api.service.ts - Didn't change the response much. But did test the community dashboard. No console errors!
  • exploration-features-backend-api.service.ts - Tested the places where this service was used.
  • state-top-answers-stats-backend-api.service.ts - Statistics tab data was displayed correctly.
  • profile-page-backend-api.service.ts - Tested the profile page. Seems fine! + e2e tests are passing for that page.

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Few minor comments and you bigger one: Why did you change SkillSummaryObjectFactory to ShortSkillSummaryObjectFactory?

@@ -51,7 +51,7 @@ require('domain/question/QuestionObjectFactory.ts');
require('domain/skill/MisconceptionObjectFactory.ts');
require('domain/skill/skill-backend-api.service.ts');
require('domain/skill/SkillDifficultyObjectFactory.ts');
require('domain/skill/SkillSummaryObjectFactory.ts');
require('domain/skill/ShortSkillSummaryObjectFactory.ts');
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

This should be moved before require('domain/skill/skill-backend-api.service.ts');. K is after h in the alphabet ditto below.

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 16, 2020

Choose a reason for hiding this comment

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

done

<md-card ng-repeat="(topicName, subtopics) in categorizedSkills" class="list-view-item">
<md-card layout="row" class="list-view-item">
<h2><label style="padding-left:10px"><[topicName]></label></h2>
</md-card>
<div ng-if="!checkIfEmpty(skills)" ng-repeat="(subtopicName, skills) in subtopics" class="list-view-item ">
<md-card class="list-view-item">
<label style="padding-top:20px;padding-bottom:20px;padding-left:10px"><[subtopicName]></label>
<md-radio-button style="margin-left:10px" class="protractor-test-skills-list-item" ng-repeat="skill in skills | filter:skillFilterText" ng-value="skill.skill_id" aria-label="<[skill.skill_description]>">
<[skill.skill_description]>
<md-radio-button style="margin-left:10px" class="protractor-test-skills-list-item" ng-repeat="skill in skills | filter:skillFilterText" ng-value="skill.id" aria-label="<[skill.description]>">
<[skill.description]>
</md-radio-button>
</md-card>
</div>
</md-card>
<md-card ng-if="hasUntriagedSkills(topicName)" ng-repeat="(topicName, skills) in categorizedSkills" class="list-view-item">
<md-card ng-if="untriagedSkillSummaries" class="list-view-item">
<md-card layout="row" class="list-view-item">
<h2><label style="padding-left:10px">Untriaged Skills</label></h2>
</md-card>
<div ng-if="!checkIfEmpty(skills)" class="list-view-item ">
<div ng-if="!checkIfEmpty(untriagedSkillSummaries)" class="list-view-item ">
<md-card class="list-view-item">
<md-radio-button style="margin-left:10px" class="protractor-test-skills-list-item" ng-repeat="skill in skills | filter:skillFilterText" ng-value="skill.skill_id" aria-label="<[skill.skill_description]>">
<[skill.skill_description]>
<md-radio-button style="margin-left:10px" class="protractor-test-skills-list-item" ng-repeat="skill in untriagedSkillSummaries | filter:skillFilterText" ng-value="skill.id" aria-label="<[skill.description]>">
<[skill.description]>
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

You changed the logic here a bit, could you please explain the changes?

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 16, 2020

Choose a reason for hiding this comment

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

Ok So before my changes categorizedSkills was a dict with keys as topic names and there was one extra key untriaged which contained the untriaged skills. hasUntriagedSkills checks if the topicName === 'untriaged'. So what this did was iterated through the complete categorizedSkills and displayed the one with untriaged as key.

I refactored the categorizedSkills to not contain that extra key. Instead use a separate variable untriagedSkillSummaries for that purpose.

@@ -20,17 +20,17 @@
import { downgradeInjectable } from '@angular/upgrade/static';
import { Injectable } from '@angular/core';

export interface ITranslationCountsDict {
export interface TranslationCountsDict {
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

Maybe TranslationCountsBackendDict?

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 16, 2020

Choose a reason for hiding this comment

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

Actually this is a type which is also used in the domain object. So, adding backend would suggest that it needs to be converted to a domain object which is not the case here.

[state: string]: AnswerStats[];
}

interface StateTopAnswerBackendDicts {
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

Maybe StateTopAnswersBackendDict.

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 16, 2020

Choose a reason for hiding this comment

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

done

}];

ctrl.userEditedExplorations = data.edited_exp_summary_dicts.sort(
ctrl.userEditedExplorations = data.editedExpSummaries.sort(
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

Thanks for fixing this!

@@ -40,7 +40,7 @@ require('domain/skill/MisconceptionObjectFactory.ts');
require('domain/skill/skill-backend-api.service.ts');
require('domain/skill/skill-creation-backend-api.service.ts');
require('domain/skill/SkillDifficultyObjectFactory.ts');
require('domain/skill/SkillSummaryObjectFactory.ts');
require('domain/skill/ShortSkillSummaryObjectFactory.ts');
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

This should be move before require('domain/skill/skill-backend-api.service.ts');.

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 16, 2020

Choose a reason for hiding this comment

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

done

@@ -56,7 +56,7 @@ describe('Subtopic editor tab', function() {
TopicEditorRoutingService = $injector.get('TopicEditorRoutingService');
SubtopicObjectFactory = $injector.get('SubtopicObjectFactory');
SubtopicPageObjectFactory = $injector.get('SubtopicPageObjectFactory');
SkillSummaryObjectFactory = $injector.get('SkillSummaryObjectFactory');
SkillSummaryObjectFactory = $injector.get('ShortSkillSummaryObjectFactory');
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

Suggested change
SkillSummaryObjectFactory = $injector.get('ShortSkillSummaryObjectFactory');
ShortSkillSummaryObjectFactory = $injector.get('ShortSkillSummaryObjectFactory');

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 16, 2020

Choose a reason for hiding this comment

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

done

StateTopAnswersStatsBackendDict,
StateTopAnswersStats,
StateTopAnswersStatsObjectFactory
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

Suggested change
StateTopAnswersStatsBackendDict,
StateTopAnswersStats,
StateTopAnswersStatsObjectFactory
StateTopAnswersStats,
StateTopAnswersStatsBackendDict,
StateTopAnswersStatsObjectFactory

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 16, 2020

Choose a reason for hiding this comment

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

done

if (isInitialized) {
return;
}
workingStateTopAnswersStats = {};
for (var stateName in stateTopAnswersStatsBackendDict.answers) {
for (var stateName in stateTopAnswersStats.answers) {
Copy link
Member

@vojtechjelinek vojtechjelinek Jul 16, 2020

Choose a reason for hiding this comment

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

Suggested change
for (var stateName in stateTopAnswersStats.answers) {
for (let stateName in stateTopAnswersStats.answers) {

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 16, 2020

Choose a reason for hiding this comment

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

done

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 16, 2020

Why did you change SkillSummaryObjectFactory to ShortSkillSummaryObjectFactory

@vojtechjelinek I changed SkillSummary to ShortSkillSummary because the actual SkillSummary (referring to the one in backend) had more info than just id and description. So, I created a new SkillSummary which matched with the backend SkillSummaryDict. And renamed the current one.

UPDATE - To avoid any confusions the new SkillSummary is also used in the TopicsAndSkillsDashboard

@nishantwrp nishantwrp requested a review from vojtechjelinek Jul 16, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 16, 2020

@vojtechjelinek ptal!

@oppiabot
Copy link

oppiabot bot commented Jul 17, 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!

@aks681 aks681 removed their assignment Jul 18, 2020
@seanlip
Copy link
Member

seanlip commented Jul 18, 2020

Looks good -- just the one comment left: #9917 (comment)

Also @nithusha21 PTAL as codeowner.

@seanlip seanlip assigned nishantwrp and unassigned seanlip Jul 18, 2020
@nishantwrp nishantwrp assigned seanlip and unassigned nishantwrp Jul 19, 2020
Copy link
Member

@nithusha21 nithusha21 left a comment

LGTM as a codeowner!

Copy link
Member

@seanlip seanlip left a comment

Thanks @nishantwrp, everything looks good to me except for one comment. I agree with your other design decisions.

@@ -26,17 +26,17 @@ export interface IQuestionSummaryBackendDict {
}

export class QuestionSummary {
_questionId: string;
_id: string;
Copy link
Member

@seanlip seanlip Jul 20, 2020

Choose a reason for hiding this comment

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

Leave this as it was (_questionId) and perhaps update the backend dict accordingly. Better to be clearer about what entity has the id (the question, not the question summary).

Copy link
Contributor Author

@nishantwrp nishantwrp Jul 20, 2020

Choose a reason for hiding this comment

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

Better to be clearer about what entity has the id (the question, not the question summary).

Got it! done!

@seanlip seanlip assigned nishantwrp and unassigned seanlip Jul 20, 2020
@seanlip seanlip removed their assignment Jul 20, 2020
@nishantwrp nishantwrp requested a review from seanlip Jul 20, 2020
@nishantwrp nishantwrp assigned seanlip and unassigned nishantwrp Jul 20, 2020
Copy link
Member

@seanlip seanlip left a comment

Thanks! LGTM.

@seanlip seanlip merged commit 369888d into oppia:develop Jul 20, 2020
6 checks passed
@seanlip
Copy link
Member

seanlip commented Jul 20, 2020

@nishantwrp could you please update #8015 accordingly with what has been done? Thanks!

@seanlip seanlip assigned nishantwrp and unassigned seanlip Jul 20, 2020
@nishantwrp nishantwrp deleted the refactor-backend-api-services-3 branch Jul 20, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Jul 20, 2020

@seanlip done!

@seanlip
Copy link
Member

seanlip commented Jul 20, 2020

@nishantwrp did you move them to "Fixed part of the issue"? If so please add your username to the ones you did and ideally the PR number so that it's easier to track.

@seanlip
Copy link
Member

seanlip commented Jul 20, 2020

Note: for clarity, I have also renamed that section to "Completed files (for reference)".

shavavo pushed a commit to shavavo/oppia that referenced this pull request Aug 20, 2020
…ces to return domain objects. (oppia#9917)

* Topics and Skills Dashboard (All checks passing)

* Backend tests

* All other files done (Tests passing)

* Fix issues with merge

* Minor Bug

* reviews

* reviews

* reviews

* reviews

* reviews

* reviews

* Fix issues with merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment