Generalised Review System Milestone 2: Cron job to automatically accept suggestions after a threshold #5155

nithusha21 · 2018-06-26T12:50:13Z

Explanation

This PR implements a cron job that will accept all suggestions which were last updated before a threshold time (i.e. no activity has occurred on them).

PTAL @anmolshkl. Thanks!

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.

anmolshkl

@nithusha21 added a few comments, PTAL!

anmolshkl · 2018-06-26T16:16:37Z

core/domain/feedback_services.py

@@ -178,6 +183,19 @@ def create_message(
    new_status = thread.status
    thread.put()

+    # We do a put on the suggestion linked to the thread, if any so that the


Perhaps - We do a put on the suggestion linked (if it exists) to the thread so that the....

anmolshkl · 2018-06-26T16:17:14Z

core/domain/feedback_services.py

@@ -178,6 +183,19 @@ def create_message(
    new_status = thread.status
    thread.put()

+    # We do a put on the suggestion linked to the thread, if any so that the
+    # last_updated time changes to show that there is activity in the thread.


How would registering an activity on the thread help?

So my idea was that we accept suggestions which have no activity on them. By activity I mean any conversation basically. So if someone messages on a feedback thread linked to a suggestion, I want to update the last updated time of the suggestion too. Does this clarify the question?

Adding on... If people are discussing about a suggestion, we don't want to auto-approve it right? If no one is discussing, and no action is being taken, then we can safely assume no activity is there and then we can accept the suggestion.

Yep, that makes sense. Thanks!

anmolshkl · 2018-06-26T16:23:46Z

core/storage/suggestion/gae_models_test.py

@@ -225,3 +225,9 @@ def test_get_suggestions_by_target_id(self):
                .get_suggestions_by_target_id(
                    suggestion_models.TARGET_TYPE_EXPLORATION, 'exp_invalid')),
            0)
+
+    def test_get_all_stale_suggestions(self):
+        with self.swap(suggestion_models, 'THRESHOLD_TIME_BEFORE_ACCEPT', 0):


Um we can do better. When we say 0, we are actually not testing the logic which takes a time delta and subtracts it from the current time. It is also fine if the delay is a few msecs only.

so you mean put about 10 msec or something like that instead of 0?

I added a separate test that checks with the actual value and returns an empty list. This makes sure that the subtraction logic works fine. Is this fine?

Well, unless you are asserting the actual time calculated, you won't know if it is correct or not. One way to verify could be to retrieve a suggestion just before that calculated time or you could abstract that functionality into a method and test that method separately.

anmolshkl · 2018-06-26T16:25:37Z

core/storage/suggestion/gae_models.py

@@ -72,6 +74,12 @@
 # The delimiter to be used in score category field.
 SCORE_CATEGORY_DELIMITER = '.'

+# Threshold time after which suggestion is considered stale and auto-accepted.
+THRESHOLD_TIME_BEFORE_ACCEPT = 14 * 24 * 60 * 60 * 1000


Are we running this weekly or bi-weekly?

I am actually not sure.. Open for your opinion here. It feels weird that I put the threshold as 14 days while the cron job runs weekly. I think weekly is good? (If so i'll make this 7*....) What do you say?

Made it weekly. If you feel otherwise, we'll change it.

codecov-io · 2018-06-26T17:21:26Z

Codecov Report

Merging #5155 into develop will decrease coverage by 0.21%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5155      +/-   ##
===========================================
- Coverage    47.49%   47.27%   -0.22%     
===========================================
  Files          441      457      +16     
  Lines        25537    26671    +1134     
  Branches      4019     4162     +143     
===========================================
+ Hits         12129    12609     +480     
- Misses       13408    14062     +654

Impacted Files	Coverage Δ
...mplates/dev/head/pages/story_editor/StoryEditor.js	`15.38% <0%> (-59.62%)`	⬇️
...re/templates/dev/head/services/RteHelperService.js	`67.5% <0%> (-18.22%)`	⬇️
...s/schema_editors/SchemaBasedHtmlEditorDirective.js	`16.66% <0%> (-16.67%)`	⬇️
...ead/domain/topic/EditableTopicBackendApiService.js	`61.29% <0%> (-13.22%)`	⬇️
...templates/dev/head/components/forms/FormBuilder.js	`37.19% <0%> (-12.81%)`	⬇️
...r/editor_tab/UnresolvedAnswersOverviewDirective.js	`2.15% <0%> (-11.19%)`	⬇️
...ead/domain/story/EditableStoryBackendApiService.js	`92.5% <0%> (-7.5%)`	⬇️
...head/pages/topic_editor/TopicEditorStateService.js	`86.45% <0%> (-6.31%)`	⬇️
core/templates/dev/head/filters.js	`66.91% <0%> (-2.44%)`	⬇️
...ploration_editor/feedback_tab/ThreadDataService.js	`23.57% <0%> (-1.16%)`	⬇️
... and 52 more

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 70a244a...d4ae46e. Read the comment docs.

anmolshkl · 2018-07-01T04:04:36Z

core/controllers/cron.py

+    def get(self):
+        """Handles get requests."""
+        suggestions = suggestion_services.get_all_stale_suggestions()
+        print suggestions


We should drop this

seanlip · 2018-07-04T20:14:48Z

core/storage/suggestion/gae_models.py

@@ -72,6 +74,12 @@
 # The delimiter to be used in score category field.
 SCORE_CATEGORY_DELIMITER = '.'

+# Threshold time after which suggestion is considered stale and auto-accepted.
+THRESHOLD_TIME_BEFORE_ACCEPT = 7 * 24 * 60 * 60 * 1000


Name of constant should include units.

Done. I added MSECS. I checked other locations and they had it in the short form too. MILLISECONDS would make the variable name too big I feel :)

seanlip · 2018-07-04T20:15:43Z

core/storage/suggestion/gae_models.py

+THRESHOLD_TIME_BEFORE_ACCEPT = 7 * 24 * 60 * 60 * 1000
+
+# The default message to be shown when accepting stale suggestions.
+DEFAULT_SUGGESTION_ACCEPT_MESSAGE = 'Accepting suggestion due to inactivity.'


Message is not clear -- whose inactivity?

"Automatically accepting suggestion after X days." would be better. Make sure X is a named constant which gets interpolated and that it is also used in THRESHOLD_TIME_BEFORE_ACCEPT.

seanlip · 2018-07-04T20:16:28Z

core/domain/suggestion_services.py

@@ -63,7 +63,7 @@ def create_suggestion(
    if target_type == suggestion_models.TARGET_TYPE_EXPLORATION:
        thread_id = feedback_services.create_thread(
            target_id, None, author_id, description,
-            DEFAULT_SUGGESTION_THREAD_SUBJECT)
+            DEFAULT_SUGGESTION_THREAD_SUBJECT, True)


If an arg has a default value, the name of the arg should be specified in caller functions. /cc @apb7 @kevinlee12

seanlip · 2018-07-04T20:19:59Z

core/controllers/cron.py

+    @acl_decorators.can_perform_cron_tasks
+    def get(self):
+        """Handles get requests."""
+        suggestions = suggestion_services.get_all_stale_suggestions()


Add a boolean flag in feconf (ENABLE_AUTO_ACCEPT_OF_SUGGESTIONS) that's set to False by default. We'll turn it on to True for the deployment. But I think we should have a mechanism to turn this off easily if we see issues in practice.

nithusha21 · 2018-07-11T11:10:20Z

Hi @seanlip, could you take another pass on this PR?

seanlip

Thanks! Looks fine from codeowner perspective.

seanlip · 2018-07-12T04:35:16Z

index.yaml

@@ -112,6 +112,12 @@ indexes:
  - name: exploration_id
  - name: last_updated

+- kind: GeneralSuggestionModel
+  properties:
+  - name: deleted


not sure why "deleted" is in here? If you remove this stanza and rerun on dev server, does this get autogenerated exactly the way it is now?

This was auto generated. Will try again and let you know.

Just confirmed. So I deleted these lines, and ran the cron job and these lines got added back. Looking around this file a few of the other models also had "deleted". But I have no idea on why "deleted" is being added here.

Hm OK, no idea either. But happy to merge! Thanks for confirming.

…pt suggestions after a threshold (#5155) * added cron job to accept suggestions weekly * fixed linting * updated last_updated field whenever a message is sent to the thread * linting fixed * review changes * linting fixes and minor change * remove print * review changes

nithusha21 added 4 commits June 26, 2018 16:57

added cron job to accept suggestions weekly

088a895

fixed linting

76abecb

updated last_updated field whenever a message is sent to the thread

5912f81

linting fixed

b3afba8

nithusha21 requested a review from anmolshkl June 26, 2018 12:51

anmolshkl suggested changes Jun 26, 2018

View reviewed changes

nithusha21 added 2 commits June 27, 2018 17:28

review changes

2c66594

linting fixes and minor change

7dc58ec

anmolshkl suggested changes Jul 3, 2018

View reviewed changes

remove print

ac57880

nithusha21 requested review from BenHenning and seanlip as code owners July 4, 2018 11:06

anmolshkl approved these changes Jul 4, 2018

View reviewed changes

seanlip reviewed Jul 4, 2018

View reviewed changes

review changes

d4ae46e

seanlip approved these changes Jul 12, 2018

View reviewed changes

seanlip merged commit bd31875 into oppia:develop Jul 12, 2018

nithusha21 deleted the suggestions-cron-job branch July 12, 2018 04:52

nithusha21 mentioned this pull request Jul 12, 2018

Fixes travis build after merging #5155 #5269

Merged

6 tasks

seanlip added PR: for current release PR: released labels Jul 12, 2018

seanlip mentioned this pull request Jul 12, 2018

Fix suggestions subject not displayed #5273

Merged

6 tasks

seanlip removed PR: for current release PR: released labels Aug 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalised Review System Milestone 2: Cron job to automatically accept suggestions after a threshold #5155

Generalised Review System Milestone 2: Cron job to automatically accept suggestions after a threshold #5155

nithusha21 commented Jun 26, 2018 •

edited

Loading

anmolshkl left a comment

anmolshkl Jun 26, 2018

nithusha21 Jun 27, 2018

anmolshkl Jun 26, 2018

nithusha21 Jun 26, 2018

nithusha21 Jun 26, 2018

anmolshkl Jun 29, 2018

anmolshkl Jun 26, 2018

nithusha21 Jun 26, 2018

nithusha21 Jun 27, 2018

anmolshkl Jun 29, 2018

anmolshkl Jun 26, 2018

nithusha21 Jun 26, 2018

nithusha21 Jun 27, 2018

codecov-io commented Jun 26, 2018 •

edited

Loading

anmolshkl Jul 1, 2018

nithusha21 Jul 4, 2018

seanlip Jul 4, 2018

nithusha21 Jul 6, 2018

seanlip Jul 4, 2018

nithusha21 Jul 6, 2018

seanlip Jul 4, 2018

nithusha21 Jul 6, 2018

seanlip Jul 4, 2018

nithusha21 Jul 6, 2018

nithusha21 commented Jul 11, 2018

seanlip left a comment

seanlip Jul 12, 2018

nithusha21 Jul 12, 2018

nithusha21 Jul 12, 2018

seanlip Jul 12, 2018

nithusha21 Jul 12, 2018

Generalised Review System Milestone 2: Cron job to automatically accept suggestions after a threshold #5155

Generalised Review System Milestone 2: Cron job to automatically accept suggestions after a threshold #5155

Conversation

nithusha21 commented Jun 26, 2018 • edited Loading

Explanation

Checklist

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

codecov-io commented Jun 26, 2018 • edited Loading

Codecov Report

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 Jul 11, 2018

seanlip 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

nithusha21 commented Jun 26, 2018 •

edited

Loading

codecov-io commented Jun 26, 2018 •

edited

Loading