Generalised review system - Feedback thread generalisation #5235

nithusha21 · 2018-07-09T11:21:56Z

Explanation

This PR aims to generalise feedback threads so that they can be used in entities other than explorations.

It is quite a complicated process as a lot of models use thread_id as a part of their id. And this PR changes thread_id itself. So the overall migration involves 5 different models.

I've added 2 new parameters in the thread model, there is one parameter "state_name" which is present in the model, but I am unable to think of what to generalise it to. I'm not sure either of the use of having state_name in the thread model (If it for knowing which state the suggestion is made to, that is handled differently). Depending on whether there would be a use for a similar parameter in other entities, I'll make changes to that field.

Also, as the migration involves editing the id of the instance, we cannot just edit the instance and save it. So we will create new instances with the same data (except different id). This would mean that each entity would be duplicated. I am unable to think of a different approach to this issue, while keeping in mind that multiple runs of the job should yield same output.

We should run a cleanup job after successfully running this job, which deletes all the duplicate instances.

PTAL @anmolshkl @seanlip. Thanks!
TODO:

Fix test suites which fail on travis (if any)
Add a backend test for one of the migration jobs

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.

…-migration

anmolshkl

@nithusha21 took the first pass. Will take a more detailed pass tomorrow. PTAL, thanks!

anmolshkl · 2018-07-09T16:40:10Z

core/storage/feedback/gae_models.py

    # ID of state the thread is for. Does not exist if the thread is about the
-    # entire exploration.
+    # entire exploration (Deprecated).
    state_name = ndb.StringProperty(indexed=True)


@nithusha21 did we check where all this is used currently?

As per Sean's comment, I have removed this field completely as it is not necessary.

anmolshkl · 2018-07-09T16:40:33Z

core/storage/feedback/gae_models.py

    """
-    # ID of the exploration the thread is about.
-    exploration_id = ndb.StringProperty(required=True, indexed=True)
+    # TODO (nithesh): After migrating data to the new fields, set those fields


I think we can be more specific as to which fields exactly we are referring to

I removed this comment, as we are using new models instead of the existing ones

anmolshkl · 2018-07-09T16:42:33Z

core/storage/feedback/gae_models.py

+
+    # ID of the exploration the thread is about (Deprecated).
+    exploration_id = ndb.StringProperty(required=False, indexed=True)
+


blank line?

anmolshkl · 2018-07-09T17:24:51Z

core/controllers/feedback_test.py


-        # The resulting thread should contain two messages.
-        response_dict = self.get_json(thread_url)
+            # The resulting thread should contain two messages.


extra space?

This is actually an indent to be within the "with self.swap(....)" block.

anmolshkl · 2018-07-09T17:34:17Z

core/domain/feedback_jobs_one_off.py

+
+class FeedbackThreadIdMigrationOneOffJob(jobs.BaseMapReduceOneOffJobManager):
+    """One-off job for migrating instances of feedback thread model and related
+    models to have thread ID of the for entity_type.entity_id.random_str.


of the for? I think we should revisit this comment

possibly "for" --> "form"

anmolshkl · 2018-07-09T17:38:51Z

core/domain/feedback_jobs_one_off.py

+            if len(old_id.split('.')) == 3:
+                new_id = '.'.join(
+                    [old_id.split('.')[0], 'exploration', old_id.split('.')[1],
+                     old_id.split('.')[2]])


I think we can/should avoid using old_id.split repeatedly

anmolshkl · 2018-07-09T17:40:19Z

core/domain/feedback_jobs_one_off.py

+                if len(thread_id.split('.')) == 2:
+                    new_thread_id = 'exploration.' + thread_id
+                    if new_thread_id not in item.feedback_thread_ids:
+                        item.feedback_thread_ids.append(new_thread_id)


we are not removing the old thread IDs?

Not yet (maintaining backward compatibility if we end up reverting)

So, what is the plan for deleting the old models and entries? Have we analyzed that keeping it around will not cause any side-effects?

Made a new field instead as suggested.

anmolshkl · 2018-07-09T17:42:53Z

core/domain/feedback_jobs_one_off.py

+                    new_thread_id = 'exploration.' + thread_id
+                    if new_thread_id not in item.feedback_thread_ids:
+                        item.feedback_thread_ids.append(new_thread_id)
+            item.put()


Not sure but does this guarantee that unnecessary (when nothing is updated) writes do not occur?

I don't think this guarantees that (because the last_updated field would change right?). However, as we are doing it differently (using new models), I don't think it is problematic.

Yep, this comment is not valid now.

seanlip · 2018-07-09T18:42:30Z

Hi @nithusha21 -- just so I understand, is this a different, completely separate migration from the one I'm supposed to run in the July release? If so, is this migration slated for the August release (and is it OK to only run it then)?

nithusha21 · 2018-07-10T04:00:31Z

Yes @seanlip, this is targetted for the August release. It's separate :)

seanlip

Took a very cursory look (not a full pass, which I'll do after @anmolshkl gives LGTM). But, in response to the questions:

state_name was indeed meant originally for knowing what state the suggestion was made to. It has no other purpose, as far as I remember.
I suggest making a new model type, like you did for suggestions. That should be easier to handle. I think you're going to run into problems if you try to migrate instances of a model class to other instances of the same class with different IDs -- you'll have to add a bunch of bookkeeping to tell what's old and what's new, and you might as well short-circuit all that by creating a new model class in the first place.

seanlip · 2018-07-10T07:24:53Z

core/controllers/feedback.py

@@ -35,7 +35,7 @@ class ThreadListHandler(base.BaseHandler):
    def get(self, exploration_id):
        self.values.update({
            'threads': [t.to_dict() for t in feedback_services.get_all_threads(
-                exploration_id, False)]})
+                'exploration', exploration_id, False)]})


Should be using a named constant instead of 'exploration', here and elsewhere.

seanlip · 2018-07-10T07:25:32Z

core/domain/feedback_jobs_one_off.py

+
+class FeedbackThreadIdMigrationOneOffJob(jobs.BaseMapReduceOneOffJobManager):
+    """One-off job for migrating instances of feedback thread model and related
+    models to have thread ID of the for entity_type.entity_id.random_str.


possibly "for" --> "form"

seanlip · 2018-07-10T07:27:29Z

core/domain/feedback_jobs_one_off.py

+                    status=item.status, subject=item.subject,
+                    summary=item.summary, has_suggestion=item.has_suggestion,
+                    message_count=item.message_count,
+                    last_updated=item.last_updated, created_on=item.created_on,


On looking at BaseModel in core.storage it says that last_updated cannot be set directly. If that's the case then ... you have a problem here, right?

Yeah. I went with your solution to override the field in the sub class and then do this (also I added this case to the backend tests and it worked fine after the model change).

codecov-io · 2018-07-10T16:41:52Z

Codecov Report

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

@@            Coverage Diff             @@
##             develop    #5235   +/-   ##
==========================================
  Coverage           ?   47.73%           
==========================================
  Files              ?      489           
  Lines              ?    28005           
  Branches           ?     4327           
==========================================
  Hits               ?    13368           
  Misses             ?    14637           
  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 e94b7ba...9c66c18. Read the comment docs.

nithusha21 · 2018-07-10T16:52:28Z

Hi @seanlip @anmolshkl I will make new models instead of the existing ones soon. I have however fixed the backend tests. Thanks!

oppiabot · 2018-07-12T08:17:29Z

Hi @nithusha21. The latest commit in this PR has resulted in a merge conflict. Please follow this link if you need help to resolve the conflict. Thanks!

anmolshkl · 2018-07-17T14:50:10Z

@nithusha21 any updates?

nithusha21 · 2018-07-17T14:53:29Z

Hi @anmolshkl. Thanks for the ping. I am currently fixing up the blocking bug PR #5273. It needs a frontend refactor which I am doing. Will update this PR by tomorrow!

…tion

anmolshkl

@nithusha21 took the first pass. I will review the test cases in the next review tomorrow.

anmolshkl · 2018-07-23T14:31:46Z

core/controllers/feedback_test.py

-        # The resulting thread should contain two messages.
-        response_dict = self.get_json(thread_url)
+            # The resulting thread should contain two messages.
+            response_dict = self.get_json(thread_url)


Actually this is an indent, to be part of the with self.swap(...) block

anmolshkl · 2018-07-23T15:06:14Z

core/controllers/tasks.py

-
-        model = email_models.FeedbackEmailReplyToIdModel.get(
-            user_id, reference_dict['thread_id'])
+        if feconf.ENABLE_GENERALIZED_FEEDBACK_THREADS:


@nithusha21 can we please file an issue to fix this once we make the release?

You mean to remove the old stuff that is getting replaced?

If so, yeah, will do 👍

Yep, in the previous projects, I have noticed that we generally tend to forget these things. So, please file an issue once this PR is merged.

Filed #5370

anmolshkl · 2018-07-23T15:06:41Z

core/controllers/feedback_test.py


-        # The resulting thread should contain two messages.
-        response_dict = self.get_json(thread_url)
+            # The resulting thread should contain two messages.


Actually this is an indent, to be part of the with self.swap(...) block

ah, right. For some reason, I always miss it :p

anmolshkl · 2018-07-23T15:24:32Z

core/domain/feedback_jobs_continuous.py

-    def get_thread_analytics_multi(cls, exploration_ids):
-        """Gets the thread analytics for the explorations specified by the
-        exploration_ids.
+    def get_thread_analytics_multi(cls, entity_ids):


@nithusha21 this looks like an entity specific continuous job. If that is the case, I think it makes sense to make it Exploration specific and call it ExplorationFeedbackAnalytics.... So, for Questions, will we need another such job? If yes, then, I suggest filling an issue for this (so it can be taken up post GSoC).

I tried to generalise here too... but I am not entirely sure how that would turn out. So for the moment, I restricted entity_type to exploration. I think it should be possible to generalise this job (may not need a new job for questions). But I am not sure of how to handle the realtime data related stuff and if we will lose some data if we just edit the job.

@seanlip what are your thoughts? I think we should create an issue and leave that for post-GSoC.

If you take a look at what this job is used for, it is for showing feedback thread analytics to creators. So I think this job should not be generalized and should still be used only for feedback threads in explorations. After the generalized review system is introduced, it should still continue to work as it is working now.

Also, there is no need to implement this job for other types of feedback threads unless a need arises.

(Also, in general, please don't hesitate to ping me if you have "product" questions. I'd be happy to help with that!)

Thanks for those inputs! Makes sense. If we require this feature for non-exploration threads in the future, we will generalise it. But for now, I'll leave it as such.

anmolshkl · 2018-07-23T15:25:54Z

core/domain/feedback_jobs_continuous.py

@@ -201,7 +214,8 @@ def _get_continuous_computation_class(cls):

    @classmethod
    def entity_classes_to_map_over(cls):
-        return [feedback_models.FeedbackThreadModel]
+        return [feedback_models.FeedbackThreadModel,


@nithusha21 the same goes for this model too - if this is exploration specific, lets make it look exploration specific. On a side note, do you think we can make the analytics model generalized?

I thought of trying to do that too. But it is slightly different (using realtime models, which I need to read up about). Basically I am not sure of what strategy we should use to generalize the continuous jobs. Also I am not sure, but I think, even without these jobs, the feedback threads can work (basic functionality to message to a thread, etc) for other entities. So I decided to leave it for later.

As an FYI, you can read up about realtime models in core/jobs.py. See the documentation for BaseContinuousComputationManager.

Oh, on reading @anmolshkl's review message again, I think the model in question was feedback_models.FeedbackThreadModel? If so, we cannot rename it as it has data already in it. I think we can rename FeedbackAnalyticsMRJobManager to ExplorationFeedbackAnalyticsMRJobManager though. Shall I do that?

Also yeah, I will read about realtime models, thanks for the pointer @seanlip

Let's leave the job name as is -- just be really clear in the docstrings that it is specific to exploration feedback and not a general job. This is because it's a continuous computation that's currently running and we don't want errors to occur in the next release just because we forget to stop and start it.

Ok. Thanks!

anmolshkl · 2018-07-23T15:28:35Z

core/domain/feedback_jobs_one_off.py

+                    new_thread_id = 'exploration.' + thread_id
+                    if new_thread_id not in item.feedback_thread_ids:
+                        item.feedback_thread_ids.append(new_thread_id)
+            item.put()


Yep, this comment is not valid now.

anmolshkl · 2018-07-23T15:55:59Z

core/domain/feedback_jobs_one_off.py

+                if len(thread_id.split('.')) == 2:
+                    new_thread_id = 'exploration.' + thread_id
+                    if new_thread_id not in item.feedback_thread_ids:
+                        item.feedback_thread_ids.append(new_thread_id)


So, what is the plan for deleting the old models and entries? Have we analyzed that keeping it around will not cause any side-effects?

seanlip · 2018-07-25T09:15:55Z

Just a quick note that we need to hurry up and get this in. It has been open too long, and is making some stuff in #5361 more complicated than necessary (as I think @nithusha21 has realized).

@nithusha21 can you sort out the remaining issues on this PR, make a checkbox for everything else so that Anmol/I can help you sort that out, and try to get this in within the next 24 hours?

nithusha21 · 2018-07-25T09:23:26Z

There were 2 notes from the previous review by Anmol,

Should we generalise the analytics code? -- decided to leave it as it is as per @seanlip's suggestion.
Generalised review system - Feedback thread generalisation #5235 (comment) does appending to the list cause any issues? -- I don't think it does cause any issues as the check for subscription is of the form "is thread id in list". However, for ease of cleanup, I think it would be better to have a new field and populate that with the new thread ids.. What do you say?

seanlip · 2018-07-25T09:38:24Z

I'm fine with a new field. As you say, it makes cleanup easier.

(Also, as a general note -- that second comment is two days old already; you probably should have answered there yesterday. Don't waste time with turnaround on comments -- it just delays things, and time is getting a bit tight.)

anmolshkl

@nithusha21 couple of comments, apart from that the PR LGTM

anmolshkl · 2018-07-25T18:17:47Z

core/domain/feedback_domain.py

@@ -42,13 +43,14 @@ class FeedbackThread(object):
    # TODO (nithesh): Generalise feedback threads so that it is applicable to


perhaps drop this comment now?

anmolshkl · 2018-07-25T18:28:56Z

core/domain/feedback_jobs_one_off_test.py

@@ -388,3 +397,88 @@ def test_suggestion_migration_validation_one_off_job(self):
        output = self._run_validation_job()
        self.assertEqual(output[0][1], output[1][1])
        self.assertEqual(output[0][1], 10)
+
+
+class FeedbackThreadIdMigrationOneJobTest(test_utils.GenericTestBase):


FeedbackThreadIdMigrationOneJobTest -> FeedbackThreadIdMigrationOneOffJobTest

anmolshkl · 2018-07-25T18:34:53Z

core/domain/feedback_services.py

+        if feconf.ENABLE_GENERALIZED_FEEDBACK_THREADS:
+            suggestion_id = thread_id
+        else:
+            suggestion_id = 'exploration.' + thread_id


Here and everywhere else: let us be consistent with the usage of 'exploration.'. At some places, we are using the constant defined in feconf, while at other places we are just using 'exploration.'. Let us use feconf.ENTITY_TYPE_EXPLORATION throughout.

Replaced throughout the code

seanlip

On the whole, looks pretty good! More proofreading issues in the storage layer section, but all comments are fairly minor (I think) and should not be too hard to address.

seanlip · 2018-07-26T07:40:09Z

core/domain/feedback_domain.py

@@ -176,20 +186,23 @@ def to_dict(self):

 class FeedbackAnalytics(object):
    """Domain object representing feedback analytics for a specific
-    exploration.
+    entity.


Nit: you can probably move this to the previous line.

seanlip · 2018-07-26T07:41:03Z

core/domain/feedback_jobs_continuous.py

-    def get_thread_analytics_multi(cls, exploration_ids):
-        """Gets the thread analytics for the explorations specified by the
-        exploration_ids.
+    def get_thread_analytics_multi(cls, entity_ids):


If this is just for explorations then why are we using the generic name entity_ids? Ditto below.

Also for the realtime part above, we should not be listening to all feedback events right? Just the ones for explorations...

Did you read up on how the realtime layer works?

Yes I did take a look at BaseContinuousComputationManager. I edited the function in feedback_services to start the corresponding events only when the entity_type is exploration.

seanlip · 2018-07-26T07:43:50Z

core/domain/feedback_jobs_continuous.py

@@ -201,7 +200,8 @@ def _get_continuous_computation_class(cls):

    @classmethod
    def entity_classes_to_map_over(cls):
-        return [feedback_models.FeedbackThreadModel]
+        return [feedback_models.FeedbackThreadModel,


Why is this mapping over both items and not just the newer one?

Should it branch based on the constant?

Or should it just be the newer one and in the release instructions you include an instruction to stop this continuous computation job and start it again after the migration?

It should be a branch based on the constant, edited appropriately.

core/domain/feedback_jobs_one_off.py

seanlip · 2018-07-26T08:00:42Z

core/domain/subscription_services_test.py

@@ -159,7 +165,8 @@ def test_posting_to_feedback_thread_results_in_subscription(self):
        # The viewer posts a message to the thread.
        message_text = 'text'
        feedback_services.create_thread(
-            'exp_id', 'state_name', self.viewer_id, 'subject', message_text)
+            'exploration', 'exp_id', 'state_name', self.viewer_id, 'subject',


Probably 'exploration' should be TARGET_TYPE_EXPLORATION instead?

seanlip · 2018-07-26T08:18:48Z

core/storage/feedback/gae_models.py

+            messages.
+
+        """
+


Drop newline.

seanlip · 2018-07-26T08:19:47Z

core/storage/feedback/gae_models_test.py

@@ -32,6 +32,9 @@ def test_put_function(self):
        feedback_thread_model = feedback_models.FeedbackThreadModel(
            exploration_id='exp_id_1')
        feedback_thread_model.put()
+        feedback_thread_model = feedback_models.GeneralFeedbackThreadModel(
+            entity_type='exploration', entity_id='exp_id_1')


'exploration' should probably be a named constant

seanlip · 2018-07-26T08:20:31Z

core/storage/user/gae_models.py

@@ -274,6 +274,9 @@ class UserSubscriptionsModel(base_models.BaseModel):
    collection_ids = ndb.StringProperty(repeated=True, indexed=True)
    # IDs of feedback thread ids that this user subscribes to.
    feedback_thread_ids = ndb.StringProperty(repeated=True, indexed=True)
+    # IDs of feedback thread ids (new framework) that this user subscribes to.


You should have a TODO/issue to remove the "(new framework)" and at the same time mark feedback_thread_ids etc as "deprecated, do not use" (see similar fields in ExplorationModel for the convention).

Filed #5370

seanlip · 2018-07-26T08:20:37Z

core/tests/load_tests/feedback_thread_summaries_test.py

                self.user_id, self.EXPECTED_THREAD_DICT['subject'],
                'not used here')
        threadlist = feedback_services.get_all_threads(
-            self.EXP_ID_1, False)
+            'exploration', self.EXP_ID_1, False)


named constant

seanlip · 2018-07-26T08:20:53Z

feconf.py

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

+ENABLE_GENERALIZED_FEEDBACK_THREADS = False


Add single-line comment.

seanlip

OK, looking good! Just 3 more small things.

seanlip · 2018-07-27T05:45:04Z

core/domain/feedback_jobs_one_off.py

@@ -212,6 +212,7 @@ def map(item):
                message_count=item.message_count,
                last_updated=item.last_updated, created_on=item.created_on,
                deleted=item.deleted).put(update_last_updated_time=False)
+            yield('GeneralFeedbackThreadModel', 1)


See other code: there should be a space between "yield" and "(". Ditto below.

seanlip · 2018-07-27T05:47:18Z

core/domain/feedback_services.py

@@ -171,12 +171,18 @@ def create_message(
            exploration_id = thread.exploration_id
        if message_id == 0:
            # New thread.
-            event_services.FeedbackThreadCreatedEventHandler.record(
-                exploration_id)
+            if not (


Add a test that ensures that other threads will not trigger this, but exploration threads will.

In general if you make an error then that indicates a hole in the test coverage. So you need to write a test that would have caught the error.

seanlip · 2018-07-27T05:50:17Z

core/storage/feedback/gae_models.py

+    # Latest subject of the thread.
+    subject = ndb.StringProperty(indexed=False)
+    # Summary text of the thread.
+    summary = ndb.TextProperty(indexed=False)


Wait, sorry. I don't understand. The scenario you're describing is intended behaviour. Is that when "required" was True? If so then required should be True.

If on the other hand that's what happens when required = False, then what changes when required=True?

nithusha21 · 2018-07-28T12:09:23Z

Thanks!

nithusha21 added 7 commits June 25, 2018 12:15

initial changes

1daed36

fix backend tests

e7e618c

Merge remote-tracking branch 'origin/develop' into feedback-thread-id…

4aa7d03

…-migration

generalised services, and fixed relevant tests

d99aa61

linting fixes

ea149b8

more linting fixes

b5c97c8

added migration jobs

2fc098a

nithusha21 requested review from anmolshkl and seanlip July 9, 2018 11:21

nithusha21 requested a review from BenHenning as a code owner July 9, 2018 11:21

nithusha21 mentioned this pull request Jul 9, 2018

Refactoring of feedback framework #4995

Closed

6 tasks

anmolshkl suggested changes Jul 9, 2018

View reviewed changes

seanlip reviewed Jul 10, 2018

View reviewed changes

review changes and fix backend tests

6103252

fix more backend tests

7eb89a2

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

nithusha21 added 6 commits July 18, 2018 18:43

fix merge conflicts

243adbf

fix travis build

22f9cdd

added new models and attempts to fix backend tests

924b6d5

fix merge conflicts

4397575

fix more backend tests

b8e4c35

linting fixes

6551882

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

nithusha21 added 3 commits July 23, 2018 16:37

use named constant, fix backend tests

6535bc7

fix failing test, make new reply-to-id model, complete test for migra…

b1d3156

…tion

linting fixes

3a23369

anmolshkl suggested changes Jul 23, 2018

View reviewed changes

nithusha21 mentioned this pull request Jul 23, 2018

Suggestions for questions part 1: Backend domain class and controllers #5361

Merged

6 tasks

nithusha21 added 3 commits July 25, 2018 16:09

use new field to store feedback thread ids subscribed to

97735d4

linting fixes

fe2b647

fix tests and remove prints

b5b72c3

anmolshkl suggested changes Jul 25, 2018

View reviewed changes

nithusha21 added 3 commits July 26, 2018 12:08

review changes

f800464

linting fixes

d74bfd1

linting fix

580ec61

seanlip requested changes Jul 26, 2018

View reviewed changes

nithusha21 added 3 commits July 26, 2018 15:57

review changes

9ebaf93

backend test fix

2b1a48f

more review changes

0ca7693

This was referenced Jul 26, 2018

Cleanup old feedback models, and related data #5370

Closed

Make message_count field in GeneralFeedbackThreadModel required #5371

Closed

seanlip reviewed Jul 27, 2018

View reviewed changes

nithusha21 added 3 commits July 27, 2018 16:39

review changes

f1bc58a

linting fixes

5ef4ce0

remove mock function and use actual one

9c66c18

seanlip approved these changes Jul 27, 2018

View reviewed changes

anmolshkl approved these changes Jul 28, 2018

View reviewed changes

nithusha21 merged commit 70d02ab into oppia:develop Jul 28, 2018

nithusha21 deleted the feedback-thread-id-migration branch July 28, 2018 12:09


		# ID of the exploration the thread is about (Deprecated).
		exploration_id = ndb.StringProperty(required=False, indexed=True)

		@@ -42,13 +43,14 @@ class FeedbackThread(object):
		# TODO (nithesh): Generalise feedback threads so that it is applicable to

Generalised review system - Feedback thread generalisation #5235

Generalised review system - Feedback thread generalisation #5235

Conversation

nithusha21 commented Jul 9, 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

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

anmolshkl Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jul 9, 2018

nithusha21 commented Jul 10, 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

codecov-io commented Jul 10, 2018 • edited Loading

Codecov Report

nithusha21 commented Jul 10, 2018

oppiabot bot commented Jul 12, 2018

anmolshkl commented Jul 17, 2018

nithusha21 commented Jul 17, 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

seanlip Jul 24, 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

Choose a reason for hiding this comment

seanlip commented Jul 25, 2018

nithusha21 commented Jul 25, 2018

seanlip commented Jul 25, 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

nithusha21 commented Jul 9, 2018 •

edited

Loading

anmolshkl Jul 9, 2018 •

edited

Loading

codecov-io commented Jul 10, 2018 •

edited

Loading

seanlip Jul 24, 2018 •

edited

Loading