{% endblock %} @@ -8,6 +12,9 @@
{% block form %} + {# (canada fork only): handle validation errors #} + {# TODO: upstream contrib?? #} + {% block errors %}{{ form.errors(errors=errors, error_summary=error_summary) }}{% endblock %}

{{ _('Are you sure you want to delete resource - {name}?').format(name=h.resource_display_name(resource_dict)) }}

diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index 7b63eaa1563..ae81c74255a 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -6,7 +6,7 @@ - {{ h.csrf_input() }} + {{ h.csrf_input() }} {% block stages %} {# An empty stages variable will not show the stages #} {% if stage %} @@ -14,7 +14,9 @@ {% endif %} {% endblock %} - {% block errors %}{{ form.errors(error_summary) }}{% endblock %} + {# (canada fork only): error_summary param #} + {# TODO: upstream contrib?? #} + {% block errors %}{{ form.errors(errors=errors, error_summary=error_summary) }}{% endblock %} diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 14bd44ccab0..18ea6f5d3ad 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -254,6 +254,22 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: except ValidationError as e: errors = e.error_dict error_summary = e.error_summary + # (canada fork only): handle other "broken" resources + # TODO: upstream contrib?? + if 'resources' in errors and isinstance(errors['resources'], dict): + error_summary = _('Could not create or update resource because other resources ' + 'in this dataset have errors:') + '\n' + for err_res_id, res_errors in errors['resources'].items(): + if res_errors: + errored_resource = get_action('resource_show')(context, {'id': err_res_id}) + error_summary += '\n- [{}]({})'.format( + h.get_translated(errored_resource, 'name'), + h.url_for('resource.read', id=errored_resource['package_id'], + resource_id=errored_resource['id'])) + for field_key, field_errs in res_errors.items(): + error_summary += '\n - {}{} {}'.format(field_key, _(':'), ', '.join([_(err) for err in field_errs])) + errors = None + error_summary = h.render_markdown(error_summary, allow_html=True) if data.get(u'url_type') == u'upload' and data.get(u'url'): data[u'url'] = u'' data[u'url_type'] = u'' @@ -380,6 +396,22 @@ def post(self, package_type: str, id: str, except ValidationError as e: errors = e.error_dict error_summary = e.error_summary + # (canada fork only): handle other "broken" resources + # TODO: upstream contrib?? + if 'resources' in errors and isinstance(errors['resources'], dict): + error_summary = _('Could not create or update resource because other resources ' + 'in this dataset have errors:') + '\n' + for err_res_id, res_errors in errors['resources'].items(): + if res_errors: + errored_resource = get_action('resource_show')(context, {'id': err_res_id}) + error_summary += '\n- [{}]({})'.format( + h.get_translated(errored_resource, 'name'), + h.url_for('resource.read', id=errored_resource['package_id'], + resource_id=errored_resource['id'])) + for field_key, field_errs in res_errors.items(): + error_summary += '\n - {}{} {}'.format(field_key, _(':'), ', '.join([_(err) for err in field_errs])) + errors = None + error_summary = h.render_markdown(error_summary, auto_link=False) return self.get( package_type, id, resource_id, data, errors, error_summary ) @@ -478,6 +510,30 @@ def post(self, package_type: str, id: str, resource_id: str) -> Response: ) else: return h.redirect_to(u'{}.read'.format(package_type), id=id) + # (canada fork only): handle validation errors + # TODO: upstream contrib?? + except ValidationError as e: + errors = e.error_dict + error_summary = e.error_summary + # (canada fork only): handle other "broken" resources + # TODO: upstream contrib?? + if 'resources' in errors and isinstance(errors['resources'], dict): + error_summary = _('Could not delete resource because other resources ' + 'in this dataset have errors:') + '\n' + for err_res_id, res_errors in errors['resources'].items(): + if res_errors: + errored_resource = get_action('resource_show')(context, {'id': err_res_id}) + error_summary += '\n- [{}]({})'.format( + h.get_translated(errored_resource, 'name'), + h.url_for('resource.read', id=errored_resource['package_id'], + resource_id=errored_resource['id'])) + for field_key, field_errs in res_errors.items(): + error_summary += '\n - {}{} {}'.format(field_key, _(':'), ', '.join([_(err) for err in field_errs])) + errors = None + error_summary = h.render_markdown(error_summary, auto_link=False) + return self.get( + package_type, id, resource_id, errors, error_summary + ) except NotAuthorized: return base.abort( 403, @@ -486,7 +542,11 @@ def post(self, package_type: str, id: str, resource_id: str) -> Response: except NotFound: return base.abort(404, _(u'Resource not found')) - def get(self, package_type: str, id: str, resource_id: str) -> str: + # (canada fork only): handle validation errors + # TODO: upstream contrib?? + def get(self, package_type: str, id: str, resource_id: str, + errors: Optional[dict[str, Any]] = None, + error_summary: Optional[dict[str, Any]] = None) -> str: context = self._prepare(id) try: resource_dict = get_action(u'resource_show')( @@ -509,6 +569,10 @@ def get(self, package_type: str, id: str, resource_id: str) -> str: return base.render( u'package/confirm_delete_resource.html', { + # (canada fork only): handle validation errors + # TODO: upstream contrib?? + 'errors': errors, + 'error_summary': error_summary, u'dataset_type': _get_package_type(id), u'resource_dict': resource_dict, u'pkg_id': pkg_id From 6446a372b6a06beaee6db44d36525754d191f0bb Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Thu, 30 Jan 2025 20:56:08 +0000 Subject: [PATCH 2/9] feat(misc): changelog; - Added change log file. --- changes/189.canada.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/189.canada.feature diff --git a/changes/189.canada.feature b/changes/189.canada.feature new file mode 100644 index 00000000000..9567d69a4ed --- /dev/null +++ b/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. From 978f6856da5ce61ec97df3334537e9aa19591fad Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Tue, 4 Feb 2025 21:53:04 +0000 Subject: [PATCH 3/9] feat(dev): cont res validation; - Better handling of resource validation errors. --- ckan/logic/action/__init__.py | 37 +++++++++++++++++++- ckan/logic/action/create.py | 24 ++++--------- ckan/logic/action/delete.py | 24 ++++--------- ckan/logic/action/update.py | 24 ++++--------- ckan/templates/macros/form/errors.html | 31 ++++++++--------- ckan/views/resource.py | 48 -------------------------- 6 files changed, 72 insertions(+), 116 deletions(-) diff --git a/ckan/logic/action/__init__.py b/ckan/logic/action/__init__.py index 7196d357401..80e5aa07162 100644 --- a/ckan/logic/action/__init__.py +++ b/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,36 @@ 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, str], str]: + """ + Checks through the error_dict to find all errors in + the Dataset and its Resources. + """ + new_error_dict = dict(error_dict) + try: + 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 not current_res_error_dict and 'resources' in error_dict and isinstance(error_dict['resources'], list): + new_error_dict = {'errors': {'resources': {}}} + new_error_dict['action'] = action + for key, res_error_dict in enumerate(error_dict['resources']): + if key <= len(pkg_dict['resources']): + errored_resource = pkg_dict['resources'][key] + if errored_resource.get('id'): + new_error_dict['errors']['resources'][errored_resource.get('id')] = res_error_dict + new_error_dict['other_resource'] = True + except (KeyError, IndexError): + new_error_dict = dict(error_dict) + return new_error_dict diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 494f1321bfa..89828a5b2e9 100644 --- a/ckan/logic/action/create.py +++ b/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,24 +332,11 @@ def resource_create(context: Context, _get_action('package_update')(context, pkg_dict) context.pop('defer_commit') except ValidationError as e: - # (canada fork only): handle other "broken" resources + # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - error_summary = '' - try: - error_dict = cast("list[ErrorDict]", e.error_dict['resources'])[-1] - if not error_dict and 'resources' in e.error_dict and isinstance(e.error_dict['resources'], list): - error_dict = {'resources': {}} - for key, res_error_dict in enumerate(e.error_dict['resources']): - if key <= len(pkg_dict['resources']): - errored_resource = pkg_dict['resources'][key] - if errored_resource.get('id'): - error_dict['resources'][errored_resource.get('id')] = res_error_dict - if res_error_dict: - error_summary += _('Could not create or update resource because another resource in ' - 'this dataset has errors: {}; ').format(errored_resource['id']) - except (KeyError, IndexError): - error_dict = e.error_dict - raise ValidationError(error_dict, error_summary) + error_dict = resource_validation_errors( + e.error_dict, action='create', pkg_dict=pkg_dict) + raise ValidationError(error_dict) # Get out resource_id resource from model as it will not appear in # package_show until after commit diff --git a/ckan/logic/action/delete.py b/ckan/logic/action/delete.py index 02812ab7d32..08bfb122d31 100644 --- a/ckan/logic/action/delete.py +++ b/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,24 +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: - # (canada fork only): handle other "broken" resources + # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - error_dict = e.error_dict - error_summary = '' - try: - if 'resources' in e.error_dict and isinstance(e.error_dict['resources'], list): - error_dict = {'resources': {}} - for key, res_error_dict in enumerate(e.error_dict['resources']): - if key <= len(pkg_dict['resources']): - errored_resource = pkg_dict['resources'][key] - if errored_resource.get('id'): - error_dict['resources'][errored_resource.get('id')] = res_error_dict - if res_error_dict: - error_summary += _('Could not delete resource because another resource in ' - 'this dataset has errors: {}; ').format(errored_resource['id']) - except (KeyError, IndexError): - error_dict = e.error_dict - raise ValidationError(error_dict, error_summary) + error_dict = resource_validation_errors( + e.error_dict, action='create', pkg_dict=pkg_dict) + raise ValidationError(error_dict) for plugin in plugins.PluginImplementations(plugins.IResourceController): plugin.after_resource_delete(context, pkg_dict.get('resources', [])) diff --git a/ckan/logic/action/update.py b/ckan/logic/action/update.py index 4a647c454ae..31a6a0fd0d6 100644 --- a/ckan/logic/action/update.py +++ b/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,24 +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: - # (canada fork only): handle other "broken" resources + # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - error_summary = '' - try: - error_dict = cast("list[ErrorDict]", e.error_dict['resources'])[n] - if not error_dict and 'resources' in e.error_dict and isinstance(e.error_dict['resources'], list): - error_dict = {'resources': {}} - for key, res_error_dict in enumerate(e.error_dict['resources']): - if key <= len(pkg_dict['resources']): - errored_resource = pkg_dict['resources'][key] - if errored_resource.get('id'): - error_dict['resources'][errored_resource.get('id')] = res_error_dict - if res_error_dict: - error_summary += _('Could not create or update resource because another resource in ' - 'this dataset has errors: {}; ').format(errored_resource['id']) - except (KeyError, IndexError): - error_dict = e.error_dict - raise ValidationError(error_dict, error_summary) + error_dict = resource_validation_errors( + e.error_dict, action='update', pkg_dict=pkg_dict, resource_index=n) + raise ValidationError(error_dict) resource = _get_action('resource_show')(context, {'id': id}) diff --git a/ckan/templates/macros/form/errors.html b/ckan/templates/macros/form/errors.html index 217c674b100..0d6c11307f1 100644 --- a/ckan/templates/macros/form/errors.html +++ b/ckan/templates/macros/form/errors.html @@ -13,24 +13,23 @@ #} -{# (canada fork only): actually error summary and error dict handling #} +{# (canada fork only): handle all errors in resource actions #} {# TODO: upstream contrib?? #} {# (canada fork only): error -> danger BS version #} -{% macro errors(errors={}, type="danger", classes=[], error_summary="") %} -{% if error_summary %} -
-

{{_('Errors')}}

-

{{ error_summary }}

-
-{% endif %} +{% macro errors(errors={}, type="danger", classes=[]) %} {% if errors %} -
-

{{ _('The form contains invalid entries:') }}

-
    - {% for key, error in errors.items() %} -
  • {% if key %}{{ key }}: {% endif %}{{ error }}
    • - {% endfor %} -
-
+
+ {% if not errors.other_resource %} +

{{ _('The form contains invalid entries:') }}

+
    + {% for key, error in errors.items() %} +
  • {% if key %}{{ key }}: {% endif %}{{ error }}
    • + {% endfor %} +
+ {% else %} +

{{ _('The dataset contains invalid resources:') }}

+ {# TODO: loop errors and output HTML #} + {% endif %} +
{% endif %} {% endmacro %} diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 18ea6f5d3ad..c5099e65099 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -254,22 +254,6 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: except ValidationError as e: errors = e.error_dict error_summary = e.error_summary - # (canada fork only): handle other "broken" resources - # TODO: upstream contrib?? - if 'resources' in errors and isinstance(errors['resources'], dict): - error_summary = _('Could not create or update resource because other resources ' - 'in this dataset have errors:') + '\n' - for err_res_id, res_errors in errors['resources'].items(): - if res_errors: - errored_resource = get_action('resource_show')(context, {'id': err_res_id}) - error_summary += '\n- [{}]({})'.format( - h.get_translated(errored_resource, 'name'), - h.url_for('resource.read', id=errored_resource['package_id'], - resource_id=errored_resource['id'])) - for field_key, field_errs in res_errors.items(): - error_summary += '\n - {}{} {}'.format(field_key, _(':'), ', '.join([_(err) for err in field_errs])) - errors = None - error_summary = h.render_markdown(error_summary, allow_html=True) if data.get(u'url_type') == u'upload' and data.get(u'url'): data[u'url'] = u'' data[u'url_type'] = u'' @@ -396,22 +380,6 @@ def post(self, package_type: str, id: str, except ValidationError as e: errors = e.error_dict error_summary = e.error_summary - # (canada fork only): handle other "broken" resources - # TODO: upstream contrib?? - if 'resources' in errors and isinstance(errors['resources'], dict): - error_summary = _('Could not create or update resource because other resources ' - 'in this dataset have errors:') + '\n' - for err_res_id, res_errors in errors['resources'].items(): - if res_errors: - errored_resource = get_action('resource_show')(context, {'id': err_res_id}) - error_summary += '\n- [{}]({})'.format( - h.get_translated(errored_resource, 'name'), - h.url_for('resource.read', id=errored_resource['package_id'], - resource_id=errored_resource['id'])) - for field_key, field_errs in res_errors.items(): - error_summary += '\n - {}{} {}'.format(field_key, _(':'), ', '.join([_(err) for err in field_errs])) - errors = None - error_summary = h.render_markdown(error_summary, auto_link=False) return self.get( package_type, id, resource_id, data, errors, error_summary ) @@ -515,22 +483,6 @@ def post(self, package_type: str, id: str, resource_id: str) -> Response: except ValidationError as e: errors = e.error_dict error_summary = e.error_summary - # (canada fork only): handle other "broken" resources - # TODO: upstream contrib?? - if 'resources' in errors and isinstance(errors['resources'], dict): - error_summary = _('Could not delete resource because other resources ' - 'in this dataset have errors:') + '\n' - for err_res_id, res_errors in errors['resources'].items(): - if res_errors: - errored_resource = get_action('resource_show')(context, {'id': err_res_id}) - error_summary += '\n- [{}]({})'.format( - h.get_translated(errored_resource, 'name'), - h.url_for('resource.read', id=errored_resource['package_id'], - resource_id=errored_resource['id'])) - for field_key, field_errs in res_errors.items(): - error_summary += '\n - {}{} {}'.format(field_key, _(':'), ', '.join([_(err) for err in field_errs])) - errors = None - error_summary = h.render_markdown(error_summary, auto_link=False) return self.get( package_type, id, resource_id, errors, error_summary ) From 7c9d10dffff8a481587f94163ef5ede9139ca8f7 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Wed, 5 Feb 2025 18:00:27 +0000 Subject: [PATCH 4/9] feat(dev): resource errors; - Handle all dataset errors in resource actions. --- ckan/logic/action/__init__.py | 52 ++++++++++++++++--- ckan/logic/action/create.py | 4 +- ckan/logic/action/delete.py | 6 +-- ckan/logic/action/update.py | 17 ++++-- .../javascript/modules/resource-reorder.js | 23 +++++++- ckan/templates/macros/form.html | 6 +++ .../templates/macros/form/dataset_errors.html | 51 ++++++++++++++++++ ckan/templates/macros/form/errors.html | 24 +++------ .../package/confirm_delete_resource.html | 10 +++- ckan/templates/package/new_resource.html | 4 +- .../package/new_resource_not_draft.html | 4 +- ckan/templates/package/resource_edit.html | 3 ++ .../package/snippets/resource_form.html | 10 +++- ckan/views/resource.py | 33 ++++++++++++ 14 files changed, 210 insertions(+), 37 deletions(-) create mode 100644 ckan/templates/macros/form/dataset_errors.html diff --git a/ckan/logic/action/__init__.py b/ckan/logic/action/__init__.py index 80e5aa07162..b08b2d2280d 100644 --- a/ckan/logic/action/__init__.py +++ b/ckan/logic/action/__init__.py @@ -83,12 +83,15 @@ def resource_validation_errors( error_dict: ErrorDict, action: str, pkg_dict: Dict[str, Any], - resource_index: int = -1) -> Tuple[Dict[str, str], str]: + resource_index: int = -1) -> Tuple[Dict[str, Any], str]: """ Checks through the error_dict to find all errors in the Dataset and its Resources. """ new_error_dict = dict(error_dict) + # define out own error summaries so we can have + # a more customized structure to the ErrorDict + error_summaries = [] try: if action == 'delete': # special case for deleting as there is no index @@ -96,15 +99,52 @@ def resource_validation_errors( 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 = current_res_error_dict if not current_res_error_dict and 'resources' in error_dict and isinstance(error_dict['resources'], list): - new_error_dict = {'errors': {'resources': {}}} - new_error_dict['action'] = action + # compile the other resource errors + new_error_dict = {'resources': {}} for key, res_error_dict in enumerate(error_dict['resources']): if key <= len(pkg_dict['resources']): errored_resource = pkg_dict['resources'][key] if errored_resource.get('id'): - new_error_dict['errors']['resources'][errored_resource.get('id')] = res_error_dict - new_error_dict['other_resource'] = True + 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')) except (KeyError, IndexError): new_error_dict = dict(error_dict) - return new_error_dict + return new_error_dict, '; '.join(error_summaries) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 89828a5b2e9..8984d218f52 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -334,9 +334,9 @@ def resource_create(context: Context, except ValidationError as e: # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - error_dict = resource_validation_errors( + error_dict, error_summary = resource_validation_errors( e.error_dict, action='create', pkg_dict=pkg_dict) - raise ValidationError(error_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 diff --git a/ckan/logic/action/delete.py b/ckan/logic/action/delete.py index 08bfb122d31..f257706ec74 100644 --- a/ckan/logic/action/delete.py +++ b/ckan/logic/action/delete.py @@ -200,9 +200,9 @@ def resource_delete(context: Context, data_dict: DataDict) -> ActionResult.Resou except ValidationError as e: # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - error_dict = resource_validation_errors( - e.error_dict, action='create', pkg_dict=pkg_dict) - raise ValidationError(error_dict) + 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', [])) diff --git a/ckan/logic/action/update.py b/ckan/logic/action/update.py index 31a6a0fd0d6..ebb6ed1f07f 100644 --- a/ckan/logic/action/update.py +++ b/ckan/logic/action/update.py @@ -119,9 +119,9 @@ def resource_update(context: Context, data_dict: DataDict) -> ActionResult.Resou except ValidationError as e: # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - error_dict = resource_validation_errors( + error_dict, error_summary = resource_validation_errors( e.error_dict, action='update', pkg_dict=pkg_dict, resource_index=n) - raise ValidationError(error_dict) + raise ValidationError(error_dict, error_summary=error_summary) resource = _get_action('resource_show')(context, {'id': id}) @@ -582,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( diff --git a/ckan/public/base/javascript/modules/resource-reorder.js b/ckan/public/base/javascript/modules/resource-reorder.js index 83be73e7833..aa590968a6b 100644 --- a/ckan/public/base/javascript/modules/resource-reorder.js +++ b/ckan/public/base/javascript/modules/resource-reorder.js @@ -30,6 +30,14 @@ this.ckan.module('resource-reorder', function($) { '', '', '' + ].join('\n'), + // (canada fork only): handle all errors in resource actions + // TODO: upstream contrib?? + errors: [ + '
', + '

', + '

', + '
' ].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(); diff --git a/ckan/templates/macros/form.html b/ckan/templates/macros/form.html index f33b7ab812d..dfd86b5ceb8 100644 --- a/ckan/templates/macros/form.html +++ b/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/dataset_errors.html" import dataset_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 dataset_errors = dataset_errors %} {% set info = info %} {% set hidden = hidden %} {% set hidden_from_list = hidden_from_list %} diff --git a/ckan/templates/macros/form/dataset_errors.html b/ckan/templates/macros/form/dataset_errors.html new file mode 100644 index 00000000000..6792cc2b8ef --- /dev/null +++ b/ckan/templates/macros/form/dataset_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 field/message pairs. +type - The alert-* class that should be applied (default: "error") +classes - A list of classes to apply to the wrapper (default: []) + +Example: + +{% import 'macros/form.html' as form %} +{{ form.errors(error_summary, type="warning") }} + +#} + + +{# (canada fork only): error -> danger BS version #} +{% macro dataset_errors(errors={}, type="danger", classes=[], pkg_name='', helpers={}) %} + {% if errors %} +
+

{{_('Errors in dataset')}}

+ {% if 'dataset' in errors and errors.dataset %} + {% set dataset_link = helpers.url_for('dataset.read', id=pkg_name) %} +

{{ _('The dataset contains errors:').format(dataset_link) }}

+
    + {% for field_id, field_errs in errors.dataset.items() %} +
  • {% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}
    • + {% endfor %} +
+ {% endif %} + {% if 'resources' in errors and errors.resources %} +

{{ _('The dataset contains invalid resources:') }}

+
    + {% for res_id, errs in errors.resources.items() %} + {% if not errs %} + {% continue %} + {% endif %} +
  • {{ _('Resource') ~ ' ' ~ loop.index }}
    • +
      + {% for field_id, field_errs in errs.items() %} +
    • {% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}
      • + {% endfor %} +
    + {% endfor %} +
+ {% endif %} +
+ {% endif %} +{% endmacro %} diff --git a/ckan/templates/macros/form/errors.html b/ckan/templates/macros/form/errors.html index 0d6c11307f1..7f144a3abd9 100644 --- a/ckan/templates/macros/form/errors.html +++ b/ckan/templates/macros/form/errors.html @@ -12,24 +12,16 @@ #} - -{# (canada fork only): handle all errors in resource actions #} -{# TODO: upstream contrib?? #} {# (canada fork only): error -> danger BS version #} {% macro errors(errors={}, type="danger", classes=[]) %} {% if errors %} -
- {% if not errors.other_resource %} -

{{ _('The form contains invalid entries:') }}

-
    - {% for key, error in errors.items() %} -
  • {% if key %}{{ key }}: {% endif %}{{ error }}
    • - {% endfor %} -
- {% else %} -

{{ _('The dataset contains invalid resources:') }}

- {# TODO: loop errors and output HTML #} - {% endif %} -
+
+

{{ _('The form contains invalid entries:') }}

+
    + {% for key, error in errors.items() %} +
  • {% if key %}{{ key }}: {% endif %}{{ error }}
    • + {% endfor %} +
+
{% endif %} {% endmacro %} diff --git a/ckan/templates/package/confirm_delete_resource.html b/ckan/templates/package/confirm_delete_resource.html index f0b13fc7ade..a8b94ba12ac 100644 --- a/ckan/templates/package/confirm_delete_resource.html +++ b/ckan/templates/package/confirm_delete_resource.html @@ -14,7 +14,15 @@ {% block form %} {# (canada fork only): handle validation errors #} {# TODO: upstream contrib?? #} - {% block errors %}{{ form.errors(errors=errors, error_summary=error_summary) }}{% endblock %} + {# (canada fork only): handle all errors in resource actions #} + {# TODO: upstream contrib?? #} + {% block errors %} + {% if dataset_errors %} + {{ form.dataset_errors(errors=dataset_errors, pkg_name=pkg_id, helpers=h) }} + {% else %} + {{ form.errors(errors=errors) }} + {% endif %} + {% endblock %}

{{ _('Are you sure you want to delete resource - {name}?').format(name=h.resource_display_name(resource_dict)) }}

diff --git a/ckan/templates/package/new_resource.html b/ckan/templates/package/new_resource.html index 370df275a36..1c7ab1bfcb9 100644 --- a/ckan/templates/package/new_resource.html +++ b/ckan/templates/package/new_resource.html @@ -12,7 +12,9 @@ {% endif %} {% endblock %} -{% block form %}{% snippet resource_form_snippet, data=data, errors=errors, error_summary=error_summary, include_metadata=false, pkg_name=pkg_name, stage=stage, dataset_type=dataset_type %}{% endblock %} +{# (canada fork only): handle all errors in resource actions #} +{# TODO: upstream contrib?? #} +{% block form %}{% snippet resource_form_snippet, data=data, errors=errors, error_summary=error_summary, dataset_errors=dataset_errors, include_metadata=false, pkg_name=pkg_name, stage=stage, dataset_type=dataset_type %}{% endblock %} {% block secondary_content %} {% snippet 'package/snippets/resource_help.html' %} diff --git a/ckan/templates/package/new_resource_not_draft.html b/ckan/templates/package/new_resource_not_draft.html index 4ab9f93e476..e139713f9a3 100644 --- a/ckan/templates/package/new_resource_not_draft.html +++ b/ckan/templates/package/new_resource_not_draft.html @@ -9,7 +9,9 @@ {% endblock %} {% block form %} - {% snippet resource_form_snippet, data=data, errors=errors, error_summary=error_summary, include_metadata=false, pkg_name=pkg_name, stage=stage, dataset_type=dataset_type %} + {# (canada fork only): handle all errors in resource actions #} + {# TODO: upstream contrib?? #} + {% snippet resource_form_snippet, data=data, errors=errors, error_summary=error_summary, dataset_errors=dataset_errors, include_metadata=false, pkg_name=pkg_name, stage=stage, dataset_type=dataset_type %} {% endblock %} {% block content_primary_nav %} diff --git a/ckan/templates/package/resource_edit.html b/ckan/templates/package/resource_edit.html index cb482bdb19d..6d7736d3b62 100644 --- a/ckan/templates/package/resource_edit.html +++ b/ckan/templates/package/resource_edit.html @@ -3,10 +3,13 @@ {% block subtitle %}{{ _('Edit') }} {{ g.template_title_delimiter }} {{ h.resource_display_name(res) }} {{ g.template_title_delimiter }} {{ h.dataset_display_name(pkg) }}{% endblock %} {% block form %} + {# (canada fork only): handle all errors in resource actions #} + {# TODO: upstream contrib?? #} {% snippet 'package/snippets/resource_edit_form.html', data=data, errors=errors, error_summary=error_summary, + dataset_errors=dataset_errors, pkg_name=pkg.name, form_action=form_action, resource_form_snippet=resource_form_snippet, diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index ae81c74255a..19ce853b174 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -14,9 +14,15 @@ {% endif %} {% endblock %} - {# (canada fork only): error_summary param #} + {# (canada fork only): handle all errors in resource actions #} {# TODO: upstream contrib?? #} - {% block errors %}{{ form.errors(errors=errors, error_summary=error_summary) }}{% endblock %} + {% block errors %} + {% if dataset_errors %} + {{ form.dataset_errors(errors=dataset_errors, pkg_name=pkg_name, helpers=h) }} + {% else %} + {{ form.errors(errors=errors) }} + {% endif %} + {% endblock %} diff --git a/ckan/views/resource.py b/ckan/views/resource.py index c5099e65099..415decb5001 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -316,12 +316,23 @@ def get(self, package_type = pkg_dict[u'type'] or package_type + # (canada fork only): handle all errors in resource actions + # TODO: upstream contrib?? + # we do this here to avoid issues with ckanext-scheming support + dataset_errors = {} + if errors and 'resources' in errors and isinstance(errors['resources'], dict): + dataset_errors = dict(errors) + errors = None + errors = errors or {} error_summary = error_summary or {} extra_vars: dict[str, Any] = { u'data': data, u'errors': errors, u'error_summary': error_summary, + # (canada fork only): handle all errors in resource actions + # TODO: upstream contrib?? + 'dataset_errors': dataset_errors, u'action': u'new', u'resource_form_snippet': _get_pkg_template( u'resource_form', package_type @@ -424,12 +435,23 @@ def get(self, package_type = pkg_dict[u'type'] or package_type + # (canada fork only): handle all errors in resource actions + # TODO: upstream contrib?? + # we do this here to avoid issues with ckanext-scheming support + dataset_errors = {} + if errors and 'resources' in errors and isinstance(errors['resources'], dict): + dataset_errors = dict(errors) + errors = None + errors = errors or {} error_summary = error_summary or {} extra_vars: dict[str, Any] = { u'data': data, u'errors': errors, u'error_summary': error_summary, + # (canada fork only): handle all errors in resource actions + # TODO: upstream contrib?? + 'dataset_errors': dataset_errors, u'action': u'edit', u'resource_form_snippet': _get_pkg_template( u'resource_form', package_type @@ -519,12 +541,23 @@ def get(self, package_type: str, id: str, resource_id: str, g.resource_dict = resource_dict g.pkg_id = pkg_id + # (canada fork only): handle all errors in resource actions + # TODO: upstream contrib?? + # we do this here to avoid issues with ckanext-scheming support + dataset_errors = {} + if errors and 'resources' in errors and isinstance(errors['resources'], dict): + dataset_errors = dict(errors) + errors = None + return base.render( u'package/confirm_delete_resource.html', { # (canada fork only): handle validation errors # TODO: upstream contrib?? 'errors': errors, 'error_summary': error_summary, + # (canada fork only): handle all errors in resource actions + # TODO: upstream contrib?? + 'dataset_errors': dataset_errors, u'dataset_type': _get_package_type(id), u'resource_dict': resource_dict, u'pkg_id': pkg_id From edb59d8c73a1bf210054a6e32f767d6e43561c94 Mon Sep 17 00:00:00 2001 From: Jesse Vickery <97247789+JVickery-TBS@users.noreply.github.com> Date: Fri, 7 Feb 2025 11:19:29 -0500 Subject: [PATCH 5/9] Update ckan/logic/action/__init__.py Co-authored-by: Ian Ward --- ckan/logic/action/__init__.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ckan/logic/action/__init__.py b/ckan/logic/action/__init__.py index b08b2d2280d..16c15abfdd3 100644 --- a/ckan/logic/action/__init__.py +++ b/ckan/logic/action/__init__.py @@ -85,8 +85,10 @@ def resource_validation_errors( pkg_dict: Dict[str, Any], resource_index: int = -1) -> Tuple[Dict[str, Any], str]: """ - Checks through the error_dict to find all errors in - the Dataset and its Resources. + 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 From 632eebfc0aa29acb3135832cadc643f755857d2d Mon Sep 17 00:00:00 2001 From: Jesse Vickery <97247789+JVickery-TBS@users.noreply.github.com> Date: Fri, 7 Feb 2025 11:19:54 -0500 Subject: [PATCH 6/9] Update ckan/logic/action/__init__.py Co-authored-by: Ian Ward --- ckan/logic/action/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/logic/action/__init__.py b/ckan/logic/action/__init__.py index 16c15abfdd3..a617acc6444 100644 --- a/ckan/logic/action/__init__.py +++ b/ckan/logic/action/__init__.py @@ -104,7 +104,7 @@ def resource_validation_errors( if current_res_error_dict: # if there are errors for the current resource # let's raise them to the user first. - new_error_dict = current_res_error_dict + 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': {}} From dc4633cb9d0185d9ef6cb695c6534a19661937fa Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Fri, 7 Feb 2025 18:52:02 +0000 Subject: [PATCH 7/9] refactor(dev): errors; - `dataset_errors` -> `resource_validation_errors` - Humanize labels. - Pass pkg_dict into resource confirm delete. - Pass pkg_dict into resource form. - Remove try/catch. - Do not store empty resource validation error dicts. --- ckan/lib/helpers.py | 5 + ckan/logic/action/__init__.py | 111 +++++++++--------- ckan/templates/macros/form.html | 4 +- ...s.html => resource_validation_errors.html} | 13 +- .../package/confirm_delete_resource.html | 4 +- ckan/templates/package/new_resource.html | 2 +- .../package/new_resource_not_draft.html | 2 +- ckan/templates/package/resource_edit.html | 3 +- .../package/snippets/resource_form.html | 4 +- ckan/views/resource.py | 23 ++-- 10 files changed, 89 insertions(+), 82 deletions(-) rename ckan/templates/macros/form/{dataset_errors.html => resource_validation_errors.html} (70%) diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 19347aba70f..02ef5b155a8 100644 --- a/ckan/lib/helpers.py +++ b/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() diff --git a/ckan/logic/action/__init__.py b/ckan/logic/action/__init__.py index a617acc6444..024e0df4882 100644 --- a/ckan/logic/action/__init__.py +++ b/ckan/logic/action/__init__.py @@ -94,59 +94,58 @@ def resource_validation_errors( # define out own error summaries so we can have # a more customized structure to the ErrorDict error_summaries = [] - try: - 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 key <= len(pkg_dict['resources']): - 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')) - except (KeyError, IndexError): - new_error_dict = dict(error_dict) - return new_error_dict, '; '.join(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) diff --git a/ckan/templates/macros/form.html b/ckan/templates/macros/form.html index dfd86b5ceb8..11f08c7e4a7 100644 --- a/ckan/templates/macros/form.html +++ b/ckan/templates/macros/form.html @@ -15,7 +15,7 @@ {% from "macros/form/errors.html" import errors %} {# (canada fork only): handle all errors in resource actions #} {# TODO: upstream contrib?? #} -{% from "macros/form/dataset_errors.html" import dataset_errors %} +{% 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 %} @@ -34,7 +34,7 @@ {% set errors = errors %} {# (canada fork only): handle all errors in resource actions #} {# TODO: upstream contrib?? #} -{% set dataset_errors = dataset_errors %} +{% set resource_validation_errors = resource_validation_errors %} {% set info = info %} {% set hidden = hidden %} {% set hidden_from_list = hidden_from_list %} diff --git a/ckan/templates/macros/form/dataset_errors.html b/ckan/templates/macros/form/resource_validation_errors.html similarity index 70% rename from ckan/templates/macros/form/dataset_errors.html rename to ckan/templates/macros/form/resource_validation_errors.html index 6792cc2b8ef..1a701edb33b 100644 --- a/ckan/templates/macros/form/dataset_errors.html +++ b/ckan/templates/macros/form/resource_validation_errors.html @@ -17,13 +17,13 @@ {# (canada fork only): error -> danger BS version #} -{% macro dataset_errors(errors={}, type="danger", classes=[], pkg_name='', helpers={}) %} +{% macro resource_validation_errors(errors={}, type="danger", classes=[], pkg_name='', helpers={}) %} {% if errors %}

-

{{_('Errors in dataset')}}

+

{{ h.humanize_entity_type('package', dataset_type, 'other errors') or _('Errors in dataset') }}

{% if 'dataset' in errors and errors.dataset %} {% set dataset_link = helpers.url_for('dataset.read', id=pkg_name) %} -

{{ _('The dataset contains errors:').format(dataset_link) }}

+

{{ h.humanize_entity_type('package', dataset_type, 'other errors package').format(dataset_link) or _('The dataset contains errors:').format(dataset_link) }}

    {% for field_id, field_errs in errors.dataset.items() %}
  • {% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}
    • @@ -31,13 +31,10 @@

    {{_('Errors in dataset')}}

{% endif %} {% if 'resources' in errors and errors.resources %} -

{{ _('The dataset contains invalid resources:') }}

+

{{ h.humanize_entity_type('package', dataset_type, 'other errors resources') or _('The dataset contains invalid resources:') }}

    {% for res_id, errs in errors.resources.items() %} - {% if not errs %} - {% continue %} - {% endif %} -
  • {{ _('Resource') ~ ' ' ~ loop.index }}
    • +
  • {{ res_id }}
      • {% for field_id, field_errs in errs.items() %}
    • {% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}
      • diff --git a/ckan/templates/package/confirm_delete_resource.html b/ckan/templates/package/confirm_delete_resource.html index a8b94ba12ac..f867880a55f 100644 --- a/ckan/templates/package/confirm_delete_resource.html +++ b/ckan/templates/package/confirm_delete_resource.html @@ -17,8 +17,8 @@ {# (canada fork only): handle all errors in resource actions #} {# TODO: upstream contrib?? #} {% block errors %} - {% if dataset_errors %} - {{ form.dataset_errors(errors=dataset_errors, pkg_name=pkg_id, helpers=h) }} + {% if resource_validation_errors %} + {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_id, helpers=h) }} {% else %} {{ form.errors(errors=errors) }} {% endif %} diff --git a/ckan/templates/package/new_resource.html b/ckan/templates/package/new_resource.html index 1c7ab1bfcb9..ed57a7aafd5 100644 --- a/ckan/templates/package/new_resource.html +++ b/ckan/templates/package/new_resource.html @@ -14,7 +14,7 @@ {# (canada fork only): handle all errors in resource actions #} {# TODO: upstream contrib?? #} -{% block form %}{% snippet resource_form_snippet, data=data, errors=errors, error_summary=error_summary, dataset_errors=dataset_errors, include_metadata=false, pkg_name=pkg_name, stage=stage, dataset_type=dataset_type %}{% endblock %} +{% block form %}{% snippet resource_form_snippet, data=data, errors=errors, error_summary=error_summary, resource_validation_errors=resource_validation_errors, include_metadata=false, pkg_name=pkg_name, stage=stage, dataset_type=dataset_type %}{% endblock %} {% block secondary_content %} {% snippet 'package/snippets/resource_help.html' %} diff --git a/ckan/templates/package/new_resource_not_draft.html b/ckan/templates/package/new_resource_not_draft.html index e139713f9a3..34ac9d7b5cd 100644 --- a/ckan/templates/package/new_resource_not_draft.html +++ b/ckan/templates/package/new_resource_not_draft.html @@ -11,7 +11,7 @@ {% block form %} {# (canada fork only): handle all errors in resource actions #} {# TODO: upstream contrib?? #} - {% snippet resource_form_snippet, data=data, errors=errors, error_summary=error_summary, dataset_errors=dataset_errors, include_metadata=false, pkg_name=pkg_name, stage=stage, dataset_type=dataset_type %} + {% snippet resource_form_snippet, data=data, errors=errors, error_summary=error_summary, resource_validation_errors=resource_validation_errors, include_metadata=false, pkg_name=pkg_name, stage=stage, dataset_type=dataset_type %} {% endblock %} {% block content_primary_nav %} diff --git a/ckan/templates/package/resource_edit.html b/ckan/templates/package/resource_edit.html index 6d7736d3b62..e5950914893 100644 --- a/ckan/templates/package/resource_edit.html +++ b/ckan/templates/package/resource_edit.html @@ -9,7 +9,8 @@ data=data, errors=errors, error_summary=error_summary, - dataset_errors=dataset_errors, + resource_validation_errors=resource_validation_errors, + pkg_dict=pkg_dict, pkg_name=pkg.name, form_action=form_action, resource_form_snippet=resource_form_snippet, diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index 19ce853b174..42dd9422e80 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -17,8 +17,8 @@ {# (canada fork only): handle all errors in resource actions #} {# TODO: upstream contrib?? #} {% block errors %} - {% if dataset_errors %} - {{ form.dataset_errors(errors=dataset_errors, pkg_name=pkg_name, helpers=h) }} + {% if resource_validation_errors %} + {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_name, helpers=h) }} {% else %} {{ form.errors(errors=errors) }} {% endif %} diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 415decb5001..a6779359333 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -319,9 +319,9 @@ def get(self, # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? # we do this here to avoid issues with ckanext-scheming support - dataset_errors = {} + resource_validation_errors = {} if errors and 'resources' in errors and isinstance(errors['resources'], dict): - dataset_errors = dict(errors) + resource_validation_errors = dict(errors) errors = None errors = errors or {} @@ -332,7 +332,7 @@ def get(self, u'error_summary': error_summary, # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - 'dataset_errors': dataset_errors, + 'resource_validation_errors': resource_validation_errors, u'action': u'new', u'resource_form_snippet': _get_pkg_template( u'resource_form', package_type @@ -438,9 +438,9 @@ def get(self, # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? # we do this here to avoid issues with ckanext-scheming support - dataset_errors = {} + resource_validation_errors = {} if errors and 'resources' in errors and isinstance(errors['resources'], dict): - dataset_errors = dict(errors) + resource_validation_errors = dict(errors) errors = None errors = errors or {} @@ -451,7 +451,7 @@ def get(self, u'error_summary': error_summary, # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - 'dataset_errors': dataset_errors, + 'resource_validation_errors': resource_validation_errors, u'action': u'edit', u'resource_form_snippet': _get_pkg_template( u'resource_form', package_type @@ -528,6 +528,10 @@ def get(self, package_type: str, id: str, resource_id: str, u'id': resource_id } ) + # (canada fork only): handle all errors in resource actions + # TODO: upstream contrib?? + pkg_dict = get_action('package_show')( + context, {'id': id}) pkg_id = id except NotAuthorized: return base.abort( @@ -544,9 +548,9 @@ def get(self, package_type: str, id: str, resource_id: str, # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? # we do this here to avoid issues with ckanext-scheming support - dataset_errors = {} + resource_validation_errors = {} if errors and 'resources' in errors and isinstance(errors['resources'], dict): - dataset_errors = dict(errors) + resource_validation_errors = dict(errors) errors = None return base.render( @@ -557,7 +561,8 @@ def get(self, package_type: str, id: str, resource_id: str, 'error_summary': error_summary, # (canada fork only): handle all errors in resource actions # TODO: upstream contrib?? - 'dataset_errors': dataset_errors, + 'resource_validation_errors': resource_validation_errors, + 'pkg_dict': pkg_dict, u'dataset_type': _get_package_type(id), u'resource_dict': resource_dict, u'pkg_id': pkg_id From 97bcdc627a025abb325ed06e29699334fd265bf2 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Fri, 7 Feb 2025 19:10:59 +0000 Subject: [PATCH 8/9] fix(templates): syntax; - `h` -> `helpers` - Humanization fixes. --- .../macros/form/resource_validation_errors.html | 12 ++++++++---- ckan/templates/package/confirm_delete_resource.html | 2 +- ckan/templates/package/snippets/resource_form.html | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/ckan/templates/macros/form/resource_validation_errors.html b/ckan/templates/macros/form/resource_validation_errors.html index 1a701edb33b..070ea2b8a91 100644 --- a/ckan/templates/macros/form/resource_validation_errors.html +++ b/ckan/templates/macros/form/resource_validation_errors.html @@ -17,13 +17,17 @@ {# (canada fork only): error -> danger BS version #} -{% macro resource_validation_errors(errors={}, type="danger", classes=[], pkg_name='', helpers={}) %} +{% macro resource_validation_errors(errors={}, type="danger", classes=[], pkg_name='', dataset_type='dataset', helpers={}) %} {% if errors %}
      -

      {{ h.humanize_entity_type('package', dataset_type, 'other errors') or _('Errors in dataset') }}

      +

      {{ helpers.humanize_entity_type('package', dataset_type, 'other errors') or _('Errors in dataset') }}

      {% if 'dataset' in errors and errors.dataset %} {% set dataset_link = helpers.url_for('dataset.read', id=pkg_name) %} -

      {{ h.humanize_entity_type('package', dataset_type, 'other errors package').format(dataset_link) or _('The dataset contains errors:').format(dataset_link) }}

      + {% set humanized_label = helpers.humanize_entity_type('package', dataset_type, 'other errors package') %} + {% if humanized_label %} + {% set humanized_label = humanized_label.format(dataset_link)|safe %} + {% endif %} +

      {{ humanized_label or _('The dataset contains errors:').format(dataset_link) }}

        {% for field_id, field_errs in errors.dataset.items() %}
      • {% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}
        • @@ -31,7 +35,7 @@

        {{ h.humanize_entity_type('package', dataset_type, 'other errors') or _('Err

      {% endif %} {% if 'resources' in errors and errors.resources %} -

      {{ h.humanize_entity_type('package', dataset_type, 'other errors resources') or _('The dataset contains invalid resources:') }}

      +

      {{ helpers.humanize_entity_type('package', dataset_type, 'other errors resources') or _('The dataset contains invalid resources:') }}

        {% for res_id, errs in errors.resources.items() %}
      • {{ res_id }}
        • diff --git a/ckan/templates/package/confirm_delete_resource.html b/ckan/templates/package/confirm_delete_resource.html index f867880a55f..97b67151713 100644 --- a/ckan/templates/package/confirm_delete_resource.html +++ b/ckan/templates/package/confirm_delete_resource.html @@ -18,7 +18,7 @@ {# TODO: upstream contrib?? #} {% block errors %} {% if resource_validation_errors %} - {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_id, helpers=h) }} + {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_id, dataset_type=dataset_type, helpers=h) }} {% else %} {{ form.errors(errors=errors) }} {% endif %} diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index 42dd9422e80..4420486fe99 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -18,7 +18,7 @@ {# TODO: upstream contrib?? #} {% block errors %} {% if resource_validation_errors %} - {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_name, helpers=h) }} + {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_name, dataset_type=dataset_type, helpers=h) }} {% else %} {{ form.errors(errors=errors) }} {% endif %} From 1864452d18ccd2aaafaea03e6f7abdc1058ad080 Mon Sep 17 00:00:00 2001 From: Jesse Vickery Date: Mon, 10 Feb 2025 17:57:22 +0000 Subject: [PATCH 9/9] fix(templates): syntax; - `helpers` -> `h` - Humanization changes. --- ckan/lib/helpers.py | 2 +- .../form/resource_validation_errors.html | 27 +++++++++---------- .../package/confirm_delete_resource.html | 2 +- .../package/snippets/resource_form.html | 2 +- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 02ef5b155a8..5cdf2dd9e08 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -1091,7 +1091,7 @@ def humanize_entity_type(entity_type: str, object_type: str, # (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 package': _('The {object_type} contains errors:'), 'other errors resources': _('The {object_type} contains invalid resources:') } diff --git a/ckan/templates/macros/form/resource_validation_errors.html b/ckan/templates/macros/form/resource_validation_errors.html index 070ea2b8a91..e13c9411274 100644 --- a/ckan/templates/macros/form/resource_validation_errors.html +++ b/ckan/templates/macros/form/resource_validation_errors.html @@ -4,30 +4,29 @@ {# Builds a list of errors for the current form. -errors - A dict of field/message pairs. -type - The alert-* class that should be applied (default: "error") -classes - A list of classes to apply to the wrapper (default: []) +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.errors(error_summary, type="warning") }} +{{ 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', helpers={}) %} +{% macro resource_validation_errors(errors={}, type="danger", classes=[], pkg_name='', dataset_type='dataset', h={}) %} {% if errors %}
        -

        {{ helpers.humanize_entity_type('package', dataset_type, 'other errors') or _('Errors in dataset') }}

        +

        {{ h.humanize_entity_type('package', dataset_type, 'other errors') or _('Errors in dataset') }}

        {% if 'dataset' in errors and errors.dataset %} - {% set dataset_link = helpers.url_for('dataset.read', id=pkg_name) %} - {% set humanized_label = helpers.humanize_entity_type('package', dataset_type, 'other errors package') %} - {% if humanized_label %} - {% set humanized_label = humanized_label.format(dataset_link)|safe %} - {% endif %} -

        {{ humanized_label or _('The dataset contains errors:').format(dataset_link) }}

        +

        {{ h.humanize_entity_type('package', dataset_type, 'other errors package') or _('The dataset contains errors:') }}

          {% for field_id, field_errs in errors.dataset.items() %}
        • {% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}
          • @@ -35,10 +34,10 @@

          {{ helpers.humanize_entity_type('package', dataset_type, 'other errors') or

        {% endif %} {% if 'resources' in errors and errors.resources %} -

        {{ helpers.humanize_entity_type('package', dataset_type, 'other errors resources') or _('The dataset contains invalid resources:') }}

        +

        {{ h.humanize_entity_type('package', dataset_type, 'other errors resources') or _('The dataset contains invalid resources:') }}

          {% for res_id, errs in errors.resources.items() %} -
        • {{ res_id }}
          • +
        • {{ res_id }}
            • {% for field_id, field_errs in errs.items() %}
          • {% if field_id %}{{ field_id }}{{_(':')}} {% endif %}{{ field_errs|join(', ') }}
            • diff --git a/ckan/templates/package/confirm_delete_resource.html b/ckan/templates/package/confirm_delete_resource.html index 97b67151713..e7f22d2d721 100644 --- a/ckan/templates/package/confirm_delete_resource.html +++ b/ckan/templates/package/confirm_delete_resource.html @@ -18,7 +18,7 @@ {# TODO: upstream contrib?? #} {% block errors %} {% if resource_validation_errors %} - {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_id, dataset_type=dataset_type, helpers=h) }} + {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_id, dataset_type=dataset_type, h=h) }} {% else %} {{ form.errors(errors=errors) }} {% endif %} diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index 4420486fe99..9fd82f86f36 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -18,7 +18,7 @@ {# TODO: upstream contrib?? #} {% block errors %} {% if resource_validation_errors %} - {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_name, dataset_type=dataset_type, helpers=h) }} + {{ form.resource_validation_errors(errors=resource_validation_errors, pkg_name=pkg_name, dataset_type=dataset_type, h=h) }} {% else %} {{ form.errors(errors=errors) }} {% endif %}