Stephen Finucane <step...@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 <d...@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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
