Merge lp:~dylanmccall/harvest/querystring-cleanup into lp:harvest

Proposed by Dylan McCall
Status: Merged
Merged at revision: 229
Proposed branch: lp:~dylanmccall/harvest/querystring-cleanup
Merge into: lp:harvest
Diff against target: 427 lines (+106/-87)
6 files modified
harvest/common/url_tools.py (+18/-30)
harvest/filters/containers.py (+16/-11)
harvest/filters/filters.py (+22/-21)
harvest/media/js/harvest.js (+42/-16)
harvest/opportunities/filters.py (+2/-2)
harvest/opportunities/wrappers.py (+6/-7)
To merge this branch: bzr merge lp:~dylanmccall/harvest/querystring-cleanup
Reviewer Review Type Date Requested Status
Daniel Holbach Approve
Review via email: mp+30063@code.launchpad.net

Description of the change

Fairly simple change, based around cleaning up our querystrings. It also cleans up the code a little, while it's at it! :)

This scraps all characters we were using that were escaped by jQuery and some web browsers, which had resulted in ugly urls. (Ideally, these can be rather human readable).

Two things here:

The full name of a filter is now something like "pkg.list," because "." isn't violently escaped and is synonymous with a parent / child relationship.

No more of that crazy "serialize lists to comma-separated strings" stuff. As far as any portion of Harvest is concerned, these are always lists. The code to generate querystrings appropriately in that case is in Django and jQuery. (They are both happy to treat a key that is repeated multiple times as an array of values by that name, and vice versa).

Example URL with this format:
/opportunities/query/results?expand=67&expand=66&pkg=set&pkg.name=&pkg.set=ubuntu-desktop&opp=list&opp.list=bitesize

A hair nicer, I think :)

To post a comment you must log in.
229. By Dylan McCall

Fixed bug in harvest.js introduced by last change, where reverting a change would not stop load countdown (because string comparisons were not like array comparisons... until now!)

Revision history for this message
Daniel Holbach (dholbach) wrote :

Great work. This makes it a LOT nicer. :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'harvest/common/url_tools.py'
2--- harvest/common/url_tools.py 2010-06-24 06:53:17 +0000
3+++ harvest/common/url_tools.py 2010-07-16 05:55:54 +0000
4@@ -1,32 +1,20 @@
5-def new_url_with_parameters(url, params_dict):
6- """
7- Returns a new URL with an added query string, described by params_dict.
8- @param params_dict: a dictionary with all the parameters to add
9- @param path: url to add the parameters to
10- @return: the url (a string) with given parameters
11- """
12- #Derived from <http://djangosnippets.org/snippets/1627/>
13-
14- def param_bit(key, value):
15- if value:
16- return "%s=%s" % (key, value)
17+def update_url_with_parameters(request, new_parameters, url=None):
18+ """
19+ Returns a URL (the current one by default) with some new querystring
20+ parameters, based on the current querystring and the given
21+ dictionary of parameters. The dictionary can contain list values as
22+ well as strings and integers.
23+ @param request: the current HttpRequest object
24+ @param new_parameters: a dictionary of new parameters
25+ @param url: a url to use instead of the current one
26+ """
27+ if url == None: url = request.path
28+ querydict = request.GET.copy()
29+ for k in new_parameters:
30+ v = new_parameters[k]
31+ if isinstance(v,list):
32+ querydict.setlist(k, v)
33 else:
34- return "%s" % key
35-
36- if len(params_dict):
37- url_params = list()
38- url += "?%s" % "&".join([param_bit(key, value) for (key, value) in params_dict.items()])
39-
40- return url
41-
42-def current_url_with_parameters(request, new_params_dict):
43- """
44- Returns the current URL with some parameters changed, which are
45- described in new_params_dict. The rest of the query string remains
46- intact.
47- """
48- params = request.GET.copy() #this includes parameters that aren't used by the FilterSystem
49- params.update(new_params_dict)
50- url = request.path
51- return new_url_with_parameters(url, params)
52+ querydict[k] = v
53+ return "%s?%s" % ( url, querydict.urlencode() )
54
55
56=== modified file 'harvest/filters/containers.py'
57--- harvest/filters/containers.py 2010-07-04 05:43:53 +0000
58+++ harvest/filters/containers.py 2010-07-16 05:55:54 +0000
59@@ -1,4 +1,4 @@
60-from harvest.common.url_tools import current_url_with_parameters
61+from harvest.common.url_tools import update_url_with_parameters
62 from harvest.common.ordereddict import OrderedDict #while we wait for Python 2.7 :)
63
64 class FilterContainer(object): #abstract
65@@ -27,12 +27,12 @@
66 def find(self, full_name): #final
67 """
68 Finds a filter inside this object or one of its children, based
69- on that filter's full name in the format container:child.
70+ on that filter's full name in the format container-child.
71 @param full_name: an object's full name
72 @return: the object described by full_name, or None
73 """
74 result = None
75- name_bits = full_name.split(':',1)
76+ name_bits = full_name.split('.',1)
77
78 #the first bit is the one this instance stores
79 if name_bits[0] in self.filters_dict:
80@@ -70,22 +70,27 @@
81 """
82 self.request = request
83 #this contains parameters that aren't relevant to the FilterSystem
84- self._set_parameters(request.GET)
85+ self.set_parameters(request.GET)
86
87- def _set_parameters(self, parameters): #final
88+ def set_parameters(self, querydict): #final
89 """
90 Updates the state of all filters based on given parameters.
91- @param parameters: dictionary of parameters, for example from request.GET
92+ @param querydict: dictionary of parameters from request.GET
93 """
94- new_params = parameters.copy()
95+ new_params = querydict.copy()
96+
97+ #apply default values for keys that have not been set
98 for key in self.default_parameters:
99- #apply default parameters for keys that have not been set
100- if not key in new_params: new_params[key] = self.default_parameters[key]
101+ value = self.default_parameters[key]
102+ if isinstance(value,list):
103+ new_params.setlistdefault(key, value)
104+ else:
105+ new_params.setdefault(key, value)
106
107 for key in new_params:
108 filter_object = self.find(key)
109 if filter_object:
110- filter_object.set_value(new_params[key])
111+ filter_object.set_value(new_params.getlist(key))
112
113 def get_url_with_parameters(self, parameters): #final
114 """
115@@ -94,5 +99,5 @@
116 @param parameters: a dictionary of new parameters
117 @return: the current url with the given parameters added to the query string
118 """
119- return current_url_with_parameters(self.request, parameters)
120+ return update_url_with_parameters(self.request, parameters)
121
122
123=== modified file 'harvest/filters/filters.py'
124--- harvest/filters/filters.py 2010-07-14 19:33:14 +0000
125+++ harvest/filters/filters.py 2010-07-16 05:55:54 +0000
126@@ -86,7 +86,7 @@
127 def get_full_name(self): #final
128 """
129 Returns the filter's full name, which should make sense from anywhere
130- in the application. This name is in the format parent:child:child.
131+ in the application. This name is in the format parent-child-child.
132 All objects in the system need to be created by the time this
133 method is called.
134 @return: the filter's full name, which is a string
135@@ -95,7 +95,7 @@
136 full_name = self.get_id()
137 container = self.get_container()
138 if isinstance(container, Filter):
139- full_name = "%s:%s" % (container.get_full_name(), full_name)
140+ full_name = "%s.%s" % (container.get_full_name(), full_name)
141 self.full_name = full_name
142 return self.full_name
143
144@@ -103,7 +103,7 @@
145 """
146 Extend this to take a value passed down from the top, probably
147 from FilterSystem.update_from_http, and do something with it.
148- @param value: a new value, as a string
149+ @param value: a new value, wrapped in a list
150 """
151 raise NotImplementedError(self.set_value)
152
153@@ -113,6 +113,15 @@
154 """
155 raise NotImplementedError(self.get_value)
156
157+ def serialize_value(self, value): #abstract
158+ """
159+ The inverse of set_value. Returns the given value as a string or
160+ a list to add to an HTTP query string.
161+ @param value: the value to serialize, in a format native to this Filter (see get_value)
162+ @return: a unicode string formatted for set_value
163+ """
164+ raise NotImplementedError(self.serialize_value)
165+
166 def serialize(self, value = None): #final
167 """
168 Creates a dictionary of parameters to describe this object, either
169@@ -126,15 +135,6 @@
170 value_str = self.serialize_value(value)
171 return {key : value_str}
172
173- def serialize_value(self, value): #abstract
174- """
175- The inverse of set_value. Returns the given value as a string that could
176- be added to an HTTP query string.
177- @param value: the value to serialize, in a format native to this Filter (see get_value)
178- @return: a unicode string formatted for set_value
179- """
180- raise NotImplementedError(self.serialize_value)
181-
182 def process_queryset(self, queryset): #abstract
183 """
184 Extend this to manipulate a given queryset and then return it.
185@@ -216,13 +216,13 @@
186 self.input_str = ""
187
188 def set_value(self, value): #overrides Filter
189- self.input_str = value
190+ self.input_str = value[0]
191
192 def get_value(self): #overrides Filter
193 return self.input_str
194
195- def serialize_value(self, value): #overrides Filter
196- return value
197+ def serialize_value(self, value):
198+ return list(value)
199
200 def render_html_value(self, **kwargs):
201 field_value = self.get_value()
202@@ -235,7 +235,7 @@
203 """
204 Holds a set of strings, with no repetition.
205
206- Serialized as a comma-separated list ("dog,cat,horse,mouse")
207+ Serialized as a list for QueryDict
208 """
209
210 html_class = 'filter setfilter'
211@@ -245,13 +245,16 @@
212 self.selected_set = set()
213
214 def set_value(self, value): #overrides Filter
215- self.selected_set = set([s for s in value.split(",") if self.id_allowed(s)])
216+ self.selected_set = set([s for s in value if self.id_allowed(s)])
217
218 def get_value(self): #overrides Filter
219 return self.selected_set.copy()
220
221 def serialize_value(self, value): #overrides Filter
222- return ",".join(value)
223+ value_list = list(value)
224+ if len(value_list) == 0:
225+ value_list = ['']
226+ return list(value_list)
227
228 def get_value_with_selection(self, item_id): #final
229 """
230@@ -281,7 +284,7 @@
231 SetFilter. In that set, any items which refer to choices that do not exist
232 are ignored.
233
234- Serialized as a comma-separated list, like SetFilter.
235+ Serialized as a list, like SetFilter.
236 """
237
238 html_class = 'filter setfilter choicefilter'
239@@ -333,8 +336,6 @@
240 rules of ChoiceFilter.
241
242 The do_queryset method combines the output from all selected Filters.
243-
244- Serialized as a comma-separated list, like ChoiceFilter.
245 """
246
247 html_class = 'filter setfilter choicefilter filtergroup'
248
249=== modified file 'harvest/media/js/harvest.js'
250--- harvest/media/js/harvest.js 2010-07-14 23:42:34 +0000
251+++ harvest/media/js/harvest.js 2010-07-16 05:55:54 +0000
252@@ -5,13 +5,35 @@
253 */
254
255
256-// Array Remove - By John Resig (MIT Licensed)
257+/* Array Remove - By John Resig (MIT Licensed) */
258+/* <http://ejohn.org/blog/javascript-array-remove/> */
259 Array.prototype.remove = function(from, to) {
260- var rest = this.slice((to || from) + 1 || this.length);
261- this.length = from < 0 ? this.length + from : from;
262- return this.push.apply(this, rest);
263-};
264-
265+ var rest = this.slice((to || from) + 1 || this.length);
266+ this.length = from < 0 ? this.length + from : from;
267+ return this.push.apply(this, rest);
268+};
269+
270+
271+
272+Array.prototype.compare = function(to) {
273+ if (this.length != to.length) { return false; }
274+ var a = this.sort(),
275+ b = to.sort();
276+ for (var i = 0; to[i]; i++) {
277+ if (a[i] !== b[i]) {
278+ return false;
279+ }
280+ }
281+ return true;
282+};
283+
284+
285+/* jQuery settings */
286+
287+jQuery.ajaxSettings.traditional = true; /* for compatibility with Django */
288+
289+
290+/* Globals and constants */
291
292 var harvest_results;
293 var harvest_filters_list;
294@@ -19,6 +41,7 @@
295 var MEDIA_PATH = '/media/' /* we should get this from Django somehow */
296
297
298+
299 function Filter (dom_node) {
300 /* Attaches to a .filter object in the document. Handles events
301 specific to each type of filter and provides some extra data
302@@ -40,8 +63,9 @@
303
304
305 this.get_value_serialized = function () {
306- /* Returns the value of this filter, serialized in the same
307- format we would use in Harvest's Django end.
308+ /* Returns the value of this filter, ready for the data input of
309+ jQuery's ajax function so it reaches Harvest's Django portion
310+ in the format it expects.
311 Does nothing by default. Implement this in each
312 "class"-specific initializer below. */
313
314@@ -125,14 +149,14 @@
315
316
317 this.get_value_serialized = function () {
318- var item_ids = Array();
319+ var item_ids = ['']; /* empty default so this overrides defaults in server when unset */
320 /* we add the data-item-id attribute of all selected items */
321 list_items.each(function () {
322 if ($(this).attr('data-selected') != null) {
323 item_ids.push($(this).attr('data-item-id'));
324 }
325 } );
326- return item_ids.join(',');
327+ return item_ids;
328 }
329
330
331@@ -430,17 +454,20 @@
332 /* dereferences existing Packages. We expect that they'll be cleaned up by the GC */
333 this.packages = {};
334 this.expanded_pkgs = [];
335+ /* With this line, new results will appear with currently expanded
336+ packages already expanded. Without it, more queries are run
337+ later to do the same.
338+ TODO: is the current approach better in terms of user experience and performance? */
339+ this.update_current_query('expand', this.expanded_pkgs);
340
341 var pkg_expanded_cb = function (pkg) {
342 results.expanded_pkgs.push(pkg.id);
343- results.update_current_query('expand_pkg', results.expanded_pkgs.join(','));
344 }
345
346 var pkg_collapsed_cb = function (pkg) {
347 var index = results.expanded_pkgs.indexOf(pkg.id);
348 if ( index > -1 ) {
349 results.expanded_pkgs.remove(index);
350- results.update_current_query('expand_pkg', results.expanded_pkgs.join(','));
351 }
352 }
353
354@@ -464,8 +491,6 @@
355 }
356 }
357 });
358-
359- this.update_current_query('expand_pkg', this.expanded_pkgs.join(','));
360 }
361
362
363@@ -564,10 +589,11 @@
364 /* Adds the given key/value pair to future_query. If it is
365 necessary, a timer is started to get the results for
366 future_query. */
367- if ( this.current_query[key] == value ) {
368+ current_value = this.current_query[key];
369+ if ( this.current_query[key] == value ||
370+ current_value instanceof Array && value instanceof Array && value.compare(current_value) ) {
371 /* the user has reverted a change; value is the same as before */
372 delete this.future_query[key];
373-
374 if ( $.isEmptyObject(this.future_query) ) {
375 /* no point getting results, so stop and wait for new input. */
376 this.stop_countdown();
377
378=== modified file 'harvest/opportunities/filters.py'
379--- harvest/opportunities/filters.py 2010-07-05 17:18:01 +0000
380+++ harvest/opportunities/filters.py 2010-07-16 05:55:54 +0000
381@@ -48,9 +48,9 @@
382 ], name='Opportunities' )
383 ],
384 default_parameters = { 'pkg' : 'set',
385- 'pkg:set' : 'ubuntu-desktop',
386+ 'pkg.set' : ['ubuntu-desktop'],
387 'opp' : 'list',
388- 'opp:list' : 'bitesize' }
389+ 'opp.list' : ['bitesize'] }
390 #TODO: change to no defaults, detect that case in view and templates and provide helpful instructions in the results pane.
391 )
392
393
394=== modified file 'harvest/opportunities/wrappers.py'
395--- harvest/opportunities/wrappers.py 2010-07-14 23:42:34 +0000
396+++ harvest/opportunities/wrappers.py 2010-07-16 05:55:54 +0000
397@@ -1,5 +1,5 @@
398 from django.db.models import Count
399-from harvest.common.url_tools import current_url_with_parameters
400+from harvest.common.url_tools import update_url_with_parameters
401
402 class PackageWrapper(object):
403 """
404@@ -17,8 +17,9 @@
405 return self.package
406
407 def get_expand_toggle_url(self):
408- parameter = {'expand_pkg' : self.package.id}
409- url = current_url_with_parameters(self.request, parameter)
410+ parameter = {'expand' : ''}
411+ if ( not self.expanded ): parameter['expand'] = self.package.id
412+ url = update_url_with_parameters(self.request, parameter)
413 return url
414
415 #FIXME: get_visible_opportunities and get_hidden_opportunities feel
416@@ -48,10 +49,8 @@
417 """
418
419 def __init__(self, request, packages_list, opportunities_list):
420- expand_list = list() #list of packages to show in detail
421- if 'expand_pkg' in request.GET:
422- expand_list = request.GET['expand_pkg'].split(',')
423- expand_list = [int(i) for i in expand_list if i.isdigit()]
424+ expand_list = request.GET.getlist('expand') #list of packages to show in detail
425+ expand_list = [int(i) for i in expand_list if i.isdigit()] #make sure these are all numbers (package ids)
426
427 related_packages = set(opportunities_list.values_list('sourcepackage', flat=True))
428

Subscribers

People subscribed via source and target branches

to all changes: