Merge lp:~dylanmccall/harvest/harvest-dylan-m into lp:harvest

[Dylan McCall]

Introducing a new view at /opportunities/filter, which aims to replace all existing views in the future.

This view presents all packages and all opportunities tracked by Harvest, where the user can filter them using a list of properties. First the list of source packages is filtered, then the list of opportunities related to those packages is filtered.

User interface is just a proof of concept so far, with very few avenues for interaction (beyond toggling some filters in a rudimentary way). The back-end lets us quickly define new properties to filter packages and opportunities by, and these are instantly reflected in the output.

Everything happens through static pages at the moment, with parameters passed in the query string. Any interaction that involves clicking a link, including expanding a package to show its related opportunities, is a full page load away.

[Dylan McCall (dylanmccall)]

Oh, I forgot. This also adds a dependency on debug-toolbar. That change can be reverted (it's just in settings.py and INSTALL), but it also doesn't hurt. It's very useful for tracking how changes affect performance.

Note that the middleware adds a whole ton of data to the page as it is sent (when enabled), so it does have a performance hit of its own. It only turns on if it's being accessed through 127.0.0.1.

[Daniel Holbach (dholbach)]

> Oh, I forgot. This also adds a dependency on debug-toolbar. That change can be
> reverted (it's just in settings.py and INSTALL), but it also doesn't hurt.
> It's very useful for tracking how changes affect performance.
>
> Note that the middleware adds a whole ton of data to the page as it is sent
> (when enabled), so it does have a performance hit of its own. It only turns on
> if it's being accessed through 127.0.0.1.

Maybe we can ask people to add that stuff to local_settings.py or we require YES_I_AM_HARVEST_HACKER to be True? We could also test if debug_toolbar can be imported.

About the rest of the new code: HOLY COW! You put quite a bit of work into this! :)

FilterContainer:
~~~~~~~~~~~~~~~~
minor only, but the description of set_filters() says "Add a set of filters to be children of this one." although self.filters_dict is reset at the beginning. Would an add_*() function be useful or should the description just be modified a bit?

I think I'd rename get() to find(), not sure which is more common. Both work for me. :-)

113 + #the first bit is the one this instance stores
114 + if name_bits[0] in self.filters_dict:
115 + result = self.filters_dict[name_bits[0]]
116 + else:
117 + result = None

Lines 116 and 117 can be removed.

119 + if isinstance(result, FilterContainer) and len(name_bits)>1:
120 + return result.get(name_bits[1]) #send remaining bits to child
121 + else:
122 + #we have reached the end of the list. Return the result (which may be None)
123 + return result

121 can be removed and 123 unidented one level.

FilterSystem:
~~~~~~~~~~~~~

Does every call to get_parameters() produce a copy? Do we need a get()ter there?

Filter:
~~~~~~~

Do we need explicit getters and setters() for the very simple members of Filter?

Do you think it's useful to add a few very quick examples to some of descriptions? ie: to me it wasn't immediately obvious what the "toggling" is about.

[Daniel Holbach (dholbach)]

(minor) Maybe FilterSystem.update_http() could be called FilterSystem.update_from_http() to make clear what is updated based on which data?

Can FilterSystem and FilterContainer be merged? Are they used separately and in different ways? I must admit FilterSystem is not quite clear to me yet. Still digging through it. :)

[Daniel Holbach (dholbach)]

Filter: I'm not sure what get_value() and get_value_string*() and get_parameters_for_value() are for and where the differences lie.

[Daniel Holbach (dholbach)]

Can we get PackageListWrapper.*visible_packages* and PackageListWrapper.*hidden_packages* into opportunity/views.py:opportunities_filter()? We could just pass hidden_packages and visible_packages instead of packages_list. Similarly we could do something for PackageWrapper. Also could we probably do some of the expand logic in javascript. What do you think?

[James Westby (james-w)]

Wow!

Thanks Dylan.

31 + url_params = list()

seems to be superfluous.

Would there ever be a desire for

36 +def current_url_with_parameters(request, new_params_dict):

to remove parameters? That can be something that is added later if needed
though.

46 \ No newline at end of file
453 \ No newline at end of file
673 \ No newline at end of file

Fixing those would be nice.

149 + #note that this currently stores parameters that aren't relevant to the FilterSystem
150 + #we need to do that so we don't eat them in get_url_with_parameters
151 + self.set_parameters(request.GET)

is that still needed with the code to get the original params in get_url_with_parameters?

Where you mark a method as abstract and don't implement in then consider

    raise NotImplementedError(self.<abstract_method>)

which will give an error if the subclass doesn't implement it, instead of
silently doing nothing.

244 + @param choices_dict: Optional value to be used instead of internal value

that's not a parameter to that method, same in the next method.

Where you use mark_safe you should add a comment stating why that string is
known to be safe.

Do the default parameters make sense? I'm not sure we should be defaulting to
"ged" at least.

Overall this is some seriously great code, nice work!

Thanks,

James

review: Approve

[Dylan McCall (dylanmccall)]

Yay! Thanks for the comments. I'll respond to them here.

There is a bit of superfluous initialization (by Python standards) going
on, indeed. Thanks for pointing them out! I was still in Vala mode when
I started this, desperately clinging to the comfort of strong typing ;)

So, in order from the top!

---FilterContainer---

        FilterContainer is currently used by two classes: FilterGroup
        under filters/filters.py, and FilterSystem under
        filters/containers.py. It is a bit wacky at the moment.
        FilterSystem is the one single root container that all the
        filters go inside of. It should be The object that the rest of
        the application uses to manipulate filters. The current HTTP
        request is handed to that object and it sorts out the rest.
        Thankfully, that means the thing can be poked at repeatedly
        until the wackiness subsides, without affecting the rest of the
        code.
        FilterGroup is used to group filters that work with a specific
        collection of objects (there's a pkg and opp group at the
        moment). It is possible to enable and disable filters inside a
        FilterGroup.

        set_filters() probably made sense the way it was at some point
        in the past. It will still only ever be called once, but I
        needn't enforce that; it just adds complexity for no particular
        reason. I renamed set_filters to add_filters and I'm
        initializing self.filters_dict under __init__. All these
        functions starting with set_ and get_ were making my head hurt,
        anyway.

        get_parameters(), as it turns out, was just redundant and isn't
        used anywhere any more. I'll strip that out. Similar
        functionality happens behind the scenes in url_tools.py, with
        current_url_with_parameters(). Come to think of it, there's _a
        lot_ of copying going on for one request. It can probably be
        sped up somewhere.

        FilterContainer.get() does imply a similarity to the get()
        method of many other types, even though it is definitely not
        like those. Good point. Changed it to find()!

        Daniel, you're awesome at naming functions! :)

        Thanks for spotting the leftover set_parameters stuff, James.
        It's all gone now! (*phew*)

---Filters---

        The default parameters are just for testing purposes, and indeed
        make no sense. I'm using gedit as a consistent query to test
        against, so when it looks exactly like the mockup I'll know! (It
        also needs some kind of weird default, because it doesn't handle
        big lists of packages very gracefully).
        pkg:name is set as such because there is no interface to change
        it yet, except editing the URL. Normally, of course, it would be
        blank.
        (Unfortunately, said interface won't be pretty. Solving it with
        Javascript is easy, but doing it without needs a pretty
        convoluted HTML form. I may just do the Javascript solution for
        now).

        get_value(), get_value_s...

[lp:~dylanmccall/harvest/harvest-dylan-m]

201. By Dylan McCall on 2010-06-24

Cleanup, as discussed at <https://code.edge.launchpad.net/~dylanmccall/harvest/harvest-dylan-m/+merge/28262>

Added newlines at end of files, where they were missing.

harvest/filters:
Finished docstrings
Removing "#abstract" comment where it makes no sense

harvest/filters/containers.py:
Renamed some methods for clarity

harvest/filters/filters.py:
raising exception for methods that need to be implemented
Rejigged get_values, get_value_string and get_parameters_for_value. Now a filter can be serialized and it's all a little bit simpler

202. By Dylan McCall on 2010-06-27

Fixed a bug where serialize_value, given an empty set for value, used the current value instead.

[Daniel Holbach (dholbach)]

Thanks a lot for your continued work on this and sorry for not replying earlier. I've been quite busy with other things. Sorry.

> The setters and getters found themselves in such quantity
> because the implementation of these things can fluctuate. A lot
> of that fluctuation has been reduced with the version you're
> seeing here, though. The other reason is I find it elegant to
> have the outside world only access an object through methods;
> never through properties. (That way the rules for accessing a
> given property are self-documenting). If that's silly, let me
> know!

In cases where setters and getters just do the minimal amount of work ("self.bla = bla" and "return bla"), I'd expect that accessing the object's property would just work. If additional work needs to be done, I'd try to do stuff in the constructor or in a function that does some kind of computation or other work.

I don't know if that's applicable here.

> The reason I have visible_packages and hidden_packages in
> wrappers.py is that there will be a bunch of other stuff there
> in the near future. For example, stuff that summarizes the two
> collections of packages. Some of this demands processing that
> may or may not happen, so it makes sense to call the appropriate
> functions from the template as appropriate (I think…).

Can you maybe explain what kind of summary you'd like to see there?

I personally feel it's a bit more work to get it right, but it might be beneficial to tune the queries in views.py right, which would also save us from loading too many objects into memory.

> PackageWrapper is similar; it has little at the moment, but in
> the future could be used to access what categories of
> opportunities lie within, including hidden ones. Throwing that
> logic in the template (even with template tags) feels like a
> horrible act, and probably wouldn't be as efficient.
> Having said that, PackageWrapper feels a bit more wrong because
> there is already a perfectly good SourcePackage object we can
> add data to. I just feel squeamish throwing that stuff directly
> at a SourcePackage model instance. I know it won't save it to
> the database or anything, but it feels wrong somehow.
>
> I kept an eye on performance when I put that together. It does
> grab the entire list of source packages from the database and
> turn them into new Python objects, but in the end we're just
> doing the same database hit that would happen later. Debug
> Toolbar says we hit the database once for each model; nothing
> blatantly redundant is happening. Always room for better
> performance, though!

I was under the impression that we wouldn't have to have all opportunities and source packages in memory if the query just asked for a specific subset. Maybe I'm wrong. I'll go and find out.

> Okay, that's a lot of writing, but this has been really helpful to get
> my thoughts straightened. It feels a lot smoother than it did this
> morning!

Y...

[Dylan McCall (dylanmccall)]

I did some pondering and poking, and I'm further convinced to change my use of accessors. Turns out the most Pythonic way is to use plain instance variables and implement property() as appropriate, which lets us specify our own getters and setters for those variables (or just a getter). That Java course nearly corrupted me!

There is a bit of divergence in filters.py in the gsoc-client-stuff branch, so I'm a little reluctant to make that change in this branch. (Merge conflicts — even simple ones — invariably give me headaches). Are you okay if I do it in a new branch from gsoc-client-stuff?

> I was under the impression that we wouldn't have to have all
> opportunities and source packages in memory if the query just asked
> for a specific subset. Maybe I'm wrong. I'll go and find out.

I mean to say that all the source packages which have been met by the package filtergroup are examined in a similar way. It isn't an extensive thing, but we end up accessing the queryset, which gives us a list of all the references it has found, both hidden and visible packages. (The opportunities remain a queryset until the template asks for them). I should move that to be smarter about hidden packages, only storing their count via a query.

With gsoc-client-stuff, I'm working on asynchronously loading results, package info and hidden packages. This means implementing new views on the same query data as before. So, that's why I am a little picky about having one object to manage all that; all these views can just talk to the same thing in a somewhat balanced way. Having said that, it's mostly a stop-gap and if all goes well it should be really easy to yank it out for something quicker and more cool once the dust is settled. To be honest I'm not sure what the final result is going to look like in views.py, so I am reluctant to devote too much to a specific approach yet ;)

[Daniel Holbach (dholbach)]

> That Java course nearly
> corrupted me!

BIG HUGS! I'm sure you'll survive, though! :-)

> There is a bit of divergence in filters.py in the gsoc-client-stuff branch, so
> I'm a little reluctant to make that change in this branch. (Merge conflicts —
> even simple ones — invariably give me headaches). Are you okay if I do it in a
> new branch from gsoc-client-stuff?

Sure. I'm also happy for small branches and small merge proposals to go in as it should be much quicker, easier to review, etc.

> > I was under the impression that we wouldn't have to have all
> > opportunities and source packages in memory if the query just asked
> > for a specific subset. Maybe I'm wrong. I'll go and find out.
>
> I mean to say that all the source packages which have been met by the package
> filtergroup are examined in a similar way. It isn't an extensive thing, but we
> end up accessing the queryset, which gives us a list of all the references it
> has found, both hidden and visible packages. (The opportunities remain a
> queryset until the template asks for them). I should move that to be smarter
> about hidden packages, only storing their count via a query.

As I told you in a separate discussion my main concern was merely the complexity of new classes we add. If we (and particularly new contributors) try to fix upcoming bugs, it'll be harder and harder to dive through various classes which sometimes have very similar names to figure out where something is done.

Having said that, we can surely merge this for now and simplify over time and as we get a better idea of what we want to get done.

> With gsoc-client-stuff, I'm working on asynchronously loading results, package
> info and hidden packages. This means implementing new views on the same query
> data as before. So, that's why I am a little picky about having one object to
> manage all that; all these views can just talk to the same thing in a somewhat
> balanced way. Having said that, it's mostly a stop-gap and if all goes well it
> should be really easy to yank it out for something quicker and more cool once
> the dust is settled. To be honest I'm not sure what the final result is going
> to look like in views.py, so I am reluctant to devote too much to a specific
> approach yet ;)

Gotcha. Thanks for your work on this and consideration.

review: Approve

=== modified file 'INSTALL'
--- INSTALL	2010-03-02 10:42:19 +0000
+++ INSTALL	2010-06-27 21:07:25 +0000
@@ -1,4 +1,4 @@
-1. sudo apt-get install python-django python-launchpadlib python-django-openid-auth bzr
+1. sudo apt-get install python-django python-launchpadlib python-django-openid-auth bzr python-django-debug-toolbar
 
 ---
 Optional for postgres usage:

=== added file 'harvest/common/url_tools.py'
--- harvest/common/url_tools.py	1970-01-01 00:00:00 +0000
+++ harvest/common/url_tools.py	2010-06-27 21:07:25 +0000
@@ -0,0 +1,32 @@
+def new_url_with_parameters(url, params_dict):
+    """
+    Returns a new URL with an added query string, described by params_dict.
+    @param params_dict: a dictionary with all the parameters to add
+    @param path: url to add the parameters to
+    @return: the url (a string) with given parameters 
+    """
+    #Derived from <http://djangosnippets.org/snippets/1627/>
+    
+    def param_bit(key, value):
+        if value:
+            return "%s=%s" % (key, value)
+        else:
+            return "%s" % key
+    
+    if len(params_dict):
+        url_params = list()
+        url += "?%s" % "&".join([param_bit(key, value) for (key, value) in params_dict.items()])
+    
+    return url
+
+def current_url_with_parameters(request, new_params_dict):
+    """
+    Returns the current URL with some parameters changed, which are
+    described in new_params_dict. The rest of the query string remains
+    intact.
+    """
+    params = request.GET.copy() #this includes parameters that aren't used by the FilterSystem
+    params.update(new_params_dict)
+    url = request.path
+    return new_url_with_parameters(url, params)
+

=== added directory 'harvest/filters'
=== added file 'harvest/filters/__init__.py'
--- harvest/filters/__init__.py	1970-01-01 00:00:00 +0000
+++ harvest/filters/__init__.py	2010-06-27 21:07:25 +0000
@@ -0,0 +1,23 @@
+"""
+Here, we define a very abstract filtering system.
+This system is entirely decoupled from presentation. A filter is simply a query
+operation intended for a particular type of model, which can be configured and
+turned on / off via structured input.
+
+There are, of course, some rules: different types of filters can be added,
+some of which store extra information (like a line of text that the user can
+input), and filters can be grouped like radio buttons where only one filter
+in a group can be selected.
+
+The included classes provide most important features for free, but to be of
+any use they need to be extended to fit the intended application. For example,
+the get_query class is unimplemented; it should return a Q object that does
+the work of selecting whatever the filter is intended to select.
+
+Groups of filters in no way refer to how they get placed in a page, or how
+they are interacted with. We leave that entirely up to templates. It is also
+something else's job to figure out how to draw filters, and how to form the
+links they talk to us with.
+
+All This Does is filter things.
+"""

=== added file 'harvest/filters/containers.py'
--- harvest/filters/containers.py	1970-01-01 00:00:00 +0000
+++ harvest/filters/containers.py	2010-06-27 21:07:25 +0000
@@ -0,0 +1,97 @@
+from harvest.common.url_tools import current_url_with_parameters
+
+class FilterContainer(object): #abstract
+    """
+    A class that contains Filter objects, which can be retrieved with the
+    find method.
+    
+    The added Filter objects are referred to as "children." They are expected
+    to exist for the entire life of the container.
+    """
+    
+    def __init__(self, filters_set): 
+        self.filters_dict = dict() #refers to Filter objects by their unique IDs
+        self.add_filters(filters_set)
+    
+    def add_filters(self, filters_set): #final
+        """
+        Adds a set of filters to be children of this one and informs
+        each child that it belongs to this container.
+        @param filter_set: a set of Filter objects
+        """
+        for child in set(filters_set):
+            self.filters_dict[child.get_id()] = child
+            child.set_container(self)
+    
+    def find(self, full_name): #final
+        """
+        Finds a filter inside this object or one of its children, based
+        on that filter's full name in the format container:child.
+        @param full_name: an object's full name
+        @return: the object described by full_name, or None
+        """
+        result = None
+        name_bits = full_name.split(':',1)
+        
+        #the first bit is the one this instance stores
+        if name_bits[0] in self.filters_dict:
+            result = self.filters_dict[name_bits[0]]
+        
+        if isinstance(result, FilterContainer) and len(name_bits)>1:
+            result = result.find(name_bits[1]) #send remaining bits to child
+        
+        #we have reached the end of the list. Return the result (which may be None)
+        return result
+
+
+
+class FilterSystem(FilterContainer):
+    """
+    This is the single all-knowing root object that should contain all
+    other filters for an application. From this object it is possible to
+    find a filter using its full name, which is the same name used to
+    serialize a filter's state in an HTTP query string.
+    
+    Before an instance of this object can be safely used,
+    update_from_http should be called with the current HttpRequest.
+    """
+    
+    def __init__(self, filters_set, default_parameters = dict()):
+        FilterContainer.__init__(self, filters_set)
+        self.request = None #current http request
+        self.default_parameters = default_parameters
+    
+    def update_from_http(self, request): #final
+        """
+        Call this method to update the state of all filters based on an
+        HttpRequest object. The request object will be stored for other uses.
+        @param request: current HttpRequest object
+        """
+        self.request = request
+        #this contains parameters that aren't relevant to the FilterSystem
+        self._set_parameters(request.GET)
+    
+    def _set_parameters(self, parameters): #final
+        """
+        Updates the state of all filters based on given parameters.
+        @param parameters: dictionary of parameters, for example from request.GET
+        """
+        new_params = parameters.copy()
+        for key in self.default_parameters:
+            #apply default parameters for keys that have not been set
+            if not key in new_params: new_params[key] = self.default_parameters[key]
+        
+        for key in new_params:
+            filter_object = self.find(key)
+            if filter_object:
+                filter_object.set_value(new_params[key])
+    
+    def get_url_with_parameters(self, parameters): #final
+        """
+        Returns a new URL where the given parameters will be applied.
+        To generate parameters, see Filter.serialize.
+        @param parameters: a dictionary of new parameters
+        @return: the current url with the given parameters added to the query string
+        """
+        return current_url_with_parameters(self.request, parameters)
+

=== added file 'harvest/filters/filters.py'
--- harvest/filters/filters.py	1970-01-01 00:00:00 +0000
+++ harvest/filters/filters.py	2010-06-27 21:07:25 +0000
@@ -0,0 +1,318 @@
+#FIXME: Make ChoiceFilter a bit nicer so we don't need to call super() and all that bother from harvest.opportunities.filters.
+#TODO: adjust Filter.render() methods for custom template tags, with django.template.Template
+
+from containers import FilterContainer, FilterSystem
+from django.utils.safestring import mark_safe
+from copy import copy
+
+class Filter(object): #abstract, extend in application
+    """
+    The abstract base class for all other filters.
+    
+    Every Filter's main objective is to process QuerySets with the method
+    Filter.process_queryset. Filter, or one of its subclasses, should be
+    extended with a new version of process_queryset that does something useful
+    for a specific application. For example, process_queryset could return
+    queryset.filter(foo="bar") so using the filter will limit the queryset
+    to objects where foo=bar.
+    
+    Every Filter has a unique id and can belong to a single container.
+    
+    A Filter can be assigned a new value from a string and its internal value
+    (which could be of any type) can be serialized back to a string.
+    The object itself can be serialized as a key / value pair, where the key
+    is completely unique.
+    
+    Every Filter can be rendered, which outputs markup (currently html)
+    describing it for a user interface.
+    """
+    
+    #TODO: figure out get_system and get_full_name when set_container is called and store the value
+    
+    def __init__(self, id_str): #final
+        self.id_str = id_str #local name
+        self.container = None #immediate container (FilterContainer)
+    
+    def get_id(self): #final
+        """
+        @return: the local id of this filter, unique to its container.
+        """
+        return self.id_str
+    
+    def set_container(self, container): #final
+        """
+        Specify that this filter belongs to a specific container. This will
+        replace any container it currently believes it belongs to.
+        @param container: a FilterContainer object that holds this Filter
+        """
+        #it would make sense to raise an exception if self.container != None
+        self.container = container
+    
+    def get_container(self): #final
+        """
+        @return: the container that this filter belongs to
+        """
+        return self.container
+    
+    def get_system(self): #final
+        """
+        @return: the FilterSystem that this filter ultimately belongs to
+        """
+        container = self.get_container()
+        system = None
+        if isinstance(container, Filter): 
+            system = container.get_system()
+        elif isinstance(container, FilterSystem):
+            system = container
+        return system
+    
+    def get_full_name(self): #final
+        """
+        Returns the filter's full name, which should make sense from anywhere
+        in the application. This name is in the format parent:child:child.
+        @return: the filter's full name, which is a string
+        """
+        full_name = self.get_id()
+        container = self.get_container()
+        if isinstance(container, Filter):
+            full_name = "%s:%s" % (container.get_full_name(), full_name)
+        return full_name
+    
+    def set_value(self, value): #abstract
+        """
+        Extend this to take a value passed down from the top, probably
+        from FilterSystem.update_from_http, and do something with it.
+        @param value: a new value, as a string
+        """
+        raise NotImplementedError(self.set_value)
+    
+    def get_value(self): #abstract
+        """
+        @return: a copy of this filter's value in its native format
+        """
+        raise NotImplementedError(self.get_value)
+    
+    def serialize_value(self, value): #abstract
+        """
+        The inverse of set_value. Returns the given value as a string that could
+        be added to an HTTP query string.
+        @param value: the value to serialize, in a format native to this Filter (see get_value)
+        @return: a unicode string formatted for set_value
+        """
+        raise NotImplementedError(self.serialize_value)
+    
+    def serialize(self, value = None): #final
+        """
+        Creates a dictionary of parameters to describe this object, either
+        as-is or with a new value.
+        The result can be sent to FilterSystem.get_url_with_parameters.
+        @param value: a different value to use, in a format native to this Filter (see get_value)
+        @return: a dictionary of key:value pairs referring to the object and its value.
+        """
+        if value == None: value = self.get_value()
+        key = self.get_full_name()
+        value_str = self.serialize_value(value)
+        return {key : value_str}
+    
+    def get_container_toggle_parameters(self): #final
+        """
+        Helper method to get the parameter for toggling this filter's state in
+        its container, if there is one.
+        @return: a dictionary of key:value pairs to generate new GET parameters
+        """
+        container = self.get_container()
+        params = dict()
+        if isinstance(container, FilterGroup):
+            params = container.serialize(container.get_value_with_selection(self.get_id()))
+        return params
+    
+    def process_queryset(self, queryset): #abstract
+        """
+        Extend this to manipulate a given queryset and then return it.
+        For example, queryset.filter(name__startswith = self.value)
+        @param queryset: a queryset to operate on
+        @return: a queryset based on the given one
+        """
+        raise NotImplementedError(self.process_queryset)
+    
+    def render(self): #final
+        """
+        @return: the default rendering of the filter itself in given context
+        """
+        return self.render_html()
+    
+    def render_html(self):
+        """
+        Extend this to return the html output for the filter itself.
+        The output should be very simple and semantically meaningful,
+        with no specific concern about formatting. It will be
+        placed within other  tags that describe its context, and it is
+        up to the template to describe style.
+        @return: a unicode string containing html representing this filter
+        """
+        system = self.get_system()
+        toggle_params = self.get_container_toggle_parameters()
+        href = system.get_url_with_parameters(toggle_params)
+        
+        return mark_safe(u'<a href="%s">(%s)</a>'
+            % (href, self.get_id()))
+
+
+
+
+class EditFilter(Filter): #abstract, extend in application
+    """
+    This Filter has a simple string value which can be edited by the user.
+    
+    Serialized as stored ("value")
+    """
+    
+    def __init__(self, id_str):
+        Filter.__init__(self, id_str)
+        self.input_str = ""
+    
+    def set_value(self, value): #overrides Filter
+        self.input_str = value
+    
+    def get_value(self): #overrides Filter
+        return self.input_str
+    
+    def serialize_value(self, value): #overrides Filter
+        return value
+    
+    def render_html(self):
+        system = self.get_system()
+        toggle_params = self.get_container_toggle_parameters()
+        href = system.get_url_with_parameters(toggle_params)
+        
+        return mark_safe(u'<a href="%s">%s: %s</a>'
+            % (href, self.get_id(), self.get_value()))
+
+
+class SetFilter(Filter): #abstract, extend in application
+    """
+    Holds a set of strings, with no repetition.
+    
+    Serialized as a comma-separated list ("dog,cat,horse,mouse")
+    """
+    
+    def __init__(self, id_str):
+        Filter.__init__(self, id_str)
+        self.selected_set = set()
+    
+    def set_value(self, value): #overrides Filter
+        self.selected_set = set([s for s in value.split(",") if self.id_allowed(s)])
+    
+    def get_value(self): #overrides Filter
+        return self.selected_set.copy()
+    
+    def serialize_value(self, value): #overrides Filter
+        return ",".join(value)
+    
+    def get_value_with_selection(self, item_id): #final
+        """
+        Returns the current value of this object with the selection referred to
+        by item_id toggled on or off, depending on its current state.
+        @param item_id: id for the item to toggle
+        @return: the value of this SetFilter with the given item toggled on or off
+        """
+        select = self.get_value()
+        if item_id in select:
+            select.remove(item_id)
+        else:
+            select.add(item_id)
+        return select
+    
+    def id_selected(self, item_id):
+        return item_id in self.selected_set
+    
+    def id_allowed(self, item_id):
+        return True
+        
+
+class ChoiceFilter(SetFilter): #abstract, extend in application
+    """
+    Has a dictionary of items, with names and values of any type. These can be
+    selected or deselected by the input, which is a set of strings, as in
+    SetFilter. In that set, any items which refer to choices that do not exist
+    are ignored.
+    
+    Serialized as a comma-separated list, like SetFilter.
+    """
+    
+    def __init__(self, id_str, choices_dict):
+        SetFilter.__init__(self, id_str)
+        self.choices_dict = choices_dict
+    
+    def id_allowed(self, item_id): #overrides SetFilter
+        return item_id in self.choices_dict
+    
+    def get_selected_items(self):
+        return [self.choices_dict[s] for s in self.selected_set]
+        
+    def render_html(self): #overrides Filter
+        choices = ""
+        
+        for c in self.choices_dict:
+            c_render = self._render_html_choice(c)
+            if self.id_selected(c):
+                c_render = "<b>%s</b>" % c_render
+            choices += "<li>%s</li>" % c_render
+        
+        system = self.get_system()
+        toggle_params = self.get_container_toggle_parameters() 
+        self_href = system.get_url_with_parameters(toggle_params)
+        
+        return mark_safe(u'<a href="%s">%s</a>:<ul>%s</ul>' % (self_href, self.get_id(), choices))
+    
+    def _render_html_choice(self, item_id):
+        system = self.get_system()
+        toggle_params = self.serialize(self.get_value_with_selection(item_id))
+        item_href = system.get_url_with_parameters(toggle_params)
+        
+        return mark_safe(u'<a href="%s">%s</a>' % (item_href, item_id))
+
+
+
+
+class FilterGroup(FilterContainer, SetFilter): #final
+    """
+    A collection of other Filters, which are selected (enabled) according to the
+    rules of SetFilter.
+    
+    The do_queryset method mixes the output from all selected Filters, so only
+    the one for this FilterGroup needs to be (or should be) called.
+    
+    Serialized as a comma-separated list, like SetFilter.
+    """
+    
+    def __init__(self, id_str, filters_set):
+        FilterContainer.__init__(self, filters_set)
+        SetFilter.__init__(self, id_str) 
+    
+    def id_allowed(self, item_id): #overrides SetFilter
+        return item_id in self.filters_dict
+    
+    def get_selected_filters(self, filter_id):
+        return [self.filters_dict[s] for s in self.selected_set]
+    
+    def process_queryset(self, queryset): #overrides Filter
+        for f in self.selected_set:
+            queryset = self.filters_dict[f].process_queryset(queryset) #returns something like QuerySet.filter(blah)
+        return queryset
+    
+    def render_html(self): #overrides Filter
+        filters = ""
+        
+        for f in self.filters_dict:
+            f_render = self._render_html_filter(f)
+            if self.id_selected(f):
+                f_render = "<em>%s</em>" % f_render
+            filters += "<li>%s</li>" % f_render
+        
+        return mark_safe(u'%s:<ul>%s</ul>' % (self.get_id(), filters))
+    
+    def _render_html_filter(self, filter_id):
+        f = self.filters_dict[filter_id]
+        return f.render_html()
+

=== added file 'harvest/opportunities/filters.py'
--- harvest/opportunities/filters.py	1970-01-01 00:00:00 +0000
+++ harvest/opportunities/filters.py	2010-06-27 21:07:25 +0000
@@ -0,0 +1,53 @@
+from harvest.filters import filters, containers
+import models
+
+class PkgNameFilter(filters.EditFilter):
+    def process_queryset(self, queryset):
+        return queryset.filter(name__startswith = self.get_value())
+
+class PkgSetFilter(filters.ChoiceFilter):
+    def __init__(self, id_str):
+        choices_dict = dict()
+        for s in models.PackageSet.objects.all():
+            choices_dict[s.name] = s
+        super(PkgSetFilter, self).__init__(id_str, choices_dict)
+    
+    def process_queryset(self, queryset):
+        return queryset.filter(packagesets__in=self.get_selected_items())
+
+
+
+class OppFeaturedFilter(filters.Filter):
+    def process_queryset(self, queryset):
+        return queryset.filter(opportunitylist__featured=True)
+
+class OppListFilter(filters.ChoiceFilter):
+    def __init__(self, id_str):
+        choices_dict = dict()
+        for l in models.OpportunityList.objects.all():
+            choices_dict[l.name] = l
+        super(OppListFilter, self).__init__(id_str, choices_dict)
+    
+    def process_queryset(self, queryset):
+        return queryset.filter(opportunitylist__in=self.get_selected_items())
+
+
+#we don't really need to create a special type here, but it may be handy
+class HarvestFilters(containers.FilterSystem):
+    def __init__(self):
+        super(HarvestFilters, self).__init__(
+            [
+                filters.FilterGroup("pkg", [
+                    PkgNameFilter("name"),
+                    PkgSetFilter("set")
+                ] ),
+                filters.FilterGroup("opp", [
+                    OppFeaturedFilter("featured"),
+                    OppListFilter("list")
+                ] )
+            ],
+            default_parameters = { "pkg" : "name,set",
+                                   "pkg:name" : "ged",
+                                   "pkg:set" : "ubuntu-desktop" }
+        )
+

=== modified file 'harvest/opportunities/urls.py'
--- harvest/opportunities/urls.py	2010-03-08 16:33:21 +0000
+++ harvest/opportunities/urls.py	2010-06-27 21:07:25 +0000
@@ -12,6 +12,10 @@
 
     url(r'^source-package/(?P<sourcepackage_slug>[-\w+.]+)/$', 'opportunities.views.sourcepackage_detail', name='sourcepackage_detail'),
     url(r'^source-package/$', 'opportunities.views.sourcepackage_list', name='sourcepackage_list'),
+    
+    url(r'^filter',
+        'opportunities.views.opportunities_filter',
+        name='opportunities_filter'),
 
     url(r'^by-type',
         'opportunities.views.opportunities_by_type',

=== modified file 'harvest/opportunities/views.py'
--- harvest/opportunities/views.py	2010-06-08 15:46:42 +0000
+++ harvest/opportunities/views.py	2010-06-27 21:07:25 +0000
@@ -13,6 +13,9 @@
 import models
 import forms
 
+from filters import HarvestFilters
+from wrappers import PackageWrapper, PackageListWrapper
+
 def opportunity_index(request):
     sources_list = models.SourcePackage.objects.all()
     paginator = Paginator(sources_list, 50)
@@ -120,6 +123,37 @@
         extra_context = {'opportunities': opportunities},
     )
 
+def opportunities_filter(request):
+    filters = HarvestFilters()
+    filters.update_from_http(request)
+    filters_pkg = filters.find('pkg')
+    filters_opp = filters.find('opp')
+    
+    packages_list = models.SourcePackage.objects.distinct()
+    packages_list = filters_pkg.process_queryset(packages_list)
+    
+    #opportunities_list is filtered right away to only check opportunities belonging to selected packages
+    opportunities_list = models.Opportunity.objects.distinct().filter(sourcepackage__in=packages_list)
+    opportunities_list = filters_opp.process_queryset(opportunities_list)
+    #TODO: need to filter out opportunities with valid=False again
+    #TODO: would it be more efficient to group opportunities by their sourcepackages first, then run filters_opp.process_queryset() for each of those groups?
+    
+    pkg_list_wrapper = PackageListWrapper(request, packages_list, opportunities_list) 
+    
+    context = {
+        'grouping': 'package',
+        'packages_list': pkg_list_wrapper,
+        'filters_pkg' : filters_pkg,
+        'filters_opp' : filters_opp
+    }
+
+    return render(
+        'opportunities/opportunities_filter.html',
+        context,
+        context_instance=RequestContext(request))
+
+#TODO: package_filter_detail(request, sourcepackage, opportunities_list)
+
 def opportunities_by_type(request):
     types_list = models.OpportunityList.objects.filter(active=True)
     paginator = Paginator(types_list, 50)

=== added file 'harvest/opportunities/wrappers.py'
--- harvest/opportunities/wrappers.py	1970-01-01 00:00:00 +0000
+++ harvest/opportunities/wrappers.py	2010-06-27 21:07:25 +0000
@@ -0,0 +1,90 @@
+from django.db.models import Count
+from harvest.common.url_tools import current_url_with_parameters
+
+class PackageWrapper(object):
+    """
+    Describes a visible source package, for specific use in a
+    template.
+    """
+    
+    def __init__(self, request, package, visible_opportunities = None, expanded = False):
+        self.request = request
+        self.package = package
+        self.visible_opportunities = visible_opportunities
+        self.expanded = expanded
+    
+    def real(self):
+        return self.package
+    
+    def get_expand_toggle_url(self):
+        parameter = {'expand_pkg' : self.package.name}
+        url = current_url_with_parameters(self.request, parameter)
+        return url
+    
+    #FIXME: get_visible_opportunities and get_hidden_opportunities feel
+    #       wasteful. Could we do exclude and filter in a single
+    #       operation? Does it affect performance?
+    def get_visible_opportunities(self):
+        """
+        Returns opportunities that belong to the given package and are
+        in opportunities_list.
+        """
+        #also check if valid?
+        return self.visible_opportunities
+    
+    def get_hidden_opportunities(self):
+        """
+        Returns opportunities that belong to the given package but have
+        been hidden from view
+        """
+        opps_visible = self.get_visible_opportunities()
+        return self.package.opportunity_set.exclude(pk__in=opps_visible)
+
+class PackageListWrapper(object):
+    """
+    Object describing a list of source packages and opportunities, to
+    be used by a template. It contains UI-specific variables and simple
+    helper functions for doing final queries to access these lists.
+    """
+    
+    def __init__(self, request, packages_list, opportunities_list):
+        expand_list = None #list of packages to show in detail
+        if 'expand_pkg' in request.GET:
+            expand_list = request.GET['expand_pkg'].split(',')
+        
+        related_packages = set(opportunities_list.values_list('sourcepackage', flat=True))
+        
+        self.visible_packages_list = list()
+        self.hidden_packages_list = list() 
+        
+        #Create a PackageWrapper around every source package.
+        #Includes a less detailed wrapper for hidden packages.
+        for package in packages_list:
+            if package.pk in related_packages:
+                opps = None
+                expand = False
+                
+                if expand_list: expand = (package.name in expand_list)
+                opps = opportunities_list.filter(sourcepackage=package)
+                
+                package_wrapper = PackageWrapper(request, package,
+                                                 visible_opportunities = opps,
+                                                 expanded = expand)
+                self.visible_packages_list.append(package_wrapper)
+            
+            else:
+                package_wrapper = PackageWrapper(request, package)
+                self.hidden_packages_list.append(package_wrapper)
+    
+    def get_visible_packages(self):
+        """
+        Returns list of packages that are are visible.
+        These are any packages that contain opportunities.
+        """
+        return self.visible_packages_list
+    
+    def get_hidden_packages(self):
+        """
+        Returns list of packages that have been hidden from view.
+        """
+        return self.hidden_packages_list
\ No newline at end of file

=== modified file 'harvest/settings.py'
--- harvest/settings.py	2010-06-01 16:16:19 +0000
+++ harvest/settings.py	2010-06-27 21:07:25 +0000
@@ -7,6 +7,7 @@
 TEMPLATE_DEBUG = DEBUG
 STATIC_SERVE = True
 PROJECT_NAME = 'harvest'
+INTERNAL_IPS = ('127.0.0.1',) #for testing
 
 from common import utils
 VERSION_STRING = utils.get_harvest_version(
@@ -70,6 +71,7 @@
     'django.contrib.sessions.middleware.SessionMiddleware',
     'django.contrib.auth.middleware.AuthenticationMiddleware',
     'django.middleware.locale.LocaleMiddleware',
+    'debug_toolbar.middleware.DebugToolbarMiddleware', #for testing
 )
 
 ROOT_URLCONF = 'harvest.urls'
@@ -98,7 +100,9 @@
     'django.contrib.sites',
     'django.contrib.admin',
     'django_openid_auth',
+    'debug_toolbar', #for testing
     'opportunities',
+    'filters',
     'common',
 )
 

=== added file 'harvest/templates/opportunities/opportunities_filter.html'
--- harvest/templates/opportunities/opportunities_filter.html	1970-01-01 00:00:00 +0000
+++ harvest/templates/opportunities/opportunities_filter.html	2010-06-27 21:07:25 +0000
@@ -0,0 +1,50 @@
+{% extends "base.html" %}
+{% load i18n %}
+
+{% block title %}{% trans "Opportunity Index" %} - {{ block.super }}{% endblock %}
+
+{% block content %}
+<div class="mainpage">
+
+<h1>{% trans "Opportunities" %}</h1>
+
+<div class="filters" style="background-color:#E0F1FF; float:left; width:15em;">
+	{{filters_pkg.render}}
+	{{filters_opp.render}}
+</div>
+
+<div class="results" style="float:left;">
+{% if packages_list %}
+<ul>
+	{% for pkg in packages_list.get_visible_packages %}
+	<li><a href="{{ pkg.get_expand_toggle_url }}">{{ pkg.real.name }}</a>
+		{% if pkg.expanded %}
+		<ul>
+		{% for opportunity in pkg.get_visible_opportunities %}
+			{% include "opportunities/opportunity_detail.inc.html" %}
+		{% endfor %}
+		
+		{% with pkg.get_hidden_opportunities.count as hidden_count %}
+		{% ifnotequal hidden_count 0 %}
+		<li><small>{{ hidden_count }} {{ hidden_count|pluralize:"opportunity,opportunities"}} hidden</small></li>
+		{% endifnotequal %}
+		{% endwith %}
+		</ul>
+		{% endif %}
+	</li>
+	{% endfor %}
+	
+	{% with packages_list.get_hidden_packages|length as hidden_count %}
+	{% ifnotequal hidden_count 0 %}
+	<li><small>{{ hidden_count }} package{{ hidden_count|pluralize:"s"}} {{ hidden_count|pluralize:"has,have"}} no matching opportunities</small></li>
+	{% endifnotequal %}
+	{% endwith %}
+</ul>
+
+{% else %}
+<p>{% trans "There are currently no opportunities in Harvest. :(" %}</p>
+{% endif %}
+</div>
+
+</div>
+{% endblock %}