Split Backend Call API from the StateTopAnswerStatisticsService #5079
Conversation
There was a problem hiding this comment.
Thanks @brianrodri! This seems reasonable to me but doesn't address #5033, so I submitted another PR #5080 that complements this and can be merged in for the release.
| $injector.get('StateTopAnswersStatsBackendApiService'); | ||
| })); | ||
|
|
||
| beforeEach(inject(function($injector) { |
There was a problem hiding this comment.
I think you can combine this with the previous beforeEach?
| }); | ||
| init: function(stateTopAnswersStatsBackendDict) { | ||
| stateTopAnswerStatsCache = {}; | ||
| Object.keys( |
There was a problem hiding this comment.
Actually, I'm only seeing it now, but this is a bit of a strange construct. It creates a copy of the keys of the object and then iterates through them with forEach. Why not just do "for stateName in stateTopAnswersStatsBackendDict.answers"?
There was a problem hiding this comment.
Ah, I thought those didn't work well in JavaScript, fixed to use that!
|
Though many tests seem to be failing... |
| // Don't make user wait on stats to begin editing the exploration. | ||
| StateTopAnswersStatsBackendApiService.fetchStats( | ||
| $scope.explorationId | ||
| ).then(StateTopAnswersStatsService.init); |
There was a problem hiding this comment.
Don't the new services need to be imported explicitly as dependencies?
Did you test this manually?
There was a problem hiding this comment.
Done, not yesterday was just focused on writing the logic. Did today and fixed what seems to have caused all the errors!
Codecov Report
@@ Coverage Diff @@
## develop #5079 +/- ##
==========================================
+ Coverage 45.19% 45.2% +0.01%
==========================================
Files 408 409 +1
Lines 23879 23889 +10
Branches 3860 3861 +1
==========================================
+ Hits 10792 10800 +8
- Misses 13087 13089 +2
Continue to review full report at Codecov.
|
| @@ -205,6 +207,13 @@ oppia.controller('ExplorationEditor', [ | |||
|
|
|||
| StateEditorTutorialFirstTimeService.init( | |||
| data.show_state_editor_tutorial_on_load, $scope.explorationId); | |||
|
|
|||
| if (ExplorationRightsService.isPublic()) { | |||
| // Don't make user wait on stats to begin editing the exploration. | |||
There was a problem hiding this comment.
I'm not sure how this comment applies to the following line (I mean, how would you fetch stats alternatively anyway)?
There was a problem hiding this comment.
Hmm, well I was considering loading it alongside all of the other exploration data, since this looks like the only other backend callout made in this function.
There was a problem hiding this comment.
Oh, so is the point to ensure that it's at the end? If so, maybe say that.
(Basically ... what are devs supposed to try and preserve in the future?)
There was a problem hiding this comment.
Alright, I've updated it. How's that?
| @@ -324,6 +326,7 @@ | |||
| <script src="{{TEMPLATE_DIR_PREFIX}}/domain/exploration/ParamSpecObjectFactory.js"></script> | |||
| <script src="{{TEMPLATE_DIR_PREFIX}}/domain/exploration/ParamSpecsObjectFactory.js"></script> | |||
| <script src="{{TEMPLATE_DIR_PREFIX}}/domain/exploration/ParamTypeObjectFactory.js"></script> | |||
| <script src="{{TEMPLATE_DIR_PREFIX}}/domain/exploration/AnswerStatsFactory.js"></script> | |||
There was a problem hiding this comment.
AnswerStatsObjectFactory.js and put it alphabetically
…a#5079) * Split backend api calls from the service. * Make backend API service initialize the service when exploration is first loaded. * s/fetchExploration/fetchStats * Don't use promise. * Rever whitespace changes. * Remove httpBackend from spec. * Add isInitialized with tests. * Make backend dict same as what's returned. * nit: add space. * Combine beforeEach. * Import new backend api * Depend on new services. * Use for each loop. * Respect None return values. * Add imports to exploration_editor * Move files * Add answers nesting. * Fix test to include answers wrapping. * nit: remove extra line. * Add whitespace again. * Fix endswith newline lint error. * Use Top10AnswerFrequencies calculation. * Consistent passing of url interpolation values. * s/AnswerStatsFactory/AnswerStatsObjectFactory * Organize inports. * Better worded comment.
This helps split up dependencies and makes the next step, adding hooks, simpler. This PR supersedes the (now closed) #5077.