Fix part of #1366: Cron job to keep history of dashboard statistics #2173

526avijitgupta · 2016-06-26T13:49:50Z

Tasks to be completed as a part of this PR:

Create model property for storing weekly stats in UserStatsModel
Create a service method to update this model with the last week's stats.
Write getter to get value of weekly stats list accompanied by tests for this service method.
Create cron job that class this service method for all users, weekly.
Testing this job manually, as well as writing tests for this.

526avijitgupta · 2016-06-26T13:55:58Z

@wxyxinyu @seanlip @AllanYangZhou I have tried to implement a cron job that keeps a copy of weekly statistics of a creator. But I'm not sure how to test this (both manually + writing tests). Do you have any ideas about how could I proceed with testing ?
Also, can you please review to see if the approach is correct or not ?
Thanks!

UPDATE:
Tested this job manually, works fine. I'm still not sure how to proceed with writing tests for this though!

codecov-io · 2016-06-26T14:56:19Z

Current coverage is 48.01%

Merging #2173 into develop will decrease coverage by 0.05%

@@            develop      #2173   diff @@
==========================================
  Files           185        185          
  Lines         15829      15857    +28   
  Methods        2717       2720     +3   
  Messages          0          0          
  Branches       2688       2692     +4   
==========================================
+ Hits           7609       7613     +4   
- Misses         8220       8244    +24   
  Partials          0          0

Powered by Codecov. Last updated by 125bc0e...36a8e81

wxyxinyu · 2016-06-28T09:00:11Z

core/domain/user_services.py

+    else:
+        return None
+
+def save_last_dashboard_stats(user_id):


I think you can pass in the entire list of users, and then run the update over all of them, and do put_multi() to store all of them at once. That way you don't need to call get() and put() for every user (which will take a long time).

Actually, I think it makes more sense to create a mapreduce job to do this instead. (Do a one-off job, not a continuous job, and let the cron start the job). It might not be possible to actually load all the users to memory.

Created a one-off mapreduce job. But I couldn't find any classmethod which can be used to kick off this job from cron.py
[start_compurtation() fulfills the very same purpose, but only for continuous jobs]

Resolved now.

Nit: 2 lines above top-level function

wxyxinyu · 2016-06-28T09:12:01Z

Would you also need to get the total number of plays in this cron job? (We were going to experiment with changing the change in rating to change in number of ratings.)

Also, how did you test this manually? Just checking, because I don't find any tests for cron jobs either. I think testing the get/set functions for the data should be fine, especially since your cron has very little logic inside the get().

seanlip · 2016-06-28T09:13:06Z

core/domain/user_services.py

+    """Save statistics for creator dashboard of a user by appending to a list
+    keyed by datetime.utcnow().date().
+
+    This method is called through a cron job which is run weekly.


This function should know (and assume) nothing about the cron job.

Removed reference about cron job.

526avijitgupta · 2016-06-28T13:22:47Z

@wxyxinyu I can add tests for number of plays as well. Just to confirm - it will require that I stop the job and re-start it, right ?
Open admin panel at localhost:8000/ . There's a "Cron Jobs" link in left menu.

UPDATE:
Added tests which include both - ratings as well as num plays: https://github.com/oppia/oppia/pull/2173/files#diff-f034774bfff184b81c6e784b604288b2R496

526avijitgupta · 2016-06-28T20:26:47Z

core/controllers/cron.py

+    def get(self):
+        """Handles GET requests."""
+        user_jobs_one_off_test.DashboardStatsOneOffJobTests.run_one_off_job()
+


@seanlip I don't think this would work. Couldn't find any classmethod that can be used here to kick off this one-off job.
[start_computation fulfills the same purpose, but only for continuous jobs!]

Any suggestions ?

We have existing one-off jobs in the codebase. How do they work?

(Note that these are started by pressing a button on the admin page.)

I knew that these can be manually tested through the admin panel. But my attempts of testing this manually falied. It kept throwing a wierd error -

No module named webtest

Not sure why this error was being thrown. I tried restarting the server a few times but it did not seem to go away. So I decided to commit without testing this line, and thus added a line comment here!

Ah, got it

user_jobs_one_off.DashboardStatsOneOffJob.enqueue( user_jobs_one_off.DashboardStatsOneOffJob.create_new())

works!

seanlip · 2016-06-29T05:07:27Z

core/controllers/cron.py

@@ -86,6 +87,15 @@ def get(self):
        email_services.send_mail_to_admin(email_subject, email_message)


+class AppendLastDashboardStatsToList(base.BaseHandler):


CronDashboardStatsHandler? (Follow existing naming conventions, unless there's a good reason not to.)

okay. Renamed

seanlip · 2016-07-05T19:59:24Z

core/domain/user_jobs_continuous.py

-                average_ratings=average_ratings).put()
+            is_average_ratings = True
+        else:
+            is_average_ratings = False


What if you set average_ratings as None here? Then you don't need the is_average_ratings variable or the branching below in line 440.

(This thought had crossed my mind during writing this code. But I went for creating a separate flag for this instead)
Done.

526avijitgupta · 2016-07-05T20:33:33Z

PTAL

526avijitgupta · 2016-07-05T21:58:16Z

Please review.

seanlip · 2016-07-05T22:39:58Z

core/domain/user_jobs_continuous.py

+            average_ratings = None
+
+        if not mr_model:
+            if average_ratings:


Why do you still have the branch?

I think a cleaner way to do this is to follow the get_or_create() pattern in other models (for example, core.storage.activity.ActivityReferencesModel). Then, you just call get_or_create, and update the model you get, so you don't need to branch on the if not mr_model.

Used get_or_create

seanlip · 2016-07-05T22:52:42Z

Hi @526avijitgupta, did a full review -- PTAL. Thanks!

526avijitgupta · 2016-07-06T08:19:29Z

@wxyxinyu @seanlip Please review.

seanlip · 2016-07-06T09:05:36Z

LGTM. Thanks @526avijitgupta!

@wxyxinyu do you have any other comments? If not I think this is mergeable once the travis checks pass.

wxyxinyu · 2016-07-07T05:56:06Z

core/domain/user_services.py

+    """Save statistics for creator dashboard of a user by appending to a list
+    keyed by a datetime string.
+    """
+    model = user_models.UserStatsModel.get(user_id, strict=False)


You could use get_or_create() here too.

wxyxinyu · 2016-07-07T06:23:19Z

I just have one really small thing, and this will be good to merge.

526avijitgupta · 2016-07-07T16:51:46Z

@wxyxinyu Addressed the review comment. Should be ready to merge now I think!

…2173) * Cron job for recording creator stats * Fixes * Write getter and tests for weekly dasboard stats * Minor changes to model * Fix initial round of review comments * Fix call to one-off job through cron * Removed extra whiteline * Fix accidental line deletion * Partially addressed review comments * Add sample test for multiple weeks * Add test to check format of datetime string * Add schema version to user dashboard stats * Lint fix * Fix brackets around conditional * Fix continuous job and modify tests accordingly * Address review comments * Create new stats model if not already present * Address review comments * Use get_or_create for model * Reused get_or_create in method

…ics (oppia#2173) * Cron job for recording creator stats * Fixes * Write getter and tests for weekly dasboard stats * Minor changes to model * Fix initial round of review comments * Fix call to one-off job through cron * Removed extra whiteline * Fix accidental line deletion * Partially addressed review comments * Add sample test for multiple weeks * Add test to check format of datetime string * Add schema version to user dashboard stats * Lint fix * Fix brackets around conditional * Fix continuous job and modify tests accordingly * Address review comments * Create new stats model if not already present * Address review comments * Use get_or_create for model * Reused get_or_create in method

Cron job for recording creator stats

c43bf60

526avijitgupta added the PR: don't merge - IN REVIEW label Jun 26, 2016

526avijitgupta assigned wxyxinyu, AllanYangZhou and seanlip Jun 26, 2016

Fixes

4716c73

526avijitgupta added 2 commits June 27, 2016 01:40

Write getter and tests for weekly dasboard stats

88fda86

Minor changes to model

d9e418f

wxyxinyu reviewed Jun 28, 2016
View reviewed changes

seanlip reviewed Jun 28, 2016
View reviewed changes

Fix initial round of review comments

5bb47d5

526avijitgupta reviewed Jun 28, 2016
View reviewed changes

526avijitgupta added 3 commits June 29, 2016 02:41

Fix call to one-off job through cron

c245cec

Removed extra whiteline

b83e6de

Fix accidental line deletion

f4c6e2c

seanlip reviewed Jun 29, 2016
View reviewed changes

seanlip reviewed Jul 5, 2016
View reviewed changes

Address review comments

ed71c4a

Create new stats model if not already present

53c5b43

seanlip reviewed Jul 5, 2016
View reviewed changes

526avijitgupta added 2 commits July 6, 2016 13:26

Address review comments

88bb3e4

Use get_or_create for model

aaf3fb2

seanlip added PR: LGTM and removed PR: don't merge - IN REVIEW labels Jul 6, 2016

wxyxinyu reviewed Jul 7, 2016
View reviewed changes

Reused get_or_create in method

36a8e81

wxyxinyu merged commit e26ee9a into oppia:develop Jul 7, 2016

526avijitgupta deleted the creator-dashboard-stats-cron-job branch July 7, 2016 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #1366: Cron job to keep history of dashboard statistics #2173

Fix part of #1366: Cron job to keep history of dashboard statistics #2173

526avijitgupta commented Jun 26, 2016 •

edited

Loading

526avijitgupta commented Jun 26, 2016 •

edited

Loading

codecov-io commented Jun 26, 2016 •

edited

Loading

wxyxinyu Jun 28, 2016

wxyxinyu Jun 28, 2016

526avijitgupta Jun 28, 2016

526avijitgupta Jun 28, 2016

seanlip Jun 29, 2016

526avijitgupta Jun 29, 2016

wxyxinyu commented Jun 28, 2016

seanlip Jun 28, 2016

526avijitgupta Jun 28, 2016

526avijitgupta commented Jun 28, 2016 •

edited

Loading

526avijitgupta Jun 28, 2016

seanlip Jun 28, 2016

seanlip Jun 28, 2016

526avijitgupta Jun 28, 2016

526avijitgupta Jun 28, 2016

seanlip Jun 29, 2016

526avijitgupta Jun 29, 2016

seanlip Jul 5, 2016

526avijitgupta Jul 5, 2016

526avijitgupta commented Jul 5, 2016

526avijitgupta commented Jul 5, 2016

seanlip Jul 5, 2016

wxyxinyu Jul 6, 2016 •

edited

Loading

526avijitgupta Jul 6, 2016

seanlip commented Jul 5, 2016

526avijitgupta commented Jul 6, 2016

seanlip commented Jul 6, 2016

wxyxinyu Jul 7, 2016

526avijitgupta Jul 7, 2016

wxyxinyu commented Jul 7, 2016

526avijitgupta commented Jul 7, 2016

		@@ -86,6 +87,15 @@ def get(self):
		email_services.send_mail_to_admin(email_subject, email_message)


		class AppendLastDashboardStatsToList(base.BaseHandler):

Fix part of #1366: Cron job to keep history of dashboard statistics #2173

Fix part of #1366: Cron job to keep history of dashboard statistics #2173

Conversation

526avijitgupta commented Jun 26, 2016 • edited Loading

526avijitgupta commented Jun 26, 2016 • edited Loading

codecov-io commented Jun 26, 2016 • edited Loading

Current coverage is 48.01%

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxyxinyu commented Jun 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

526avijitgupta commented Jun 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

526avijitgupta commented Jul 5, 2016

526avijitgupta commented Jul 5, 2016

Choose a reason for hiding this comment

wxyxinyu Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jul 5, 2016

526avijitgupta commented Jul 6, 2016

seanlip commented Jul 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxyxinyu commented Jul 7, 2016

526avijitgupta commented Jul 7, 2016

526avijitgupta commented Jun 26, 2016 •

edited

Loading

526avijitgupta commented Jun 26, 2016 •

edited

Loading

codecov-io commented Jun 26, 2016 •

edited

Loading

526avijitgupta commented Jun 28, 2016 •

edited

Loading

wxyxinyu Jul 6, 2016 •

edited

Loading