Merge branch 'master' into 715-resource-preview-ux-changes

ckan/controllers/group.py

@@ -281,7 +281,7 @@ def pager_url(q=None, page=None):
             default_facet_titles = {'groups': _('Groups'),
                                     'tags': _('Tags'),
                                     'res_format': _('Formats'),
-                                    'license': _('License')}
+                                    'license_id': _('License')}
 
             for facet in g.facets:
                 if facet in default_facet_titles:

ckan/controllers/organization.py

@@ -19,7 +19,7 @@ def _group_form(self, group_type=None):
         return 'organization/new_organization_form.html'
 
     def _form_to_db_schema(self, group_type=None):
-        return lookup_group_plugin(group_type).form_to_db_schema()
+        return group.lookup_group_plugin(group_type).form_to_db_schema()
 
     def _db_to_form_schema(self, group_type=None):
         '''This is an interface to manipulate data from the database
@@ -48,7 +48,7 @@ def _read_template(self, group_type):
         return 'organization/read.html'
 
     def _history_template(self, group_type):
-        return lookup_group_plugin(group_type).history_template()
+        return group.lookup_group_plugin(group_type).history_template()
 
     def _edit_template(self, group_type):
         return 'organization/edit.html'

ckan/controllers/package.py

@@ -1319,9 +1319,9 @@ def resource_datapreview(self, id, resource_id):
         '''
         Embeded page for a resource data-preview.
 
-        Depending on the type, different previews are loaded.
-        This could be an img tag where the image is loaded directly or an iframe that
-        embeds a webpage, recline or a pdf preview.
+        Depending on the type, different previews are loaded.  This could be an
+        img tag where the image is loaded directly or an iframe that embeds a
+        webpage, recline or a pdf preview.
         '''
         context = {
             'model': model,
@@ -1335,30 +1335,17 @@ def resource_datapreview(self, id, resource_id):
             c.package = get_action('package_show')(context, {'id': id})
 
             data_dict = {'resource': c.resource, 'package': c.package}
-            on_same_domain = datapreview.resource_is_on_same_domain(data_dict)
-            data_dict['resource']['on_same_domain'] = on_same_domain
-
-            # FIXME this wants to not use plugins as it is an imported name
-            # and we already import it an p should really only be in
-            # extensu=ions in my opinion also just make it look nice and be
-            # readable grrrrrr
-            plugins = p.PluginImplementations(p.IResourcePreview)
-            plugins_that_can_preview = [plugin for plugin in plugins
-                                    if plugin.can_preview(data_dict)]
-            if len(plugins_that_can_preview) == 0:
-                abort(409, _('No preview has been defined.'))
-            if len(plugins_that_can_preview) > 1:
-                log.warn('Multiple previews are possible. {0}'.format(
-                                            plugins_that_can_preview))
 
-            plugin = plugins_that_can_preview[0]
-            plugin.setup_template_variables(context, data_dict)
+            preview_plugin = datapreview.get_preview_plugin(data_dict)
 
-            c.resource_json = json.dumps(c.resource)
+            if preview_plugin is None:
+                abort(409, _('No preview has been defined.'))
 
+            preview_plugin.setup_template_variables(context, data_dict)
+            c.resource_json = json.dumps(c.resource)
         except NotFound:
             abort(404, _('Resource not found'))
         except NotAuthorized:
             abort(401, _('Unauthorized to read resource %s') % id)
         else:
-            return render(plugin.preview_template(context, data_dict))
+            return render(preview_plugin.preview_template(context, data_dict))

ckan/lib/datapreview.py

@@ -6,6 +6,7 @@
 """
 
 import urlparse
+import logging
 
 import pylons.config as config
 
@@ -16,6 +17,8 @@
                            'n3', 'n-triples', 'turtle', 'plain',
                            'atom', 'rss', 'txt']
 
+log = logging.getLogger(__name__)
+
 
 def compare_domains(urls):
     ''' Return True if the domains of the provided are the same.
@@ -41,22 +44,56 @@ def compare_domains(urls):
     return True
 
 
-def resource_is_on_same_domain(data_dict):
+def _on_same_domain(data_dict):
     # compare CKAN domain and resource URL
     ckan_url = config.get('ckan.site_url', '//localhost:5000')
     resource_url = data_dict['resource']['url']
 
     return compare_domains([ckan_url, resource_url])
 
 
-def can_be_previewed(data_dict):
-    '''
-    Determines whether there is an extension that can preview the resource.
+def get_preview_plugin(data_dict):
+    '''Determines whether there is an extension that can preview the resource.
 
     :param data_dict: contains a resource and package dict.
         The resource dict has to have a value for ``on_same_domain``
     :type data_dict: dictionary
-    '''
-    data_dict['resource']['on_same_domain'] = resource_is_on_same_domain(data_dict)
-    plugins = p.PluginImplementations(p.IResourcePreview)
-    return any(plugin.can_preview(data_dict) for plugin in plugins)
+
+    Returns a dict of plugins that can preview or ones that are fixable'''
+
+    data_dict['resource']['on_same_domain'] = _on_same_domain(data_dict)
+
+    plugins_that_can_preview = []
+    plugins_fixable = []
+    for plugin in p.PluginImplementations(p.IResourcePreview):
+        p_info = {'plugin': plugin, 'quality': 1}
+        data = plugin.can_preview(data_dict)
+        # old school plugins return true/False
+        if isinstance(data, bool):
+            p_info['can_preview'] = data
+        else:
+            # new school provide a dict
+            p_info.update(data)
+        # if we can preview
+        if p_info['can_preview']:
+            plugins_that_can_preview.append(p_info)
+        elif p_info.get('fixable'):
+            plugins_fixable.append(p_info)
+
+    num_plugins = len(plugins_that_can_preview)
+    if num_plugins == 0:
+        # we didn't find any.  see if any could be made to work
+        for plug in plugins_fixable:
+            log.info('%s would allow previews to fix: %s' % (
+                plug['plugin'], plug['fixable']))
+        preview_plugin = None
+    elif num_plugins == 1:
+        # just one available
+        preview_plugin = plugins_that_can_preview[0]['plugin']
+    else:
+        # multiple plugins so get the best one
+        plugs = [pl['plugin'] for pl in plugins_that_can_preview]
+        log.warn('Multiple previews are possible. {0}'.format(plugs))
+        preview_plugin = max(plugins_that_can_preview,
+                             key=lambda x: x['quality'])['plugin']
+    return preview_plugin

ckan/lib/helpers.py

@@ -1534,7 +1534,7 @@ def resource_preview(resource, pkg_id):
     if not loadable_in_iframe:
         loadable_in_iframe = datapreview.DEFAULT_LOADABLE_IFRAME
 
-    if datapreview.can_be_previewed(data_dict):
+    if datapreview.get_preview_plugin(data_dict):
         url = url_for(controller='package', action='resource_datapreview',
                       resource_id=resource['id'], id=pkg_id, qualified=True)
     elif format_lower in direct_embed:

ckan/plugins/interfaces.py

@@ -202,8 +202,20 @@ class IResourcePreview(Interface):
 
     def can_preview(self, data_dict):
         '''
-        Return True if the extension can preview the resource. The ``data_dict``
-        contains the resource and the package.
+        Returns info on whether the plugin can preview the resource.
+
+        This can be done in two ways.
+        The old way is to just return True or False.
+        The new way is to return a dict with the following
+        {
+            'can_preview': bool - if the extension can preview the resource
+            'fixable': string - if the extension cannot preview but could for
+                                example if the resource_proxy was enabled.
+            'quality': int - how good the preview is 1-poor, 2-average, 3-good
+                          used if multiple extensions can preview
+        }
+
+        The ``data_dict`` contains the resource and the package.
 
         Make sure to ckeck the ``on_same_domain`` value of the
         resource or the url if your preview requires the resource to be on

ckan/tests/test_coding_standards.py

@@ -592,7 +592,6 @@ class TestPep8(object):
         'ckan/lib/captcha.py',
         'ckan/lib/cli.py',
         'ckan/lib/create_test_data.py',
-        'ckan/lib/datapreview.py',
         'ckan/lib/dictization/__init__.py',
         'ckan/lib/dictization/model_dictize.py',
         'ckan/lib/dictization/model_save.py',
@@ -847,7 +846,6 @@ class TestPep8(object):
         'ckanext/example_itemplatehelpers/plugin.py',
         'ckanext/multilingual/plugin.py',
         'ckanext/multilingual/tests/test_multilingual_plugin.py',
-        'ckanext/pdfpreview/plugin.py',
         'ckanext/reclinepreview/plugin.py',
         'ckanext/reclinepreview/tests/test_preview.py',
         'ckanext/resourceproxy/plugin.py',

ckanext/pdfpreview/plugin.py

@@ -1,26 +1,17 @@
-from logging import getLogger
+import logging
 
 import ckan.plugins as p
-import ckan.lib.base as base
 
-log = getLogger(__name__)
+log = logging.getLogger(__name__)
 
-proxy = False
 try:
     import ckanext.resourceproxy.plugin as proxy
 except ImportError:
     pass
 
 
 class PdfPreview(p.SingletonPlugin):
-    """This extension previews PDFs
-
-    This extension implements two interfaces
-
-      - ``IConfigurer`` allows to modify the configuration
-      - ``IConfigurable`` get the configuration
-      - ``IResourcePreview`` allows to add previews
-    """
+    '''This extension previews PDFs. '''
     p.implements(p.IConfigurer, inherit=True)
     p.implements(p.IConfigurable, inherit=True)
     p.implements(p.IResourcePreview, inherit=True)
@@ -29,24 +20,31 @@ class PdfPreview(p.SingletonPlugin):
     proxy_is_enabled = False
 
     def update_config(self, config):
-        ''' Set up the resource library, public directory and
-        template directory for the preview
-        '''
         p.toolkit.add_public_directory(config, 'theme/public')
         p.toolkit.add_template_directory(config, 'theme/templates')
         p.toolkit.add_resource('theme/public', 'ckanext-pdfpreview')
 
     def configure(self, config):
-        self.proxy_is_enabled = config.get('ckan.resource_proxy_enabled', False)
+        enabled = config.get('ckan.resource_proxy_enabled', False)
+        self.proxy_is_enabled = enabled
 
     def can_preview(self, data_dict):
         resource = data_dict['resource']
         format_lower = resource['format'].lower()
-        return format_lower in self.PDF and (resource['on_same_domain'] or self.proxy_is_enabled)
+        if format_lower in self.PDF:
+            if resource['on_same_domain'] or self.proxy_is_enabled:
+                return {'can_preview': True, 'quality': 2}
+            else:
+                return {'can_preview': False,
+                        'fixable': 'Enable resource_proxy',
+                        'quality': 2}
+        return {'can_preview': False}
 
     def setup_template_variables(self, context, data_dict):
-        if self.proxy_is_enabled and not data_dict['resource']['on_same_domain']:
-            base.c.resource['url'] = proxy.get_proxified_resource_url(data_dict)
+        if (self.proxy_is_enabled
+                and not data_dict['resource']['on_same_domain']):
+            url = proxy.get_proxified_resource_url(data_dict)
+            p.toolkit.c.resource['url'] = url
 
     def preview_template(self, context, data_dict):
         return 'pdf.html'

ckanext/pdfpreview/tests/test_preview.py

@@ -54,31 +54,31 @@ def test_can_preview(self):
                 'on_same_domain': True
             }
         }
-        assert self.p.can_preview(data_dict)
+        assert self.p.can_preview(data_dict)['can_preview']
 
         data_dict = {
             'resource': {
                 'format': 'x-pdf',
                 'on_same_domain': True
             }
         }
-        assert self.p.can_preview(data_dict)
+        assert self.p.can_preview(data_dict)['can_preview']
 
         data_dict = {
             'resource': {
                 'format': 'pdf',
                 'on_same_domain': True
             }
         }
-        assert self.p.can_preview(data_dict)
+        assert self.p.can_preview(data_dict)['can_preview']
 
         data_dict = {
             'resource': {
                 'format': 'pdf',
                 'on_same_domain': False
             }
         }
-        assert not self.p.can_preview(data_dict)
+        assert not self.p.can_preview(data_dict)['can_preview']
 
     def test_js_included(self):
         res_id = self.resource['id']