From 7a032998ed052c6f8c3795bc1cb10f7b904f650d Mon Sep 17 00:00:00 2001 From: Toby Date: Fri, 1 Jun 2012 13:41:17 +0100 Subject: [PATCH 1/7] [#2484] move follower functionality into helpers This stops the need for repeating code in the controllers and helps de-couple the follower functionality It also fixes the translation of the updated buttons --- ckan/controllers/package.py | 29 -------------------- ckan/controllers/related.py | 7 ----- ckan/controllers/user.py | 19 ------------- ckan/lib/helpers.py | 31 ++++++++++++++++++++++ ckan/public/scripts/application.js | 7 +++-- ckan/templates/js_strings.html | 4 ++- ckan/templates/package/layout.html | 9 +++---- ckan/templates/snippets/follow_button.html | 11 ++++++++ ckan/templates/user/layout.html | 9 +++---- 9 files changed, 54 insertions(+), 72 deletions(-) create mode 100644 ckan/templates/snippets/follow_button.html diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index 9403a0271e3..fd1a08b5ea5 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -94,14 +94,6 @@ def _guess_package_type(self, expecting_name=False): return pt - def _setup_follow_button(self, context): - '''Setup some template context variables used for the Follow button.''' - - # If the user is logged in set the am_following variable. - if c.user: - c.pkg_dict['am_following'] = get_action('am_following_dataset')( - context, {'id': c.pkg.id}) - authorizer = ckan.authz.Authorizer() def search(self): @@ -293,10 +285,6 @@ def read(self, id, format='html'): get_action('package_activity_list_html')(context, {'id': c.current_package_id}) - c.num_followers = get_action('dataset_follower_count')(context, - {'id':c.pkg.id}) - self._setup_follow_button(context) - PackageSaver().render_package(c.pkg_dict, context) template = self._read_template( package_type ) @@ -360,10 +348,6 @@ def history(self, id): except NotFound: abort(404, _('Dataset not found')) - c.num_followers = get_action('dataset_follower_count')( - context, {'id':c.pkg.id}) - self._setup_follow_button(context) - format = request.params.get('format', '') if format == 'atom': # Generate and return Atom 1.0 document. @@ -486,10 +470,6 @@ def edit(self, id, data=None, errors=None, error_summary=None): else: c.form = render(self._package_form(package_type=package_type), extra_vars=vars) - c.num_followers = get_action('dataset_follower_count')(context, - {'id':c.pkg.id}) - self._setup_follow_button(context) - if (c.action == u'editresources'): return render('package/editresources.html') else: @@ -665,10 +645,6 @@ def authz(self, id): roles = self._handle_update_of_authz(pkg) self._prepare_authz_info_for_render(roles) - c.num_followers = get_action('dataset_follower_count')(context, - {'id':c.pkg.id}) - self._setup_follow_button(context) - # c.related_count = len(pkg.related) return render('package/authz.html') @@ -757,9 +733,6 @@ def resource_read(self, id, resource_id): qualified=True) c.related_count = len(c.pkg.related) - c.num_followers = get_action('dataset_follower_count')(context, - {'id':c.pkg.id}) - self._setup_follow_button(context) return render('package/resource_read.html') def resource_download(self, id, resource_id): @@ -791,8 +764,6 @@ def followers(self, id=None): c.pkg = context['package'] c.followers = get_action('dataset_follower_list')(context, {'id': c.pkg_dict['id']}) - c.num_followers = len(c.followers) - self._setup_follow_button(context) c.related_count = len(c.pkg.related) except NotFound: diff --git a/ckan/controllers/related.py b/ckan/controllers/related.py index 56adf72e280..f2aeff0448f 100644 --- a/ckan/controllers/related.py +++ b/ckan/controllers/related.py @@ -34,12 +34,5 @@ def list(self, id): c.related_count = len(c.pkg.related) - c.num_followers = logic.get_action('dataset_follower_count')(context, - {'id': c.pkg_dict['id']}) - # If the user is logged in set the am_following variable. - if c.user: - c.pkg_dict['am_following'] = logic.get_action('am_following_dataset')( - context, {'id': c.pkg.id}) - return base.render( "package/related_list.html") diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index 001a078307f..76f28a3ee1b 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -46,22 +46,6 @@ def _db_to_edit_form_schema(self): '''This is an interface to manipulate data from the database into a format suitable for the form (optional)''' - def _setup_follow_button(self, context): - '''Setup some template context variables needed for the Follow/Unfollow - button. - - ''' - - # If the user is logged in set the am_following variable. - userid = context.get('user') - if not userid: - return - userobj = model.User.get(userid) - if not userobj: - return - c.user_dict['am_following'] = get_action('am_following_user')(context, - {'id': c.user_dict['id']}) - def _setup_template_variables(self, context, data_dict): context = {'model': context.get('model'), 'session': context.get('session'), @@ -75,9 +59,6 @@ def _setup_template_variables(self, context, data_dict): abort(401, _('Not authorized to see this page')) c.user_dict = user_dict c.is_myself = user_dict['name'] == c.user - c.num_followers = get_action('user_follower_count')(context, - {'id':c.user_dict['id']}) - self._setup_follow_button(context) ## end hooks diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 5b4fc60d01b..75373ad0d63 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -819,6 +819,35 @@ def process_names(items): items.append(item) return items +# these are the types of objects that can be followed +_follow_objects = ['dataset', 'user'] + +def follow_button(obj_type, obj_id): + ''' returns a follow button for the given object type and id if the + user is logged in. ''' + import ckan.logic as logic + obj_type = obj_type.lower() + assert obj_type in _follow_objects + # If the user is logged in show the follow/unfollow button + if c.user: + context = {'model' : model, 'session':model.Session, 'user':c.user} + action = 'am_following_%s' % obj_type + following = logic.get_action(action)(context, {'id': obj_id}) + return snippet('snippets/follow_button.html', + following=following, + obj_id=obj_id, + obj_type=obj_type) + return '' + +def follow_count(obj_type, obj_id): + ''' returns the number of followers for the object type and id given ''' + import ckan.logic as logic + obj_type = obj_type.lower() + assert obj_type in _follow_objects + action = '%s_follower_count' % obj_type + context = {'model' : model, 'session':model.Session, 'user':c.user} + return logic.get_action(action)(context, {'id': obj_id}) + # these are the functions that will end up in `h` template helpers # if config option restrict_template_vars is true @@ -873,6 +902,8 @@ def process_names(items): 'activity_div', 'lang_native_name', 'unselected_facet_items', + 'follow_button', + 'follow_count', # imported into ckan.lib.helpers 'literal', 'link_to', diff --git a/ckan/public/scripts/application.js b/ckan/public/scripts/application.js index 72124f00f18..54fc41ab6c6 100644 --- a/ckan/public/scripts/application.js +++ b/ckan/public/scripts/application.js @@ -1406,16 +1406,15 @@ CKAN.Utils = function($, my) { function followButtonClicked(event) { var button = event.currentTarget; if (button.id === 'user_follow_button') { - var object_id = button.getAttribute('data-user-id'); var object_type = 'user'; } else if (button.id === 'dataset_follow_button') { - var object_id = button.getAttribute('data-dataset-id'); var object_type = 'dataset'; } else { // This shouldn't happen. return; } + var object_id = button.getAttribute('data-obj-id'); if (button.getAttribute('data-state') === "follow") { if (object_type == 'user') { var url = '/api/action/follow_user'; @@ -1429,7 +1428,7 @@ CKAN.Utils = function($, my) { id: object_id, }); var nextState = 'unfollow'; - var nextString = 'Unfollow'; + var nextString = CKAN.Strings.unfollow; } else if (button.getAttribute('data-state') === "unfollow") { if (object_type == 'user') { var url = '/api/action/unfollow_user'; @@ -1443,7 +1442,7 @@ CKAN.Utils = function($, my) { id: object_id, }); var nextState = 'follow'; - var nextString = 'Follow'; + var nextString = CKAN.Strings.follow; } else { // This shouldn't happen. diff --git a/ckan/templates/js_strings.html b/ckan/templates/js_strings.html index 1b757bb1115..7dd09bd2282 100644 --- a/ckan/templates/js_strings.html +++ b/ckan/templates/js_strings.html @@ -67,7 +67,9 @@ youCanUseMarkdown = _('You can use %aMarkdown formatting%b here.'), shouldADataStoreBeEnabled = _('Should a %aDataStore table and Data API%b be enabled for this resource?'), datesAreInISO = _('Dates are in %aISO Format%b — eg. %c2012-12-25%d or %c2010-05-31T14:30%d.'), - dataFileUploaded = _('Data File (Uploaded)') + dataFileUploaded = _('Data File (Uploaded)'), + follow = _('Follow'), + unfollow = _('Unfollow'), ), indent=4)} diff --git a/ckan/templates/package/layout.html b/ckan/templates/package/layout.html index bd8721f8d41..219c2a4d95b 100644 --- a/ckan/templates/package/layout.html +++ b/ckan/templates/package/layout.html @@ -37,8 +37,8 @@
  • ${h.subnav_link(h.icon('page_stack') + _('History'), controller='package', action='history', id=c.pkg.name)}
  • ${h.subnav_link( - h.icon('authorization_group') + _('Followers ({num_followers})').format(num_followers=c.num_followers), - controller='package', + h.icon('authorization_group') + _('Followers ({num_followers})').format(num_followers=h.follow_count('dataset', c.pkg_dict.id)), + controller='package', action='followers', id=c.pkg.name)}
    • @@ -58,10 +58,7 @@ ${h.subnav_link(h.icon('lock') + _('Authorization'), controller='package', action='authz', id=c.pkg.name)}
  • - - Unfollow - Follow - + ${h.follow_button('dataset', c.pkg_dict.id)}
    • diff --git a/ckan/templates/snippets/follow_button.html b/ckan/templates/snippets/follow_button.html new file mode 100644 index 00000000000..1e0a0d99e51 --- /dev/null +++ b/ckan/templates/snippets/follow_button.html @@ -0,0 +1,11 @@ + + + Unfollow + Follow + + diff --git a/ckan/templates/user/layout.html b/ckan/templates/user/layout.html index a61778d9d39..9524379ad5e 100644 --- a/ckan/templates/user/layout.html +++ b/ckan/templates/user/layout.html @@ -13,7 +13,7 @@
  • Log out
  • ${h.subnav_link( - h.icon('authorization_group') + _('My Followers ({num_followers})').format(num_followers=c.num_followers), + h.icon('authorization_group') + _('My Followers ({num_followers})').format(num_followers=h.follow_count('user', c.user_dict.id)), controller='user', action='followers', id=c.user_dict.name)} @@ -24,16 +24,13 @@
  • View Profile
  • ${h.subnav_link( - h.icon('authorization_group') + _('Followers ({num_followers})').format(num_followers=c.num_followers), + h.icon('authorization_group') + _('Followers ({num_followers})').format(num_followers=h.follow_count('user', c.user_dict.id)), controller='user', action='followers', id=c.user_dict.name)}
  • - - Unfollow - Follow - + ${h.follow_button('user', c.user_dict.id)}
    • From 6d3b56b42c67f6837b346df41bc405ad2728c5f8 Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 25 May 2012 16:50:17 +0100 Subject: [PATCH 2/7] Helpful SOLR hint. --- doc/solr-setup.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/doc/solr-setup.rst b/doc/solr-setup.rst index 5d530712ec1..ffc044a080d 100644 --- a/doc/solr-setup.rst +++ b/doc/solr-setup.rst @@ -199,6 +199,18 @@ Some problems that can be found during the install: ${dataDir} +* When running Solr it says `Unable to find a javac compiler; com.sun.tools.javac.Main is not on the classpath. Perhaps JAVA_HOME does not point to the JDK.` + + See the note above about JAVA_HOME. Alternatively you may not have installed the JDK. Check by seeing if javac is installed:: + + which javac + + If it isn't do:: + + sudo apt-get install openjdk-6-jdk + + and restart SOLR. + Handling changes in the CKAN schema ----------------------------------- From ebd836504ff2e323eb93de4dd89a3c579e724c1a Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 15 Jun 2012 13:45:46 +0100 Subject: [PATCH 3/7] Fix minor bug that caused create_users to not commit changes. --- ckan/lib/create_test_data.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 53743e6c2cc..1b8d3b601a2 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -537,6 +537,7 @@ def _create_user_without_commit(cls, name='', **user_dict): user = model.User(name=unicode(name), **user_dict) model.Session.add(user) cls.user_refs.append(user_ref) + return user @classmethod def create_user(cls, name='', **kwargs): From 0ed19599772213631c627614383fe6d1d9dea2fa Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 15 Jun 2012 13:46:04 +0100 Subject: [PATCH 4/7] Improve logging in useful places. --- ckan/controllers/package.py | 2 ++ ckan/lib/authenticator.py | 6 ++++++ ckan/lib/base.py | 3 +++ ckan/lib/search/__init__.py | 13 +++++++------ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index 9403a0271e3..a7df2cf1849 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -592,6 +592,8 @@ def _save_new(self, context, package_type=None): def _save_edit(self, name_or_id, context): from ckan.lib.search import SearchIndexError + log.debug('Package save request name: %s POST: %r', + name_or_id, request.POST) try: data_dict = clean_dict(unflatten( tuplize_dict(parse_params(request.POST)))) diff --git a/ckan/lib/authenticator.py b/ckan/lib/authenticator.py index b56711a3427..6f061caad91 100644 --- a/ckan/lib/authenticator.py +++ b/ckan/lib/authenticator.py @@ -1,8 +1,12 @@ +import logging + from zope.interface import implements from repoze.who.interfaces import IAuthenticator from ckan.model import User, Session +log = logging.getLogger(__name__) + class OpenIDAuthenticator(object): implements(IAuthenticator) @@ -25,8 +29,10 @@ def authenticate(self, environ, identity): return None user = User.by_name(identity.get('login')) if user is None: + log.debug('Login failed - username %r not found', identity.get('login')) return None if user.validate_password(identity.get('password')): return user.name + log.debug('Login as %r failed - password not valid', identity.get('login')) return None diff --git a/ckan/lib/base.py b/ckan/lib/base.py index f2cc447fdea..52604d48e7f 100644 --- a/ckan/lib/base.py +++ b/ckan/lib/base.py @@ -29,6 +29,8 @@ from ckan.lib.helpers import json import ckan.model as model +log = logging.getLogger(__name__) + PAGINATE_ITEMS_PER_PAGE = 50 APIKEY_HEADER_NAME_KEY = 'apikey_header_name' @@ -139,6 +141,7 @@ def render_template(): response.headers["Cache-Control"] = "private" # Prevent any further rendering from being cached. request.environ['__no_cache__'] = True + log.debug('Template cache-control: %s' % response.headers["Cache-Control"]) # Render Time :) try: diff --git a/ckan/lib/search/__init__.py b/ckan/lib/search/__init__.py index abc78b798fd..b68e7850c47 100644 --- a/ckan/lib/search/__init__.py +++ b/ckan/lib/search/__init__.py @@ -128,11 +128,11 @@ def rebuild(package_id=None,only_missing=False,force=False,refresh=False): If a dataset id is provided, only this dataset will be reindexed. When reindexing all datasets, if only_missing is True, only the datasets not already indexed will be processed. If force equals - True, if an execption is found, the exception will be logged, but + True, if an exception is found, the exception will be logged, but the process will carry on. ''' from ckan import model - log.debug("Rebuilding search index...") + log.info("Rebuilding search index...") package_index = index_for(model.Package) @@ -140,21 +140,22 @@ def rebuild(package_id=None,only_missing=False,force=False,refresh=False): pkg_dict = get_action('package_show')( {'model': model, 'ignore_auth': True, 'validate': False}, {'id': package_id}) + log.info('Indexing just package %r...', pkg_dict['name']) package_index.remove_dict(pkg_dict) package_index.insert_dict(pkg_dict) else: package_ids = [r[0] for r in model.Session.query(model.Package.id).filter(model.Package.state == 'active').all()] if only_missing: - log.debug('Indexing only missing packages...') + log.info('Indexing only missing packages...') package_query = query_for(model.Package) indexed_pkg_ids = set(package_query.get_all_entity_ids(max_results=len(package_ids))) package_ids = set(package_ids) - indexed_pkg_ids # Packages not indexed if len(package_ids) == 0: - log.debug('All datasets are already indexed') + log.info('All datasets are already indexed') return else: - log.debug('Rebuilding the whole index...') + log.info('Rebuilding the whole index...') # When refreshing, the index is not previously cleared if not refresh: package_index.clear() @@ -176,7 +177,7 @@ def rebuild(package_id=None,only_missing=False,force=False,refresh=False): raise model.Session.commit() - log.debug('Finished rebuilding search index.') + log.info('Finished rebuilding search index.') def check(): from ckan import model From 84d674dac775f93f50b35436c38b6d0c5d0dc2fd Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 21 Jun 2012 11:22:36 +0100 Subject: [PATCH 5/7] Comment required, else Toby would just delete the method out of hand. --- ckan/lib/create_test_data.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ckan/lib/create_test_data.py b/ckan/lib/create_test_data.py index 1b8d3b601a2..7b4a7815359 100644 --- a/ckan/lib/create_test_data.py +++ b/ckan/lib/create_test_data.py @@ -511,6 +511,7 @@ def create(cls, auth_profile="", package_type=None): model.repo.commit_and_remove() + # method used in DGU and all good tests elsewhere @classmethod def create_users(cls, user_dicts): needs_commit = False From 496ee0504907ed1f60d45cbcdfec927303adbdb7 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 21 Jun 2012 14:15:39 +0200 Subject: [PATCH 6/7] [#2484] Improve docstrings of follow helper functions --- ckan/lib/helpers.py | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 75373ad0d63..4e29c44da4e 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -823,8 +823,21 @@ def process_names(items): _follow_objects = ['dataset', 'user'] def follow_button(obj_type, obj_id): - ''' returns a follow button for the given object type and id if the - user is logged in. ''' + '''Return a follow button for the given object type and id. + + If the user is not logged in return an empty string instead. + + :param obj_type: the type of the object to be followed when the follow + button is clicked, e.g. 'user' or 'dataset' + :type obj_type: string + :param obj_id: the id of the object to be followed when the follow button + is clicked + :type obj_id: string + + :returns: a follow button as an HTML snippet + :rtype: string + + ''' import ckan.logic as logic obj_type = obj_type.lower() assert obj_type in _follow_objects @@ -840,7 +853,17 @@ def follow_button(obj_type, obj_id): return '' def follow_count(obj_type, obj_id): - ''' returns the number of followers for the object type and id given ''' + '''Return the number of followers of an object. + + :param obj_type: the type of the object, e.g. 'user' or 'dataset' + :type obj_type: string + :param obj_id: the id of the object + :type obj_id: string + + :returns: the number of followers of the object + :rtype: int + + ''' import ckan.logic as logic obj_type = obj_type.lower() assert obj_type in _follow_objects From c7cd7a1e0740ed8983d868804989779b29a740a4 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Thu, 21 Jun 2012 14:19:51 +0100 Subject: [PATCH 7/7] [2500] get_action now raises an exception on missing action Previously get_action returned None for missing actions, but this led to opaque errors in templates whereas now it raises an exception which is clear in describing the problem. Also lots of PEP8 cleanup of the non-test code. --- ckan/controllers/api.py | 176 +++++++++++++++++++------------- ckan/logic/__init__.py | 39 ++++--- ckan/tests/logic/test_action.py | 26 +++-- 3 files changed, 147 insertions(+), 94 deletions(-) diff --git a/ckan/controllers/api.py b/ckan/controllers/api.py index ad6d8410e19..723d4bbf6cc 100644 --- a/ckan/controllers/api.py +++ b/ckan/controllers/api.py @@ -34,7 +34,9 @@ 'text': 'text/plain;charset=utf-8', 'html': 'text/html;charset=utf-8', 'json': 'application/json;charset=utf-8', - } +} + + class ApiController(base.BaseController): _actions = {} @@ -50,10 +52,11 @@ def __call__(self, environ, start_response): self._identify_user() try: - context = {'model':model,'user': c.user or c.author} - logic.check_access('site_read',context) + context = {'model': model, 'user': c.user or c.author} + logic.check_access('site_read', context) except NotAuthorized: - response_msg = self._finish(403, _('Not authorized to see this page')) + response_msg = self._finish(403, + _('Not authorized to see this page')) # Call start_response manually instead of the parent __call__ # because we want to end the request instead of continuing. response_msg = response_msg.encode('utf8') @@ -83,9 +86,9 @@ def _finish(self, status_int, response_data=None, else: response_msg = response_data # Support "JSONP" callback. - if status_int==200 and request.params.has_key('callback') and \ - (request.method == 'GET' or \ - c.logic_function and request.method == 'POST'): + if status_int == 200 and 'callback' in request.params and \ + (request.method == 'GET' or + c.logic_function and request.method == 'POST'): # escape callback to remove '<', '&', '>' chars callback = cgi.escape(request.params['callback']) response_msg = self._wrap_jsonp(callback, response_msg) @@ -133,8 +136,9 @@ def _set_response_header(self, name, value): try: value = str(value) except Exception, inst: - msg = "Couldn't convert '%s' header value '%s' to string: %s" % (name, value, inst) - raise Exception, msg + msg = "Couldn't convert '%s' header value '%s' to string: %s" % \ + (name, value, inst) + raise Exception(msg) response.headers[name] = value def get_api(self, ver=None): @@ -143,19 +147,21 @@ def get_api(self, ver=None): return self._finish_ok(response_data) def action(self, logic_function, ver=None): - function = get_action(logic_function) - if not function: + try: + function = get_action(logic_function) + except KeyError: log.error('Can\'t find logic function: %s' % logic_function) return self._finish_bad_request( gettext('Action name not known: %s') % str(logic_function)) context = {'model': model, 'session': model.Session, 'user': c.user, - 'api_version':ver} + 'api_version': ver} model.Session()._context = context return_dict = {'help': function.__doc__} try: side_effect_free = getattr(function, 'side_effect_free', False) - request_data = self._get_request_data(try_url_params=side_effect_free) + request_data = self._get_request_data(try_url_params= + side_effect_free) except ValueError, inst: log.error('Bad request data: %s' % str(inst)) return self._finish_bad_request( @@ -164,8 +170,8 @@ def action(self, logic_function, ver=None): # this occurs if request_data is blank log.error('Bad request data - not dict: %r' % request_data) return self._finish_bad_request( - gettext('Bad request data: %s') % \ - 'Request data JSON decoded to %r but ' \ + gettext('Bad request data: %s') % + 'Request data JSON decoded to %r but ' 'it needs to be a dictionary.' % request_data) try: result = function(context, request_data) @@ -174,8 +180,8 @@ def action(self, logic_function, ver=None): except DataError, e: log.error('Format incorrect: %s - %s' % (e.error, request_data)) #TODO make better error message - return self._finish(400, _(u'Integrity Error') + \ - ': %s - %s' % (e.error, request_data)) + return self._finish(400, _(u'Integrity Error') + + ': %s - %s' % (e.error, request_data)) except NotAuthorized: return_dict['error'] = {'__type': 'Authorization Error', 'message': _('Access denied')} @@ -197,25 +203,27 @@ def action(self, logic_function, ver=None): return self._finish(409, return_dict, content_type='json') except logic.ParameterError, e: return_dict['error'] = {'__type': 'Parameter Error', - 'message': '%s: %s' % \ + 'message': '%s: %s' % (_('Parameter Error'), e.extra_msg)} return_dict['success'] = False log.error('Parameter error: %r' % e.extra_msg) return self._finish(409, return_dict, content_type='json') except search.SearchQueryError, e: return_dict['error'] = {'__type': 'Search Query Error', - 'message': 'Search Query is invalid: %r' % e.args } + 'message': 'Search Query is invalid: %r' % + e.args} return_dict['success'] = False return self._finish(400, return_dict, content_type='json') except search.SearchError, e: return_dict['error'] = {'__type': 'Search Error', - 'message': 'Search error: %r' % e.args } + 'message': 'Search error: %r' % e.args} return_dict['success'] = False return self._finish(409, return_dict, content_type='json') return self._finish_ok(return_dict) def _get_action_from_map(self, action_map, register, subregister): - # Helper function to get the action function specified in the action map + ''' Helper function to get the action function specified in + the action map''' # translate old package calls to use dataset if register == 'package': @@ -258,7 +266,8 @@ def list(self, ver=None, register=None, subregister=None, id=None): except NotAuthorized: return self._finish_not_authz() - def show(self, ver=None, register=None, subregister=None, id=None, id2=None): + def show(self, ver=None, register=None, subregister=None, + id=None, id2=None): action_map = { 'revision': 'revision_show', 'group': 'group_show_rest', @@ -289,15 +298,17 @@ def show(self, ver=None, register=None, subregister=None, id=None, id2=None): return self._finish_not_authz() def _represent_package(self, package): - return package.as_dict(ref_package_by=self.ref_package_by, ref_group_by=self.ref_group_by) + return package.as_dict(ref_package_by=self.ref_package_by, + ref_group_by=self.ref_group_by) - def create(self, ver=None, register=None, subregister=None, id=None, id2=None): + def create(self, ver=None, register=None, subregister=None, + id=None, id2=None): action_map = { - 'group': 'group_create_rest', - 'dataset': 'package_create_rest', - 'rating': 'rating_create', - 'related': 'related_create', + 'group': 'group_create_rest', + 'dataset': 'package_create_rest', + 'rating': 'rating_create', + 'related': 'related_create', ('dataset', 'relationships'): 'package_relationship_create_rest', } for type in model.PackageRelationship.get_all_types(): @@ -317,14 +328,15 @@ def create(self, ver=None, register=None, subregister=None, id=None, id2=None): action = self._get_action_from_map(action_map, register, subregister) if not action: return self._finish_bad_request( - gettext('Cannot create new entity of this type: %s %s') % \ + gettext('Cannot create new entity of this type: %s %s') % (register, subregister)) try: response_data = action(context, data_dict) location = None if "id" in data_dict: - location = str('%s/%s' % (request.path.replace('package', 'dataset'), + location = str('%s/%s' % (request.path.replace('package', + 'dataset'), data_dict.get("id"))) return self._finish_ok(response_data, resource_location=location) @@ -339,19 +351,23 @@ def create(self, ver=None, register=None, subregister=None, id=None, id2=None): except DataError, e: log.error('Format incorrect: %s - %s' % (e.error, request_data)) #TODO make better error message - return self._finish(400, _(u'Integrity Error') + \ - ': %s - %s' % (e.error, request_data)) + return self._finish(400, _(u'Integrity Error') + + ': %s - %s' % (e.error, request_data)) except search.SearchIndexError: - log.error('Unable to add package to search index: %s' % request_data) - return self._finish(500, _(u'Unable to add package to search index') % request_data) + log.error('Unable to add package to search index: %s' % + request_data) + return self._finish(500, + _(u'Unable to add package to search index') % + request_data) except: model.Session.rollback() raise - def update(self, ver=None, register=None, subregister=None, id=None, id2=None): + def update(self, ver=None, register=None, subregister=None, + id=None, id2=None): action_map = { - 'dataset': 'package_update_rest', - 'group': 'group_update_rest', + 'dataset': 'package_update_rest', + 'group': 'group_update_rest', ('dataset', 'relationships'): 'package_relationship_update_rest', } for type in model.PackageRelationship.get_all_types(): @@ -371,8 +387,8 @@ def update(self, ver=None, register=None, subregister=None, id=None, id2=None): action = self._get_action_from_map(action_map, register, subregister) if not action: return self._finish_bad_request( - gettext('Cannot update entity of this type: %s') % \ - register.encode('utf-8')) + gettext('Cannot update entity of this type: %s') % + register.encode('utf-8')) try: response_data = action(context, data_dict) return self._finish_ok(response_data) @@ -387,17 +403,19 @@ def update(self, ver=None, register=None, subregister=None, id=None, id2=None): except DataError, e: log.error('Format incorrect: %s - %s' % (e.error, request_data)) #TODO make better error message - return self._finish(400, _(u'Integrity Error') + \ - ': %s - %s' % (e.error, request_data)) + return self._finish(400, _(u'Integrity Error') + + ': %s - %s' % (e.error, request_data)) except search.SearchIndexError: log.error('Unable to update search index: %s' % request_data) - return self._finish(500, _(u'Unable to update search index') % request_data) + return self._finish(500, _(u'Unable to update search index') % + request_data) - def delete(self, ver=None, register=None, subregister=None, id=None, id2=None): + def delete(self, ver=None, register=None, subregister=None, + id=None, id2=None): action_map = { - 'group': 'group_delete', - 'dataset': 'package_delete', - 'related': 'related_delete', + 'group': 'group_delete', + 'dataset': 'package_delete', + 'related': 'related_delete', ('dataset', 'relationships'): 'package_relationship_delete_rest', } for type in model.PackageRelationship.get_all_types(): @@ -413,7 +431,7 @@ def delete(self, ver=None, register=None, subregister=None, id=None, id2=None): action = self._get_action_from_map(action_map, register, subregister) if not action: return self._finish_bad_request( - gettext('Cannot delete entity of this type: %s %s') %\ + gettext('Cannot delete entity of this type: %s %s') % (register, subregister or '')) try: response_data = action(context, data_dict) @@ -432,7 +450,7 @@ def search(self, ver=None, register=None): log.debug('search %s params: %r' % (register, request.params)) if register == 'revision': since_time = None - if request.params.has_key('since_id'): + if 'since_id' in request.params: id = request.params['since_id'] if not id: return self._finish_bad_request( @@ -442,7 +460,7 @@ def search(self, ver=None, register=None): return self._finish_not_found( gettext(u'There is no revision with id: %s') % id) since_time = rev.timestamp - elif request.params.has_key('since_time'): + elif 'since_time' in request.params: since_time_str = request.params['since_time'] try: since_time = h.date_str_to_datetime(since_time_str) @@ -450,8 +468,10 @@ def search(self, ver=None, register=None): return self._finish_bad_request('ValueError: %s' % inst) else: return self._finish_bad_request( - gettext("Missing search term ('since_id=UUID' or 'since_time=TIMESTAMP')")) - revs = model.Session.query(model.Revision).filter(model.Revision.timestamp>since_time) + gettext("Missing search term ('since_id=UUID' or " + + " 'since_time=TIMESTAMP')")) + revs = model.Session.query(model.Revision).\ + filter(model.Revision.timestamp > since_time) return self._finish_ok([rev.id for rev in revs]) elif register in ['dataset', 'package', 'resource']: try: @@ -482,7 +502,7 @@ def search(self, ver=None, register=None): for field, value in params.items(): field = field.strip() if field in search.DEFAULT_OPTIONS.keys() or \ - field in IGNORE_FIELDS: + field in IGNORE_FIELDS: continue values = [value] if isinstance(value, list): @@ -491,14 +511,18 @@ def search(self, ver=None, register=None): query_fields.add(field, v) results = query.run( - query=params.get('q'), fields=query_fields, options=options + query=params.get('q'), + fields=query_fields, + options=options ) else: # For package searches in API v3 and higher, we can pass # parameters straight to Solr. if ver in [1, 2]: - # Otherwise, put all unrecognised ones into the q parameter - params = search.convert_legacy_parameters_to_solr(params) + # Otherwise, put all unrecognised ones into the q + # parameter + params = search.\ + convert_legacy_parameters_to_solr(params) query = search.query_for(model.Package) results = query.run(params) return self._finish_ok(results) @@ -512,20 +536,23 @@ def search(self, ver=None, register=None): @classmethod def _get_search_params(cls, request_params): - if request_params.has_key('qjson'): + if 'qjson' in request_params: try: params = h.json.loads(request_params['qjson'], encoding='utf8') except ValueError, e: - raise ValueError, gettext('Malformed qjson value') + ': %r' % e + raise ValueError(gettext('Malformed qjson value') + ': %r' + % e) elif len(request_params) == 1 and \ - len(request_params.values()[0]) < 2 and \ - request_params.keys()[0].startswith('{'): + len(request_params.values()[0]) < 2 and \ + request_params.keys()[0].startswith('{'): # e.g. {some-json}='1' or {some-json}='' params = h.json.loads(request_params.keys()[0], encoding='utf8') else: params = request_params if not isinstance(params, (UnicodeMultiDict, dict)): - raise ValueError, _('Request params must be in form of a json encoded dictionary.') + msg = _('Request params must be in form ' + + 'of a json encoded dictionary.') + raise ValueError(msg) return params def markdown(self, ver=None): @@ -559,7 +586,8 @@ def _calc_throughput(self, ver=None): for t in range(0, period): expr = '%s/%s*' % ( timing_cache_path, - (datetime.datetime.now() - datetime.timedelta(0,t)).isoformat()[0:19], + (datetime.datetime.now() - + datetime.timedelta(0, t)).isoformat()[0:19], ) call_count += len(glob.glob(expr)) # Todo: Clear old records. @@ -574,9 +602,9 @@ def user_autocomplete(self): context = {'model': model, 'session': model.Session, 'user': c.user or c.author} - data_dict = {'q':q,'limit':limit} + data_dict = {'q': q, 'limit': limit} - user_list = get_action('user_autocomplete')(context,data_dict) + user_list = get_action('user_autocomplete')(context, data_dict) return user_list @jsonp.jsonpify @@ -591,16 +619,17 @@ def group_autocomplete(self): limit = min(50, limit) query = model.Group.search_by_name_or_title(q, t) + def convert_to_dict(user): out = {} for k in ['id', 'name', 'title']: out[k] = getattr(user, k) return out + query = query.limit(limit) out = map(convert_to_dict, query.all()) return out - @jsonp.jsonpify def authorizationgroup_autocomplete(self): q = request.params.get('q', '') @@ -612,11 +641,13 @@ def authorizationgroup_autocomplete(self): limit = min(50, limit) query = model.AuthorizationGroup.search(q) + def convert_to_dict(user): out = {} for k in ['id', 'name']: out[k] = getattr(user, k) return out + query = query.limit(limit) out = map(convert_to_dict, query.all()) return out @@ -626,13 +657,13 @@ def is_slug_valid(self): slugtype = request.params.get('type') or '' # TODO: We need plugins to be able to register new disallowed names disallowed = ['new', 'edit', 'search'] - if slugtype==u'package': + if slugtype == u'package': response_data = dict(valid=not bool(common.package_exists(slug) - or slug in disallowed )) + or slug in disallowed)) return self._finish_ok(response_data) - if slugtype==u'group': + if slugtype == u'group': response_data = dict(valid=not bool(common.group_exists(slug) or - slug in disallowed )) + slug in disallowed)) return self._finish_ok(response_data) return self._finish_bad_request('Bad slug type: %s' % slugtype) @@ -647,7 +678,8 @@ def dataset_autocomplete(self): data_dict = {'q': q, 'limit': limit} - package_dicts = get_action('package_autocomplete')(context, data_dict) + package_dicts = get_action('package_autocomplete')(context, + data_dict) resultSet = {'ResultSet': {'Result': package_dicts}} return self._finish_ok(resultSet) @@ -706,9 +738,9 @@ def munge_tag(self): def format_icon(self): f = request.params.get('format') out = { - 'format' : f, - 'icon' : h.icon_url(h.format_icon(f)) - } + 'format': f, + 'icon': h.icon_url(h.format_icon(f)) + } return self._finish_ok(out) def status(self): diff --git a/ckan/logic/__init__.py b/ckan/logic/__init__.py index 098b7edae04..bb7d94b4b7c 100644 --- a/ckan/logic/__init__.py +++ b/ckan/logic/__init__.py @@ -11,12 +11,13 @@ log = logging.getLogger(__name__) + class AttributeDict(dict): def __getattr__(self, name): try: return self[name] except KeyError: - raise AttributeError('No such attribute %r'%name) + raise AttributeError('No such attribute %r' % name) def __setattr__(self, name, value): raise AttributeError( @@ -33,12 +34,15 @@ def __str__(self): self.extra_msg) return ' - '.join([str(err_msg) for err_msg in err_msgs if err_msg]) + class NotFound(ActionError): pass + class NotAuthorized(ActionError): pass + class ParameterError(ActionError): pass @@ -56,6 +60,7 @@ def __str__(self): log = logging.getLogger(__name__) + def parse_params(params, ignore_keys=None): '''Takes a dict and returns it with some values standardised. This is done on a dict before calling tuplize_dict on it. @@ -109,6 +114,7 @@ def clean_dict(data_dict): clean_dict(inner_dict) return data_dict + def tuplize_dict(data_dict): '''Takes a dict with keys of the form 'table__0__key' and converts them to a tuple like ('table', 0, 'key'). @@ -130,6 +136,7 @@ def tuplize_dict(data_dict): tuplized_dict[tuple(key_list)] = value return tuplized_dict + def untuplize_dict(tuplized_dict): data_dict = {} @@ -138,30 +145,33 @@ def untuplize_dict(tuplized_dict): data_dict[new_key] = value return data_dict + def flatten_to_string_key(dict): flattented = flatten_dict(dict) return untuplize_dict(flattented) + def check_access(action, context, data_dict=None): model = context['model'] user = context.get('user') - log.debug('check access - user %r, action %s' % (user,action)) - + log.debug('check access - user %r, action %s' % (user, action)) + if action: - #if action != model.Action.READ and user in (model.PSEUDO_USER__VISITOR, ''): + #if action != model.Action.READ and user in + # (model.PSEUDO_USER__VISITOR, ''): # # TODO Check the API key is valid at some point too! # log.debug('Valid API key needed to make changes') # raise NotAuthorized logic_authorization = is_authorized(action, context, data_dict) if not logic_authorization['success']: - msg = logic_authorization.get('msg','') + msg = logic_authorization.get('msg', '') raise NotAuthorized(msg) elif not user: msg = _('No valid API key provided.') log.debug(msg) - raise NotAuthorized(msg) + raise NotAuthorized(msg) log.debug('Access OK.') return True @@ -172,7 +182,7 @@ def check_access_old(entity, action, context): user = context.get('user') if context.get('ignore_auth'): return True - log.debug('check access - user %r, action %s' % (user,action)) + log.debug('check access - user %r, action %s' % (user, action)) if action and entity and not isinstance(entity, model.PackageRelationship): if action != model.Action.READ and user == '': log.debug('Valid API key needed to make changes') @@ -189,20 +199,23 @@ def check_access_old(entity, action, context): #raise NotAuthorized log.debug('Access OK.') - return True + return True _actions = {} + def get_action(action): if _actions: + if not action in _actions: + raise KeyError("Action '%s' not found" % action) return _actions.get(action) # Otherwise look in all the plugins to resolve all possible # First get the default ones in the ckan/logic/action directory # Rather than writing them out in full will use __import__ # to load anything from ckan.logic.action that looks like it might # be an action - for action_module_name in ['get', 'create', 'update','delete']: - module_path = 'ckan.logic.action.'+action_module_name + for action_module_name in ['get', 'create', 'update', 'delete']: + module_path = 'ckan.logic.action.' + action_module_name module = __import__(module_path) for part in module_path.split('.')[1:]: module = getattr(module, part) @@ -214,8 +227,8 @@ def get_action(action): # Whitelist all actions defined in logic/action/get.py as # being side-effect free. - v.side_effect_free = getattr(v, 'side_effect_free', True) and \ - action_module_name == 'get' + v.side_effect_free = getattr(v, 'side_effect_free', True)\ + and action_module_name == 'get' # Then overwrite them with any specific ones in the plugins: resolved_action_plugins = {} @@ -236,6 +249,7 @@ def get_action(action): _actions.update(fetched_actions) return _actions.get(action) + def get_or_bust(data_dict, keys): '''Try and get values from dictionary and if they are not there raise a validation error. @@ -264,6 +278,7 @@ def get_or_bust(data_dict, keys): return values[0] return tuple(values) + def side_effect_free(action): '''A decorator that marks the given action as side-effect-free. diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index f22858ee1ab..d104c5d3e94 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -870,16 +870,16 @@ def test_34_roles_show_for_authgroup_on_authgroup(self): annafan = model.User.by_name(u'annafan') authgroup = model.AuthorizationGroup.by_name(u'anauthzgroup') authgroup2 = model.AuthorizationGroup.by_name(u'anotherauthzgroup') - + model.add_authorization_group_to_role(authgroup2, 'editor', authgroup) model.repo.commit_and_remove() - + postparams = '%s=1' % json.dumps({'domain_object': authgroup.id, 'authorization_group': authgroup2.id}) res = self.app.post('/api/action/roles_show', params=postparams, extra_environ={'Authorization': str(annafan.apikey)}, status=200) - + authgroup_roles = TestRoles.get_roles(authgroup.id, authgroup_ref=authgroup2.name) assert_equal(authgroup_roles, ['"anotherauthzgroup" is "editor" on "anauthzgroup"']) @@ -904,7 +904,7 @@ def test_35_user_role_update(self): assert_equal(results['roles'][0]['role'], 'reader') assert_equal(results['roles'][0]['package_id'], anna.id) assert_equal(results['roles'][0]['user_id'], tester.id) - + roles_after = get_action('roles_show') \ ({'model': model, 'session': model.Session}, \ {'domain_object': anna.id, @@ -934,12 +934,12 @@ def test_36_user_role_update_for_auth_group(self): assert_equal(results['roles'][0]['role'], 'editor') assert_equal(results['roles'][0]['package_id'], anna.id) assert_equal(results['roles'][0]['authorized_group_id'], authgroup.id) - + all_roles_after = TestRoles.get_roles(anna.id) authgroup_roles_after = TestRoles.get_roles(anna.id, authgroup_ref=authgroup.name) assert_equal(set(all_roles_before) ^ set(all_roles_after), set([u'"anauthzgroup" is "editor" on "annakarenina"'])) - + roles_after = get_action('roles_show') \ ({'model': model, 'session': model.Session}, \ {'domain_object': anna.id, @@ -1016,6 +1016,12 @@ def test_40_task_resource_status(self): assert res['success'] is True assert res['result'] == [{"status": "FAILURE", "entity_id": "749cdcf2-3fc8-44ae-aed0-5eff8cc5032c", "task_type": "qa", "last_updated": "2012-04-20T21:32:45.553986", "date_done": "2012-04-20T21:33:01.622557", "entity_type": "resource", "traceback": "Traceback", "value": "51f2105d-85b1-4393-b821-ac11475919d9", "state": None, "key": "celery_task_id", "error": "", "id": "5753adae-cd0d-4327-915d-edd832d1c9a3"}] + def test_41_missing_action(self): + try: + get_action('unicorns') + assert False, "We found a non-existent action" + except KeyError: + assert True class TestActionTermTranslation(WsgiAppCase): @@ -1240,7 +1246,7 @@ def after_search(self, search_results, search_params): return search_results def before_view(self, data_dict): - + data_dict['title'] = 'string_not_found_in_rest_of_template' return data_dict @@ -1306,7 +1312,7 @@ def test_before_index(self): res = self.app.post('/api/action/package_search', params=search_params) res_dict = json.loads(res.body)['result'] - assert res_dict['count'] == 0 + assert res_dict['count'] == 0 assert len(res_dict['results']) == 0 # all datasets should get abcabcabc @@ -1323,8 +1329,8 @@ def test_before_view(self): res = self.app.get('/dataset/annakarenina') assert 'string_not_found_in_rest_of_template' in res.body - + res = self.app.get('/dataset?q=') assert res.body.count('string_not_found_in_rest_of_template') == 2 - +