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

[Dylan McCall]

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

[lp:~dylanmccall/harvest/querystring-cleanup]

229. By Dylan McCall on 2010-07-16

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!)

[Daniel Holbach (dholbach)]

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

review: Approve

=== modified file 'harvest/common/url_tools.py'
--- harvest/common/url_tools.py	2010-06-24 06:53:17 +0000
+++ harvest/common/url_tools.py	2010-07-16 05:55:54 +0000
@@ -1,32 +1,20 @@
-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)
+def update_url_with_parameters(request, new_parameters, url=None):
+    """
+    Returns a URL (the current one by default) with some new querystring
+    parameters, based on the current querystring and the given
+    dictionary of parameters. The dictionary can contain list values as
+    well as strings and integers. 
+    @param request: the current HttpRequest object
+    @param new_parameters: a dictionary of new parameters
+    @param url: a url to use instead of the current one
+    """
+    if url == None: url = request.path
+    querydict = request.GET.copy()
+    for k in new_parameters:
+        v = new_parameters[k]
+        if isinstance(v,list):
+            querydict.setlist(k, v)
         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)
+            querydict[k] = v
+    return "%s?%s" % ( url, querydict.urlencode() )
 

=== modified file 'harvest/filters/containers.py'
--- harvest/filters/containers.py	2010-07-04 05:43:53 +0000
+++ harvest/filters/containers.py	2010-07-16 05:55:54 +0000
@@ -1,4 +1,4 @@
-from harvest.common.url_tools import current_url_with_parameters
+from harvest.common.url_tools import update_url_with_parameters
 from harvest.common.ordereddict import OrderedDict #while we wait for Python 2.7 :)
 
 class FilterContainer(object): #abstract
@@ -27,12 +27,12 @@
     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.
+        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)
+        name_bits = full_name.split('.',1)
         
         #the first bit is the one this instance stores
         if name_bits[0] in self.filters_dict:
@@ -70,22 +70,27 @@
         """
         self.request = request
         #this contains parameters that aren't relevant to the FilterSystem
-        self._set_parameters(request.GET)
+        self.set_parameters(request.GET)
     
-    def _set_parameters(self, parameters): #final
+    def set_parameters(self, querydict): #final
         """
         Updates the state of all filters based on given parameters.
-        @param parameters: dictionary of parameters, for example from request.GET
+        @param querydict: dictionary of parameters from request.GET
         """
-        new_params = parameters.copy()
+        new_params = querydict.copy()
+        
+        #apply default values for keys that have not been set
         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]
+            value = self.default_parameters[key]
+            if isinstance(value,list):
+                new_params.setlistdefault(key, value)
+            else:
+                new_params.setdefault(key, value)
         
         for key in new_params:
             filter_object = self.find(key)
             if filter_object:
-                filter_object.set_value(new_params[key])
+                filter_object.set_value(new_params.getlist(key))
     
     def get_url_with_parameters(self, parameters): #final
         """
@@ -94,5 +99,5 @@
         @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)
+        return update_url_with_parameters(self.request, parameters)
 

=== modified file 'harvest/filters/filters.py'
--- harvest/filters/filters.py	2010-07-14 19:33:14 +0000
+++ harvest/filters/filters.py	2010-07-16 05:55:54 +0000
@@ -86,7 +86,7 @@
     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.
+        in the application. This name is in the format parent-child-child.
         All objects in the system need to be created by the time this
         method is called.
         @return: the filter's full name, which is a string
@@ -95,7 +95,7 @@
             full_name = self.get_id()
             container = self.get_container()
             if isinstance(container, Filter):
-                full_name = "%s:%s" % (container.get_full_name(), full_name)
+                full_name = "%s.%s" % (container.get_full_name(), full_name)
             self.full_name = full_name
         return self.full_name
     
@@ -103,7 +103,7 @@
         """
         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
+        @param value: a new value, wrapped in a list
         """
         raise NotImplementedError(self.set_value)
     
@@ -113,6 +113,15 @@
         """
         raise NotImplementedError(self.get_value)
     
+    def serialize_value(self, value): #abstract
+        """
+        The inverse of set_value. Returns the given value as a string or
+        a list to add 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
@@ -126,15 +135,6 @@
         value_str = self.serialize_value(value)
         return {key : value_str}
     
-    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 process_queryset(self, queryset): #abstract
         """
         Extend this to manipulate a given queryset and then return it.
@@ -216,13 +216,13 @@
         self.input_str = ""
     
     def set_value(self, value): #overrides Filter
-        self.input_str = value
+        self.input_str = value[0]
     
     def get_value(self): #overrides Filter
         return self.input_str
     
-    def serialize_value(self, value): #overrides Filter
-        return value
+    def serialize_value(self, value):
+        return list(value)
     
     def render_html_value(self, **kwargs):
         field_value = self.get_value()
@@ -235,7 +235,7 @@
     """
     Holds a set of strings, with no repetition.
     
-    Serialized as a comma-separated list ("dog,cat,horse,mouse")
+    Serialized as a list for QueryDict 
     """
     
     html_class = 'filter setfilter'
@@ -245,13 +245,16 @@
         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)])
+        self.selected_set = set([s for s in value 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)
+        value_list = list(value)
+        if len(value_list) == 0:
+            value_list = ['']
+        return list(value_list)
     
     def get_value_with_selection(self, item_id): #final
         """
@@ -281,7 +284,7 @@
     SetFilter. In that set, any items which refer to choices that do not exist
     are ignored.
     
-    Serialized as a comma-separated list, like SetFilter.
+    Serialized as a list, like SetFilter.
     """
     
     html_class = 'filter setfilter choicefilter'
@@ -333,8 +336,6 @@
     rules of ChoiceFilter.
     
     The do_queryset method combines the output from all selected Filters.
-    
-    Serialized as a comma-separated list, like ChoiceFilter.
     """
     
     html_class = 'filter setfilter choicefilter filtergroup'

=== modified file 'harvest/media/js/harvest.js'
--- harvest/media/js/harvest.js	2010-07-14 23:42:34 +0000
+++ harvest/media/js/harvest.js	2010-07-16 05:55:54 +0000
@@ -5,13 +5,35 @@
 */
 
 
-// Array Remove - By John Resig (MIT Licensed)
+/* Array Remove - By John Resig (MIT Licensed) */
+/* <http://ejohn.org/blog/javascript-array-remove/> */
 Array.prototype.remove = function(from, to) {
-  var rest = this.slice((to || from) + 1 || this.length);
-  this.length = from < 0 ? this.length + from : from;
-  return this.push.apply(this, rest);
-};
-
+	var rest = this.slice((to || from) + 1 || this.length);
+	this.length = from < 0 ? this.length + from : from;
+	return this.push.apply(this, rest);
+};
+
+
+
+Array.prototype.compare = function(to) {
+	if (this.length != to.length) { return false; }
+	var a = this.sort(),
+		b = to.sort();
+	for (var i = 0; to[i]; i++) {
+		if (a[i] !== b[i]) { 
+			return false;
+		}
+	}
+	return true;
+};
+
+
+/* jQuery settings */
+
+jQuery.ajaxSettings.traditional = true; /* for compatibility with Django */
+
+
+/* Globals and constants */
 
 var harvest_results;
 var harvest_filters_list;
@@ -19,6 +41,7 @@
 var MEDIA_PATH = '/media/' /* we should get this from Django somehow */
 
 
+
 function Filter (dom_node) {
 	/* Attaches to a .filter object in the document. Handles events
 	   specific to each type of filter and provides some extra data
@@ -40,8 +63,9 @@
 	
 	
 	this.get_value_serialized = function () {
-		/* Returns the value of this filter, serialized in the same
-		   format we would use in Harvest's Django end.
+		/* Returns the value of this filter, ready for the data input of 
+		   jQuery's ajax function so it reaches Harvest's Django portion
+		   in the format it expects.
 		   Does nothing by default. Implement this in each
 		   "class"-specific initializer below. */
 		
@@ -125,14 +149,14 @@
 		
 		
 		this.get_value_serialized = function () {
-			var item_ids = Array();
+			var item_ids = ['']; /* empty default so this overrides defaults in server when unset */
 			/* we add the data-item-id attribute of all selected items */
 			list_items.each(function () {
 				if ($(this).attr('data-selected') != null) {
 					item_ids.push($(this).attr('data-item-id'));
 				}
 			} );
-			return item_ids.join(',');
+			return item_ids;
 		}
 		
 		
@@ -430,17 +454,20 @@
 		/* dereferences existing Packages. We expect that they'll be cleaned up by the GC */
 		this.packages = {};
 		this.expanded_pkgs = [];
+		/* With this line, new results will appear with currently expanded
+		   packages already expanded. Without it, more queries are run
+		   later to do the same.
+		   TODO: is the current approach better in terms of user experience and performance? */
+		this.update_current_query('expand', this.expanded_pkgs);
 		
 		var pkg_expanded_cb = function (pkg) {
 			results.expanded_pkgs.push(pkg.id);
-			results.update_current_query('expand_pkg', results.expanded_pkgs.join(','));
 		}
 		
 		var pkg_collapsed_cb = function (pkg) {
 			var index = results.expanded_pkgs.indexOf(pkg.id);
 			if ( index > -1 ) {
 				results.expanded_pkgs.remove(index);
-				results.update_current_query('expand_pkg', results.expanded_pkgs.join(','));
 			}
 		}
 		
@@ -464,8 +491,6 @@
 				}
 			}
 		});
-		
-		this.update_current_query('expand_pkg', this.expanded_pkgs.join(','));
 	}
 	
 	
@@ -564,10 +589,11 @@
 		/* Adds the given key/value pair to future_query. If it is
 		   necessary, a timer is started to get the results for
 		   future_query. */
-		if ( this.current_query[key] == value ) {
+		current_value = this.current_query[key];
+		if ( this.current_query[key] == value ||
+		     current_value instanceof Array && value instanceof Array && value.compare(current_value) ) {
 			/* the user has reverted a change; value is the same as before */
 			delete this.future_query[key];
-			
 			if ( $.isEmptyObject(this.future_query) ) {
 				/* no point getting results, so stop and wait for new input. */
 				this.stop_countdown();

=== modified file 'harvest/opportunities/filters.py'
--- harvest/opportunities/filters.py	2010-07-05 17:18:01 +0000
+++ harvest/opportunities/filters.py	2010-07-16 05:55:54 +0000
@@ -48,9 +48,9 @@
                 ], name='Opportunities' )
             ],
             default_parameters = { 'pkg' : 'set',
-                                   'pkg:set' : 'ubuntu-desktop',
+                                   'pkg.set' : ['ubuntu-desktop'],
                                    'opp' : 'list',
-                                   'opp:list' : 'bitesize' }
+                                   'opp.list' : ['bitesize'] }
             #TODO: change to no defaults, detect that case in view and templates and provide helpful instructions in the results pane.
         )
 

=== modified file 'harvest/opportunities/wrappers.py'
--- harvest/opportunities/wrappers.py	2010-07-14 23:42:34 +0000
+++ harvest/opportunities/wrappers.py	2010-07-16 05:55:54 +0000
@@ -1,5 +1,5 @@
 from django.db.models import Count
-from harvest.common.url_tools import current_url_with_parameters
+from harvest.common.url_tools import update_url_with_parameters
 
 class PackageWrapper(object):
     """
@@ -17,8 +17,9 @@
         return self.package
     
     def get_expand_toggle_url(self):
-        parameter = {'expand_pkg' : self.package.id}
-        url = current_url_with_parameters(self.request, parameter)
+        parameter = {'expand' : ''}
+        if ( not self.expanded ): parameter['expand'] = self.package.id
+        url = update_url_with_parameters(self.request, parameter)
         return url
     
     #FIXME: get_visible_opportunities and get_hidden_opportunities feel
@@ -48,10 +49,8 @@
     """
     
     def __init__(self, request, packages_list, opportunities_list):
-        expand_list = list() #list of packages to show in detail
-        if 'expand_pkg' in request.GET:
-            expand_list = request.GET['expand_pkg'].split(',')
-            expand_list = [int(i) for i in expand_list if i.isdigit()]
+        expand_list = request.GET.getlist('expand') #list of packages to show in detail
+        expand_list = [int(i) for i in expand_list if i.isdigit()] #make sure these are all numbers (package ids)
         
         related_packages = set(opportunities_list.values_list('sourcepackage', flat=True))