Daniel Axtens <d...@axtens.net> writes: > 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. >
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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork