Stephen Finucane <step...@that.guru> writes:
> On Fri, 2021-08-06 at 11:39 +1000, Daniel Axtens wrote:
>> 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 :)
>
> 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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
