Changes from code reviews of several commits

shashaBot · May 30, 2014 · 266ec1e · 266ec1e
1 parent 65ae3bc
commit 266ec1e
Show file tree

Hide file tree

Showing 6 changed files with 145 additions and 50 deletions.
diff --git a/app.yaml b/app.yaml
@@ -73,6 +73,9 @@ handlers:
   script: mapreduce.main.APP
   login: admin
   secure: always
+- url: /cron/.*
+  login: admin
+  script: main_cron.app
 - url: /.*
   script: main.app
   secure: always

diff --git a/core/controllers/cron.py b/core/controllers/cron.py
@@ -20,8 +20,8 @@
 class StatisticsHandler(base.BaseHandler):
     """Handler for statistics cron job."""
     def get(self):
-      """Handles get requests."""
-      for klass in jobs_registry.JOB_MANAGER_CLASSES:
-         if klass.__name__ == 'StatisticsPageJobManager':
-           klass.enqueue(klass.create_new())
-           break
+        """Handles get requests."""
+        for klass in jobs_registry.JOB_MANAGER_CLASSES:
+           if klass.__name__ == 'StatisticsPageJobManager':
+             klass.enqueue(klass.create_new())
+             break
diff --git a/core/domain/stats_jobs.py b/core/domain/stats_jobs.py
@@ -15,16 +15,20 @@
 """Jobs for statistics views."""
 
 import ast
-import feconf
 import logging
-import utils
+
 from core import jobs
 from core.platform import models
-
 (stats_models,) = models.Registry.import_models([models.NAMES.statistics])
+import feconf
+import logging
+import utils
 
 class StatisticsPageJobManager(jobs.BaseMapReduceJobManager):
-    """Job that calculates and creates stats models for exploration view."""
+    """Job that calculates and creates stats models for exploration view.
+       Includes: * number of visits to the exploration
+                 * number of completions of the exploration
+    """
 
     @classmethod
     def entity_classes_to_map_over(cls):
@@ -40,25 +44,23 @@ def map(item):
         yield (item.exploration_id, map_value)
 
     @staticmethod
-    def reduce(key, values):
+    def reduce(key, stringified_values):
         started_session_ids = set()
-        leave_by_session_id = {}
-        for value_str in values:
+        last_maybe_leave_by_session_id = {}
+        for value_str in stringified_values:
             value = ast.literal_eval(value_str)
             if value['event_type'] == feconf.EVENT_TYPE_START:
                 started_session_ids.add(value['session_id'])
-            if value['event_type'] == feconf.EVENT_TYPE_LEAVE:
+            elif value['event_type'] == feconf.EVENT_TYPE_LEAVE:
                 session_id = value['session_id']
-                if session_id in leave_by_session_id:
-                    former_value = leave_by_session_id[session_id]
-                    leave_by_session_id[session_id] = (
-                        former_value 
-                          if former_value['created_on'] > value['created_on']
-                          else value)
-                else:
-                    leave_by_session_id[session_id] = value 
-        complete_events = [e for e in leave_by_session_id.values() 
+                if (session_id not in last_maybe_leave_by_session_id or
+                    value['created_on'] > 
+                    last_maybe_leave_by_session_id[session_id]['created_on']):
+                    last_maybe_leave_by_session_id[session_id] = value
+        complete_events = [e for 
+                           e in last_maybe_leave_by_session_id.values() 
                            if e['state_name'] == feconf.END_DEST]
-        stats_models.ExplorationAnnotationModel(id=key,
+        stats_models.ExplorationAnnotationModel(
+            id=key,
             num_visits=len(started_session_ids),
             num_completions=len(complete_events)).put()
diff --git a/core/domain/stats_jobs_test.py b/core/domain/stats_jobs_test.py
@@ -18,18 +18,18 @@
 
 """Tests for statistics MapReduce jobs."""
 
-import feconf
+from datetime import datetime
+
 from core import jobs
 from core.platform import models
-from time import sleep
+(stats_models,) = models.Registry.import_models([models.NAMES.statistics])
 from core.domain import stats_jobs
 from core.domain import stats_services
+import feconf
 import test_utils
 
-(stats_models,) = models.Registry.import_models([models.NAMES.statistics])
-
 class StatsPageJobIntegrationTests(test_utils.GenericTestBase):
-    """Tests for exlporation annotations."""
+    """Tests for exploration annotations."""
 
     def test_no_completion(self):
         exp_id = 'eid'
@@ -43,6 +43,7 @@ def test_no_completion(self):
            stats_jobs.StatisticsPageJobManager.create_new())
         stats_jobs.StatisticsPageJobManager.enqueue(job_id)
         self.assertEqual(self.count_jobs_in_taskqueue(), 1)
+
         self.process_and_flush_pending_tasks()
         self.assertEqual(
             stats_jobs.StatisticsPageJobManager.get_status_code(job_id), 
@@ -68,6 +69,7 @@ def test_all_complete(self):
         job_id = stats_jobs.StatisticsPageJobManager.create_new()
         stats_jobs.StatisticsPageJobManager.enqueue(job_id)
         self.assertEqual(self.count_jobs_in_taskqueue(), 1)
+
         self.process_and_flush_pending_tasks()
         self.assertEqual(
             stats_jobs.StatisticsPageJobManager.get_status_code(job_id),
@@ -76,35 +78,78 @@ def test_all_complete(self):
         self.assertEqual(output_model.num_visits, 2)
         self.assertEqual(output_model.num_completions, 2)
 
-    def test_multiple_completes_same_session(self):
+    def test_multiple_maybe_leaves_same_session(self):
         exp_id = 'eid'
         version = 1
         state = 'sid'
         stats_services.EventHandler.start_exploration(
             exp_id, version, state, 'session1', {}, feconf.PLAY_TYPE_PLAYTEST)
-        stats_services.EventHandler.maybe_leave_exploration(
-            exp_id, version, state, 'session1', 27, {},
-            feconf.PLAY_TYPE_PLAYTEST)
-        sleep(0.2)
-        stats_services.EventHandler.maybe_leave_exploration(
-            exp_id, version, state, 'session1', 27, {},
-            feconf.PLAY_TYPE_PLAYTEST)
-        sleep(0.2)
-        stats_services.EventHandler.maybe_leave_exploration(
-            exp_id, version, feconf.END_DEST, 'session1', 27, {},
-            feconf.PLAY_TYPE_PLAYTEST)
+        leave1 = stats_models.MaybeLeaveExplorationEventLogEntryModel(
+            event_type=feconf.EVENT_TYPE_LEAVE,
+            exploration_id=exp_id,
+            exploration_version=version,
+            state_name=state,
+            session_id='session1',
+            client_time_spent_in_secs=27.0,
+            params={},
+            play_type=feconf.PLAY_TYPE_PLAYTEST)
+        leave1.put()
+        leave1.created_on = datetime.fromtimestamp(0)
+        leave1.put()
+        leave2 = stats_models.MaybeLeaveExplorationEventLogEntryModel(
+            event_type=feconf.EVENT_TYPE_LEAVE,
+            exploration_id=exp_id,
+            exploration_version=version,
+            state_name=state,
+            session_id='session1',
+            client_time_spent_in_secs=27.0,
+            params={},
+            play_type=feconf.PLAY_TYPE_PLAYTEST)
+        leave2.put()
+        leave2.created_on = datetime.fromtimestamp(1)
+        leave2.put()
+        leave3 = stats_models.MaybeLeaveExplorationEventLogEntryModel(
+            event_type=feconf.EVENT_TYPE_LEAVE,
+            exploration_id=exp_id,
+            exploration_version=version,
+            state_name=feconf.END_DEST,
+            session_id='session1',
+            client_time_spent_in_secs=27.0,
+            params={},
+            play_type=feconf.PLAY_TYPE_PLAYTEST)
+        leave3.put()
+        leave3.created_on = datetime.fromtimestamp(2)
+        leave3.put()
         stats_services.EventHandler.start_exploration(
             exp_id, version, state, 'session2', {}, feconf.PLAY_TYPE_PLAYTEST)
-        stats_services.EventHandler.maybe_leave_exploration(
-            exp_id, version, state, 'session2', 27, {},
-            feconf.PLAY_TYPE_PLAYTEST)
-        sleep(0.2)
-        stats_services.EventHandler.maybe_leave_exploration(
-            exp_id, version, state, 'session2', 27, {},
-            feconf.PLAY_TYPE_PLAYTEST)
+        leave4 = stats_models.MaybeLeaveExplorationEventLogEntryModel(
+            event_type=feconf.EVENT_TYPE_LEAVE,
+            exploration_id=exp_id,
+            exploration_version=version,
+            state_name=state,
+            session_id='session2',
+            client_time_spent_in_secs=27.0,
+            params={},
+            play_type=feconf.PLAY_TYPE_PLAYTEST)
+        leave4.put()
+        leave4.created_on = datetime.fromtimestamp(3)
+        leave4.put()
+        leave5 = stats_models.MaybeLeaveExplorationEventLogEntryModel(
+            event_type=feconf.EVENT_TYPE_LEAVE,
+            exploration_id=exp_id,
+            exploration_version=version,
+            state_name=state,
+            session_id='session2',
+            client_time_spent_in_secs=27.0,
+            params={},
+            play_type=feconf.PLAY_TYPE_PLAYTEST)
+        leave5.put()
+        leave5.created_on = datetime.fromtimestamp(4)
+        leave5.put()
         job_id = stats_jobs.StatisticsPageJobManager.create_new()
         stats_jobs.StatisticsPageJobManager.enqueue(job_id)
         self.assertEqual(self.count_jobs_in_taskqueue(), 1)
+
         self.process_and_flush_pending_tasks()
         self.assertEqual(
             stats_jobs.StatisticsPageJobManager.get_status_code(job_id),

diff --git a/main.py b/main.py
@@ -20,7 +20,6 @@
 
 from core.controllers import admin
 from core.controllers import base
-from core.controllers import cron
 from core.controllers import editor
 from core.controllers import galleries
 from core.controllers import moderator
@@ -150,8 +149,6 @@ def ui_access_wrapper(self, *args, **kwargs):
     get_redirect_route(r'/admin', admin.AdminPage, 'admin_page'),
     get_redirect_route(r'/adminhandler', admin.AdminHandler, 'admin_handler'),
 
-    get_redirect_route(r'/cron/statistics', cron.StatisticsHandler, 'statistics_handler'),
-
     get_redirect_route(
         r'/imagehandler/<exploration_id>/<encoded_filepath>',
         resources.ImageHandler, 'image_handler'),

diff --git a/main_cron.py b/main_cron.py
@@ -0,0 +1,48 @@
+# Copyright 2014 The Oppia Authors. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS-IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Main package for URL routing and the index page."""
+
+__author__ = 'Sean Lip'
+
+import feconf
+
+from core.controllers import cron
+from core.platform import models
+transaction_services = models.Registry.import_transaction_services()
+
+import webapp2
+from webapp2_extras.routes import RedirectRoute
+
+
+def get_redirect_route(regex_route, handler, name, defaults=None):
+    """Returns a route that redirects /foo/ to /foo.
+
+    Warning: this method strips off parameters after the trailing slash. URLs
+    with parameters should be formulated without the trailing slash.
+    """
+    if defaults is None:
+        defaults = {}
+    return RedirectRoute(
+        regex_route, handler, name, strict_slash=True, defaults=defaults)
+
+
+# Register the URL with the responsible classes
+urls = [
+    get_redirect_route(r'/cron/statistics', cron.StatisticsHandler, 'statistics_handler'),
+]
+
+
+app = transaction_services.toplevel_wrapper(
+    webapp2.WSGIApplication(urls, debug=feconf.DEBUG))