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

Tue Aug 31 23:24:20 AEST 2021

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.

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