From 83d2ef7bc43da4ec671016b5cb8429bb7f006d9d Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 21 Nov 2012 23:36:17 +0100 Subject: [PATCH] Remove duplicate methods to get a group's packages Refactor active_packages() and get_package_revisions(), both of which return a group's packages (but in slightly different ways), replace with just one method packages(). We now have just one way to get a group's packages, the packages() method of the group model. The group_package_show() action function calls it. --- ckan/controllers/group.py | 2 +- ckan/lib/dictization/model_dictize.py | 2 +- ckan/logic/action/get.py | 2 +- ckan/model/group.py | 63 ++++++++++++----------- ckan/tests/functional/test_group.py | 18 +++---- ckan/tests/lib/test_dictization_schema.py | 2 +- ckan/tests/models/test_group.py | 4 +- 7 files changed, 47 insertions(+), 46 deletions(-) diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index d6bffcc8be0..1867079529b 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -342,7 +342,7 @@ def _force_reindex(self, grp): appearing on the read page for the group (as they're connected via the group name)''' group = model.Group.get(grp['name']) - for dataset in group.active_packages().all(): + for dataset in group.packages(): search.rebuild(dataset.name) def _save_edit(self, id, context): diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index 8a08ff50b03..6926e4a3395 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -32,7 +32,7 @@ def group_list_dictize(obj_list, context, group_dict['display_name'] = obj.display_name group_dict['packages'] = \ - len(obj.active_packages(with_private=with_private).all()) + len(obj.packages(with_private=with_private)) if context.get('for_view'): for item in plugins.PluginImplementations( diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index 8bfb4f5095d..f1fc99b05a7 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -764,7 +764,7 @@ def group_package_show(context, data_dict): _check_access('group_show', context, data_dict) result = [] - for pkg_rev in group.get_package_revisions(limit=limit, + for pkg_rev in group.packages(limit=limit, return_query=context.get('return_query')): result.append(model_dictize.package_dictize(pkg_rev, context)) diff --git a/ckan/model/group.py b/ckan/model/group.py index 5692e48e0cc..35dfb73b461 100644 --- a/ckan/model/group.py +++ b/ckan/model/group.py @@ -155,47 +155,48 @@ def get_children_groups(self, type='group'): return [{"id":idf, "name": name, "title": title} for idf, name, title in results] - def active_packages(self, load_eager=True, with_private=False): - query = meta.Session.query(_package.Package).\ - filter_by(state=vdm.sqlalchemy.State.ACTIVE).\ - filter(group_table.c.id == self.id).\ - filter(member_table.c.state == 'active') + def packages(self, with_private=False, limit=None, + return_query=False): + '''Return this group's active and pending packages. - if not with_private: - query = query.filter(member_table.c.capacity == 'public') - - query = query.join(member_table, member_table.c.table_id == - _package.Package.id).\ - join(group_table, group_table.c.id == member_table.c.group_id) - - return query + Returns all packages in this group with VDM revision state ACTIVE or + PENDING. - def get_package_revisions(self, limit=None, return_query=False): - '''Return a group's packages. + :param with_private: if True, include the group's private packages + :type with_private: boolean :param limit: the maximum number of packages to return :type limit: int - :returns: a list of the group's packages, sorted by name - :rtype: list of PackageRevision objects + :param return_query: if True, return the SQLAlchemy query object + instead of the list of Packages resulting from the query + :type return_query: boolean + + :returns: a list of this group's packages + :rtype: list of ckan.model.package.Package objects ''' - import ckan.model as model - q = model.Session.query(model.PackageRevision) - q = q.filter(model.PackageRevision.state == 'active') - q = q.filter(model.PackageRevision.current == True) - q = q.join(model.Member, - model.Member.table_id == model.PackageRevision.id) - q = q.join(model.Group, model.Group.id == model.Member.group_id) - q = q.filter_by(id=self.id) - q = q.order_by(model.PackageRevision.name) + query = meta.Session.query(_package.Package) + query = query.filter( + or_(_package.Package.state == vdm.sqlalchemy.State.ACTIVE, + _package.Package.state == vdm.sqlalchemy.State.PENDING)) + query = query.filter(group_table.c.id == self.id) + + if not with_private: + query = query.filter(member_table.c.capacity == 'public') + + query = query.join(member_table, + member_table.c.table_id == _package.Package.id) + query = query.join(group_table, + group_table.c.id == member_table.c.group_id) + if limit is not None: - q = q.limit(limit) + query = query.limit(limit) + if return_query: - return q + return query else: - return q.all() - + return query.all() @classmethod def search_by_name_or_title(cls, text_query, group_type=None): @@ -212,7 +213,7 @@ def add_package_by_name(self, package_name): return package = _package.Package.by_name(package_name) assert package - if not package in self.active_packages().all(): + if not package in self.packages(): member = Member(group=self, table_id=package.id, table_name='package') meta.Session.add(member) diff --git a/ckan/tests/functional/test_group.py b/ckan/tests/functional/test_group.py index 7bfc04aaadd..57e2c62574c 100644 --- a/ckan/tests/functional/test_group.py +++ b/ckan/tests/functional/test_group.py @@ -187,7 +187,7 @@ def test_index(self): groupname = 'david' group = model.Group.by_name(unicode(groupname)) group_title = group.title - group_packages_count = len(group.active_packages().all()) + group_packages_count = len(group.packages()) group_description = group.description self.check_named_element(res, 'tr', group_title, group_packages_count, @@ -321,7 +321,7 @@ def test_2_edit(self): assert group.description == newdesc, group # now look at datasets - assert len(group.active_packages().all()) == 3 + assert len(group.packages()) == 3 def test_3_edit_form_has_new_package(self): # check for dataset in autocomplete @@ -369,7 +369,7 @@ def test_4_new_duplicate_package(self): # check package only added to the group once group = model.Group.by_name(group_name) - pkg_names = [pkg.name for pkg in group.active_packages().all()] + pkg_names = [pkg.name for pkg in group.packages()] assert_equal(pkg_names, [self.packagename]) def test_edit_plugin_hook(self): @@ -435,7 +435,7 @@ def update_group(res, name, with_pkg=True): # We have the datasets in the DB, but we should also see that many # on the group read page. - assert len(group.active_packages().all()) == 3 + assert len(group.packages()) == 3 offset = url_for(controller='group', action='read', id='newname') res = self.app.get(offset, status=200, @@ -543,9 +543,9 @@ def test_2_new(self): group = model.Group.by_name(group_name) assert group.title == group_title, group assert group.description == group_description, group - assert len(group.active_packages().all()) == 1 + assert len(group.packages()) == 1 pkg = model.Package.by_name(self.packagename) - assert group.active_packages().all() == [pkg] + assert group.packages() == [pkg] def test_3_new_duplicate_group(self): prefix = '' @@ -702,7 +702,7 @@ def test_index(self): groupname = 'david' group = model.Group.by_name(unicode(groupname)) group_title = group.title - group_packages_count = len(group.active_packages().all()) + group_packages_count = len(group.packages()) group_description = group.description self.check_named_element(res, 'tr', group_title, group_packages_count, @@ -820,7 +820,7 @@ def test_2_edit(self): assert group.description == newdesc, group # now look at datasets - assert len(group.active_packages().all()) == 3 + assert len(group.packages()) == 3 def test_3_edit_form_has_new_package(self): # check for dataset in autocomplete @@ -868,7 +868,7 @@ def test_4_new_duplicate_package(self): # check package only added to the group once group = model.Group.by_name(group_name) - pkg_names = [pkg.name for pkg in group.active_packages().all()] + pkg_names = [pkg.name for pkg in group.packages()] assert_equal(pkg_names, [self.packagename]) def test_edit_plugin_hook(self): diff --git a/ckan/tests/lib/test_dictization_schema.py b/ckan/tests/lib/test_dictization_schema.py index ad9fa290cf7..ef1391b98b8 100644 --- a/ckan/tests/lib/test_dictization_schema.py +++ b/ckan/tests/lib/test_dictization_schema.py @@ -148,7 +148,7 @@ def test_2_group_schema(self): del data['extras'] converted_data, errors = validate(data, default_group_schema(), self.context) - group_pack = sorted(group.active_packages().all(), key=lambda x:x.id) + group_pack = sorted(group.packages(), key=lambda x:x.id) converted_data["packages"] = sorted(converted_data["packages"], key=lambda x:x["id"]) diff --git a/ckan/tests/models/test_group.py b/ckan/tests/models/test_group.py index 703b63778f8..424e8f3c198 100644 --- a/ckan/tests/models/test_group.py +++ b/ckan/tests/models/test_group.py @@ -25,7 +25,7 @@ def test_1_basic(self): grp = model.Group.by_name(u'group1') assert grp.title == u'Test Group' assert grp.description == u'This is a test group' - assert grp.active_packages().all() == [] + assert grp.packages() == [] def test_2_add_packages(self): model.repo.new_revision() @@ -50,7 +50,7 @@ def test_2_add_packages(self): assert grp.title == u'Russian Group' anna = model.Package.by_name(u'annakarenina') war = model.Package.by_name(u'warandpeace') - assert set(grp.active_packages().all()) == set((anna, war)), grp.active_packages().all() + assert set(grp.packages()) == set((anna, war)), grp.packages() assert grp in anna.get_groups() def test_3_search(self):