Generalised Review System, complete email functionality #5387

nithusha21 · 2018-07-29T10:44:40Z

Explanation

This PR completes the email code, including finding the next reviewer in cycle, and adding a cron job to mail this user. The actual calls to the functions to email the user have not been written yet as it depends on #5245. I will update the PR after that.

PTAL @anmolshkl

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
The PR explanation includes the words "Fixes #bugnum: ...".
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

codecov-io · 2018-07-29T11:39:28Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@a80bd6d). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             develop   #5387   +/-   ##
=========================================
  Coverage           ?   46.3%           
=========================================
  Files              ?     500           
  Lines              ?   29275           
  Branches           ?    4452           
=========================================
  Hits               ?   13557           
  Misses             ?   15718           
  Partials           ?       0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a80bd6d...e5bbf74. Read the comment docs.

anmolshkl

@nithusha21 took the first pass. PTAL, thanks!

anmolshkl · 2018-07-29T12:23:49Z

core/controllers/cron.py

+            if len(suggestions) > 0:
+                reviewer_id = suggestion_services.get_next_user_in_rotation(
+                    score_category)
+                print reviewer_id


Please drop the print statement.

anmolshkl · 2018-07-29T12:38:15Z

core/domain/suggestion_services.py

+    if (
+            get_all_scores_of_user(
+                suggestion.author_id)[suggestion.score_category] >=
+            feconf.MINIMUM_SCORE_REQUIRED_TO_REVIEW):


We just need to send the email once the user crosses this threshold, right?

Also, do we need to add the user to the reviewer's list at this point?

Done, the reviewer list is actually dynamically generated here https://github.com/oppia/oppia/pull/5387/files#diff-1d286750bff5aba07f586681357b8024R431

The model just stores the "index" we are at

anmolshkl · 2018-07-29T12:41:28Z

core/domain/suggestion_services.py

+            get_all_scores_of_user(
+                suggestion.author_id)[suggestion.score_category] >=
+            feconf.MINIMUM_SCORE_REQUIRED_TO_REVIEW):
+        # Send email if not sent already.


Is it yet to be completed?

anmolshkl · 2018-07-29T17:21:33Z

core/storage/suggestion/gae_models.py

+        return [data.score_category for data in query_set]
+
+
+class ReviewerRotationTrackingModel(base_models.BaseModel):


when (and where) do you add entries to the list in this model?

Same as #5387 (comment)

oppiabot · 2018-08-06T21:17:01Z

Hi @nithusha21. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

anmolshkl · 2018-08-08T17:55:31Z

@nithusha21 we need to move quickly on all our PRs otherwise we risk leaving everything for the last minute.

anmolshkl

@nithusha21 lgtm!

seanlip · 2018-08-12T19:12:38Z

core/domain/suggestion_services.py

@@ -258,6 +259,23 @@ def reject_suggestion(suggestion, reviewer_id, review_message):
    feedback_services.create_message(
        suggestion.suggestion_id, reviewer_id,
        feedback_models.STATUS_CHOICES_IGNORED, None, review_message)
+    if feconf.ENABLE_RECORDING_OF_SCORES:
+        increment_score_for_user(


Why are we incrementing scores when we reject a suggestion?

Oops, I think I misplaced this call. My bad

Missed this.

seanlip · 2018-08-12T19:13:53Z

core/domain/suggestion_services.py

+    score = (
+        user_models.UserContributionScoringModel.get_score_of_user_for_category(
+            user_id, score_category))
+    if not score:


I think this is misnamed. Typically, a score should be a number. It shouldn't have a "has email been sent" property.

So: scoring_model_instance perhaps, and get_model_of_user_for_category (instead of get_score_...). Ditto elsewhere.

And then here: use "if score is None".

I had made some errors here. Added a test for these 2 functions and verified behavior. Also changed names

seanlip · 2018-08-12T19:16:48Z

core/domain/suggestion_services.py

@@ -317,6 +336,40 @@ def check_user_can_review_in_category(user_id, score_category):
    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 recieved an email.


Spelling, here and below: "received".

seanlip · 2018-08-12T19:20:23Z

core/domain/suggestion_services.py

+            score_category))
+
+    next_user_id = None
+    if not position_tracking_model:


if position_tracking_model is None

seanlip · 2018-08-12T19:20:46Z

core/domain/suggestion_services.py

+            there are reviewers for the given category. Else None.
+    """
+    reviewer_ids = get_all_user_ids_who_are_allowed_to_review(score_category)
+    reviewer_names = user_services.get_usernames(reviewer_ids)


Why not just sort by reviewer IDs? I don't see the need for converting to reviewer names...

Also, user IDs are guaranteed to be immutable. User names may change (in the future). Use the stable identifier.

Hmmm... ok. I'll do this.

seanlip · 2018-08-12T19:22:05Z

core/storage/suggestion/gae_models.py

+        """
+        instance = cls.get_by_id(score_category)
+
+        if not instance:


if instance is None

nithusha21 · 2018-08-12T20:11:02Z

PTAL @seanlip

seanlip

LGTM (I think). Only looked at it briefly, but confirmed all comments appear to have been addressed.

Has this gone through full e2e manual testing?

nithusha21 · 2018-08-13T12:58:52Z

Hmmm, I haven't tested it manually, but I did write down backend tests for the functions added. I have verified that the Rotation tracking model works correctly, along with the scoring models.

seanlip · 2018-08-13T19:45:51Z

OK-- we should make a plan for full e2e testing of this, but approving in the meantime. Feel free to merge when Travis passes. Thanks!

nithusha21 added 2 commits July 28, 2018 20:13

added new models, services and cron job

d5bd8e0

linting fixes

e5b2c3c

nithusha21 requested a review from anmolshkl July 29, 2018 10:44

nithusha21 requested review from BenHenning and seanlip as code owners July 29, 2018 10:44

anmolshkl suggested changes Jul 29, 2018

View reviewed changes

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Aug 6, 2018

nithusha21 mentioned this pull request Aug 9, 2018

Generalised Review System - Email notification functionality #5245

Merged

6 tasks

nithusha21 added 3 commits August 12, 2018 18:18

merge develop

c6e4eb9

review changes

a69970b

linting fix

2c19265

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Aug 12, 2018

anmolshkl approved these changes Aug 12, 2018

View reviewed changes

seanlip reviewed Aug 12, 2018

View reviewed changes

nithusha21 added 2 commits August 13, 2018 01:30

review changes

869bcf0

minor fix

1d631ed

seanlip approved these changes Aug 12, 2018

View reviewed changes

minor fix

e5bbf74

seanlip added the PR: LGTM label Aug 13, 2018

seanlip merged commit 994ac33 into oppia:develop Aug 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalised Review System, complete email functionality #5387

Generalised Review System, complete email functionality #5387

nithusha21 commented Jul 29, 2018

codecov-io commented Jul 29, 2018 •

edited

Loading

anmolshkl left a comment

anmolshkl Jul 29, 2018

nithusha21 Aug 12, 2018

anmolshkl Jul 29, 2018

anmolshkl Jul 29, 2018

nithusha21 Aug 12, 2018

anmolshkl Jul 29, 2018

nithusha21 Aug 12, 2018

anmolshkl Jul 29, 2018

nithusha21 Aug 12, 2018

oppiabot bot commented Aug 6, 2018

anmolshkl commented Aug 8, 2018

anmolshkl left a comment

seanlip Aug 12, 2018

nithusha21 Aug 12, 2018

nithusha21 Aug 12, 2018

nithusha21 Aug 12, 2018

seanlip Aug 12, 2018

nithusha21 Aug 12, 2018

nithusha21 Aug 12, 2018

seanlip Aug 12, 2018

nithusha21 Aug 12, 2018

seanlip Aug 12, 2018

nithusha21 Aug 12, 2018

seanlip Aug 12, 2018

nithusha21 Aug 12, 2018

nithusha21 Aug 12, 2018

seanlip Aug 12, 2018

nithusha21 Aug 12, 2018

nithusha21 commented Aug 12, 2018

seanlip left a comment

nithusha21 commented Aug 13, 2018

seanlip commented Aug 13, 2018

		return [data.score_category for data in query_set]


		class ReviewerRotationTrackingModel(base_models.BaseModel):

Generalised Review System, complete email functionality #5387

Generalised Review System, complete email functionality #5387

Conversation

nithusha21 commented Jul 29, 2018

Explanation

Checklist

codecov-io commented Jul 29, 2018 • edited Loading

Codecov Report

anmolshkl left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Aug 6, 2018

anmolshkl commented Aug 8, 2018

anmolshkl left a 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

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

nithusha21 commented Aug 12, 2018

seanlip left a comment

Choose a reason for hiding this comment

nithusha21 commented Aug 13, 2018

seanlip commented Aug 13, 2018

codecov-io commented Jul 29, 2018 •

edited

Loading