Bug fixes for the feedback framework #5467

nithusha21 · 2018-08-08T17:38:04Z

Explanation

Fixes multiple bugs with the feedback framework, which were introduced because of inconsistencies across PRs. Also, on flipping ENABLE_GENERALISED_FEEDBACK_THREADS flag, some backend test failed, and the UI broke. I have fixed those issues too.

PTAL, 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 sorry if I'm being pernickety, just a couple of minor changes. Apart from that, the PR LGTM

anmolshkl · 2018-08-08T18:23:33Z

core/domain/feedback_jobs_continuous_test.py

@@ -267,29 +274,34 @@ def test_thread_closed_job_running(self):
            thread_1.put()

            # Start job.


nit: please move this comment below with

anmolshkl · 2018-08-08T18:24:17Z

core/domain/feedback_jobs_continuous_test.py

@@ -303,14 +315,16 @@ def test_thread_closed_reopened_again(self):
            thread_1.put()

            # Start job.


anmolshkl · 2018-08-08T18:24:34Z

core/domain/feedback_jobs_continuous_test.py

@@ -321,14 +335,16 @@ def test_thread_closed_reopened_again(self):
            thread.put()

            # Restart job.


anmolshkl · 2018-08-08T18:25:02Z

core/domain/feedback_jobs_continuous_test.py

@@ -357,14 +375,16 @@ def test_thread_closed_status_changed(self):
            thread_1.put()

            # Start job.


nit: ditto for this comment

seanlip

One general comment: for every self.swap(..., True) test you should also have a self.swap(..., False) test. We had too many errors here last release and this is a way to make sure that things work correctly in both cases.

I suggest putting the "False" tests into a corresponding *_old_test.py file so we can just delete the file later. Everything in the main codebase should be swapped to True.

seanlip · 2018-08-08T21:21:43Z

assets/constants.js

@@ -474,6 +474,8 @@ var constants = {

  "USE_NEW_SUGGESTION_FRAMEWORK": false,

+  "ENABLE_GENERALIZED_FEEDBACK_THREADS": false,


Either have this in constants.js or feconf.py, but not both. It is possible to import values from constants.js in the backend code.

seanlip · 2018-08-08T21:22:24Z

core/controllers/feedback_test.py

                'subject': self.UNICODE_TEST_STRING,
                'text': self.UNICODE_TEST_STRING,
            }, csrf_token=self.csrf_token,
            expect_errors=True, expected_status_int=401)

-        thread_url = '%s/%s' % (
-            feconf.FEEDBACK_THREAD_URL_PREFIX, '0.dummy_thread_id')
+        with self.swap(feconf, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', True):


why do you need self.swap here? I don't see anything that's dependent on a function.

codecov-io · 2018-08-11T09:08:22Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@994ac33). Click here to learn what that means.
The diff coverage is 15.38%.

@@            Coverage Diff             @@
##             develop    #5467   +/-   ##
==========================================
  Coverage           ?   46.34%           
==========================================
  Files              ?      500           
  Lines              ?    29217           
  Branches           ?     4435           
==========================================
  Hits               ?    13542           
  Misses             ?    15675           
  Partials           ?        0

Impacted Files	Coverage Δ
...ges/exploration_editor/feedback_tab/FeedbackTab.js	`0.7% <0%> (ø)`
...v/head/pages/exploration_player/LearnerLocalNav.js	`3.7% <0%> (ø)`
...ploration_editor/feedback_tab/ThreadDataService.js	`20.75% <12.5%> (ø)`
.../head/domain/suggestion/SuggestionObjectFactory.js	`94.73% <75%> (ø)`

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 994ac33...21c68a2. Read the comment docs.

anmolshkl

@nithusha21 lgtm!

seanlip

Not sure why this is LGTM. @nithusha21 did you see my comments?

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

core/controllers/feedback_test.py

-                'suggestion_html': u'Ֆݓॵক',
-            }, csrf_token=csrf_token)
-        self.logout()
+        with self.swap(constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):


Better to put all the "False" tests in a feedback_old_test.py file, so it can be deleted after the flag is flipped for good.

Moved the old feedback tests to a different suite, also made a suite to test the new suggestions framework with the old feedback framework.

seanlip · 2018-08-12T19:32:41Z

core/controllers/incoming_emails_test.py

-            self.assertEqual(msg.author_id, self.user_id_a)
-            self.assertTrue(msg.received_via_email)
+            with self.swap(
+                constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', True):


What about the False case?

nithusha21 · 2018-08-12T20:55:39Z

PTAL @seanlip

seanlip · 2018-08-13T08:16:46Z

assets/constants.js

@@ -474,6 +474,8 @@ var constants = {

  "USE_NEW_SUGGESTION_FRAMEWORK": false,


Isn't this supposed to be set to true? We flipped it in the last release, it needs to be flipped in develop (or, better still, removed entirely).

Removed completely

seanlip · 2018-08-13T08:21:09Z

core/domain/exp_services_test.py

-            feedback_services.create_suggestion(
-                self.EXP_ID2, self.user_id, 3, self.initial_state_name,
-                'description', 'new text')
+        with self.swap(constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):


Anything not in an "old" test file should be True. Otherwise it'll break when we remove the constant.

There should be corresponding "old" test files with the False conditions.

This is supposed to only work with the flag set as false.

seanlip · 2018-08-13T08:21:28Z

core/domain/feedback_jobs_continuous_test.py

-                    'num_total_threads': 1,
-                })
+            with self.swap(
+                constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):


Same note as above.

Created new "old" test file

seanlip · 2018-08-13T08:21:46Z

core/domain/feedback_jobs_one_off_test.py

@@ -88,7 +88,7 @@ def _run_one_off_job(self):

    def test_message_count(self):
        """Test if the job returns the correct message count."""
-        with self.swap(feconf, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):
+        with self.swap(constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):


Same note as above.

One off jobs are all mapped to FeedbackThreadModel (the old one). So all of these tests will work only on the old framework.

So what happens when we switch to the new framework and run any of these one-off jobs?

Also if these are going to be deprecated after the flag change, then they should go in an old_feedback_one_off_jobs_test.py file, just like all the rest.

On running after switching, It will just run on entities of the old model, and the changes will not reflect in the newly created models. The jobs here also include jobs to convert from old models to new models. I think these tests are not applicable after the flip, and need to be deprecated after.

I see. The codebase needs notes to that effect and there needs to be a TODO recording the specific deprecation tasks required.

(Sorry, not a TODO -- I meant a GitHub issue.)

Hang on -- I looked at this again. The correct thing to have done here would have been to modify the job so that there's a "new" job and an "old" job. The handler would redirect to the correct one based on the status of the flag.

However, I don't think this job is actively being used, and it hasn't been run in a while. So -- in the interest of time, let's just delete it altogether. We can reimplement it later if needed.

Renamed the test file.

seanlip · 2018-08-13T08:22:21Z

core/domain/feedback_services_test.py

@@ -90,8 +91,9 @@ def test_status_of_newly_created_thread_is_open(self):

    def test_get_exp_id_from_thread_id(self):
        thread_id = 'exp1.1234'
-        self.assertEqual(
-            feedback_services.get_exp_id_from_thread_id(thread_id), 'exp1')
+        with self.swap(constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):


Same note as above. "False" tests go in an "old" file. True tests go in this one. The coverage of both files should be the same.

seanlip · 2018-08-13T08:22:32Z

core/domain/feedback_services_test.py

-                self.EXP_ID2, self.user_id, 3, 'state_name', 'description', '')
-        suggestion = feedback_services.get_suggestion(self.THREAD_ID6)
+
+        with self.swap(constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):


nithusha21 · 2018-08-13T19:25:39Z

Not sure why the last commit didn't trigger a travis build to start....

seanlip · 2018-08-13T19:30:32Z

assets/constants.js

@@ -470,7 +470,9 @@ var constants = {
    "\\u001b", "\\u001c", "\\u001d", "\\u001e", "\\u001f"
  ],

-  "USE_NEW_SUGGESTION_FRAMEWORK": false,
+  "ENABLE_GCS_STORAGE_FOR_IMAGES": false,


I don't understand, why is this here? It's been removed already in develop.

Looks like I incorrectly merged. Removed!

seanlip

Approving since this PR is important for the release and we should get this in. It's fine to address remaining comments either in this PR or a separate one.

Thanks @nithusha21!

seanlip · 2018-08-13T19:33:13Z

core/domain/acl_decorators_test.py

-        response = self.mock_testapp.get(
-            '/mock/%s.thread1' % self.published_exp_id, expect_errors=True)
-        self.assertEqual(response.status_int, 302)
+        with self.swap(constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', True):


Do we need to test the False case, or do these handlers simply not exist in that case?

Hmmm. I missed these earlier. They do exist in the false case too. Will add the tests.

seanlip · 2018-08-13T19:34:08Z

core/domain/exp_services_test.py

@@ -3150,130 +3148,6 @@ def test_loading_old_exploration_does_not_break_domain_object_ctor(self):
        self.assertEqual(exploration.to_yaml(), self.UPGRADED_EXP_YAML)


-class SuggestionActionUnitTests(test_utils.GenericTestBase):


Where are the corresponding unit tests for the "True" case? These are important to ensure the whole system still works fine after the flag flip.

These are tests for the old suggestion framework, which is deprecated. And we are basically at a point of no return to that framework (since we removed the flag that allowed us to go back to that framework). This is not intended to work with the flag set as true.

OK -- please add inline comments specifying what exactly this test is testing that is no longer available in the new framework. (Be specific -- don't just say "new framework", "old framework" -- talk about exactly what calls are missing in the new framework that prevent this from being tested there.)

Added a comment

seanlip · 2018-08-13T19:35:44Z

core/domain/feedback_jobs_one_off_test.py

@@ -88,7 +88,7 @@ def _run_one_off_job(self):

    def test_message_count(self):
        """Test if the job returns the correct message count."""
-        with self.swap(feconf, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):
+        with self.swap(constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):


So what happens when we switch to the new framework and run any of these one-off jobs?

Also if these are going to be deprecated after the flag change, then they should go in an old_feedback_one_off_jobs_test.py file, just like all the rest.

seanlip · 2018-08-13T19:38:02Z

core/domain/feedback_services_test.py

-            feedback_services.get_exp_id_from_thread_id(thread_id), 'exp1')
-
-
-class SuggestionQueriesUnitTests(test_utils.GenericTestBase):


Where are the corresponding tests for the "True" case (to ensure that things still work fine after the flag is flipped?

Same as above (regarding old framework)

See my response above too.

Added a comment

seanlip · 2018-08-13T19:40:28Z

core/domain/user_jobs_one_off_test.py

@@ -434,7 +434,7 @@ def test_feedback_thread_subscription(self):
                subscription_services, 'subscribe_to_exploration', self._null_fn
            ):
            with self.swap(
-                feconf, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):
+                constants, 'ENABLE_GENERALIZED_FEEDBACK_THREADS', False):


Why are these not in an "old_*_test.py" file?

What about the corresponding "new" tests?

This one off job was mapping over FeedbackThreadModel (the old model). That's why I forced the constant to false. I am not sure, but I feel this job needs work on the new framework too? If so, I'll add tests (and change the job appropriately) for the new framework. But as of now, as the model under consideration was the old one, I had set the flag to false

This job runs in a cron that is triggered weekly (cron.yaml). If you don't fix this properly for the new framework then that cron job will fail in production.

So this needs the same thorough treatment as the other cases. The URL handler called by the cron should redirect to the old job if the flag hasn't been flipped, and the new job if the flag has been flipped. There should be backend tests for each.

Fixed the job, and also added tests for flag true and false cases.

oppiabot · 2018-08-14T06:14:36Z

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!

seanlip · 2018-08-14T07:12:52Z

feconf.py

@@ -374,6 +374,14 @@ def get_empty_ratings():
 # Disables all the new structures' pages, till they are completed.
 ENABLE_NEW_STRUCTURES = False

+# Flag to disable sending emails related to reviews for suggestions. To be


We really need fewer flags. If you can group these into a single flag, or introduce them only later, that would be helpful. At the moment all the flags for the feedback framework are interfering with each other and you need a matrix of test files.

In fact since you don't use these anywhere I recommend just getting rid of them altogether.

Ah, ignore please. This is already in develop. Not sure why it's showing up as an addition.

seanlip

Just one more thing. LGTM otherwise.

seanlip · 2018-08-14T07:30:44Z

Unconditional LGTM. Confirmed with @nithusha21 that extra flags were in develop, and were also very small in scope (unlike the two large previous ones).

* bug fixes for the feedback framework * review changes * review changes (1) * linting fixes * WIP * split into old test suites * linting fixes, and bug fix * remove imports * review changes * remove constant * add tests for acl decorators * review changes * rename file * linting fix

bug fixes for the feedback framework

28fa69c

nithusha21 requested review from seanlip and anmolshkl August 8, 2018 17:38

anmolshkl suggested changes Aug 8, 2018

View reviewed changes

anmolshkl assigned nithusha21 Aug 8, 2018

review changes

1646037

seanlip requested changes Aug 8, 2018

View reviewed changes

nithusha21 added 2 commits August 11, 2018 14:06

review changes (1)

59f1e05

linting fixes

32ade68

WIP

d4880ce

nithusha21 mentioned this pull request Aug 12, 2018

Generalised Review System: Common suggestion view #5419

Merged

6 tasks

anmolshkl approved these changes Aug 12, 2018

View reviewed changes

seanlip reviewed Aug 12, 2018

View reviewed changes

nithusha21 added 3 commits August 13, 2018 02:03

split into old test suites

7502e5a

linting fixes, and bug fix

33b4d6c

remove imports

096036e

seanlip added this to the Blocking bugs milestone Aug 13, 2018

seanlip mentioned this pull request Aug 13, 2018

Remove flags: USE_NEW_SUGGESTIONS_FRAMEWORK, ENABLE_GCS_STORAGE_FOR_IMAGES #5446

Closed

seanlip removed this from the Blocking bugs milestone Aug 13, 2018

seanlip reviewed Aug 13, 2018

View reviewed changes

nithusha21 added 3 commits August 13, 2018 15:50

Merge remote-tracking branch 'origin/develop' into fix-bugs

c6f6677

review changes

eeeee59

fix merge conflict

90c27bd

seanlip reviewed Aug 13, 2018

View reviewed changes

remove constant

64356a1

seanlip approved these changes Aug 13, 2018

View reviewed changes

add tests for acl decorators

054c954

seanlip added the PR: for current release label Aug 14, 2018

seanlip added this to the Blocking bugs milestone Aug 14, 2018

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

nithusha21 added 4 commits August 14, 2018 12:05

review changes

b8442e4

rename file

dabc6a3

fix merge conflicts

e854929

linting fix

21c68a2

seanlip reviewed Aug 14, 2018

View reviewed changes

seanlip approved these changes Aug 14, 2018

View reviewed changes

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

seanlip added the PR: LGTM label Aug 14, 2018

nithusha21 merged commit 465bf76 into oppia:develop Aug 14, 2018

nithusha21 deleted the fix-bugs branch August 14, 2018 08:18

seanlip added the PR: released label Aug 14, 2018

BenHenning removed PR: released PR: for current release labels Sep 3, 2018

BenHenning removed this from the Blocking bugs milestone Sep 3, 2018

		@@ -267,29 +274,34 @@ def test_thread_closed_job_running(self):
		thread_1.put()

		# Start job.

		@@ -303,14 +315,16 @@ def test_thread_closed_reopened_again(self):
		thread_1.put()

		# Start job.

		@@ -321,14 +335,16 @@ def test_thread_closed_reopened_again(self):
		thread.put()

		# Restart job.

		@@ -357,14 +375,16 @@ def test_thread_closed_status_changed(self):
		thread_1.put()

		# Start job.

		@@ -474,6 +474,8 @@ var constants = {

		"USE_NEW_SUGGESTION_FRAMEWORK": false,

		"ENABLE_GENERALIZED_FEEDBACK_THREADS": false,

		@@ -3150,130 +3148,6 @@ def test_loading_old_exploration_does_not_break_domain_object_ctor(self):
		self.assertEqual(exploration.to_yaml(), self.UPGRADED_EXP_YAML)


		class SuggestionActionUnitTests(test_utils.GenericTestBase):

		feedback_services.get_exp_id_from_thread_id(thread_id), 'exp1')


		class SuggestionQueriesUnitTests(test_utils.GenericTestBase):

Bug fixes for the feedback framework #5467

Bug fixes for the feedback framework #5467

Conversation

nithusha21 commented Aug 8, 2018

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

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

codecov-io commented Aug 11, 2018 • edited Loading

Codecov Report

anmolshkl left a comment

Choose a reason for hiding this comment

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

nithusha21 commented Aug 12, 2018

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

seanlip Aug 13, 2018 • 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

nithusha21 commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

oppiabot bot commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Aug 14, 2018

codecov-io commented Aug 11, 2018 •

edited

Loading

seanlip Aug 13, 2018 •

edited

Loading