Merge pull request #189 from open-data/feature/handle-dataset-resource-validation

Handle Nested Resource Validation Errors

Author: JVickery-TBS

changes/189.canada.feature

@@ -0,0 +1 @@
+Now handles nested resource validation errors. Creating, updating, and deleting a resource can catch and handle validation errors from other resources in the dataset.

ckan/lib/helpers.py

@@ -1088,6 +1088,11 @@ def humanize_entity_type(entity_type: str, object_type: str,
         u'search placeholder': _(u'Search {object_type}s...'),
         u'you not member': _(u'You are not a member of any {object_type}s.'),
         u'update label': _(u"Update {object_type}"),
+        # (canada fork only): handle all errors in resource actions
+        # TODO: upstream contrib??
+        'other errors': _('Errors in {object_type}'),
+        'other errors package': _('The {object_type} contains errors:'),
+        'other errors resources': _('The {object_type} contains invalid resources:')
     }
 
     type_label = object_type.replace(u"_", u" ").capitalize()

ckan/logic/action/__init__.py

@@ -3,7 +3,9 @@
 
 import re
 from copy import deepcopy
-from typing import Any, Mapping, cast
+# (canada fork only): handle all errors in resource actions
+# TODO: upstream contrib??
+from typing import Any, Mapping, cast, Tuple, Dict
 
 
 from ckan.logic import NotFound
@@ -73,3 +75,77 @@ def prettify(field_name: str):
         else:
             summary[_(prettify(key))] = error[0]
     return summary
+
+
+# (canada fork only): handle all errors in resource actions
+# TODO: upstream contrib??
+def resource_validation_errors(
+        error_dict: ErrorDict,
+        action: str,
+        pkg_dict: Dict[str, Any],
+        resource_index: int = -1) -> Tuple[Dict[str, Any], str]:
+    """
+    Returns a modified (error_dict, error_summary) with errors from the
+    resource_index resource fields in error_dict, errors from the dataset
+    fields under error_dict['dataset'] and errors from other resource
+    fields under error_dict['resources'][resource_id].
+    """
+    new_error_dict = dict(error_dict)
+    # define out own error summaries so we can have
+    # a more customized structure to the ErrorDict
+    error_summaries = []
+    if action == 'delete':
+        # special case for deleting as there is no index
+        # for a non-existent resource in the pkg_dict.
+        current_res_error_dict = False
+    else:
+        current_res_error_dict = cast("list[ErrorDict]", error_dict['resources'])[resource_index]
+    if current_res_error_dict:
+        # if there are errors for the current resource
+        # let's raise them to the user first.
+        new_error_dict = dict(current_res_error_dict)
+    if not current_res_error_dict and 'resources' in error_dict and isinstance(error_dict['resources'], list):
+        # compile the other resource errors
+        new_error_dict = {'resources': {}}
+        for key, res_error_dict in enumerate(error_dict['resources']):
+            if not res_error_dict:
+                continue
+            if key <= len(pkg_dict['resources']):  # case for resource_delete
+                errored_resource = pkg_dict['resources'][key]
+                if errored_resource.get('id'):
+                    new_error_dict['resources'][errored_resource.get('id')] = res_error_dict
+                if res_error_dict:
+                    if action == 'create' or action == 'update':
+                        error_summaries.append(
+                            _('Could not create or update resource because another resource in '
+                              'this dataset has errors: {}').format(errored_resource['id']))
+                    elif action == 'delete':
+                        error_summaries.append(
+                            _('Could not delete resource because another resource in '
+                              'this dataset has errors: {}').format(errored_resource['id']))
+                    elif action == 'reorder':
+                        error_summaries.append(
+                            _('Could not reorder resources because a resource in '
+                              'this dataset has errors: {}').format(errored_resource['id']))
+    if error_dict:
+        # compile the dataset errors
+        for key, errors in error_dict.items():
+            if key == 'resources':
+                continue
+            if 'dataset' not in new_error_dict:
+                new_error_dict['dataset'] = {}
+            new_error_dict['dataset'][key] = errors
+        if 'dataset' in new_error_dict:
+            if action == 'create' or action == 'update':
+                error_summaries.append(
+                    _('Could not create or update resource because '
+                        'the dataset has errors'))
+            elif action == 'delete':
+                error_summaries.append(
+                    _('Could not delete resource because '
+                        'the dataset has errors'))
+            elif action == 'reorder':
+                error_summaries.append(
+                    _('Could not reorder resources because '
+                        'the dataset has errors'))
+    return new_error_dict, _('; ').join(error_summaries)

ckan/logic/action/create.py

@@ -33,6 +33,9 @@
 import ckan.authz as authz
 import ckan.model
 
+# (canada fork only): handle all errors in resource actions
+# TODO: upstream contrib??
+from . import resource_validation_errors
 from ckan.common import _
 from ckan.types import Context, DataDict, ErrorDict, Schema
 
@@ -329,11 +332,11 @@ def resource_create(context: Context,
         _get_action('package_update')(context, pkg_dict)
         context.pop('defer_commit')
     except ValidationError as e:
-        try:
-            error_dict = cast("list[ErrorDict]", e.error_dict['resources'])[-1]
-        except (KeyError, IndexError):
-            error_dict = e.error_dict
-        raise ValidationError(error_dict)
+        # (canada fork only): handle all errors in resource actions
+        # TODO: upstream contrib??
+        error_dict, error_summary = resource_validation_errors(
+            e.error_dict, action='create', pkg_dict=pkg_dict)
+        raise ValidationError(error_dict, error_summary=error_summary)
 
     # Get out resource_id resource from model as it will not appear in
     # package_show until after commit

ckan/logic/action/delete.py

@@ -19,6 +19,9 @@
 from  ckan.lib.navl.dictization_functions import validate
 from ckan.model.follower import ModelFollowingModel
 
+# (canada fork only): handle all errors in resource actions
+# TODO: upstream contrib??
+from . import resource_validation_errors
 from ckan.common import _
 from ckan.types import Context, DataDict, ErrorDict, Schema
 
@@ -195,8 +198,11 @@ def resource_delete(context: Context, data_dict: DataDict) -> ActionResult.Resou
     try:
         pkg_dict = _get_action('package_update')(context, pkg_dict)
     except ValidationError as e:
-        errors = cast("list[ErrorDict]", e.error_dict['resources'])[-1]
-        raise ValidationError(errors)
+        # (canada fork only): handle all errors in resource actions
+        # TODO: upstream contrib??
+        error_dict, error_summary = resource_validation_errors(
+            e.error_dict, action='delete', pkg_dict=pkg_dict)
+        raise ValidationError(error_dict, error_summary=error_summary)
 
     for plugin in plugins.PluginImplementations(plugins.IResourceController):
         plugin.after_resource_delete(context, pkg_dict.get('resources', []))

ckan/logic/action/update.py

@@ -28,6 +28,9 @@
 #TODO: upstream contrib!!
 import ckan.lib.search.jobs as search_jobs
 
+# (canada fork only): handle all errors in resource actions
+# TODO: upstream contrib??
+from . import resource_validation_errors
 from ckan.common import _, config
 from ckan.types import Context, DataDict, ErrorDict
 
@@ -114,11 +117,11 @@ def resource_update(context: Context, data_dict: DataDict) -> ActionResult.Resou
         context['use_cache'] = False
         updated_pkg_dict = _get_action('package_update')(context, pkg_dict)
     except ValidationError as e:
-        try:
-            error_dict = cast("list[ErrorDict]", e.error_dict['resources'])[n]
-        except (KeyError, IndexError):
-            error_dict = e.error_dict
-        raise ValidationError(error_dict)
+        # (canada fork only): handle all errors in resource actions
+        # TODO: upstream contrib??
+        error_dict, error_summary = resource_validation_errors(
+            e.error_dict, action='update', pkg_dict=pkg_dict, resource_index=n)
+        raise ValidationError(error_dict, error_summary=error_summary)
 
     resource = _get_action('resource_show')(context, {'id': id})
 
@@ -579,9 +582,18 @@ def package_resource_reorder(
     package_dict['resources'] = new_resources
 
     _check_access('package_resource_reorder', context, package_dict)
-    _get_action('package_update')(context, package_dict)
+    # (canada fork only): handle all errors in resource actions
+    # TODO: upstream contrib??
+    try:
+        _get_action('package_update')(context, package_dict)
+    except ValidationError as e:
+        error_dict, error_summary = resource_validation_errors(
+            e.error_dict, action='reorder', pkg_dict=package_dict)
+        return {'id': id, 'order': [resource['id'] for resource in new_resources],
+                'errors': error_dict, 'error_summary': error_summary}
 
-    return {'id': id, 'order': [resource['id'] for resource in new_resources]}
+    return {'id': id, 'order': [resource['id'] for resource in new_resources],
+            'errors': None, 'error_summary': None}
 
 
 def _update_package_relationship(

ckan/public/base/javascript/modules/resource-reorder.js

@@ -30,6 +30,14 @@ this.ckan.module('resource-reorder', function($) {
         '<i class="fa fa-spinner fa-spin"></i>',
         '<span></span>',
         '</span>'
+      ].join('\n'),
+      // (canada fork only): handle all errors in resource actions
+      // TODO: upstream contrib??
+      errors: [
+        '<div class="error-explanation alert alert-danger">',
+        '<h3></h3>',
+        '<p></p>',
+        '</div>'
       ].join('\n')
     },
     is_reordering: false,
@@ -47,6 +55,13 @@ this.ckan.module('resource-reorder', function($) {
         .insertBefore(this.el)
         .hide();
 
+      // (canada fork only): handle all errors in resource actions
+      // TODO: upstream contrib??
+      this.html_errors = $(this.template.errors)
+        .insertBefore(this.el)
+        .hide();
+      $(this.html_errors).find('h3').text(this._('Errors in dataset'));
+
       this.html_help_text = $(this.template.help_text)
         .text(helpText)
         .insertBefore(this.el)
@@ -126,7 +141,13 @@ this.ckan.module('resource-reorder', function($) {
         module.sandbox.client.call('POST', 'package_resource_reorder', {
           id: module.options.id,
           order: order
-        }, function() {
+        }, function(data) {
+          // (canada fork only): handle all errors in resource actions
+          // TODO: upstream contrib??
+          if( typeof data.result !== 'undefined' && typeof data.result.error_summary !== 'undefined' ){
+            $(module.html_errors).find('p').text(data.result.error_summary);
+            $(module.html_errors).show();
+          }
           module.html_saving.hide();
           $('.save, .cancel', module.html_form_actions).removeClass('disabled');
           module.cache = module.el.html();

ckan/templates/macros/form.html

@@ -13,6 +13,9 @@
 {% from "macros/form/prepend.html" import prepend %}
 {% from "macros/form/custom.html" import custom %}
 {% from "macros/form/errors.html" import errors %}
+{# (canada fork only): handle all errors in resource actions #}
+{# TODO: upstream contrib?? #}
+{% from "macros/form/resource_validation_errors.html" import resource_validation_errors %}
 {% from "macros/form/info.html" import info %}
 {% from "macros/form/hidden.html" import hidden %}
 {% from "macros/form/hidden_from_list.html" import hidden_from_list %}
@@ -29,6 +32,9 @@
 {% set prepend = prepend %}
 {% set custom = custom %}
 {% set errors = errors %}
+{# (canada fork only): handle all errors in resource actions #}
+{# TODO: upstream contrib?? #}
+{% set resource_validation_errors = resource_validation_errors %}
 {% set info = info %}
 {% set hidden = hidden %}
 {% set hidden_from_list = hidden_from_list %}

ckan/templates/macros/form/errors.html

@@ -12,15 +12,16 @@
 
 #}
 
-{% macro errors(errors={}, type="error", classes=[]) %}
+{# (canada fork only): error -> danger BS version #}
+{% macro errors(errors={}, type="danger", classes=[]) %}
 {% if errors %}
 <div class="error-explanation alert alert-{{ type }}{{ " " ~ classes | join(' ') }}">
-<p>{{ _('The form contains invalid entries:') }}</p>
-<ul>
-    {% for key, error in errors.items() %}
-    <li data-field-label="{{ key }}">{% if key %}{{ key }}: {% endif %}{{ error }}</li>
-    {% endfor %}
-</ul>
+  <p>{{ _('The form contains invalid entries:') }}</p>
+  <ul>
+      {% for key, error in errors.items() %}
+      <li data-field-label="{{ key }}">{% if key %}{{ key }}: {% endif %}{{ error }}</li>
+      {% endfor %}
+  </ul>
 </div>
 {% endif %}
-{% endmacro %}
+{% endmacro %}

ckan/templates/macros/form/resource_validation_errors.html

@@ -0,0 +1,51 @@
+{# (canada fork only): handle all errors in resource actions #}
+{# TODO: upstream contrib?? #}
+
+{#
+Builds a list of errors for the current form.
+
+errors        - A dict of object/field/message pairs from the resource_validation_errors method.
+                'resources' and 'dataset' keys.
+type          - The alert-* class that should be applied (default: "danger")
+classes       - A list of classes to apply to the wrapper (default: [])
+pkg_name      - String; The name or ID of the package.
+dataset_type  - String; The type of package
+h             - The template helpers object as Jinja macros are scoped.
+
+Example:
+
+{% import 'macros/form.html' as form %}
+{{ form.resource_validation_errors(errors=resource_validation_errors, type="warning", pkg_name=pkg_dict.id, h=h) }}
+
+#}
+
+
+{# (canada fork only): error -> danger BS version #}
+{% macro resource_validation_errors(errors={}, type="danger", classes=[], pkg_name='', dataset_type='dataset', h={}) %}
+  {% if errors %}
+    <div class="error-explanation alert alert-{{ type }}{{ " " ~ classes | join(' ') }}">
+      <h3>{{ h.humanize_entity_type('package', dataset_type, 'other errors') or _('Errors in dataset') }}</h3>
+      {% if 'dataset' in errors and errors.dataset %}
+        <p>{{ h.humanize_entity_type('package', dataset_type, 'other errors package') or _('The dataset contains errors:') }}</p>
+        <ul>
+          {% for field_id, field_errs in errors.dataset.items() %}
+            <li data-field-label="{{ field_id }}">{% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}</li>
+          {% endfor %}
+        </ul>
+      {% endif %}
+      {% if 'resources' in errors and errors.resources %}
+        <p>{{ h.humanize_entity_type('package', dataset_type, 'other errors resources') or _('The dataset contains invalid resources:') }}</p>
+        <ul>
+          {% for res_id, errs in errors.resources.items() %}
+            <li><a href="{{ h.url_for('resource.read', id=pkg_name, resource_id=res_id) }}" target="_blank">{{ res_id }}</a></li>
+            <ul>
+              {% for field_id, field_errs in errs.items() %}
+                <li data-field-label="{{ field_id }}">{% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}</li>
+              {% endfor %}
+            </ul>
+          {% endfor %}
+        </ul>
+      {% endif %}
+    </div>
+  {% endif %}
+{% endmacro %}