Merge lp:~dholbach/harvest/504299 into lp:harvest

[Daniel Holbach]

[Daniel Holbach (dholbach)]

http://people.canonical.com/~dholbach/tmp/Bildschirmfoto.png is what it looks like.

[James Westby (james-w)]

Hi Daniel, this looks great.

326 + explanation = models.TextField(_("Explanatory instructions"), max_length=500, null=True,

is null=True or blank=True the right thing here, I'm never sure?

Thanks,

James

review: Approve

[Daniel Holbach (dholbach)]

Thanks for the review and thanks for your sharp eye. The blank/null mis-match was in there for a long time.

"""
Field.null
If True, Django will store empty values as NULL in the database. Default is False.

Note that empty string values will always get stored as empty strings, not as NULL. Only use null=True for non-string fields such as integers, booleans and dates. For both types of fields, you will also need to set blank=True if you wish to permit empty values in forms, as the null parameter only affects database storage (see blank).

Avoid using null on string-based fields such as CharField and TextField unless you have an excellent reason. If a string-based field has null=True, that means it has two possible values for “no data”: NULL, and the empty string. In most cases, it’s redundant to have two possible values for “no data;” Django convention is to use the empty string, not NULL.

Field.blank
If True, the field is allowed to be blank. Default is False.

Note that this is different than null. null is purely database-related, whereas blank is validation-related. If a field has blank=True, validation on Django’s admin site will allow entry of an empty value. If a field has blank=False, the field will be required.
"""

In this branch I basically just changed the description and help_text of Opportunity.explanation. I'll file a separate bug about the null/blank mistake

[Daniel Holbach (dholbach)]

https://bugs.launchpad.net/harvest/+bug/613439

[Daniel Holbach (dholbach)]

Dylan: can you please comment too? I'm sure there's a much neater way to do it UI-wise. :)

[Dylan McCall (dylanmccall)]

Yay! It looks good, but there are a few things.

For code readability, it would be good to move your changes in filters/filters.py over to opportunities/filters.py instead.
filters/filters.py is generic so far, and it would be nice to keep it that way :)
So, we'd be overriding render_html_value_choice in OppListFilter.

I think I would prefer a really simple symbolic style for that help icon. One that matches the text colour, so it doesn't shout for attention. The rest of the site's design gives any bright colour a very powerful presence. That can wait a little while, though (unless you have something handy!).
The help icon is 14px, but the text size is 12px. I don't think it's causing any trouble for spacing (because it's a floated element and there's 5px extra padding on the bottom of the list item), but if someone is already thinking of a nice monochrome icon, one that looks good at 12px would be cool.
Being a fan of plain text, I tried scratching the image and using just a bold question mark character. I thought it looked rather nice — especially with the very pretty UbuntuBeta font, which I'm sure it can use happily some day. It also gives us a lot of room to play with colours. I tried giving it the colour we use for other links — rgb(110,64,84) — and it seemed okay :)
http://people.ubuntu.com/~dylanmccall/harvest/media/gsoc-week11/opportunitylist-questionmark.png

Speaking of floated elements, we need this…
Below line 204:
#filters > .filtergroup > .filter-value > ul > li
Add:
clear:both;
That will force the list item to always add a line between itself and a floated element. (Very important if the help icon wraps to a new line).

The link for the explanation has the explanation itself as the url right now. Probably the best thing to do is a new view function to get an opportunity set's explanation. (Hm… and other details? Could be fun!). It could just be ridiculously simple for now and we'll make it all fancy later on.

I haven't done it yet, but I think you'll want to use:
from django.core.urlresolvers import reverse
(The reverse function works just like the {% url url_name %} template tag).

Thanks!

review: Needs Fixing

[Daniel Holbach (dholbach)]

Thanks a lot for your comments Dylan. I tried to implement them.

Can you elaborate on the last two abstracts you wrote? I'm not quite sure I understand.

[James Westby (james-w)]

Still looks ok to me.

Thanks,

James

review: Approve

[Dylan McCall (dylanmccall)]

Oh, looking at the admin interface I see that the explanation field has help text saying it's a link. So, I was just using it wrong :)
I had written an explanation itself in that field earlier, so the link didn't work for me.
In that case, Django does have a URLField available we can use:
http://docs.djangoproject.com/en/dev/ref/models/fields/#urlfield

Another option is to store the explanation itself on the object, just like we do with the description. That link on the help icon would lead to something like "/opportunities/help/opplist/bitesize", and that page (doesn't need to be anything fancy — yet) would show the opportunity list's explanation. An advantage there is, at some point, it's trivial to have the explanation as a little popup instead of a whole new page.

[Daniel Holbach (dholbach)]

> Oh, looking at the admin interface I see that the explanation field has help
> text saying it's a link. So, I was just using it wrong :)
> I had written an explanation itself in that field earlier, so the link didn't
> work for me.
> In that case, Django does have a URLField available we can use:
> http://docs.djangoproject.com/en/dev/ref/models/fields/#urlfield

Will do.

> Another option is to store the explanation itself on the object, just like we
> do with the description. That link on the help icon would lead to something
> like "/opportunities/help/opplist/bitesize", and that page (doesn't need to be
> anything fancy — yet) would show the opportunity list's explanation. An
> advantage there is, at some point, it's trivial to have the explanation as a
> little popup instead of a whole new page.

The problem with that is that we have problems getting that data in. Shall Harvest Admins put it into the db through the admin interface? Should it be part of the harvest-data .csv files? We have some docs already for things like merges (https://wiki.ubuntu.com/UbuntuDevelopment/Merging) - it'd be sweet to just link to that info. :-)

[Dylan McCall (dylanmccall)]

Ah, I get it now! In that case I'm quite content with this :)
Thank you!

review: Approve

[Daniel Holbach (dholbach)]

Thanks for the review!

[James Westby (james-w)]

review: Approve

=== modified file 'harvest/common/opportunity_lists.py'
--- harvest/common/opportunity_lists.py	2009-07-09 09:40:47 +0000
+++ harvest/common/opportunity_lists.py	2010-08-05 17:04:49 +0000
@@ -33,14 +33,26 @@
     sock.close()
     return ([map(unicode, [a for a in l]) for l in csv.reader(lines)], datetime_lm)
 
+def unify_list_entry(entry):
+    for e in entry:
+        e = unicode(e.strip())
+    # only contains url and short description
+    if len(entry) == 2:
+        entry.extend([None])
+    return entry
 
 def read_lists(list_dir):
     list_file = os.path.join(list_dir, "opportunities")
     if not os.path.exists(list_file):
         return []
     lines = open(list_file).readlines()
+    
+    # comment out empty entries, or entries that were commented out
     lines = filter(lambda b: b.strip() != "" and not b.startswith("#"), lines)
-    return [map(unicode, [a.strip() for a in l]) for l in csv.reader(lines)]
+    
+    csv_entries = [l for l in csv.reader(lines)]
+    entries = map(unify_list_entry, csv_entries)
+    return entries
 
 
 def pull_lists(data_dir):

=== modified file 'harvest/locale/harvest.pot'
--- harvest/locale/harvest.pot	2010-07-12 09:33:48 +0000
+++ harvest/locale/harvest.pot	2010-08-05 17:04:49 +0000
@@ -8,10 +8,11 @@
 msgstr ""
 "Project-Id-Version: PACKAGE VERSION\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2010-07-12 04:32-0500\n"
+"POT-Creation-Date: 2010-08-04 06:14-0500\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <[e-mail]>\n"
+"Language: \n"
 "MIME-Version: 1.0\n"
 "Content-Type: text/plain; charset=UTF-8\n"
 "Content-Transfer-Encoding: 8bit\n"
@@ -21,15 +22,15 @@
 msgid "Name"
 msgstr ""
 
-#: opportunities/models.py:50 opportunities/models.py:77
+#: opportunities/models.py:50 opportunities/models.py:78
 msgid "URL"
 msgstr ""
 
-#: opportunities/models.py:51 opportunities/models.py:76
+#: opportunities/models.py:51 opportunities/models.py:77
 msgid "Description"
 msgstr ""
 
-#: opportunities/models.py:52 opportunities/models.py:78
+#: opportunities/models.py:52 opportunities/models.py:79
 msgid "Last Updated"
 msgstr ""
 
@@ -46,90 +47,69 @@
 msgstr ""
 
 #: opportunities/models.py:56
-msgid "Explanatory help text"
+msgid "Explanatory instructions"
 msgstr ""
 
 #: opportunities/models.py:57
+msgid "Link to more instructions about this type of opportunities."
+msgstr ""
+
+#: opportunities/models.py:58
 msgid "Commentable"
 msgstr ""
 
-#: opportunities/models.py:58
+#: opportunities/models.py:59
 msgid "Can opportunities on this list have comments?"
 msgstr ""
 
-#: opportunities/models.py:59 opportunities/models.py:86
+#: opportunities/models.py:60 opportunities/models.py:87
 msgid "Required Experience"
 msgstr ""
 
-#: opportunities/models.py:60
+#: opportunities/models.py:61
 msgid "Level of experience required for this type of opportunities."
 msgstr ""
 
-#: opportunities/models.py:79
+#: opportunities/models.py:80
 msgid "Since"
 msgstr ""
 
-#: opportunities/models.py:79
+#: opportunities/models.py:80
 msgid "On the list since"
 msgstr ""
 
-#: opportunities/models.py:80
+#: opportunities/models.py:81
 msgid "Irrelevant"
 msgstr ""
 
-#: opportunities/models.py:81
+#: opportunities/models.py:82
 msgid "Applied"
 msgstr ""
 
-#: opportunities/models.py:84
+#: opportunities/models.py:85
 msgid "Comment"
 msgstr ""
 
-#: opportunities/models.py:85
+#: opportunities/models.py:86
 msgid "Valid"
 msgstr ""
 
-#: opportunities/models.py:87
+#: opportunities/models.py:88
 msgid "Level of experience required for this specific opportunity."
 msgstr ""
 
-#: opportunities/models.py:101
+#: opportunities/models.py:102
 msgid "Timestamp"
 msgstr ""
 
-#: opportunities/models.py:103
+#: opportunities/models.py:104
 msgid "Action"
 msgstr ""
 
-#: opportunities/views.py:81
+#: opportunities/views.py:53
 msgid "Opportunity details could not be saved."
 msgstr ""
 
-#: opportunities/templates/opportunities/opportunities_by_package.html:9
-msgid "Opportunities By Package"
-msgstr ""
-
-#: opportunities/templates/opportunities/opportunities_by_package.html:18
-#: opportunities/templates/opportunities/opportunities_by_type.html:18
-#: templates/opportunities/opportunitylist_list.html:11
-#: templates/opportunities/sourcepackage_list.html:11
-#, python-format
-msgid "(1 opportunity)"
-msgid_plural "(%(counter)s opportunities)"
-msgstr[0] ""
-msgstr[1] ""
-
-#: opportunities/templates/opportunities/opportunities_by_package.html:49
-#: opportunities/templates/opportunities/opportunities_by_type.html:24
-#: templates/opportunities/opportunities_filter.html:45
-#: templates/opportunities/opportunity_index.html:41
-msgid "There are currently no opportunities in Harvest. :("
-msgstr ""
-
-#: opportunities/templates/opportunities/opportunities_by_type.html:9
-msgid "Opportunities By Type"
-msgstr ""
-
 #: templates/404.html:5 templates/404.html.py:8
 msgid "Page Not Found"
 msgstr ""
@@ -146,12 +126,25 @@
 msgid "A server error has occurred."
 msgstr ""
 
-#: templates/base.html:7 templates/base.html.py:82 templates/index.html:9
+#: templates/base.html:7 templates/base.html.py:31 templates/base.html:62
+#: templates/base.html.py:73 templates/index.html:9
 #: templates/admin/base_site.html:7
 msgid "Harvest"
 msgstr ""
 
-#: templates/base.html:73 templates/index.html:22
+#: templates/base.html:62
+msgid "Help"
+msgstr ""
+
+#: templates/base.html:62
+msgid "Bugs"
+msgstr ""
+
+#: templates/base.html:62
+msgid "Code"
+msgstr ""
+
+#: templates/base.html:75 templates/index.html:16
 msgid "Translated by:"
 msgstr ""
 
@@ -159,15 +152,19 @@
 msgid "Harvest Admin"
 msgstr ""
 
-#: templates/opportunities/opportunities_filter.html:4
-#: templates/opportunities/opportunity_index.html:4
+#: templates/opportunities/filter.html:4
 msgid "Opportunity Index"
 msgstr ""
 
-#: templates/opportunities/opportunities_filter.html:9
-#: templates/opportunities/opportunity_index.html:9
-#: templates/opportunities/opportunity_list.html:5
-msgid "Opportunities"
+#: templates/opportunities/filter_results.html:25
+#, python-format
+msgid "%(counter)s package has no matching opportunities"
+msgid_plural "%(counter)s packages have no matching opportunities"
+msgstr[0] ""
+msgstr[1] ""
+
+#: templates/opportunities/filter_results.html:31
+msgid "There are currently no opportunities in Harvest. :("
 msgstr ""
 
 #: templates/opportunities/opportunity_edit.html:10
@@ -178,14 +175,9 @@
 msgid "Update Information Now!"
 msgstr ""
 
-#: templates/opportunities/opportunitylist_list.html:5
-msgid "Opportunity Lists"
-msgstr ""
-
-#: templates/opportunities/packageset_list.html:5
-msgid "Packagesets"
-msgstr ""
-
-#: templates/opportunities/sourcepackage_list.html:5
-msgid "Source Packages"
-msgstr ""
+#: templates/opportunities/package_details.html:10
+#, python-format
+msgid "%(counter)s opportunity hidden"
+msgid_plural "%(counter)s opportunities hidden"
+msgstr[0] ""
+msgstr[1] ""

=== modified file 'harvest/media/css/style.css'
--- harvest/media/css/style.css	2010-07-31 20:22:58 +0000
+++ harvest/media/css/style.css	2010-08-05 17:04:49 +0000
@@ -196,6 +196,12 @@
 }
 #filters > .filtergroup > .filter-value > ul > li {
 	padding-bottom:5px;
+	clear:both;
+}
+
+#filters > .filtergroup > .filter-value > ul > li a.help {
+	font-weight: bold;
+	text-color: rgb(110,64,84);
 }
 
 #filters .editfilter input {

=== modified file 'harvest/opportunities/filters.py'
--- harvest/opportunities/filters.py	2010-07-16 05:21:21 +0000
+++ harvest/opportunities/filters.py	2010-08-05 17:04:49 +0000
@@ -32,6 +32,20 @@
     def process_queryset(self, queryset):
         return queryset.filter(opportunitylist__in = self.get_selected_choices())
 
+    def render_html_value_choice(self, item_id):
+        toggle_params = self.serialize(self.get_value_with_selection(item_id))
+        item_href = self.get_system().get_url_with_parameters(toggle_params)
+        title_attribute = ''
+        help_html = ''
+        if hasattr(self.choices_dict[item_id], 'description') and \
+            self.choices_dict[item_id].description:
+            title_attribute = 'title="%s"' % self.choices_dict[item_id].description
+        if hasattr(self.choices_dict[item_id], 'explanation') and \
+            self.choices_dict[item_id].explanation:
+            help_html = '<a class="help" href="%s">?</a>' % \
+                            self.choices_dict[item_id].explanation
+        return '<a class="item-toggle" href="%s" %s>%s</a> %s' % (item_href, title_attribute, item_id, help_html)
+
 
 #we don't really need to create a special type here, but it may be handy
 class HarvestFilters(containers.FilterSystem):

=== modified file 'harvest/opportunities/management/commands/updatelists.py'
--- harvest/opportunities/management/commands/updatelists.py	2010-03-21 15:32:06 +0000
+++ harvest/opportunities/management/commands/updatelists.py	2010-08-05 17:04:49 +0000
@@ -19,9 +19,13 @@
             os.makedirs(data_dir)
         list_dir = opportunity_lists.pull_lists(data_dir)
 
-        for (list_url, list_description) in opportunity_lists.read_lists(list_dir):
+        for (list_url, 
+             list_description, 
+             list_explanation) in opportunity_lists.read_lists(list_dir):
             op_list, created = OpportunityList.objects.get_or_create(url=list_url)
             op_list.description = list_description
+            if list_explanation:
+                op_list.explanation = list_explanation
             op_list.active = True
             op_list.name = self.chop_name(list_url)
             op_list.save()

=== modified file 'harvest/opportunities/models.py'
--- harvest/opportunities/models.py	2010-07-29 02:57:09 +0000
+++ harvest/opportunities/models.py	2010-08-05 17:04:49 +0000
@@ -47,13 +47,15 @@
 
 class OpportunityList(models.Model):
     name = models.SlugField(_("Name"), max_length=70)
-    url = models.URLField(_("URL"))
+    url = models.URLField(_("URL"), verify_exists=False)
     description = models.TextField(_("Description"), max_length=350)
     last_updated = models.DateTimeField(_("Last Updated"), null=True)
     active = models.BooleanField(_("Active"), default=True)
     featured = models.BooleanField(_("Featured"), default=False, 
                                    help_text=_("Specially feature this list of opportunities?"))
-    explanation = models.TextField(_("Explanatory help text"), max_length=500, null=True)
+    explanation = models.URLField(_("Explanatory instructions"), max_length=500, null=True, 
+                                   help_text=_("Link to more instructions about this type of opportunities."),
+                                   verify_exists=False)
     commentable = models.BooleanField(_("Commentable"), default=True, 
                                       help_text=_("Can opportunities on this list have comments?"))
     experience = models.IntegerField(_("Required Experience"), default=0, choices=EXPERIENCE_CHOICES,
@@ -74,7 +76,7 @@
 
 class Opportunity(models.Model):
     description = models.TextField(_("Description"), max_length=350)
-    url = models.URLField(_("URL"), max_length=350)
+    url = models.URLField(_("URL"), max_length=350, verify_exists=False)
     last_updated = models.DateTimeField(_("Last Updated"), null=True)
     since = models.DateTimeField(_("Since"), help_text=_("On the list since"), null=True)
     reviewed = models.BooleanField(_("Irrelevant"), default=False, blank=True)