[PATCH 1/3] Allow a project to restrict submitter state changes

[Daniel Axtens]

Daniel Axtens <dja at axtens.net> writes:

> Stephen Finucane <stephen at that.guru> writes:
>
>> On Fri, 2021-08-06 at 11:39 +1000, Daniel Axtens wrote:
>>> Stephen Finucane <stephen at that.guru> writes:
>>> 
>>> > On Tue, 2021-08-03 at 01:27 +1000, Daniel Axtens wrote:
>>> > > In discussions about how to make patchwork more user-friendly and
>>> > > suitable for more projects, we realised that perhaps the current
>>> > > ability for submitters to change their patch state to any value
>>> > > isn't the most appropriate setting for all maintainers, especially
>>> > > in light of increasing automation.
>>> > > 
>>> > > Allow a project to stop a submitter from changing the state of
>>> > > their patches. This is not the default but can be set by a patchwork
>>> > > administrator.
>>> > 
>>> > Couple of comments below. Unfortunately two of them are of the "I don't know
>>> > about this so can you investigate?" variety. I can help resolve them if
>>> > necessary but I'm hoping you've already done said investigation :-)
>>> > 
>>> > Stephen
>>> > 
>>> > > Signed-off-by: Daniel Axtens <dja at axtens.net>
>>> > > ---
>>> > >  ...45_project_submitter_state_change_rules.py | 24 +++++++++++++
>>> > >  patchwork/models.py | 36 +++++++++++++++++++
>>> > >  patchwork/views/__init__.py | 8 +++++
>>> > >  patchwork/views/patch.py | 14 ++++++--
>>> > >  4 files changed, 80 insertions(+), 2 deletions(-)
>>> > >  create mode 100644 patchwork/migrations/0045_project_submitter_state_change_rules.py
>>> > > 
>>> > > diff --git a/patchwork/migrations/0045_project_submitter_state_change_rules.py b/patchwork/migrations/0045_project_submitter_state_change_rules.py
>>> > > new file mode 100644
>>> > > index 000000000000..9d0b2892bd5c
>>> > > --- /dev/null
>>> > > +++ b/patchwork/migrations/0045_project_submitter_state_change_rules.py
>>> > > @@ -0,0 +1,24 @@
>>> > > +# Generated by Django 3.1.12 on 2021-08-03 00:32
>>> > > +
>>> > > +from django.db import migrations, models
>>> > > +
>>> > > +
>>> > > +class Migration(migrations.Migration):
>>> > > +
>>> > > + dependencies = [
>>> > > + ('patchwork', '0044_add_project_linkname_validation'),
>>> > > + ]
>>> > > +
>>> > > + operations = [
>>> > > + migrations.AddField(
>>> > > + model_name='project',
>>> > > + name='submitter_state_change_rules',
>>> > > + field=models.SmallIntegerField(
>>> > > + choices=[
>>> > > + (0, 'Submitters may not change patch states'),
>>> > > + (1, 'Submitters may set any patch state')],
>>> > > + default=1,
>>> > > + help_text='What state changes can patch submitters make?'
>>> > > + ' Does not affect maintainers.'),
>>> > > + ),
>>> > 
>>> > This feels like a BooleanField rather than a SmallIntegerField (even if they
>>> > resolve to the same thing on e.g. MySQL 5.x, iirc). afaict, you can pass the
>>> > 'choices' argument for BooleanField too [1]. Any chance we could change this?
>>> > 
>>> > [1] https://code.djangoproject.com/ticket/9640
>>> 
>>> I picked this because I want to add another mode that permits submitters
>>> to change states but restricts the states that submitters can change
>>> between (so for example allowing them to move around the {New, RFC,
>>> Superseded, Not Applicable, Changes Requested} group but not to mark
>>> their own patches as Accepted). And I don't want to do a migration then
>>> if I can avoid it :)
>>
>> Is that necessary? What's the use case? fwiw, I'd like to get rid of State
>> objects entirely. I've a long-term goal of being able to mark a Series as open
>> or closed and make Series more of a first class citizen ('git-pw series list' is
>> effectively useless right now), but doing so requires Patches to also have a
>> boolean open/closed state (assuming we don't want the series and patch states to
>> be totally disconnected and for users to be forced to manually manage series
>> states, that is). I've been envisioning two solutions:
>>
>>  * Transform the 'Patch.state' field to a combination of a boolean
>>    'Patch.resolved' field and a 'Patch.resolution' field, with the latter being
>>    set only if 'Patch.resolved' == 'True'
>>  * Transform the 'Patch.state' field to a boolean and add support for patch
>>    tags, with the current States all becoming tags
>>
>> I've mostly focused on the latter approach since it seems more useful in the
>> long term. The only reason I haven't finished this work is because each time I
>> try, I get stuck writing a migration and shims for the XML-RPC and REST APIs
>> that don't suck. I also don't know how to integrate them nicely with the "tags"
>> people include in patch subject lines (i.e. '[RFC]' or '[stable-2.7]').
>>
>> All this is to say that I think what you've proposed right now would for this
>> new boolean-only future but making this a tertiary thing would not, so ideally
>> I'd like to avoid adding this.
>
> Sorry this kind of dropped off my radar. The usecase is for a maintainer
> with scripts that assume pw patches are in particular states. With some
> limited exceptions, they don't want people moving their patches between
> states, they want to do that themselves.
>

Amusingly(?) the linux-media folk have just accidentally given us a perfect
demonstration of this: https://lore.kernel.org/linux-media/20210901104026.GB2129@kadam/

> I haven't thought a lot about how your model but I would encourage you
> to have a look at the state transitions used on linuxppc-dev on Ozlabs
> and at https://patchwork.kernel.org/project/netdevbpf/list/ (which has
> also added its own bespoke states like "Needs ACK"). I think on
> heavily-used patchworks states have become something of a rich tapestry
> and I don't know how your model will work for those usecases.
>
> (netdevbpf is also a _great_ example of why I want to add another
> setting making delegate-changing a maintainer-only option: you _do not_
> want people to randomly redelegate an ethtool patch over to bpf! You
> want autodelegation rules to do all that work for you and then to never
> have a human touch it.)
>
> Kind regards,
> Daniel
>
>>
>>> 
>>> > 
>>> > > + ]
>>> > > diff --git a/patchwork/models.py b/patchwork/models.py
>>> > > index 00273da9f5bd..706b912c349a 100644
>>> > > --- a/patchwork/models.py
>>> > > +++ b/patchwork/models.py
>>> > > @@ -93,6 +93,19 @@ class Project(models.Model):
>>> > >  send_notifications = models.BooleanField(default=False)
>>> > >  use_tags = models.BooleanField(default=True)
>>> > > + # how much can a patch submitter change?
>>> > > + SUBMITTER_NO_STATE_CHANGES = 0
>>> > > + SUBMITTER_ALL_STATE_CHANGES = 1
>>> > > + SUBMITTER_STATE_CHANGE_CHOICES = (
>>> > > + (SUBMITTER_NO_STATE_CHANGES, 'Submitters may not change patch states'),
>>> > > + (SUBMITTER_ALL_STATE_CHANGES, 'Submitters may set any patch state'),
>>> > > + )
>>> > > + submitter_state_change_rules = models.SmallIntegerField(
>>> > > + choices=SUBMITTER_STATE_CHANGE_CHOICES,
>>> > > + default=SUBMITTER_ALL_STATE_CHANGES,
>>> > > + help_text='What state changes can patch submitters make?'
>>> > > + ' Does not affect maintainers.')
>>> > 
>>> > Ditto.
>>> > 
>>> > > +
>>> > >  def is_editable(self, user):
>>> > >  if not user.is_authenticated:
>>> > >  return False
>>> > > @@ -518,6 +531,29 @@ class Patch(SubmissionMixin):
>>> > >  return True
>>> > >  return False
>>> > > + def can_set_state(self, user):
>>> > > + # an unauthenticated user can never change state
>>> > > + if not user.is_authenticated:
>>> > > + return False
>>> > > +
>>> > > + # a maintainer can always set state
>>> > > + if self.project.is_editable(user):
>>> > > + self._edited_by = user
>>> > > + return True
>>> > > +
>>> > > + # a delegate can always set state
>>> > > + if user == self.delegate:
>>> > > + self._edited_by = user
>>> > > + return True
>>> > > +
>>> > > + # if the state change rules prohibit it, no other user can set change
>>> > > + if (self.project.submitter_state_change_rules ==
>>> > > + Project.SUBMITTER_NO_STATE_CHANGES):
>>> > > + return False
>>> > > +
>>> > > + # otherwise, a submitter can change state
>>> > > + return self.is_editable(user)
>>> > > +
>>> > >  @staticmethod
>>> > >  def filter_unique_checks(checks):
>>> > >  """Filter the provided checks to generate the unique list."""
>>> > > diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py
>>> > > index 3efe90cd6929..9f5d316d18b5 100644
>>> > > --- a/patchwork/views/__init__.py
>>> > > +++ b/patchwork/views/__init__.py
>>> > > @@ -312,6 +312,14 @@ def process_multiplepatch_form(request, form, action, patches, context):
>>> > >  % patch.name)
>>> > >  continue
>>> > > + field = form.fields.get('state', None)
>>> > > + if field and not field.is_no_change(form.cleaned_data['state']) \
>>> > > + and not patch.can_set_state(request.user):
>>> > 
>>> > style nit: can we wrap this like e.g.
>>> > 
>>> >  if (
>>> >  field and
>>> >  not field.is_no_change(form.cleaned_data['state']) and
>>> >  not patch.can_set_state(request.user)
>>> >  ):
>>> > 
>>> > which is slightly longer vertically but more obvious, IMO.
>>> 
>>> Totally, if flake8 will let me do it I agree that that layout makes more
>>> sense.
>>
>> Yup, flake8 will be more than happy so long as you dedent the closing bracket
>> again.
>>
>>> 
>>> > I surprised that Django doesn't have a way to do field-level validation. I did
>>> > take a quick look and found some extensions for it, but we probably don't want
>>> > to bring in those dependencies for this single use case.
>>> 
>>> Yeah, it's a bit frustrating because of the amount of context we have to
>>> carry around.
>>> 
>>> > > + errors.append(
>>> > > + "You don't have the permissions to set the state of patch '%s'"
>>> > > + % patch.name)
>>> > > + continue
>>> > > +
>>> > >  changed_patches += 1
>>> > >  form.save(patch)
>>> > > diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
>>> > > index 3e6874ae1e5d..72c199135cbb 100644
>>> > > --- a/patchwork/views/patch.py
>>> > > +++ b/patchwork/views/patch.py
>>> > > @@ -101,8 +101,18 @@ def patch_detail(request, project_id, msgid):
>>> > >  elif action is None:
>>> > >  form = PatchForm(data=request.POST, instance=patch)
>>> > >  if form.is_valid():
>>> > > - form.save()
>>> > > - messages.success(request, 'Patch updated')
>>> > > + old_patch = Patch.objects.get(id=patch.id)
>>> > > + if old_patch.state != form.cleaned_data['state'] and \
>>> > > + not old_patch.can_set_state(request.user):
>>> > 
>>> > ditto (wrapping)
>>> > 
>>> > > + messages.error(
>>> > > + request,
>>> > > + "You don't have the permissions to set the state of "
>>> > > + "patch '%s'" % patch.name)
>>> > > + patch = old_patch
>>> > > + form = PatchForm(instance=patch)
>>> > 
>>> > I'm not certain about this, but it seems weird that 'is_valid' is returning True
>>> > yet clearly the request isn't valid. Is there no way to extend the form to do
>>> > this validation instead?
>>> > 
>>> I'll have a look.
>>> 
>>> > > + else:
>>> > > + form.save()
>>> > > + messages.success(request, 'Patch updated')
>>> > >  if request.user.is_authenticated:
>>> > >  context['bundles'] = request.user.bundles.all()
>>
>> Let me know if you want to discuss the State thing in more detail. I have about
>> four different attempts sitting locally, all in various states of
>> incompleteness, so I could probably push these somewhere if you needed a more
>> in-depth view of where my head was at.
>>
>> Stephen
>>
>>> 
>>> Kind regards,
>>> Daniel