filters: re-add the possibility of filtering undelegated patches

Tue Jun 4 21:39:49 AEST 2019

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

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. I've left review comments below assuming the
latter.

> 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