filters: re-add the possibility of filtering undelegated patches
Mauro Carvalho Chehab
mchehab at infradead.org
Wed Jun 5 04:21:25 AEST 2019
Em Tue, 04 Jun 2019 12:39:49 +0100
Stephen Finucane <stephen at that.guru> escreveu:
> On Mon, 2019-06-03 at 23:34 -0300, Mauro Carvalho Chehab wrote:
> > Em Tue, 04 Jun 2019 10:10:51 +1000
> > Daniel Axtens <dja at axtens.net> escreveu:
> >
> > > Mauro Carvalho Chehab <mchehab at infradead.org> writes:
> > >
> > > > The filters.py redesign that happened for patchwork 1.1 removed
> > > > a functionality that we use a lot: to filter patches that weren't
> > > > delegated to anyone.
> > > >
> > > > Also, it is a way harder to find someone to delegate with a free
> > > > text input. Use, instead a combo-box just like before.
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab at osg.samsung.com>
> > > >
> > > > ---
> > > >
> > > > LinuxTV was using some pre-version 1.0 patchwork instance since
> > > > last week. Migrating was not easy, due to the delegation patches we
> > > > developed for 1.x, that caused the migration scripts to fail.
> > > >
> > > > On Friday, we finally migrated to the latest stable version:
> > > >
> > > > https://patchwork.linuxtv.org
> > > >
> > > > After the migration, we noticed that one feature that we used a
> > > > lot got removed from patchwork: the filter was not allowing anymore
> > > > to filter not-delegated patches.
> > > >
> > > > On media, we receive a large amount of patches per week. Just after
> > > > the migration, we received ~30 patches, and that's because it is
> > > > a weekend!
> > > >
> > > > In order to handle such high volume, we have several sub-maintainers,
> > > > each one responsible for one part of the subsystem. While we do have
> > > > delegation rules on patchwork, we still do manual delegation. So,
> > > > being able to see what patches are not on someone's queue is very
> > > > important to us.
> > > >
> > > > This patch restores such feature to patchwork. As a plus, it now
> > > > shows a combo-box with just the names of people that maintain
> > > > projects, instead of a free-text input that would otherwise seek
> > > > the entire user's database.
> > >
> > > Right, that seems like a feature we probably should not have killed,
> > > sorry.
> > >
> > > It looks like it went away in 2015:
> > >
> > > commit f439f5414206c94d486c60801a86acc57ad3f273
> > > Author: Stephen Finucane <stephen.finucane at intel.com>
> > > Date: Mon Aug 24 11:05:47 2015 +0100
> > >
> > > Add delegate filter autocomplete support
> > >
> > > It would be helpful to provide autocomplete functionality for the
> > > delegate filter field, similar to that provided for the submitter
> > > field. This will provide a way to handle larger delegate lists without
> > > overloading the user with options.
> > >
> > > Add this functionality through use of Selectize, which is already used
> > > to provide similar functionality for filtering of "submitters".
> > >
> > > Signed-off-by: Stephen Finucane <stephen.finucane at intel.com>
> > >
> > > So perhaps reverting that all the way back might not be the way to go,
> > > but we should have something for your usecase.
> > >
> > > Stephen, thoughts?
> >
> > Maybe the change was done in order to allow delegating patches to
> > anyone:
> >
> > commit e0fd7cd91a5fbe0a0077c46bea870ccd09c8920d (v1.0.0-154-ge0fd7cd)
> > Author: Stephen Finucane <stephen.finucane at intel.com>
> > Date: Mon Aug 24 14:21:43 2015 +0100
> >
> > Allow assigning of any user as delegate
> >
> > With was reverted one year later:
> >
> > commit 198139e4112cf337ffea403000441931b4ddad06 (v1.1.0-227-g198139e)
> > Author: Stephen Finucane <stephenfinucane at hotmail.com>
> > Date: Sun Sep 25 22:37:11 2016 +0100
> >
> > Revert "Allow assigning of any user as delegate"
> >
> > > > def _form(self):
> > > > - return mark_safe('<input type="text" name="delegate" '
> > > > - 'id="delegate_input" class="form-control">')
> > > > + delegates = User.objects.filter(profile__maintainer_projects__isnull = False)
> > >
> > > If we were to go down this route, I think we'd want to be filtering
> > > against the project as well?
>
> Yup, that was exactly why we'd done this. That's still a feature I'd
> like to have but I haven't had time to finish it. I was thinking this
> issue was familiar and that perhaps I'd already fixed it, so I went and
> found this local branch:
>
> https://github.com/stephenfin/patchwork/commits/issues/60
Interesting.
>
> The reason I didn't push that was because I wasn't able to get the
> output in the format I wanted, which was something like this:
>
> No delegate
> ----
> User A
> User B
> ...
>
> So the "no delegate" option would be at the top of the list, followed
> by a separator, then the prepopulated project maintainers. If you
> wanted to assign to other users, you'd be able to search for them (to
> avoid loading all users into a <select>, which is a lot of data). I did
> hack around with Selectize but JS isn't my strong suit so I wasn't able
> to figure this out. If someone was able to figure this out for me, I'd
> personally rather go that way since it's less work when we start
> supporting delegating to all users again. If not though, this patch as
> you've proposed would be perfectly fine until such a time as I get
> around to finishing this.
Well, for the filter, you don't need to load all users into a select.
Just the ones with already have delegated patches are enough, e. g.:
SELECT DISTINCT(p.delegate_id) FROM `patchwork_patch` AS p, patchwork_submission AS s WHERE p.submission_ptr_id = s.id
(or even better, having an "and project_id = $project" at the end)
For a "Delegate to:", indeed the best would be to show an input
box that would look like a "mixed combo-box". On a quick search,
it seems that html5 supports it (I don't do html programming for a
long time, so I never tried to implement it):
https://stackoverflow.com/questions/5650457/html-select-form-with-option-to-enter-custom-value
Does patchwork accept html5? If so, that would be an option to
implement it, perhaps mixing it with with serialize.js.
> I've left review comments below assuming the
> latter.
Well, let's do it by parts. At least for media, not having a filter for
delegation to nobody is a regression. So, I would prefer to apply
the patch I wrote (or a variation to it) upstream, and then work on
a way to allow a maintainer to delegate a patch to a non-maintainer.
I just submitted a patch that should be addressing the review below.
This time, it is applied against the "master" branch (the previous
version was against the latest stable, with is the branch we're
currently using at linuxtv.org).
I also removed the serialize.js-specific code on the version I
just submitted.
>
> > Before f439f5414206c94d486c60801a86acc57ad3f273, it used to check for
> > the project ID, but the code with was calling set_project() at the
> > Filter initialization, but this got removed. I don't know enough about
> > Django to re-add it.
> >
> > > I'm thinking of OzLabs where there are a
> > > couple dozen different projects with completely different sets of
> > > people.
> >
> > Either that or the Django's equivalent to:
> >
> > SELECT DISTINCT(p.delegate_id) FROM `patchwork_patch` AS p, patchwork_submission AS s WHERE p.submission_ptr_id = s.id and project_id = <current project>
> >
> > That would, IMHO, be better, as it will only filter for the ones that
> > actually have patches delegated. As a plus, if we ever re-introduce a
> > feature to allow delegating to anyone, this will still work.
> >
> > We're actually considering to have a way to delegate to driver
> > maintainers (with aren't project maintainers at Django).
> >
> > > Regards,
> > > Daniel
> > >
> > > > +
> > > > + str = '<select name="delegate">'
> > > > +
> > > > + selected = ''
> > > > + if not self.applied:
> > > > + selected = 'selected'
> > > > +
> > > > + str += '<option %s value="">------</option>' % selected
> > > > +
> > > > + selected = ''
> > > > + if self.applied and self.delegate is None:
> > > > + selected = 'selected'
> > > > +
> > > > + str += '<option %s value="%s">%s</option>' % \
> > > > + (selected, self.no_delegate_str, self.no_delegate_str)
>
> No need for the backslash - just bring the opening bracket up a line,
> like:
>
> str += '<option %s value="%s">%s</option>' % (
> selected, self.no_delegate_str, self.no_delegate_str)
>
> > > > +
> > > > + for d in delegates:
>
> Let's use longer variable names.
>
> for delegate in delegates:
>
> > > > + selected = ''
> > > > + if d == self.delegate:
> > > > + selected = ' selected'
> > > > +
> > > > + str += '<option %s value="%s">%s</option>' % (selected,
> > > > + d.id, d.username)
>
> You need to drag 'selected' down to the next line or pep8 will moan.
>
> Is this styled correctly (I don't see any Bootstrap'y classes being
> added)? We should also get a GitHub issue and a release note so we can
> backport this. You should probably use the 'fixes' section.
>
> https://patchwork.readthedocs.io/en/latest/development/contributing/#release-notes
>
> > > > + str += '</select>'
> > > > +
> > > > + return mark_safe(str)
> > > >
> > > > def key(self):
> > > > if self.delegate:
>
> Stephen
>
Thanks for the review! Hopefully, I addressed all points above.
Thanks,
Mauro
More information about the Patchwork
mailing list