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

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.

Signed-off-by: Daniel Axtens <dja@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

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@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

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

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.

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

> +                else:
> +                    form.save()
> +                    messages.success(request, 'Patch updated')
>  
>      if request.user.is_authenticated:
>          context['bundles'] = request.user.bundles.all()

Stephen Finucane <stephen@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@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 :)

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

> 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()

Kind regards,
Daniel

On Fri, 2021-08-06 at 11:39 +1000, Daniel Axtens wrote:
> Stephen Finucane <stephen@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@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

Stephen Finucane <stephen@that.guru> writes:

> On Fri, 2021-08-06 at 11:39 +1000, Daniel Axtens wrote:
>> Stephen Finucane <stephen@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@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

Daniel Axtens <dja@axtens.net> writes:

> Stephen Finucane <stephen@that.guru> writes:
>
>> On Fri, 2021-08-06 at 11:39 +1000, Daniel Axtens wrote:
>>> Stephen Finucane <stephen@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@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