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

Proposed by Dylan McCall on 2010-06-23
Status: Merged
Merged at revision: 186
Proposed branch: lp:~dylanmccall/harvest/harvest-dylan-m
Merge into: lp:harvest
Diff against target: 806 lines (+706/-1)
11 files modified
INSTALL (+1/-1)
harvest/common/url_tools.py (+32/-0)
harvest/filters/__init__.py (+23/-0)
harvest/filters/containers.py (+97/-0)
harvest/filters/filters.py (+318/-0)
harvest/opportunities/filters.py (+53/-0)
harvest/opportunities/urls.py (+4/-0)
harvest/opportunities/views.py (+34/-0)
harvest/opportunities/wrappers.py (+90/-0)
harvest/settings.py (+4/-0)
harvest/templates/opportunities/opportunities_filter.html (+50/-0)
To merge this branch: bzr merge lp:~dylanmccall/harvest/harvest-dylan-m
Reviewer Review Type Date Requested Status
Daniel Holbach Approve on 2010-07-05
James Westby 2010-06-23 Approve on 2010-06-23
Review via email: mp+28262@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Dylan McCall (dylanmccall) wrote :

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) wrote :

> 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) wrote :

(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) wrote :

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) wrote :

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) wrote :

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) wrote :
Download full text (8.2 KiB)

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...

Read more...

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) wrote :
Download full text (3.2 KiB)

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...

Read more...

Dylan McCall (dylanmccall) wrote :

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) wrote :

> 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'INSTALL'
2--- INSTALL 2010-03-02 10:42:19 +0000
3+++ INSTALL 2010-06-27 21:07:25 +0000
4@@ -1,4 +1,4 @@
5-1. sudo apt-get install python-django python-launchpadlib python-django-openid-auth bzr
6+1. sudo apt-get install python-django python-launchpadlib python-django-openid-auth bzr python-django-debug-toolbar
7
8 ---
9 Optional for postgres usage:
10
11=== added file 'harvest/common/url_tools.py'
12--- harvest/common/url_tools.py 1970-01-01 00:00:00 +0000
13+++ harvest/common/url_tools.py 2010-06-27 21:07:25 +0000
14@@ -0,0 +1,32 @@
15+def new_url_with_parameters(url, params_dict):
16+ """
17+ Returns a new URL with an added query string, described by params_dict.
18+ @param params_dict: a dictionary with all the parameters to add
19+ @param path: url to add the parameters to
20+ @return: the url (a string) with given parameters
21+ """
22+ #Derived from <http://djangosnippets.org/snippets/1627/>
23+
24+ def param_bit(key, value):
25+ if value:
26+ return "%s=%s" % (key, value)
27+ else:
28+ return "%s" % key
29+
30+ if len(params_dict):
31+ url_params = list()
32+ url += "?%s" % "&".join([param_bit(key, value) for (key, value) in params_dict.items()])
33+
34+ return url
35+
36+def current_url_with_parameters(request, new_params_dict):
37+ """
38+ Returns the current URL with some parameters changed, which are
39+ described in new_params_dict. The rest of the query string remains
40+ intact.
41+ """
42+ params = request.GET.copy() #this includes parameters that aren't used by the FilterSystem
43+ params.update(new_params_dict)
44+ url = request.path
45+ return new_url_with_parameters(url, params)
46+
47
48=== added directory 'harvest/filters'
49=== added file 'harvest/filters/__init__.py'
50--- harvest/filters/__init__.py 1970-01-01 00:00:00 +0000
51+++ harvest/filters/__init__.py 2010-06-27 21:07:25 +0000
52@@ -0,0 +1,23 @@
53+"""
54+Here, we define a very abstract filtering system.
55+This system is entirely decoupled from presentation. A filter is simply a query
56+operation intended for a particular type of model, which can be configured and
57+turned on / off via structured input.
58+
59+There are, of course, some rules: different types of filters can be added,
60+some of which store extra information (like a line of text that the user can
61+input), and filters can be grouped like radio buttons where only one filter
62+in a group can be selected.
63+
64+The included classes provide most important features for free, but to be of
65+any use they need to be extended to fit the intended application. For example,
66+the get_query class is unimplemented; it should return a Q object that does
67+the work of selecting whatever the filter is intended to select.
68+
69+Groups of filters in no way refer to how they get placed in a page, or how
70+they are interacted with. We leave that entirely up to templates. It is also
71+something else's job to figure out how to draw filters, and how to form the
72+links they talk to us with.
73+
74+All This Does is filter things.
75+"""
76
77=== added file 'harvest/filters/containers.py'
78--- harvest/filters/containers.py 1970-01-01 00:00:00 +0000
79+++ harvest/filters/containers.py 2010-06-27 21:07:25 +0000
80@@ -0,0 +1,97 @@
81+from harvest.common.url_tools import current_url_with_parameters
82+
83+class FilterContainer(object): #abstract
84+ """
85+ A class that contains Filter objects, which can be retrieved with the
86+ find method.
87+
88+ The added Filter objects are referred to as "children." They are expected
89+ to exist for the entire life of the container.
90+ """
91+
92+ def __init__(self, filters_set):
93+ self.filters_dict = dict() #refers to Filter objects by their unique IDs
94+ self.add_filters(filters_set)
95+
96+ def add_filters(self, filters_set): #final
97+ """
98+ Adds a set of filters to be children of this one and informs
99+ each child that it belongs to this container.
100+ @param filter_set: a set of Filter objects
101+ """
102+ for child in set(filters_set):
103+ self.filters_dict[child.get_id()] = child
104+ child.set_container(self)
105+
106+ def find(self, full_name): #final
107+ """
108+ Finds a filter inside this object or one of its children, based
109+ on that filter's full name in the format container:child.
110+ @param full_name: an object's full name
111+ @return: the object described by full_name, or None
112+ """
113+ result = None
114+ name_bits = full_name.split(':',1)
115+
116+ #the first bit is the one this instance stores
117+ if name_bits[0] in self.filters_dict:
118+ result = self.filters_dict[name_bits[0]]
119+
120+ if isinstance(result, FilterContainer) and len(name_bits)>1:
121+ result = result.find(name_bits[1]) #send remaining bits to child
122+
123+ #we have reached the end of the list. Return the result (which may be None)
124+ return result
125+
126+
127+
128+class FilterSystem(FilterContainer):
129+ """
130+ This is the single all-knowing root object that should contain all
131+ other filters for an application. From this object it is possible to
132+ find a filter using its full name, which is the same name used to
133+ serialize a filter's state in an HTTP query string.
134+
135+ Before an instance of this object can be safely used,
136+ update_from_http should be called with the current HttpRequest.
137+ """
138+
139+ def __init__(self, filters_set, default_parameters = dict()):
140+ FilterContainer.__init__(self, filters_set)
141+ self.request = None #current http request
142+ self.default_parameters = default_parameters
143+
144+ def update_from_http(self, request): #final
145+ """
146+ Call this method to update the state of all filters based on an
147+ HttpRequest object. The request object will be stored for other uses.
148+ @param request: current HttpRequest object
149+ """
150+ self.request = request
151+ #this contains parameters that aren't relevant to the FilterSystem
152+ self._set_parameters(request.GET)
153+
154+ def _set_parameters(self, parameters): #final
155+ """
156+ Updates the state of all filters based on given parameters.
157+ @param parameters: dictionary of parameters, for example from request.GET
158+ """
159+ new_params = parameters.copy()
160+ for key in self.default_parameters:
161+ #apply default parameters for keys that have not been set
162+ if not key in new_params: new_params[key] = self.default_parameters[key]
163+
164+ for key in new_params:
165+ filter_object = self.find(key)
166+ if filter_object:
167+ filter_object.set_value(new_params[key])
168+
169+ def get_url_with_parameters(self, parameters): #final
170+ """
171+ Returns a new URL where the given parameters will be applied.
172+ To generate parameters, see Filter.serialize.
173+ @param parameters: a dictionary of new parameters
174+ @return: the current url with the given parameters added to the query string
175+ """
176+ return current_url_with_parameters(self.request, parameters)
177+
178
179=== added file 'harvest/filters/filters.py'
180--- harvest/filters/filters.py 1970-01-01 00:00:00 +0000
181+++ harvest/filters/filters.py 2010-06-27 21:07:25 +0000
182@@ -0,0 +1,318 @@
183+#FIXME: Make ChoiceFilter a bit nicer so we don't need to call super() and all that bother from harvest.opportunities.filters.
184+#TODO: adjust Filter.render() methods for custom template tags, with django.template.Template
185+
186+from containers import FilterContainer, FilterSystem
187+from django.utils.safestring import mark_safe
188+from copy import copy
189+
190+class Filter(object): #abstract, extend in application
191+ """
192+ The abstract base class for all other filters.
193+
194+ Every Filter's main objective is to process QuerySets with the method
195+ Filter.process_queryset. Filter, or one of its subclasses, should be
196+ extended with a new version of process_queryset that does something useful
197+ for a specific application. For example, process_queryset could return
198+ queryset.filter(foo="bar") so using the filter will limit the queryset
199+ to objects where foo=bar.
200+
201+ Every Filter has a unique id and can belong to a single container.
202+
203+ A Filter can be assigned a new value from a string and its internal value
204+ (which could be of any type) can be serialized back to a string.
205+ The object itself can be serialized as a key / value pair, where the key
206+ is completely unique.
207+
208+ Every Filter can be rendered, which outputs markup (currently html)
209+ describing it for a user interface.
210+ """
211+
212+ #TODO: figure out get_system and get_full_name when set_container is called and store the value
213+
214+ def __init__(self, id_str): #final
215+ self.id_str = id_str #local name
216+ self.container = None #immediate container (FilterContainer)
217+
218+ def get_id(self): #final
219+ """
220+ @return: the local id of this filter, unique to its container.
221+ """
222+ return self.id_str
223+
224+ def set_container(self, container): #final
225+ """
226+ Specify that this filter belongs to a specific container. This will
227+ replace any container it currently believes it belongs to.
228+ @param container: a FilterContainer object that holds this Filter
229+ """
230+ #it would make sense to raise an exception if self.container != None
231+ self.container = container
232+
233+ def get_container(self): #final
234+ """
235+ @return: the container that this filter belongs to
236+ """
237+ return self.container
238+
239+ def get_system(self): #final
240+ """
241+ @return: the FilterSystem that this filter ultimately belongs to
242+ """
243+ container = self.get_container()
244+ system = None
245+ if isinstance(container, Filter):
246+ system = container.get_system()
247+ elif isinstance(container, FilterSystem):
248+ system = container
249+ return system
250+
251+ def get_full_name(self): #final
252+ """
253+ Returns the filter's full name, which should make sense from anywhere
254+ in the application. This name is in the format parent:child:child.
255+ @return: the filter's full name, which is a string
256+ """
257+ full_name = self.get_id()
258+ container = self.get_container()
259+ if isinstance(container, Filter):
260+ full_name = "%s:%s" % (container.get_full_name(), full_name)
261+ return full_name
262+
263+ def set_value(self, value): #abstract
264+ """
265+ Extend this to take a value passed down from the top, probably
266+ from FilterSystem.update_from_http, and do something with it.
267+ @param value: a new value, as a string
268+ """
269+ raise NotImplementedError(self.set_value)
270+
271+ def get_value(self): #abstract
272+ """
273+ @return: a copy of this filter's value in its native format
274+ """
275+ raise NotImplementedError(self.get_value)
276+
277+ def serialize_value(self, value): #abstract
278+ """
279+ The inverse of set_value. Returns the given value as a string that could
280+ be added to an HTTP query string.
281+ @param value: the value to serialize, in a format native to this Filter (see get_value)
282+ @return: a unicode string formatted for set_value
283+ """
284+ raise NotImplementedError(self.serialize_value)
285+
286+ def serialize(self, value = None): #final
287+ """
288+ Creates a dictionary of parameters to describe this object, either
289+ as-is or with a new value.
290+ The result can be sent to FilterSystem.get_url_with_parameters.
291+ @param value: a different value to use, in a format native to this Filter (see get_value)
292+ @return: a dictionary of key:value pairs referring to the object and its value.
293+ """
294+ if value == None: value = self.get_value()
295+ key = self.get_full_name()
296+ value_str = self.serialize_value(value)
297+ return {key : value_str}
298+
299+ def get_container_toggle_parameters(self): #final
300+ """
301+ Helper method to get the parameter for toggling this filter's state in
302+ its container, if there is one.
303+ @return: a dictionary of key:value pairs to generate new GET parameters
304+ """
305+ container = self.get_container()
306+ params = dict()
307+ if isinstance(container, FilterGroup):
308+ params = container.serialize(container.get_value_with_selection(self.get_id()))
309+ return params
310+
311+ def process_queryset(self, queryset): #abstract
312+ """
313+ Extend this to manipulate a given queryset and then return it.
314+ For example, queryset.filter(name__startswith = self.value)
315+ @param queryset: a queryset to operate on
316+ @return: a queryset based on the given one
317+ """
318+ raise NotImplementedError(self.process_queryset)
319+
320+ def render(self): #final
321+ """
322+ @return: the default rendering of the filter itself in given context
323+ """
324+ return self.render_html()
325+
326+ def render_html(self):
327+ """
328+ Extend this to return the html output for the filter itself.
329+ The output should be very simple and semantically meaningful,
330+ with no specific concern about formatting. It will be
331+ placed within other tags that describe its context, and it is
332+ up to the template to describe style.
333+ @return: a unicode string containing html representing this filter
334+ """
335+ system = self.get_system()
336+ toggle_params = self.get_container_toggle_parameters()
337+ href = system.get_url_with_parameters(toggle_params)
338+
339+ return mark_safe(u'<a href="%s">(%s)</a>'
340+ % (href, self.get_id()))
341+
342+
343+
344+
345+class EditFilter(Filter): #abstract, extend in application
346+ """
347+ This Filter has a simple string value which can be edited by the user.
348+
349+ Serialized as stored ("value")
350+ """
351+
352+ def __init__(self, id_str):
353+ Filter.__init__(self, id_str)
354+ self.input_str = ""
355+
356+ def set_value(self, value): #overrides Filter
357+ self.input_str = value
358+
359+ def get_value(self): #overrides Filter
360+ return self.input_str
361+
362+ def serialize_value(self, value): #overrides Filter
363+ return value
364+
365+ def render_html(self):
366+ system = self.get_system()
367+ toggle_params = self.get_container_toggle_parameters()
368+ href = system.get_url_with_parameters(toggle_params)
369+
370+ return mark_safe(u'<a href="%s">%s: %s</a>'
371+ % (href, self.get_id(), self.get_value()))
372+
373+
374+class SetFilter(Filter): #abstract, extend in application
375+ """
376+ Holds a set of strings, with no repetition.
377+
378+ Serialized as a comma-separated list ("dog,cat,horse,mouse")
379+ """
380+
381+ def __init__(self, id_str):
382+ Filter.__init__(self, id_str)
383+ self.selected_set = set()
384+
385+ def set_value(self, value): #overrides Filter
386+ self.selected_set = set([s for s in value.split(",") if self.id_allowed(s)])
387+
388+ def get_value(self): #overrides Filter
389+ return self.selected_set.copy()
390+
391+ def serialize_value(self, value): #overrides Filter
392+ return ",".join(value)
393+
394+ def get_value_with_selection(self, item_id): #final
395+ """
396+ Returns the current value of this object with the selection referred to
397+ by item_id toggled on or off, depending on its current state.
398+ @param item_id: id for the item to toggle
399+ @return: the value of this SetFilter with the given item toggled on or off
400+ """
401+ select = self.get_value()
402+ if item_id in select:
403+ select.remove(item_id)
404+ else:
405+ select.add(item_id)
406+ return select
407+
408+ def id_selected(self, item_id):
409+ return item_id in self.selected_set
410+
411+ def id_allowed(self, item_id):
412+ return True
413+
414+
415+class ChoiceFilter(SetFilter): #abstract, extend in application
416+ """
417+ Has a dictionary of items, with names and values of any type. These can be
418+ selected or deselected by the input, which is a set of strings, as in
419+ SetFilter. In that set, any items which refer to choices that do not exist
420+ are ignored.
421+
422+ Serialized as a comma-separated list, like SetFilter.
423+ """
424+
425+ def __init__(self, id_str, choices_dict):
426+ SetFilter.__init__(self, id_str)
427+ self.choices_dict = choices_dict
428+
429+ def id_allowed(self, item_id): #overrides SetFilter
430+ return item_id in self.choices_dict
431+
432+ def get_selected_items(self):
433+ return [self.choices_dict[s] for s in self.selected_set]
434+
435+ def render_html(self): #overrides Filter
436+ choices = ""
437+
438+ for c in self.choices_dict:
439+ c_render = self._render_html_choice(c)
440+ if self.id_selected(c):
441+ c_render = "<b>%s</b>" % c_render
442+ choices += "<li>%s</li>" % c_render
443+
444+ system = self.get_system()
445+ toggle_params = self.get_container_toggle_parameters()
446+ self_href = system.get_url_with_parameters(toggle_params)
447+
448+ return mark_safe(u'<a href="%s">%s</a>:<ul>%s</ul>' % (self_href, self.get_id(), choices))
449+
450+ def _render_html_choice(self, item_id):
451+ system = self.get_system()
452+ toggle_params = self.serialize(self.get_value_with_selection(item_id))
453+ item_href = system.get_url_with_parameters(toggle_params)
454+
455+ return mark_safe(u'<a href="%s">%s</a>' % (item_href, item_id))
456+
457+
458+
459+
460+class FilterGroup(FilterContainer, SetFilter): #final
461+ """
462+ A collection of other Filters, which are selected (enabled) according to the
463+ rules of SetFilter.
464+
465+ The do_queryset method mixes the output from all selected Filters, so only
466+ the one for this FilterGroup needs to be (or should be) called.
467+
468+ Serialized as a comma-separated list, like SetFilter.
469+ """
470+
471+ def __init__(self, id_str, filters_set):
472+ FilterContainer.__init__(self, filters_set)
473+ SetFilter.__init__(self, id_str)
474+
475+ def id_allowed(self, item_id): #overrides SetFilter
476+ return item_id in self.filters_dict
477+
478+ def get_selected_filters(self, filter_id):
479+ return [self.filters_dict[s] for s in self.selected_set]
480+
481+ def process_queryset(self, queryset): #overrides Filter
482+ for f in self.selected_set:
483+ queryset = self.filters_dict[f].process_queryset(queryset) #returns something like QuerySet.filter(blah)
484+ return queryset
485+
486+ def render_html(self): #overrides Filter
487+ filters = ""
488+
489+ for f in self.filters_dict:
490+ f_render = self._render_html_filter(f)
491+ if self.id_selected(f):
492+ f_render = "<em>%s</em>" % f_render
493+ filters += "<li>%s</li>" % f_render
494+
495+ return mark_safe(u'%s:<ul>%s</ul>' % (self.get_id(), filters))
496+
497+ def _render_html_filter(self, filter_id):
498+ f = self.filters_dict[filter_id]
499+ return f.render_html()
500+
501
502=== added file 'harvest/opportunities/filters.py'
503--- harvest/opportunities/filters.py 1970-01-01 00:00:00 +0000
504+++ harvest/opportunities/filters.py 2010-06-27 21:07:25 +0000
505@@ -0,0 +1,53 @@
506+from harvest.filters import filters, containers
507+import models
508+
509+class PkgNameFilter(filters.EditFilter):
510+ def process_queryset(self, queryset):
511+ return queryset.filter(name__startswith = self.get_value())
512+
513+class PkgSetFilter(filters.ChoiceFilter):
514+ def __init__(self, id_str):
515+ choices_dict = dict()
516+ for s in models.PackageSet.objects.all():
517+ choices_dict[s.name] = s
518+ super(PkgSetFilter, self).__init__(id_str, choices_dict)
519+
520+ def process_queryset(self, queryset):
521+ return queryset.filter(packagesets__in=self.get_selected_items())
522+
523+
524+
525+class OppFeaturedFilter(filters.Filter):
526+ def process_queryset(self, queryset):
527+ return queryset.filter(opportunitylist__featured=True)
528+
529+class OppListFilter(filters.ChoiceFilter):
530+ def __init__(self, id_str):
531+ choices_dict = dict()
532+ for l in models.OpportunityList.objects.all():
533+ choices_dict[l.name] = l
534+ super(OppListFilter, self).__init__(id_str, choices_dict)
535+
536+ def process_queryset(self, queryset):
537+ return queryset.filter(opportunitylist__in=self.get_selected_items())
538+
539+
540+#we don't really need to create a special type here, but it may be handy
541+class HarvestFilters(containers.FilterSystem):
542+ def __init__(self):
543+ super(HarvestFilters, self).__init__(
544+ [
545+ filters.FilterGroup("pkg", [
546+ PkgNameFilter("name"),
547+ PkgSetFilter("set")
548+ ] ),
549+ filters.FilterGroup("opp", [
550+ OppFeaturedFilter("featured"),
551+ OppListFilter("list")
552+ ] )
553+ ],
554+ default_parameters = { "pkg" : "name,set",
555+ "pkg:name" : "ged",
556+ "pkg:set" : "ubuntu-desktop" }
557+ )
558+
559
560=== modified file 'harvest/opportunities/urls.py'
561--- harvest/opportunities/urls.py 2010-03-08 16:33:21 +0000
562+++ harvest/opportunities/urls.py 2010-06-27 21:07:25 +0000
563@@ -12,6 +12,10 @@
564
565 url(r'^source-package/(?P<sourcepackage_slug>[-\w+.]+)/$', 'opportunities.views.sourcepackage_detail', name='sourcepackage_detail'),
566 url(r'^source-package/$', 'opportunities.views.sourcepackage_list', name='sourcepackage_list'),
567+
568+ url(r'^filter',
569+ 'opportunities.views.opportunities_filter',
570+ name='opportunities_filter'),
571
572 url(r'^by-type',
573 'opportunities.views.opportunities_by_type',
574
575=== modified file 'harvest/opportunities/views.py'
576--- harvest/opportunities/views.py 2010-06-08 15:46:42 +0000
577+++ harvest/opportunities/views.py 2010-06-27 21:07:25 +0000
578@@ -13,6 +13,9 @@
579 import models
580 import forms
581
582+from filters import HarvestFilters
583+from wrappers import PackageWrapper, PackageListWrapper
584+
585 def opportunity_index(request):
586 sources_list = models.SourcePackage.objects.all()
587 paginator = Paginator(sources_list, 50)
588@@ -120,6 +123,37 @@
589 extra_context = {'opportunities': opportunities},
590 )
591
592+def opportunities_filter(request):
593+ filters = HarvestFilters()
594+ filters.update_from_http(request)
595+ filters_pkg = filters.find('pkg')
596+ filters_opp = filters.find('opp')
597+
598+ packages_list = models.SourcePackage.objects.distinct()
599+ packages_list = filters_pkg.process_queryset(packages_list)
600+
601+ #opportunities_list is filtered right away to only check opportunities belonging to selected packages
602+ opportunities_list = models.Opportunity.objects.distinct().filter(sourcepackage__in=packages_list)
603+ opportunities_list = filters_opp.process_queryset(opportunities_list)
604+ #TODO: need to filter out opportunities with valid=False again
605+ #TODO: would it be more efficient to group opportunities by their sourcepackages first, then run filters_opp.process_queryset() for each of those groups?
606+
607+ pkg_list_wrapper = PackageListWrapper(request, packages_list, opportunities_list)
608+
609+ context = {
610+ 'grouping': 'package',
611+ 'packages_list': pkg_list_wrapper,
612+ 'filters_pkg' : filters_pkg,
613+ 'filters_opp' : filters_opp
614+ }
615+
616+ return render(
617+ 'opportunities/opportunities_filter.html',
618+ context,
619+ context_instance=RequestContext(request))
620+
621+#TODO: package_filter_detail(request, sourcepackage, opportunities_list)
622+
623 def opportunities_by_type(request):
624 types_list = models.OpportunityList.objects.filter(active=True)
625 paginator = Paginator(types_list, 50)
626
627=== added file 'harvest/opportunities/wrappers.py'
628--- harvest/opportunities/wrappers.py 1970-01-01 00:00:00 +0000
629+++ harvest/opportunities/wrappers.py 2010-06-27 21:07:25 +0000
630@@ -0,0 +1,90 @@
631+from django.db.models import Count
632+from harvest.common.url_tools import current_url_with_parameters
633+
634+class PackageWrapper(object):
635+ """
636+ Describes a visible source package, for specific use in a
637+ template.
638+ """
639+
640+ def __init__(self, request, package, visible_opportunities = None, expanded = False):
641+ self.request = request
642+ self.package = package
643+ self.visible_opportunities = visible_opportunities
644+ self.expanded = expanded
645+
646+ def real(self):
647+ return self.package
648+
649+ def get_expand_toggle_url(self):
650+ parameter = {'expand_pkg' : self.package.name}
651+ url = current_url_with_parameters(self.request, parameter)
652+ return url
653+
654+ #FIXME: get_visible_opportunities and get_hidden_opportunities feel
655+ # wasteful. Could we do exclude and filter in a single
656+ # operation? Does it affect performance?
657+ def get_visible_opportunities(self):
658+ """
659+ Returns opportunities that belong to the given package and are
660+ in opportunities_list.
661+ """
662+ #also check if valid?
663+ return self.visible_opportunities
664+
665+ def get_hidden_opportunities(self):
666+ """
667+ Returns opportunities that belong to the given package but have
668+ been hidden from view
669+ """
670+ opps_visible = self.get_visible_opportunities()
671+ return self.package.opportunity_set.exclude(pk__in=opps_visible)
672+
673+class PackageListWrapper(object):
674+ """
675+ Object describing a list of source packages and opportunities, to
676+ be used by a template. It contains UI-specific variables and simple
677+ helper functions for doing final queries to access these lists.
678+ """
679+
680+ def __init__(self, request, packages_list, opportunities_list):
681+ expand_list = None #list of packages to show in detail
682+ if 'expand_pkg' in request.GET:
683+ expand_list = request.GET['expand_pkg'].split(',')
684+
685+ related_packages = set(opportunities_list.values_list('sourcepackage', flat=True))
686+
687+ self.visible_packages_list = list()
688+ self.hidden_packages_list = list()
689+
690+ #Create a PackageWrapper around every source package.
691+ #Includes a less detailed wrapper for hidden packages.
692+ for package in packages_list:
693+ if package.pk in related_packages:
694+ opps = None
695+ expand = False
696+
697+ if expand_list: expand = (package.name in expand_list)
698+ opps = opportunities_list.filter(sourcepackage=package)
699+
700+ package_wrapper = PackageWrapper(request, package,
701+ visible_opportunities = opps,
702+ expanded = expand)
703+ self.visible_packages_list.append(package_wrapper)
704+
705+ else:
706+ package_wrapper = PackageWrapper(request, package)
707+ self.hidden_packages_list.append(package_wrapper)
708+
709+ def get_visible_packages(self):
710+ """
711+ Returns list of packages that are are visible.
712+ These are any packages that contain opportunities.
713+ """
714+ return self.visible_packages_list
715+
716+ def get_hidden_packages(self):
717+ """
718+ Returns list of packages that have been hidden from view.
719+ """
720+ return self.hidden_packages_list
721\ No newline at end of file
722
723=== modified file 'harvest/settings.py'
724--- harvest/settings.py 2010-06-01 16:16:19 +0000
725+++ harvest/settings.py 2010-06-27 21:07:25 +0000
726@@ -7,6 +7,7 @@
727 TEMPLATE_DEBUG = DEBUG
728 STATIC_SERVE = True
729 PROJECT_NAME = 'harvest'
730+INTERNAL_IPS = ('127.0.0.1',) #for testing
731
732 from common import utils
733 VERSION_STRING = utils.get_harvest_version(
734@@ -70,6 +71,7 @@
735 'django.contrib.sessions.middleware.SessionMiddleware',
736 'django.contrib.auth.middleware.AuthenticationMiddleware',
737 'django.middleware.locale.LocaleMiddleware',
738+ 'debug_toolbar.middleware.DebugToolbarMiddleware', #for testing
739 )
740
741 ROOT_URLCONF = 'harvest.urls'
742@@ -98,7 +100,9 @@
743 'django.contrib.sites',
744 'django.contrib.admin',
745 'django_openid_auth',
746+ 'debug_toolbar', #for testing
747 'opportunities',
748+ 'filters',
749 'common',
750 )
751
752
753=== added file 'harvest/templates/opportunities/opportunities_filter.html'
754--- harvest/templates/opportunities/opportunities_filter.html 1970-01-01 00:00:00 +0000
755+++ harvest/templates/opportunities/opportunities_filter.html 2010-06-27 21:07:25 +0000
756@@ -0,0 +1,50 @@
757+{% extends "base.html" %}
758+{% load i18n %}
759+
760+{% block title %}{% trans "Opportunity Index" %} - {{ block.super }}{% endblock %}
761+
762+{% block content %}
763+<div class="mainpage">
764+
765+<h1>{% trans "Opportunities" %}</h1>
766+
767+<div class="filters" style="background-color:#E0F1FF; float:left; width:15em;">
768+ {{filters_pkg.render}}
769+ {{filters_opp.render}}
770+</div>
771+
772+<div class="results" style="float:left;">
773+{% if packages_list %}
774+<ul>
775+ {% for pkg in packages_list.get_visible_packages %}
776+ <li><a href="{{ pkg.get_expand_toggle_url }}">{{ pkg.real.name }}</a>
777+ {% if pkg.expanded %}
778+ <ul>
779+ {% for opportunity in pkg.get_visible_opportunities %}
780+ {% include "opportunities/opportunity_detail.inc.html" %}
781+ {% endfor %}
782+
783+ {% with pkg.get_hidden_opportunities.count as hidden_count %}
784+ {% ifnotequal hidden_count 0 %}
785+ <li><small>{{ hidden_count }} {{ hidden_count|pluralize:"opportunity,opportunities"}} hidden</small></li>
786+ {% endifnotequal %}
787+ {% endwith %}
788+ </ul>
789+ {% endif %}
790+ </li>
791+ {% endfor %}
792+
793+ {% with packages_list.get_hidden_packages|length as hidden_count %}
794+ {% ifnotequal hidden_count 0 %}
795+ <li><small>{{ hidden_count }} package{{ hidden_count|pluralize:"s"}} {{ hidden_count|pluralize:"has,have"}} no matching opportunities</small></li>
796+ {% endifnotequal %}
797+ {% endwith %}
798+</ul>
799+
800+{% else %}
801+<p>{% trans "There are currently no opportunities in Harvest. :(" %}</p>
802+{% endif %}
803+</div>
804+
805+</div>
806+{% endblock %}

Subscribers

People subscribed via source and target branches

to all changes: