[PATCH 1/3] Allow a project to restrict submitter state changes
Stephen Finucane
stephen at that.guru
Wed Aug 4 01:15:16 AEST 2021
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
> + ]
> 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()
More information about the Patchwork
mailing list