Merge lp:~dholbach/harvest/schema-changes into lp:harvest

[Daniel Holbach]

[James Westby (james-w)]

Hi,

Looks ok to me.

Two comments though:

38 +class Person(models.Model):
39 + lpid = models.TextField(_("Who"), max_length=80)

is that long enough?

70 + if form.cleaned_data["comment"] != opportunity.comment:
71 + models.log_action(request.user.username,
72 + action="changed comment to: '%s'" % \
73 + form.cleaned_data["comment"],
74 + opportunity=opportunity)

that will overflow

    action = models.TextField(_("Action"), max_length=120)

if the comment is long won't it?

You probably want to ellipsise the comment in the action if it is
long.

Thanks,

James

review: Needs Fixing

[Daniel Holbach (dholbach)]

You're right. Dylan also suggested to drop "Person" and just use the regular User or whatever it's called in core-Django.

Thanks for the review and the note about the too long comments.

[Dylan McCall (dylanmccall)]

Adding a separate Person class seems a bit funny to me, since the built in User class (in django.contrib.auth) handles a lot of this for us. Granted, the built in class has that ugly distinction between first and last name, but adding an additional, separate class feels wrong to me. I'm concerned it could corrupt some of the beautiful magic being performed for us behind the scenes.

Besides, we will be working with User already :)

There is a way to store arbitrary extra information on auth's User object as well:
http://docs.djangoproject.com/en/dev/topics/auth/#storing-additional-information-about-users

Here's a chunk of code which uses that:
http://www.arnebrodowski.de/blog/482-Tracking-user-activity-with-Django.html

[Daniel Holbach (dholbach)]

I'm happy to have a UserProfile model related to User once we think it's necessary (connected upload rights, settings(?), …)

[James Westby (james-w)]

Looks good.

Thanks,

James

review: Approve

=== modified file 'harvest/common/launchpad.py'
--- harvest/common/launchpad.py	2010-01-27 12:34:37 +0000
+++ harvest/common/launchpad.py	2010-06-03 11:20:38 +0000
@@ -4,7 +4,6 @@
 
 from django.conf import settings
 
-import sys
 import os
 
 def lp_login(lp_instance=EDGE_SERVICE_ROOT):

=== modified file 'harvest/opportunities/models.py'
--- harvest/opportunities/models.py	2010-03-08 16:32:02 +0000
+++ harvest/opportunities/models.py	2010-06-03 11:20:38 +0000
@@ -1,5 +1,8 @@
 from django.db import models
 from django.utils.translation import ugettext as _
+from django.contrib.auth.models import User
+
+import datetime
 
 PACKAGE_GREEN_THRESHOLD = 5
 PACKAGE_RED_THRESHOLD = 20
@@ -84,6 +87,7 @@
     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)
+    applied = models.BooleanField(_("Applied"), default=False, blank=True)
     sourcepackage = models.ForeignKey(SourcePackage)
     opportunitylist = models.ForeignKey(OpportunityList)
     comment = models.TextField(_("Comment"), blank=True)
@@ -101,3 +105,16 @@
     @models.permalink
     def get_absolute_url(self):
         return ('opportunity_detail', [self.id])
+
+class ActionLogEntry(models.Model):
+    timestamp = models.DateTimeField(_("Timestamp"))
+    who = models.ForeignKey(User)
+    action = models.TextField(_("Action"), max_length=200)
+    opportunity = models.ForeignKey(Opportunity, blank=True)
+
+def log_action(who, action=None, opportunity=None):
+    log_entry = ActionLogEntry(timestamp=datetime.datetime.now(),
+                               who=who, action=action,
+                               opportunity=opportunity)
+    log_entry.save()
+

=== modified file 'harvest/opportunities/views.py'
--- harvest/opportunities/views.py	2010-03-08 16:33:21 +0000
+++ harvest/opportunities/views.py	2010-06-03 11:20:38 +0000
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
 from django.contrib.auth.decorators import login_required
 from django.core.paginator import Paginator, InvalidPage, EmptyPage
 from django.utils.translation import ugettext as _
@@ -54,6 +55,23 @@
     if request.method == "POST":
         form = forms.OpportunityForm(data=request.POST, instance=opportunity)
         if form.is_valid():
+            if form.cleaned_data["reviewed"] != opportunity.reviewed:
+                models.log_action(request.user, action="marked as reviewed", 
+                                  opportunity=opportunity)
+            if form.cleaned_data["applied"] != opportunity.applied:
+                models.log_action(request.user, action="marked as applied", 
+                                  opportunity=opportunity)
+            if form.cleaned_data["experience"] != opportunity.experience:
+                models.log_action(request.user, 
+                                  action="changed experience to: %s" % form.cleaned_data["experience"],
+                                  opportunity=opportunity)
+            if form.cleaned_data["comment"] != opportunity.comment:
+                if len(form.cleaned_data["comment"]) > 178:
+                    action = "changed comment to: '%s'" % (form.cleaned_data["comment"][:177]+u"…")
+                else:
+                    action = "changed comment to: '%s'" % form.cleaned_data["comment"]
+                models.log_action(request.user, action=action,
+                                  opportunity=opportunity)
             form.save()
             return HttpResponseRedirect(request.POST["next"])
         else: