Fix part of #1366: Display relative (weekly) change in average ratings statistic #2252
Conversation
|
@wxyxinyu @AllanYangZhou Please review whenever free (not high priority, this PR doesn't block on any further work). |
| }, | ||
| getRelativeChangeInAverageRating: function(newRating, oldRating) { | ||
| if (!oldRating) { | ||
| return '+' + parseFloat(newRating).toFixed(2); |
There was a problem hiding this comment.
I'm not sure I understand the use of parseFloat here. From the tests you've written, it appears the arguments newRating and oldRating are numbers, not strings, right?
There was a problem hiding this comment.
It's used to limit the result to just 2 places after the decimal. If I don't use this, then I get results which have many digits after decimal places!
Yes, both args are numbers.
There was a problem hiding this comment.
Ah, what I mean is that since numbers already have the toFixed method (see here) you should just be able to call newRating.toFixed(2)
parseFloat is for converting strings to numbers, if your arguments are already numbers you shouldn't need it
There was a problem hiding this comment.
Yes, you are right. I've always been using toFixed with parseFloat without realizing that it can be directly used with numbers as well!
Removed use of parseFloat.
Current coverage is 48.01% (diff: 0.00%)@@ develop #2252 diff @@
==========================================
Files 185 190 +5
Lines 15852 15917 +65
Methods 2720 2733 +13
Messages 0 0
Branches 2689 2694 +5
==========================================
+ Hits 7609 7642 +33
- Misses 8243 8275 +32
Partials 0 0
|
|
code here lgtm |
|
@AllanYangZhou Do you have any other comments ? |
|
Are you going to do the last item on the checklist before merging though |
|
@526avijitgupta No, what you've pushed looks good. Although, same question as @wxyxinyu. |
|
@wxyxinyu @AllanYangZhou Yes, I was actually waiting for #2238 to be merged before this. Since merge conflicts were inevitable! Sorry, I failed to mention this. |
|
@AllanYangZhou @wxyxinyu PTAL. Made a bunch of changes. |
|
Could you test that the truncation works as intended? |
|
Other than that, LGTM, thanks! |
|
@wxyxinyu I had tested it manually a couple of times and it works fine. Did you mean that I move the truncation method to either utils or user_services, and then write tests for it in their corresponding test files ? |
| @@ -41,6 +41,9 @@ <h1 class="stat-value"><[dashboardStats.average_ratings || 'N/A']></h1> | |||
| <div class="total-plays stats-card"> | |||
| <p class="stat-description">Total Plays</p> | |||
| <h1 class="stat-value"><[dashboardStats.total_plays]></h1> | |||
| <p ng-hide="(dashboardStats.total_plays - lastWeekStats.total_plays) == 0"> | |||
There was a problem hiding this comment.
Recommend moving this calculation to the controller, since you're repeating it here twice.
|
@526avijitgupta -- Right, I think I was slightly confused about the tests earlier, I misread and thought you were storing the data truncated. I don't think you need to add anything. This can be merged once you address Allan's comment. |
|
@AllanYangZhou PTAL and merge if you think it's ok. |
…s statistic (#2252) * Fetch and compute relative change in average rating * Removed use of parseFloat * Fix average ratings precision, changes to relative change * minor changes * Move computation to controller instead of view
…atings statistic (oppia#2252) * Fetch and compute relative change in average rating * Removed use of parseFloat * Fix average ratings precision, changes to relative change * minor changes * Move computation to controller instead of view
Tasks to be completed as a part of this PR: