Bug#754413: add a description field to keywords

[Christophe Siraut <tobald@debian.org>]

Hi,

Raphael Hertzog wrote:
> On Fri, 17 Jul 2015, Christophe Siraut wrote:
> > Adding a Keyword description field with data works fine, please review
> > the attached patch.
> 
> I'll take a look at that later. But a few comments are below already.
> 
> > The keywords modification form seems broken: here boxes do not shown
> > checked for already active subscription keywords; do you remember it
> > working recently?
> 
> What do you mean? You have no keywords checked at all? Or you only see
> a subset of keywords?

I meant no keywords checked at all.

I found out the javascript is sometimes correctly filling the boxes in
the subscription popup, if you have more than one email registered for
your account; but I could not spot in the code where it is broken.

> I just tried on tracker.debian.org:
> - when I click on "Modify keywords" next to an email, then I get nothing
>   checked
> - when I click on "Modify" next to a source package, then I always get
>   all the default keywords selected, and you're right that it does not
>   match the set of actual keywords (checked with a package where I had
>   only the "vcs" keyword activated, namely "tracker.debian.org" :))
> - when I click on "Modify" in a "Team subscription", there I get the
>   correct set of keywords, but the modify button is still hidden
>   in the expandable part...
> 
> => there's definitely something fishy, but are you sure that the
> regression is not cause by your move of the "Modify" button because
> strangely it seems to work for the team subscription.

Nope it is not caused by the move.
 
> => we should harmonize the button to say "Modify keywords" in all cases

Please pick up 1e231b7 and 3 other cherries.

> => for the keywords at the email level, I'm not sure why it would be empty
> by default. It would seem natural to also have the default keywords
> checked when there's no user-customize keywords at the email level.

I don't really get the point of modifying keywords at the email level,
do we need to keep this feature? Playing with it reveals bugs.

> > I do not get the point of making several requests using javascript in
> > order to compose that form, see accounts/static/js/profile.js Why do not
> > we rely on standard django form facilities?
> 
> It's dating back to quite some time so I don't know. If you want to
> rewrite it in a cleaner way, by all means go ahead!

Yes I will evaluate a rewrite, replacing javascript with django forms.
Keyword model migration can wait until then.
 
> > --- a/distro_tracker/accounts/templates/accounts/subscriptions.html
> > +++ b/distro_tracker/accounts/templates/accounts/subscriptions.html
> > @@ -60,7 +60,7 @@
> >      <div class="default-keywords" style="display: none;" id="default-keywords-{{ forloop.counter }}">
> >          <ul>
> >              {% for keyword in email.default_keywords.all %}
> > -            <li class="keyword">{{ keyword }}</li>
> > +            <li title="{{ keyword.description }}" class="keyword">{{ keyword }}</li>
> >              {% endfor %}
> >          </ul>
> >      </div>
> 
> In the popup I would like the description to be immediately visible, so
> not hidden in a mouse-over title, we have enough width for this.

Yes, this change relates to the accordion thing not the popup. Anyway
the accordion is not useful, it only duplicates information in the
popup, the rewrite should keep only one.

> > @@ -0,0 +1,43 @@
> > +# -*- coding: utf-8 -*-
> > +from __future__ import unicode_literals
> > +
> > +from django.db import migrations
> > +
> > +
> > +def describe_keywords(apps, schema_editor):
> > +    Keyword = apps.get_model('core', 'Keyword')
> > +    Keyword.objects.filter(name='default').update(description=' all the other \
> > +mails (those which aren\'t automatic)')
> 
> Should be rewritten with a data structure and a loop:

Ok

> keywords = {
>     'default': 'All other discussions',
>     [...]
> }
> 
> for keyword, description in keywords.items():
>     Keywords.objects.filter(name=keyword).update(description=description)
> 
> Also please use the descriptions we had here:
> https://packages.qa.debian.org/cgi-bin/pts.cgi?package=dpkg&email=foof@bar&what=advanced

Right.

Thanks,
Christophe