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

Thu Aug 12 20:32:43 AEST 2021

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.

> 
> > 
> > > + ]
> > > 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