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: editable.question.backend.api.service.ts now returns a domain object … #8986

Conversation

robatras
Copy link
Contributor

@robatras robatras commented Apr 5, 2020

Overview

  1. This PR fixes or fixes part of Backend api service in frontend should return domain object instead of dict #8015.
  2. This PR does the following: editable-question-backend-api now returns a question object instead of question dict and associated skills objects instead of associated skills dicts. questions-list.directive is modified to directly use objects returned from the service.
    modified the tests spec to expect domain object.

Essential Checklist

  • [ X] 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: ...".)
  • [ X] The linter/Karma presubmit checks have passed locally on your machine.
  • [ X] "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • [ X] 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 Apr 5, 2020

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

@robatras
Copy link
Contributor Author

robatras commented Apr 5, 2020

@seanlip @DubeySandeep PTAL! @yudani-s and I worked on this together

Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

Code looks okay, have you verified it by running a local server as well?

@kevinlee12
Copy link
Contributor

@robatras Please edit your PR title so that it fits what point 1 in the essential checklist is asking. Also, did you bypass the push verification checks?

@oppiabot
Copy link

oppiabot bot commented Apr 6, 2020

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

@kevinlee12 kevinlee12 assigned robatras and unassigned ankita240796 Apr 6, 2020
@robatras robatras changed the title editable.question.backend.api.service.ts now returns a domain object … Fix part of #8015: editable.question.backend.api.service.ts now returns a domain object … Apr 6, 2020
@robatras
Copy link
Contributor Author

robatras commented Apr 6, 2020

@robatras Please edit your PR title so that it fits what point 1 in the essential checklist is asking. Also, did you bypass the push verification checks?

Just edited it! Currently working on fixing the frontend issue test. And, i followed the instructions for pushing it; however, somehow, it did bypass the checks. @kevinlee12

@kevinlee12
Copy link
Contributor

Did you run python -m scripts.start?

@yudani-s
Copy link

yudani-s commented Apr 7, 2020

Did you run python -m scripts.start?

Hi, I worked with @robatras on this task. Yes, we did run python -m scripts.start, and the development server seemed to have the same functionality from what we could tell.

@kevinlee12
Copy link
Contributor

What was the result after you run the frontend tests locally? The command is python -m scripts.run_frontend_tests

@robatras
Copy link
Contributor Author

robatras commented Apr 8, 2020

@kevinlee12 When we run the frontend command locally, all tests pass and we receive this:
TOTAL: 2035 SUCCESS

@kevinlee12
Copy link
Contributor

Can you try to merge from upstream/develop and push again?

@robatras
Copy link
Contributor Author

robatras commented Apr 8, 2020

@kevinlee12 Just pushed it again and looks like all tests are passing like before, but the karma coverage error at the bottom still exists.

@kevinlee12
Copy link
Contributor

Ah, sorry, I missed the coverage error the first time. The prepush checks doesn't check for coverage, so in this case, you need to write more tests to ensure that there's 100% coverage. If you need assistance, let me know and I'll redirect you to the right person to assist.

@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #8986 into develop will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff            @@
##           develop    #8986   +/-   ##
========================================
  Coverage    53.41%   53.42%           
========================================
  Files          870      870           
  Lines        35764    35766    +2     
  Branches      4250     4250           
========================================
+ Hits         19103    19105    +2     
  Misses       15728    15728           
  Partials       933      933           
Flag Coverage Δ
#frontend 53.42% <75.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
...ectives/questions-list/questions-list.directive.ts 5.78% <0.00%> (ø)
.../question/editable-question-backend-api.service.ts 84.13% <100.00%> (+0.52%) ⬆️

@kevinlee12 kevinlee12 merged commit efd3492 into oppia:develop Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants