Generalised Review System, complete email functionality (oppia#5387)

* added new models, services and cron job * linting fixes * review changes * linting fix * review changes * minor fix * minor fix
quantumferret · Aug 14, 2018 · 994ac33 · 994ac33
1 parent e8db6b9
commit 994ac33
Show file tree

Hide file tree

Showing 11 changed files with 329 additions and 10 deletions.
diff --git a/core/controllers/cron.py b/core/controllers/cron.py
@@ -204,3 +204,24 @@ def get(self):
                 suggestion_services.accept_suggestion(
                     suggestion, feconf.SUGGESTION_BOT_USER_ID,
                     suggestion_models.DEFAULT_SUGGESTION_ACCEPT_MESSAGE, None)
+
+
+class CronMailReviewersInRotationHandler(base.BaseHandler):
+    """Handler to send emails notifying reviewers that there are suggestions
+    that need reviews.
+    """
+
+    @acl_decorators.can_perform_cron_tasks
+    def get(self):
+        """Handles get requests."""
+        if feconf.SEND_SUGGESTION_REVIEW_RELATED_EMAILS:
+            score_categories = suggestion_models.get_all_score_categories()
+            for score_category in score_categories:
+                suggestions = suggestion_services.query_suggestions(
+                    [('score_category', score_category),
+                     ('status', suggestion_models.STATUS_ACCEPTED)])
+                if len(suggestions) > 0:
+                    reviewer_id = suggestion_services.get_next_user_in_rotation(
+                        score_category)
+                    email_manager.send_mail_to_notify_users_to_review(
+                        reviewer_id, score_category)
diff --git a/core/domain/suggestion_services.py b/core/domain/suggestion_services.py
@@ -16,6 +16,7 @@
 suggestions.
 """
 
+from core.domain import email_manager
 from core.domain import exp_services
 from core.domain import feedback_services
 from core.domain import suggestion_registry
@@ -235,6 +236,23 @@ def accept_suggestion(suggestion, reviewer_id, commit_message, review_message):
         suggestion.suggestion_id, reviewer_id,
         feedback_models.STATUS_CHOICES_FIXED, None, review_message)
 
+    if feconf.ENABLE_RECORDING_OF_SCORES:
+        increment_score_for_user(
+            suggestion.author_id, suggestion.score_category,
+            suggestion_models.INCREMENT_SCORE_OF_AUTHOR_BY)
+        if feconf.SEND_SUGGESTION_REVIEW_RELATED_EMAILS:
+            scores = get_all_scores_of_user(suggestion.author_id)
+            if (
+                    suggestion.score_category in scores and
+                    scores[suggestion.score_category] >=
+                    feconf.MINIMUM_SCORE_REQUIRED_TO_REVIEW):
+                if check_if_email_has_been_sent_to_user(
+                        suggestion.author_id, suggestion.score_category):
+                    email_manager.send_mail_to_onboard_new_reviewers(
+                        suggestion.author_id, suggestion.score_category)
+                    mark_email_has_been_sent_to_user(
+                        suggestion.author_id, suggestion.score_category)
+
 
 def reject_suggestion(suggestion, reviewer_id, review_message):
     """Rejects the suggestion.
@@ -274,7 +292,8 @@ def get_user_contribution_scoring_from_model(userContributionScoringModel):
     return user_domain.UserContributionScoring(
         userContributionScoringModel.user_id,
         userContributionScoringModel.score_category,
-        userContributionScoringModel.score)
+        userContributionScoringModel.score,
+        userContributionScoringModel.has_email_been_sent)
 
 
 def get_all_scores_of_user(user_id):
@@ -312,11 +331,44 @@ def check_user_can_review_in_category(user_id, score_category):
     score = (
         user_models.UserContributionScoringModel.get_score_of_user_for_category(
             user_id, score_category))
-    if not score:
+    if score is None:
         return False
     return score >= feconf.MINIMUM_SCORE_REQUIRED_TO_REVIEW
 
 
+def check_if_email_has_been_sent_to_user(user_id, score_category):
+    """Checks if user has already received an email.
+
+    Args:
+        user_id: str. The id of the user.
+        score_category: str. The score category.
+
+    Returns:
+        bool. Whether the email has already been sent to the user.
+    """
+    scoring_model_instance = user_models.UserContributionScoringModel.get_by_id(
+        '%s.%s' % (score_category, user_id))
+    if scoring_model_instance is None:
+        return False
+    return scoring_model_instance.has_email_been_sent
+
+
+def mark_email_has_been_sent_to_user(user_id, score_category):
+    """Marks that the user has already received an email.
+
+    Args:
+        user_id: str. The id of the user.
+        score_category: str. The score category.
+    """
+    scoring_model_instance = user_models.UserContributionScoringModel.get_by_id(
+        '%s.%s' % (score_category, user_id))
+
+    if scoring_model_instance is None:
+        raise Exception('Expected user scoring model to exist for user')
+    scoring_model_instance.has_email_been_sent = True
+    scoring_model_instance.put()
+
+
 def get_all_user_ids_who_are_allowed_to_review(score_category):
     """Gets all user_ids of users who are allowed to review (as per their
     scores) suggestions to a particular category.
@@ -361,3 +413,60 @@ def create_new_user_contribution_scoring_model(user_id, score_category, score):
     """
     user_models.UserContributionScoringModel.create(
         user_id, score_category, score)
+
+
+def get_next_user_in_rotation(score_category):
+    """Gets the id of the next user in the reviewer rotation for the given
+    score_category. The order is alphabetical, and the next user in the
+    alphabetical order is returned.
+
+    Args:
+        score_category: str. The score category.
+
+    Returns:
+        str|None. The user id of the next user in the reviewer rotation, if
+            there are reviewers for the given category. Else None.
+    """
+    reviewer_ids = get_all_user_ids_who_are_allowed_to_review(score_category)
+    reviewer_ids.sort()
+
+    if len(reviewer_ids) == 0:
+        # No reviewers available for the given category.
+        return None
+
+    position_tracking_model = (
+        suggestion_models.ReviewerRotationTrackingModel.get_by_id(
+            score_category))
+
+    next_user_id = None
+    if position_tracking_model is None:
+        # No rotation has started yet, start rotation at index 0.
+        next_user_id = reviewer_ids[0]
+    else:
+        current_position_user_id = (
+            position_tracking_model.current_position_in_rotation)
+
+        for reviewer_id in reviewer_ids:
+            if reviewer_id > current_position_user_id:
+                next_user_id = reviewer_id
+                break
+
+        if next_user_id is None:
+            # All names are lexicographically smaller than or equal to the
+            # current position username. Hence, Rotating back to the front.
+            next_user_id = reviewer_ids[0]
+
+    update_position_in_rotation(score_category, next_user_id)
+    return next_user_id
+
+
+def update_position_in_rotation(score_category, user_id):
+    """Updates the current position in the rotation to the given user_id.
+
+    Args:
+        score_category: str. The score category.
+        user_id: str. The ID of the user who completed their turn in the
+            rotation for the given category.
+    """
+    suggestion_models.ReviewerRotationTrackingModel.update_position_in_rotation(
+        score_category, user_id)
diff --git a/core/domain/suggestion_services_test.py b/core/domain/suggestion_services_test.py
@@ -690,6 +690,13 @@ def setUp(self):
         suggestion_services.create_new_user_contribution_scoring_model(
             'user2', 'category1', 0)
 
+        self.signup('user_a@example.com', 'userA')
+        self.signup('user_b@example.com', 'userB')
+        self.signup('user_c@example.com', 'userC')
+        self.user_a_id = self.get_user_id_from_email('user_a@example.com')
+        self.user_b_id = self.get_user_id_from_email('user_b@example.com')
+        self.user_c_id = self.get_user_id_from_email('user_c@example.com')
+
     def test_update_score_for_user(self):
         suggestion_services.increment_score_for_user('user1', 'category1', 1)
         suggestion_services.increment_score_for_user('user2', 'category1', 5)
@@ -736,3 +743,53 @@ def get_all_user_ids_who_are_allowed_to_review(self):
             self.assertFalse(
                 suggestion_services.check_user_can_review_in_category(
                     'invalid_user', 'category1'))
+
+    def test_check_if_email_has_been_sent_to_user(self):
+        suggestion_services.create_new_user_contribution_scoring_model(
+            self.user_a_id, 'category_a', 15)
+        self.assertFalse(
+            suggestion_services.check_if_email_has_been_sent_to_user(
+                self.user_a_id, 'category_a'))
+        suggestion_services.mark_email_has_been_sent_to_user(
+            self.user_a_id, 'category_a')
+        self.assertTrue(
+            suggestion_services.check_if_email_has_been_sent_to_user(
+                self.user_a_id, 'category_a'))
+
+    def test_get_next_user_in_rotation(self):
+        suggestion_services.create_new_user_contribution_scoring_model(
+            self.user_a_id, 'category_a', 15)
+        suggestion_services.create_new_user_contribution_scoring_model(
+            self.user_b_id, 'category_a', 15)
+        suggestion_services.create_new_user_contribution_scoring_model(
+            self.user_c_id, 'category_a', 15)
+
+        user_ids = [self.user_a_id, self.user_b_id, self.user_c_id]
+        user_ids.sort()
+        self.assertEqual(suggestion_services.get_next_user_in_rotation(
+            'category_a'), user_ids[0])
+        self.assertEqual(
+            suggestion_models.ReviewerRotationTrackingModel.get_by_id(
+                'category_a').current_position_in_rotation, user_ids[0])
+
+        self.assertEqual(suggestion_services.get_next_user_in_rotation(
+            'category_a'), user_ids[1])
+        self.assertEqual(
+            suggestion_models.ReviewerRotationTrackingModel.get_by_id(
+                'category_a').current_position_in_rotation, user_ids[1])
+
+        self.assertEqual(suggestion_services.get_next_user_in_rotation(
+            'category_a'), user_ids[2])
+        self.assertEqual(
+            suggestion_models.ReviewerRotationTrackingModel.get_by_id(
+                'category_a').current_position_in_rotation, user_ids[2])
+
+        # Rotates back.
+        self.assertEqual(suggestion_services.get_next_user_in_rotation(
+            'category_a'), user_ids[0])
+        self.assertEqual(
+            suggestion_models.ReviewerRotationTrackingModel.get_by_id(
+                'category_a').current_position_in_rotation, user_ids[0])
+
+        self.assertEqual(suggestion_services.get_next_user_in_rotation(
+            'category_invalid'), None)
diff --git a/core/domain/user_domain.py b/core/domain/user_domain.py
@@ -271,7 +271,8 @@ def remove_collection_id(self, collection_id):
 class UserContributionScoring(object):
     """Domain object for UserContributionScoringModel."""
 
-    def __init__(self, user_id, score_category, score):
+    def __init__(self, user_id, score_category, score, has_email_been_sent):
         self.user_id = user_id
         self.score_category = score_category
         self.score = score
+        self.has_email_been_sent = has_email_been_sent
diff --git a/core/domain/user_domain_test.py b/core/domain/user_domain_test.py
@@ -395,11 +395,9 @@ class UserContributionScoringTests(test_utils.GenericTestBase):
     def test_initialization(self):
         """Testing init method."""
         user_contribution_scoring = (user_domain.UserContributionScoring(
-            'user_id0', 'category0', 5))
+            'user_id0', 'category0', 5, True))
 
-        self.assertEqual(
-            user_contribution_scoring.user_id, 'user_id0')
-        self.assertEqual(
-            user_contribution_scoring.score_category, 'category0')
-        self.assertEqual(
-            user_contribution_scoring.score, 5)
+        self.assertEqual(user_contribution_scoring.user_id, 'user_id0')
+        self.assertEqual(user_contribution_scoring.score_category, 'category0')
+        self.assertEqual(user_contribution_scoring.score, 5)
+        self.assertEqual(user_contribution_scoring.has_email_been_sent, True)
diff --git a/core/storage/suggestion/gae_models.py b/core/storage/suggestion/gae_models.py
@@ -91,6 +91,10 @@
 DEFAULT_SUGGESTION_ACCEPT_MESSAGE = ('Automatically accepting suggestion after'
                                      ' %d days' % THRESHOLD_DAYS_BEFORE_ACCEPT)
 
+# The amount to increase the score of the author by after successfuly getting an
+# accepted suggestion.
+INCREMENT_SCORE_OF_AUTHOR_BY = 1
+
 # Action types for incoming requests to the suggestion action handlers.
 ACTION_TYPE_ACCEPT = 'accept'
 ACTION_TYPE_REJECT = 'reject'
@@ -204,3 +208,59 @@ def get_all_stale_suggestions(cls):
                 0, 0, 0, THRESHOLD_TIME_BEFORE_ACCEPT_IN_MSECS))
         return cls.get_all().filter(cls.status == STATUS_IN_REVIEW).filter(
             cls.last_updated < threshold_time).fetch()
+
+    @classmethod
+    def get_all_score_categories(cls):
+        """Gets all the score categories for which suggestions have been
+        created.
+
+        Returns:
+            list(str). A list of all the score categories.
+        """
+        query_set = cls.query(projection=['score_category'], distinct=True)
+        return [data.score_category for data in query_set]
+
+
+class ReviewerRotationTrackingModel(base_models.BaseModel):
+    """Model to keep track of the position in the reviewer rotation. This model
+    is keyed by the score category.
+    """
+
+    # The ID of the user whose turn is just completed in the rotation.
+    current_position_in_rotation = ndb.StringProperty(
+        required=True, indexed=False)
+
+    @classmethod
+    def create(cls, score_category, user_id):
+        """Creates a new ReviewerRotationTrackingModel instance.
+
+        Args:
+            score_category: str. The score category.
+            user_id: str. The ID of the user who completed their turn in the
+                rotation for the given category.
+
+        Raises:
+            Exception: There is already an instance with the given id.
+        """
+        if cls.get_by_id(score_category):
+            raise Exception(
+                'There already exists an instance with the given id: %s' % (
+                    score_category))
+        cls(id=score_category, current_position_in_rotation=user_id).put()
+
+    @classmethod
+    def update_position_in_rotation(cls, score_category, user_id):
+        """Updates current position in rotation for the given score_category.
+
+        Args:
+            score_category: str. The score category.
+            user_id: str. The ID of the user who completed their turn in the
+                rotation for the given category.
+        """
+        instance = cls.get_by_id(score_category)
+
+        if instance is None:
+            cls.create(score_category, user_id)
+        else:
+            instance.current_position_in_rotation = user_id
+            instance.put()