Skip to content

Commit

Permalink
[#362,bugfix][s]: remove remaining reference to routing for old datas…
Browse files Browse the repository at this point in the history
…tore (by removing api_data method in package controller).

The ultimate change was reasonably substantial - removal of an entire method on
a controller and associated template. It began simply with the need to
remove/update reference this routing call in api_data method on package
controller

    url = h.url_for('datastore_read', id=id, qualified=True)

I spent about half an hour trying to understand if this method was used at all.

I ultimately determined that we had a setup where clicking on the Data API
button in the resource view had 2 possible effect routes:

* Via href={url} => package.py/api_data method =>
  template/package/resource_api_data.html =>
  template/ajax_snippets/api_info.html
* Via JS intercept of click => api/snippets controller =>
  template/ajax_snippets/api_info.html

But there was a subtle difference: the first route (which is *not* normally
followed since JS is usally enabled) still uses the old datastore URL (this is
why I'd started this investigation!).

(Aside: this was hard to debug through the interface as JS is enabled and I
note we do not have resource view tests so the non AJAX route is not tested).

I first tried to correct this by setting the URL to the correct datastore API
but then discovered other parameters were missing - it seems the signature of
the call to ajax_snippets template has changed but that the relevant code in
package/resource_data_api.html has not been updated:

    {% snippet 'ajax_snippets/api_info.html', datastore_root_url=datastore_root_url, embedded=true %}

Rather than continue fixing I decided at this point to remove this non-AJAX
fallback code in its entirety.

Comment: this was a subtle bug that has crept in because there are now 2 ways to do
something and one of these routes (the non-AJAX route) is almost never used ...

I wonder whether it is necessary always to have both options (AJAX and
non-AJAX) as it likely to multiply code complexity and, hence, these sort of bugs.
  • Loading branch information
rufuspollock committed Feb 16, 2013
1 parent 61aacc8 commit 27361b8
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 23 deletions.
4 changes: 0 additions & 4 deletions ckan/controllers/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -1217,10 +1217,6 @@ def resource_download(self, id, resource_id):
abort(404, _('No download is available'))
redirect(rsc['url'])

def api_data(self, id=None):
url = h.url_for('datastore_read', id=id, qualified=True)
return render('package/resource_api_data.html', {'datastore_root_url': url})

def follow(self, id):
'''Start following this dataset.'''
context = {'model': model,
Expand Down
18 changes: 0 additions & 18 deletions ckan/templates/package/resource_api_data.html

This file was deleted.

2 changes: 1 addition & 1 deletion ckan/templates/package/snippets/data_api_button.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#}
{% if true or resource.webstore_url %}
{% set loading_text = _('Loading...') %}
<a class="btn btn-success" href="{% url_for controller='package', action='api_data', id=resource.id %}" data-module="api-info" data-module-template="{% url_for controller='api', action='snippet', ver=1, snippet_path='api_info.html', datastore_root_url=datastore_root_url, resource_id=resource.id %}" data-loading-text="{{ loading_text }}"><i class="icon-beaker icon-large"></i> Data API</a>
<a class="btn btn-success" href="#" data-module="api-info" data-module-template="{% url_for controller='api', action='snippet', ver=1, snippet_path='api_info.html', datastore_root_url=datastore_root_url, resource_id=resource.id %}" data-loading-text="{{ loading_text }}"><i class="icon-beaker icon-large"></i> Data API</a>
{% else %}
<a class="btn disabled" rel="tooltip" title="Data API is unavailable for this resource as DataStore is disabled"><i class="icon-beaker icon-large"></i> Data API</a>
{% endif %}

0 comments on commit 27361b8

Please sign in to comment.