From 460615d51a0c27c2a860ad15f7a1a651c952a1bf Mon Sep 17 00:00:00 2001 From: John Glover Date: Mon, 19 Dec 2011 17:12:10 +0000 Subject: [PATCH 1/4] [model,lib,tests]: Added (much needed) tests for calculation of metadata_modified. Fixed up float breakage. If it cannot calculate metadata_modified it returns None instead of now() - this should never happen and if it does it would be good to find out why as its probably an error. Optimised package dictization to avoid calculation metadata_modified and created twice. --- ckan/lib/dictization/model_dictize.py | 10 +++-- ckan/model/group.py | 2 +- ckan/model/package.py | 23 ++++------ ckan/tests/functional/api/test_action.py | 45 +++++++++++++++++++ ckan/tests/models/test_package.py | 55 +++++++++++++++++++++++- doc/apiv3.rst | 2 +- 6 files changed, 115 insertions(+), 22 deletions(-) diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index 675d47f1712..356c8277845 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -263,10 +263,12 @@ def package_to_api1(pkg, context): site_url = config.get('ckan.site_url', None) if site_url: dictized['ckan_url'] = '%s/dataset/%s' % (site_url, pkg.name) - dictized['metadata_modified'] = pkg.metadata_modified.isoformat() \ - if pkg.metadata_modified else None - dictized['metadata_created'] = pkg.metadata_created.isoformat() \ - if pkg.metadata_created else None + metadata_modified = pkg.metadata_modified + dictized['metadata_modified'] = metadata_modified.isoformat() \ + if metadata_modified else None + metadata_created = pkg.metadata_created + dictized['metadata_created'] = metadata_created.isoformat() \ + if metadata_created else None subjects = dictized.pop("relationships_as_subject") objects = dictized.pop("relationships_as_object") diff --git a/ckan/model/group.py b/ckan/model/group.py index 24aa84e21b6..5fbec961608 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -12,7 +12,7 @@ __all__ = ['group_table', 'Group', 'package_revision_table', 'PackageGroup', 'GroupRevision', 'PackageGroupRevision', - 'package_group_revision_table'] + 'package_group_table', 'package_group_revision_table'] package_group_table = Table('package_group', metadata, Column('id', UnicodeText, primary_key=True, default=make_uuid), diff --git a/ckan/model/package.py b/ckan/model/package.py index d854bca097d..01239ad0028 100644 --- a/ckan/model/package.py +++ b/ckan/model/package.py @@ -454,11 +454,11 @@ def diff(self, to_revision=None, from_revision=None): results[key] = value_diff return results - @staticmethod - def last_modified(*av): + @property + def metadata_modified(self): """ - Return most recent timestamp for a package revision, with optionally - extra where clause. + Return most recent timestamp for revisions related to this package. + NB Excludes changes to the package's groups """ from ckan import model where = [] @@ -490,19 +490,12 @@ def last_modified(*av): result = conn.execute(query).fetchone() if result: result_datetime = iso_date_to_datetime_for_sqlite(result[0]) - timestamp = result_datetime.utctimetuple() + timestamp_without_usecs = result_datetime.utctimetuple() usecs = float(result_datetime.microsecond) / 1e6 - else: - timestamp, usecs = gmtime(), 0 - # use timegm instead of mktime, because we don't want it localised - return timegm(timestamp) + usecs + # use timegm instead of mktime, because we don't want it localised + timestamp_float = timegm(timestamp_without_usecs) + usecs + return datetime.datetime.utcfromtimestamp(timestamp_float) - @property - def metadata_modified(self): - import ckan.model as model - epochtime = self.last_modified(model.package_table.c.id==self.id) - return datetime.datetime.utcfromtimestamp(epochtime) - @property def metadata_created(self): import ckan.model as model diff --git a/ckan/tests/functional/api/test_action.py b/ckan/tests/functional/api/test_action.py index 33363cca1f0..b96dd1fc0ce 100644 --- a/ckan/tests/functional/api/test_action.py +++ b/ckan/tests/functional/api/test_action.py @@ -934,3 +934,48 @@ def test_3_bad_param(self): status=400) assert '"message": "Search Query is invalid:' in res.body, res.body assert '"Invalid search parameters: [u\'weird_param\']' in res.body, res.body + + def test_4_sort_by_metadata_modified(self): + search_params = '%s=1' % json.dumps({ + 'q': '*:*', + 'fl': 'name, metadata_modified', + 'sort': u'metadata_modified desc' + }) + + # modify warandpeace, check that it is the first search result + rev = model.repo.new_revision() + pkg = model.Package.get('warandpeace') + pkg.title = "War and Peace [UPDATED]" + model.repo.commit_and_remove() + + res = self.app.post('/api/action/package_search', params=search_params) + result = json.loads(res.body)['result'] + result_names = [r['name'] for r in result['results']] + assert result_names == ['warandpeace', 'annakarenina'], result_names + + # modify annakarenina, check that it is the first search result + rev = model.repo.new_revision() + pkg = model.Package.get('annakarenina') + pkg.title = "A Novel By Tolstoy [UPDATED]" + model.repo.commit_and_remove() + + res = self.app.post('/api/action/package_search', params=search_params) + result = json.loads(res.body)['result'] + result_names = [r['name'] for r in result['results']] + assert result_names == ['annakarenina', 'warandpeace'], result_names + + # add a tag to warandpeace, check that it is the first result + pkg = model.Package.get('warandpeace') + pkg_params = '%s=1' % json.dumps({'id': pkg.id}) + res = self.app.post('/api/action/package_show', params=pkg_params) + pkg_dict = json.loads(res.body)['result'] + pkg_dict['tags'].append({'name': 'new-tag'}) + pkg_params = '%s=1' % json.dumps(pkg_dict) + res = self.app.post('/api/action/package_update', params=pkg_params, + extra_environ={'Authorization': 'tester'}) + + res = self.app.post('/api/action/package_search', params=search_params) + result = json.loads(res.body)['result'] + result_names = [r['name'] for r in result['results']] + assert result_names == ['warandpeace', 'annakarenina'], result_names + diff --git a/ckan/tests/models/test_package.py b/ckan/tests/models/test_package.py index dbc682b2290..cd0de947603 100644 --- a/ckan/tests/models/test_package.py +++ b/ckan/tests/models/test_package.py @@ -128,7 +128,7 @@ def test_metadata_created_and_modified(self): assert package.metadata_modified == created_timestamp,\ (package.metadata_modified, created_timestamp) - # update the package it + # update the package rev = model.repo.new_revision() package = model.Package.by_name(name) package.title = "test_metadata_new_title" @@ -138,6 +138,59 @@ def test_metadata_created_and_modified(self): package = model.Package.by_name(name) assert package.metadata_created == created_timestamp assert package.metadata_modified == modified_timestamp + last_modified_timestamp = modified_timestamp + + # update a package's tag + rev = model.repo.new_revision() + package = model.Package.by_name(name) + package.add_tag_by_name('new-tag') + modified_timestamp = model.Session().revision.timestamp + assert modified_timestamp != last_modified_timestamp + model.repo.commit_and_remove() + + package = model.Package.by_name(name) + assert package.metadata_created == created_timestamp + assert package.metadata_modified == modified_timestamp + last_modified_timestamp = modified_timestamp + + # update a package's extra + rev = model.repo.new_revision() + package = model.Package.by_name(name) + package.extras['new-key'] = 'value' + modified_timestamp = model.Session().revision.timestamp + assert modified_timestamp != last_modified_timestamp + model.repo.commit_and_remove() + + package = model.Package.by_name(name) + assert package.metadata_created == created_timestamp + assert package.metadata_modified == modified_timestamp + last_modified_timestamp = modified_timestamp + + # update a package's relationship + rev = model.repo.new_revision() + package = model.Package.by_name(name) + anna = model.Package.by_name(u'annakarenina') + package.add_relationship(u'child_of', anna) + modified_timestamp = model.Session().revision.timestamp + assert modified_timestamp != last_modified_timestamp + model.repo.commit_and_remove() + + package = model.Package.by_name(name) + assert package.metadata_created == created_timestamp + assert package.metadata_modified == modified_timestamp + last_modified_timestamp = modified_timestamp + + # update a package's group - NB no change this time + rev = model.repo.new_revision() + group = model.Group.by_name('roger') + group.add_package_by_name(name) + modified_timestamp = model.Session().revision.timestamp + assert modified_timestamp != last_modified_timestamp + model.repo.commit_and_remove() + + package = model.Package.by_name(name) + assert package.metadata_created == created_timestamp + assert package.metadata_modified == last_modified_timestamp # no change class TestPackageWithTags: diff --git a/doc/apiv3.rst b/doc/apiv3.rst index 92acef984be..c9d290c435b 100644 --- a/doc/apiv3.rst +++ b/doc/apiv3.rst @@ -369,7 +369,7 @@ These parameters are all the standard SOLR syntax (in contrast to the syntax use | | || fl=* | | +-----------------------+---------------+----------------------------------+----------------------------------+ | sort | field name, || sort=name asc | Changes the sort order according | -| | asc / dec | | to the field and direction given.| +| | asc / dec || sort=metadata_modified asc | to the field and direction given.| | | | | default: score desc, name asc | +-----------------------+---------------+----------------------------------+----------------------------------+ | start, rows | result-int | start=40&rows=20 | Pagination options. Start is the | From d80654e8c836b5ea78a03cb164ef71c2397e85f6 Mon Sep 17 00:00:00 2001 From: John Glover Date: Mon, 19 Dec 2011 16:38:18 +0000 Subject: [PATCH 2/4] [xs, pagination] Bug fix for #1543. Refactor code added in #1501 so that the search controller pagination can still use it's own url generator function. Users, groups and revisions use the new h.pager_url function. --- ckan/controllers/group.py | 2 ++ ckan/controllers/revision.py | 1 + ckan/controllers/user.py | 3 ++- ckan/lib/helpers.py | 20 ++++++++------------ ckan/tests/functional/test_pagination.py | 12 +++++++++++- 5 files changed, 24 insertions(+), 14 deletions(-) diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index ac292c8dcaa..5a65a20cdfa 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -62,6 +62,7 @@ def index(self): c.page = Page( collection=results, page=request.params.get('page', 1), + url=h.pager_url, items_per_page=20 ) return render('group/index.html') @@ -99,6 +100,7 @@ def read(self, id): c.page = Page( collection=results, page=request.params.get('page', 1), + url=h.pager_url, items_per_page=50 ) diff --git a/ckan/controllers/revision.py b/ckan/controllers/revision.py index b111572e2f2..e43d2bb6e68 100644 --- a/ckan/controllers/revision.py +++ b/ckan/controllers/revision.py @@ -116,6 +116,7 @@ def list(self): c.page = Page( collection=query, page=request.params.get('page', 1), + url=h.pager_url, items_per_page=20 ) return render('revision/list.html') diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index 6a96c93ccfa..59d10384476 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -76,9 +76,10 @@ def index(self): c.page = h.Page( collection=users_list, page=page, + url=h.pager_url, item_count=users_list.count(), items_per_page=LIMIT - ) + ) return render('user/list.html') def read(self, id=None): diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 3030cf68ac4..5396442fa20 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -236,29 +236,25 @@ def linked_gravatar(email_hash, size=100, default="mm"): def gravatar(email_hash, size=100, default="mm"): return literal('' % (email_hash, size, default)) +def pager_url(page, partial=None, **kwargs): + routes_dict = url.environ['pylons.routes_dict'] + kwargs['controller'] = routes_dict['controller'] + kwargs['action'] = routes_dict['action'] + if routes_dict.get('id'): + kwargs['id'] = routes_dict['id'] + kwargs['page'] = page + return url(**kwargs) class Page(paginate.Page): - - def _page_url(self, page, partial=None, **kwargs): - routes_dict = url.environ['pylons.routes_dict'] - kwargs['controller'] = routes_dict['controller'] - kwargs['action'] = routes_dict['action'] - if routes_dict.get('id'): - kwargs['id'] = routes_dict['id'] - kwargs['page'] = page - return url(**kwargs) - # Curry the pager method of the webhelpers.paginate.Page class, so we have # our custom layout set as default. def pager(self, *args, **kwargs): - self._url_generator = self._page_url kwargs.update( format=u"
$link_previous ~2~ $link_next
", symbol_previous=u'« Prev', symbol_next=u'Next »' ) return super(Page, self).pager(*args, **kwargs) - def render_datetime(datetime_): '''Render a datetime object or timestamp string as a pretty string (Y-m-d H:m). diff --git a/ckan/tests/functional/test_pagination.py b/ckan/tests/functional/test_pagination.py index b87f1fc6555..7f3aad43cbe 100644 --- a/ckan/tests/functional/test_pagination.py +++ b/ckan/tests/functional/test_pagination.py @@ -1,10 +1,11 @@ from ckan.lib.create_test_data import CreateTestData import ckan.model as model -from ckan.tests import TestController, url_for +from ckan.tests import TestController, url_for, setup_test_search_index class TestPagination(TestController): @classmethod def setup_class(cls): + setup_test_search_index() model.repo.init_db() # no. entities per page is hardcoded into the controllers, so @@ -30,6 +31,15 @@ def setup_class(cls): def teardown_class(self): model.repo.rebuild_db() + def test_search(self): + res = self.app.get(url_for(controller='package', action='search', q='groups:group_00')) + assert 'href="/dataset?q=groups%3Agroup_00&page=2"' in res + assert 'href="/dataset/package_19"' in res + + res = self.app.get(url_for(controller='package', action='search', q='groups:group_00', page=2)) + assert 'href="/dataset?q=groups%3Agroup_00&page=1"' in res + assert 'href="/dataset/package_20"' in res + def test_group_index(self): res = self.app.get(url_for(controller='group', action='index')) assert 'href="/group?page=2"' in res From 34f4d190e68d807be1ffa22a614d97cb1dd1bc59 Mon Sep 17 00:00:00 2001 From: John Glover Date: Mon, 19 Dec 2011 17:25:45 +0000 Subject: [PATCH 3/4] [xs, model] Fix merge error. This should have been in commit d80654e8c836b5ea78a03cb164ef71c2397e85f6 --- ckan/model/package.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ckan/model/package.py b/ckan/model/package.py index 01239ad0028..9ee79caedca 100644 --- a/ckan/model/package.py +++ b/ckan/model/package.py @@ -461,10 +461,7 @@ def metadata_modified(self): NB Excludes changes to the package's groups """ from ckan import model - where = [] - for arg in av: - if isinstance(arg, expression.ClauseElement) or isinstance(arg, basestring): - where.append(arg) + where = [model.package_table.c.id == self.id] where_clauses = [ and_(model.package_table.c.revision_id == model.revision_table.c.id, *where), and_(model.package_extra_table.c.package_id == model.package_table.c.id, From 9abdd0977b8f06fbd5ac621b17dcda7d5f389733 Mon Sep 17 00:00:00 2001 From: John Glover Date: Tue, 20 Dec 2011 10:17:50 +0000 Subject: [PATCH 4/4] [xs, 1451] Add css class names to resource urls for analytics (eg: ckanext-googleanalytics) --- ckan/templates/package/read_core.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/templates/package/read_core.html b/ckan/templates/package/read_core.html index 786aedc7b3e..8d14b4c3974 100644 --- a/ckan/templates/package/read_core.html +++ b/ckan/templates/package/read_core.html @@ -23,7 +23,7 @@

Resources

- + ${res.get('description') or res.get('name')}