[PATCH v2 1/5] api: add addressed field and detail endpoint for patch comments

[Daniel Axtens]

> Thinking out loud: I suspect this will be an expensive migration since it will
> add a new non-nullable field to all patchcomment rows. Maybe that's something we
> can live with, but making this nullable might make more sense, particularly if
> we want to make this feature enableable/disableable on a project-by-project
> basis (I don't know if we do).

There's probably some context here that hopefully you've caught up with
Raxel and co about.

But anyway, the addressed/unaddressed thing just appears in the header
bar of each comment along with the commenter, date and the
permalink. Maintainers/submitters/commenters can chose whether they want
to look at addressed/unaddressed or they want to ignore it. I suspect
the main value will be for submitters to make sure they've addressed the
comments before they send out their next version so in that sense it
isn't even all that dependent on the whims of maintainers. So I was
thinking of just making the whole thing on all the time rather than
introducing the complexity of a per-project feature-flag.

My vibe from the kernel lists I hang out on is that we don't tend to get
quite as many comments as patches, but it could be a pretty noisy
distribution so I might have underestimated it. Anyway, I'll do a test
with and without nullable on my largish data set tomorrow, hopefully.

Kind regards,
Daniel

>> > +        ),
>> > +    ]
>> > diff --git a/patchwork/models.py b/patchwork/models.py
>> > index 00273da..ef52f2c 100644
>> > --- a/patchwork/models.py
>> > +++ b/patchwork/models.py
>> > @@ -693,6 +693,7 @@ class PatchComment(EmailMixin, models.Model):
>> >          related_query_name='comment',
>> >          on_delete=models.CASCADE,
>> >      )
>> > +    addressed = models.BooleanField(default=False)
>> >  
>> >      @property
>> >      def list_archive_url(self):
>> > @@ -718,7 +719,9 @@ class PatchComment(EmailMixin, models.Model):
>> >          self.patch.refresh_tag_counts()
>> >  
>> >      def is_editable(self, user):
>> > -        return False
>> > +        if user == self.submitter.user:
>> > +            return True
>> > +        return self.patch.is_editable(user)
>> >  
>> >      class Meta:
>> >          ordering = ['date']
>> > diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py
>> > index 5bbebf2..59450d8 100644
>> > --- a/patchwork/tests/api/test_comment.py
>> > +++ b/patchwork/tests/api/test_comment.py
>> > @@ -90,7 +90,7 @@ class TestPatchComments(utils.APITestCase):
>> >          kwargs = {}
>> >          if version:
>> >              kwargs['version'] = version
>> > -        kwargs['pk'] = patch.id
>> > +        kwargs['patch_id'] = patch.id
>> 
>> I think we might be able to get away with renaming the parameter
>> internally even if we can't rename it in old versions of the API, but
>> please split the 'pk'->'patch_id' change into a different patch.
>> 
>> Kind regards,
>> Daniel
>
> Cheers,
> Stephen