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

Daniel Axtens dja at axtens.net
Fri Aug 6 11:39:41 AEST 2021


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

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


More information about the Patchwork mailing list