Separates exploration model getters from exp_services #7093

DubeySandeep · 2019-07-05T08:22:38Z

Explanation

Separates exploration model getters from exp_services.

Reason for this change (in brief):

Most of the other services use exp_services only for fetching exploration models and sometimes it creates circular dependencies situation. To avoid this we are trying to create a separate file for exploration fetching functionality.

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.

seanlip · 2019-07-05T08:52:13Z

Hi @DubeySandeep -- better to not submit WIP PRs until you're ready for a review, otherwise they just clutter up the PR queue. Should this be closed in the interim?

DubeySandeep · 2019-07-05T09:54:46Z

@seanlip, I'm waiting for my PR to pass the tests and I'll assign reviewer once it passes the e2e tests (As I'm not sure about it although I've done the manual test.)

Also, I've removed the WIP tag as this PR is just waiting on tests.

seanlip · 2019-07-05T19:51:00Z

Travis is failing (even for backend tests), please check.

DubeySandeep · 2019-07-05T20:08:41Z

core/domain/exp_services.py

-    return result
-
-
-def get_exploration_and_exploration_rights_by_id(exploration_id):


This method isn't used anywhere (except in tests) so I've deleted this method.

seanlip

Hi @DubeySandeep, I have no general concerns. Just try to get the tests passing!

seanlip · 2019-07-05T20:49:27Z

Also note, ExplorationRetrivalTests should be spelled "Retrieval"

DubeySandeep · 2019-07-06T01:54:53Z

@kevinlee12, @prasanna08, @vojtechjelinek, @nithusha21, @aks681, @apb7, can please review this PR as a code-owner?

codecov · 2019-07-06T02:08:23Z

Codecov Report

Merging #7093 into develop will increase coverage by <.01%.
The diff coverage is 98.5%.

@@             Coverage Diff             @@
##           develop    #7093      +/-   ##
===========================================
+ Coverage    97.61%   97.61%   +<.01%     
===========================================
  Files          379      381       +2     
  Lines        63408    63449      +41     
===========================================
+ Hits         61893    61934      +41     
  Misses        1515     1515

Impacted Files	Coverage Δ
core/domain/user_jobs_continuous.py	`96.22% <100%> (ø)`	⬆️
core/domain/dependency_registry_test.py	`100% <100%> (ø)`	⬆️
core/controllers/tasks.py	`100% <100%> (ø)`	⬆️
core/controllers/reader.py	`81.47% <100%> (ø)`	⬆️
core/domain/learner_progress_services.py	`100% <100%> (ø)`	⬆️
core/domain/stats_jobs_one_off_test.py	`100% <100%> (ø)`	⬆️
core/domain/suggestion_services_test.py	`100% <100%> (ø)`	⬆️
core/controllers/classifier_test.py	`100% <100%> (ø)`	⬆️
core/domain/exp_services_test.py	`99.8% <100%> (-0.03%)`	⬇️
core/domain/rating_services.py	`100% <100%> (ø)`	⬆️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22d0ea8...6c2ed74. Read the comment docs.

codecov · 2019-07-06T02:08:23Z

Codecov Report

Merging #7093 into develop will increase coverage by <.01%.
The diff coverage is 98.5%.

@@             Coverage Diff             @@
##           develop    #7093      +/-   ##
===========================================
+ Coverage    97.61%   97.61%   +<.01%     
===========================================
  Files          379      381       +2     
  Lines        63408    63449      +41     
===========================================
+ Hits         61893    61934      +41     
  Misses        1515     1515

Impacted Files	Coverage Δ
core/domain/user_jobs_continuous.py	`96.22% <100%> (ø)`	⬆️
core/domain/dependency_registry_test.py	`100% <100%> (ø)`	⬆️
core/controllers/tasks.py	`100% <100%> (ø)`	⬆️
core/controllers/reader.py	`81.47% <100%> (ø)`	⬆️
core/domain/learner_progress_services.py	`100% <100%> (ø)`	⬆️
core/domain/stats_jobs_one_off_test.py	`100% <100%> (ø)`	⬆️
core/domain/suggestion_services_test.py	`100% <100%> (ø)`	⬆️
core/controllers/classifier_test.py	`100% <100%> (ø)`	⬆️
core/domain/exp_services_test.py	`99.8% <100%> (-0.03%)`	⬇️
core/domain/rating_services.py	`100% <100%> (ø)`	⬆️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22d0ea8...6c2ed74. Read the comment docs.

kevinlee12

lgtm!

apb7

LGTM from code owner's perspective, thanks!

kevinlee12 · 2019-07-07T02:00:13Z

Hi @DubeySandeep, is this still in WIP? (I ask since it's still in the description).

DubeySandeep · 2019-07-07T02:03:37Z

@kevinlee12, Description wasn't updated, I've updated it now! (Sorry)

kevinlee12 · 2019-07-07T02:06:00Z

Ah, no worries, thanks!

nithusha21

LGTM! Thanks :)

DubeySandeep · 2019-07-08T07:27:09Z

@prasanna08 @vojtechjelinek @aks681, PTAL!

vojtechjelinek

LGTM from code owner's perspective. Thanks!

prasanna08

LGTM

kevinlee12 · 2019-07-09T20:24:53Z

@brianrodri @ankita240796, PTAL code owner review!

ankita240796

LGTM as a codeowner, Thanks @DubeySandeep!

Created exp_fetchers.py file.

b801d5d

DubeySandeep requested review from aks681, apb7, kevinlee12, nithusha21, prasanna08, seanlip and vojtechjelinek as code owners July 5, 2019 08:22

Fixed import issue.

6ce3403

DubeySandeep changed the title ~~[WIP] Separates exploration model getters from exp_services~~ Separates exploration model getters from exp_services Jul 5, 2019

apb7 assigned DubeySandeep Jul 5, 2019

DubeySandeep commented Jul 5, 2019

View reviewed changes

seanlip approved these changes Jul 5, 2019

View reviewed changes

DubeySandeep added 2 commits July 6, 2019 02:28

Fix mock method issues.

0ef64bf

Fixed test issue.

6c2ed74

DubeySandeep assigned kevinlee12, prasanna08, vojtechjelinek, nithusha21, aks681 and apb7 and unassigned DubeySandeep Jul 6, 2019

kevinlee12 approved these changes Jul 6, 2019

View reviewed changes

kevinlee12 removed their assignment Jul 6, 2019

apb7 approved these changes Jul 6, 2019

View reviewed changes

apb7 removed their assignment Jul 6, 2019

nithusha21 approved these changes Jul 8, 2019

View reviewed changes

nithusha21 removed their assignment Jul 8, 2019

vojtechjelinek approved these changes Jul 8, 2019

View reviewed changes

kevinlee12 unassigned vojtechjelinek Jul 8, 2019

prasanna08 approved these changes Jul 9, 2019

View reviewed changes

DubeySandeep unassigned prasanna08 Jul 9, 2019

aks681 approved these changes Jul 9, 2019

View reviewed changes

aks681 removed their assignment Jul 9, 2019

kevinlee12 requested review from ankita240796 and brianrodri July 9, 2019 20:24

kevinlee12 assigned brianrodri and ankita240796 Jul 9, 2019

ankita240796 approved these changes Jul 14, 2019

View reviewed changes

ankita240796 removed their assignment Jul 14, 2019

brianrodri approved these changes Jul 14, 2019

View reviewed changes

brianrodri merged commit 574b643 into oppia:develop Jul 14, 2019

DubeySandeep mentioned this pull request Jul 14, 2019

Fix backend test issues on develop branch #7142

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separates exploration model getters from exp_services #7093

Separates exploration model getters from exp_services #7093

DubeySandeep commented Jul 5, 2019 •

edited

Loading

seanlip commented Jul 5, 2019

DubeySandeep commented Jul 5, 2019

seanlip commented Jul 5, 2019

DubeySandeep Jul 5, 2019

seanlip left a comment

seanlip commented Jul 5, 2019

DubeySandeep commented Jul 6, 2019

codecov bot commented Jul 6, 2019

codecov bot commented Jul 6, 2019

kevinlee12 left a comment

apb7 left a comment

kevinlee12 commented Jul 7, 2019

DubeySandeep commented Jul 7, 2019 •

edited

Loading

kevinlee12 commented Jul 7, 2019

nithusha21 left a comment

DubeySandeep commented Jul 8, 2019

vojtechjelinek left a comment

prasanna08 left a comment

kevinlee12 commented Jul 9, 2019

ankita240796 left a comment

		return result


		def get_exploration_and_exploration_rights_by_id(exploration_id):

Separates exploration model getters from exp_services #7093

Separates exploration model getters from exp_services #7093

Conversation

DubeySandeep commented Jul 5, 2019 • edited Loading

Explanation

Checklist

seanlip commented Jul 5, 2019

DubeySandeep commented Jul 5, 2019

seanlip commented Jul 5, 2019

DubeySandeep Jul 5, 2019

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Jul 5, 2019

DubeySandeep commented Jul 6, 2019

codecov bot commented Jul 6, 2019

Codecov Report

codecov bot commented Jul 6, 2019

Codecov Report

kevinlee12 left a comment

Choose a reason for hiding this comment

apb7 left a comment

Choose a reason for hiding this comment

kevinlee12 commented Jul 7, 2019

DubeySandeep commented Jul 7, 2019 • edited Loading

kevinlee12 commented Jul 7, 2019

nithusha21 left a comment

Choose a reason for hiding this comment

DubeySandeep commented Jul 8, 2019

vojtechjelinek left a comment

Choose a reason for hiding this comment

prasanna08 left a comment

Choose a reason for hiding this comment

kevinlee12 commented Jul 9, 2019

ankita240796 left a comment

Choose a reason for hiding this comment

DubeySandeep commented Jul 5, 2019 •

edited

Loading

DubeySandeep commented Jul 7, 2019 •

edited

Loading