harvest-dev team mailing list archive

[Dylan McCall <dylanmccall@xxxxxxxxx>]

Dylan McCall has proposed merging lp:~dylanmccall/harvest/opportunityqueryset into lp:harvest.

Requested reviews:
  harvest-dev (harvest-dev)

For more details, see:
https://code.launchpad.net/~dylanmccall/harvest/opportunityqueryset/+merge/59703

I had implemented a rather horrible way of selecting only valid opportunities; a function in opportunities.wrappers which had to be called with a queryset as a parameter. This branch does something a little tidier, with a custom OpportunityManager and OpportunityQuerySet implementing an only_valid method. This way we can grab valid opportunities without writing gigantically long lines of code :)

It also fixes an issue that popped up recently where opportunities with no visible opportunities were still listed in the filter view (because they weren't being removed properly).
-- 
https://code.launchpad.net/~dylanmccall/harvest/opportunityqueryset/+merge/59703
Your team harvest-dev is requested to review the proposed merge of lp:~dylanmccall/harvest/opportunityqueryset into lp:harvest.

=== modified file 'harvest/opportunities/feeds.py'
--- harvest/opportunities/feeds.py	2011-05-02 02:39:20 +0000
+++ harvest/opportunities/feeds.py	2011-05-02 21:04:54 +0000
@@ -5,7 +5,6 @@
 from django.utils.translation import string_concat
 
 import models
-from wrappers import get_valid_opportunities
 
 class _OpportunitiesFeed(Feed):
 	def item_title(self, opp):
@@ -35,7 +34,7 @@
 		return reverse('filter')
 	
 	def items(self):
-		valid_opps = get_valid_opportunities(models.Opportunity.objects.order_by('-last_updated'))[:25]
+		valid_opps = models.Opportunity.objects.only_valid().order_by('-last_updated')[:25]
 		if (valid_opps):
 			return valid_opps
 		else:
@@ -52,7 +51,7 @@
 		return reverse('single_package', args=[pkg.name])
 	
 	def items(self, pkg):
-		return get_valid_opportunities(pkg.opportunity_set.order_by('-last_updated'))
+		return pkg.opportunity_set.only_valid().order_by('-last_updated')
 	
 	def item_title(self, opp):
 		return opp.description

=== modified file 'harvest/opportunities/models.py'
--- harvest/opportunities/models.py	2010-12-08 08:15:50 +0000
+++ harvest/opportunities/models.py	2011-05-02 21:04:54 +0000
@@ -2,6 +2,7 @@
 
 from django.contrib.auth.models import User
 from django.db import models
+from django.db.models import F
 from django.utils.translation import ugettext_lazy as _
 
 PACKAGE_GREEN_THRESHOLD = 5
@@ -55,6 +56,23 @@
         return self.name
 
 
+class OpportunityQuerySet(models.query.QuerySet):
+	def only_valid(self):
+		return self.filter(valid=True,
+                           opportunitylist__active=True,
+                           last_updated=F('opportunitylist__last_updated'))
+
+class OpportunityManager(models.Manager):
+    def get_query_set(self):
+        return OpportunityQuerySet(self.model)
+    
+    def __getattr__(self, attr, *args):
+		#pass getattr through to our custom queryset
+        try:
+            return getattr(self.__class__, attr, *args)
+        except AttributeError:
+            return getattr(self.get_query_set(), attr, *args)
+
 class Opportunity(models.Model):
     description = models.TextField(_("Description"), max_length=350)
     url = models.URLField(_("URL"), max_length=350, verify_exists=False)
@@ -75,6 +93,9 @@
     def __unicode__(self):
         return self.description
     
+    objects = OpportunityManager()
+    objects.use_for_related_fields = True
+    
     @property
     def summary(self):
         summary_list = []
@@ -110,7 +131,7 @@
 
 def log_action(who, action=None, opportunity=None):
     log_entry = ActionLogEntry(timestamp=datetime.datetime.now(),
-                               who=who, action=action,
-                               opportunity=opportunity)
+                            who=who, action=action,
+                            opportunity=opportunity)
     log_entry.save()
 

=== modified file 'harvest/opportunities/views.py'
--- harvest/opportunities/views.py	2011-02-22 04:13:14 +0000
+++ harvest/opportunities/views.py	2011-05-02 21:04:54 +0000
@@ -22,7 +22,7 @@
     sourcepackages_list = filters_pkg.process_queryset(sourcepackages_list)
     
     #opportunities_list is filtered right away to only check opportunities belonging to selected packages
-    opportunities_list = models.Opportunity.objects.all()
+    opportunities_list = models.Opportunity.objects.only_valid()
     opportunities_list = opportunities_list.filter(sourcepackage__in=sourcepackages_list)
     opportunities_list = filters_opp.process_queryset(opportunities_list)
     
@@ -60,7 +60,7 @@
 def single_package(request, package_name):
     package = get_object_or_404(models.SourcePackage, name=package_name)
     
-    opportunities_list = package.opportunity_set
+    opportunities_list = package.opportunity_set.only_valid()
     package_wrapper = PackageWrapper(request, package, visible_opportunities = opportunities_list)
     
     context = {
@@ -74,7 +74,7 @@
 
 def json_package_summary(request, package_name):
     package = get_object_or_404(models.SourcePackage, name=package_name)
-    opportunities_list = package.opportunity_set
+    opportunities_list = package.opportunity_set.only_valid()
     data = {}
     for opportunity in opportunities_list:
         if not data.has_key(opportunity.opportunitylist.name):
@@ -170,8 +170,8 @@
     
     package = get_object_or_404(models.SourcePackage, id=package_id)
     
-    opportunities_list = package.opportunity_set
-    opportunities_list = filters.find('opp').process_queryset(opportunities_list).all()
+    opportunities_list = package.opportunity_set.only_valid()
+    opportunities_list = filters.find('opp').process_queryset(opportunities_list)
     
     package_wrapper = PackageWrapper(request, package, visible_opportunities = opportunities_list, expanded = True)
     

=== modified file 'harvest/opportunities/wrappers.py'
--- harvest/opportunities/wrappers.py	2011-02-22 03:25:18 +0000
+++ harvest/opportunities/wrappers.py	2011-05-02 21:04:54 +0000
@@ -1,13 +1,5 @@
-from django.db.models import F
 from harvest.common.url_tools import update_url_with_parameters
 
-# Returns all valid opportunities in the given QuerySet
-def get_valid_opportunities(opportunities_list):
-    if opportunities_list:
-        return opportunities_list.filter(valid=True,
-                                         opportunitylist__active=True,
-                                         last_updated=F('opportunitylist__last_updated'))
-
 class PackageWrapper(object):
     """
     Describes a visible source package, for specific use in a
@@ -40,7 +32,7 @@
         #order_by should really happen in the template, but the template
         #language's dictsort is unstable pending a fix to Django bug
         #11008: <http://code.djangoproject.com/ticket/11008>
-        opps = get_valid_opportunities(self.visible_opportunities)
+        opps = self.visible_opportunities
         return opps.order_by('-since', 'opportunitylist', 'experience')
     
     def get_hidden_opportunities(self):
@@ -48,9 +40,9 @@
         Returns opportunities that belong to the given package but have
         been hidden from view
         """
-        visible_opps = self.get_visible_opportunities()
-        all_opportunities = self.package.opportunity_set.exclude(id__in=visible_opps)
-        return get_valid_opportunities(all_opportunities)
+        visible_opps = self.visible_opportunities
+        all_opportunities = self.package.opportunity_set.only_valid().exclude(id__in=visible_opps)
+        return all_opportunities
 
 class PackageListWrapper(object):
     """