From 449da3fc7cf89fa9601f7e860816f4c4a0bbdfcd Mon Sep 17 00:00:00 2001 From: Ian Ward Date: Sun, 30 Jun 2024 20:43:02 -0400 Subject: [PATCH 1/7] [#8308] prevent server error/race condition on publish draft --- ckan/views/resource.py | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 7f22152d6dc..715f1a86f7f 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -236,12 +236,13 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: error_summary = {_(u'Error'): msg} return self.get(package_type, id, data, errors, error_summary) - # XXX race condition if another user edits/deletes - data_dict = get_action(u'package_show')(context, {u'id': id}) - get_action(u'package_update')( - cast(Context, dict(context, allow_state_change=True)), - dict(data_dict, state=u'active') - ) + try: + get_action('package_patch')( + Context(context, allow_state_change=True), + {'id': id, 'state': 'active'} + ) + except ValidationError: + pass return h.redirect_to(u'{}.read'.format(package_type), id=id) data[u'package_id'] = id @@ -266,12 +267,13 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: 404, _(u'The dataset {id} could not be found.').format(id=id) ) if save_action == u'go-metadata': - # XXX race condition if another user edits/deletes - data_dict = get_action(u'package_show')(context, {u'id': id}) - get_action(u'package_update')( - cast(Context, dict(context, allow_state_change=True)), - dict(data_dict, state=u'active') - ) + try: + get_action('package_patch')( + Context(context, allow_state_change=True), + {'id': id, 'state': 'active'} + ) + except ValidationError: + pass return h.redirect_to(u'{}.read'.format(package_type), id=id) elif save_action == u'go-dataset': # go to first stage of add dataset From e19993131ef889db190774cc952c92901a3267ad Mon Sep 17 00:00:00 2001 From: Ian Ward Date: Sun, 28 Jul 2024 11:18:39 -0400 Subject: [PATCH 2/7] [#8308] merge new_package_form into snippets/package_form --- ckan/lib/plugins.py | 2 +- ckan/templates/package/new_package_form.html | 27 +++---------------- .../package/snippets/package_form.html | 16 ++++++++--- 3 files changed, 17 insertions(+), 28 deletions(-) diff --git a/ckan/lib/plugins.py b/ckan/lib/plugins.py index e77c264c10e..cf3d321b177 100644 --- a/ckan/lib/plugins.py +++ b/ckan/lib/plugins.py @@ -401,7 +401,7 @@ def resource_template(self) -> str: return 'package/resource_read.html' def package_form(self) -> str: - return 'package/new_package_form.html' + return 'package/snippets/package_form.html' def resource_form(self) -> str: return 'package/snippets/resource_form.html' diff --git a/ckan/templates/package/new_package_form.html b/ckan/templates/package/new_package_form.html index d6669f1f53f..9c88d906618 100644 --- a/ckan/templates/package/new_package_form.html +++ b/ckan/templates/package/new_package_form.html @@ -1,27 +1,6 @@ {% extends 'package/snippets/package_form.html' %} -{% block stages %} - {% if form_style != 'edit' %} - {{ super() }} - {% endif %} -{% endblock %} +{# this template exists for backwards compatibility with old + IDatasetForm plugins. -{% block save_button_text %} - {% if form_style != 'edit' %} - {{ super() }} - {% else %} - {{ h.humanize_entity_type('package', pkg_dict.type, 'update label') or _('Update Dataset') }} - {% endif %} -{% endblock %} - -{% block cancel_button %} - {% if form_style != 'edit' %} - {{ super() }} - {% endif %} -{% endblock %} - -{% block delete_button %} - {% if form_style == 'edit' and h.check_access('package_delete', {'id': pkg_dict.id}) %} - {{ super() }} - {% endif %} -{% endblock %} + Please extend package/snippets/package_form.html instead. #} diff --git a/ckan/templates/package/snippets/package_form.html b/ckan/templates/package/snippets/package_form.html index 3d9bfd99ec8..b0158db4e75 100644 --- a/ckan/templates/package/snippets/package_form.html +++ b/ckan/templates/package/snippets/package_form.html @@ -6,7 +6,9 @@
{{ h.csrf_input() }} {% block stages %} - {{ h.snippet('package/snippets/stages.html', stages=stage, dataset_type=dataset_type) }} + {% if form_style != 'edit' %} + {% snippet 'package/snippets/stages.html', stages=stage, dataset_type=dataset_type %} + {% endif %} {% endblock %} @@ -36,12 +38,20 @@

{% endblock %} {% block delete_button %} - {% if h.check_access('package_delete', {'id': data.id}) and not data.state == 'deleted' %} + {% if form_style == 'edit' and h.check_access('package_delete', {'id': data.id}) and not data.state == 'deleted' %} {% block delete_button_text %}{{ _('Delete') }}{% endblock %} {% endif %} {% endblock %} {% block save_button %} - + {% endblock %} {{ form.required_message() }} From 55b38fe15bc5e32d08778a810312bfa0e477a073 Mon Sep 17 00:00:00 2001 From: Ian Ward Date: Mon, 14 Oct 2024 12:54:49 -0400 Subject: [PATCH 3/7] [#8308] report errors on state change --- ckan/views/resource.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 715f1a86f7f..00c84e859de 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -238,11 +238,13 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: try: get_action('package_patch')( - Context(context, allow_state_change=True), + logic.fresh_context(context), {'id': id, 'state': 'active'} ) - except ValidationError: - pass + except ValidationError as e: + errors = e.error_dict.get('resources',[{}])[-1] + error_summary = e.error_summary + return self.get(package_type, id, data, errors, error_summary) return h.redirect_to(u'{}.read'.format(package_type), id=id) data[u'package_id'] = id @@ -269,11 +271,13 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: if save_action == u'go-metadata': try: get_action('package_patch')( - Context(context, allow_state_change=True), + logic.fresh_context(context), {'id': id, 'state': 'active'} ) - except ValidationError: - pass + except ValidationError as e: + errors = e.error_dict.get('resources',[{}])[-1] + error_summary = e.error_summary + return self.get(package_type, id, data, errors, error_summary) return h.redirect_to(u'{}.read'.format(package_type), id=id) elif save_action == u'go-dataset': # go to first stage of add dataset From 9c0dd9ef699696a7bc783d416ef73e680bd695be Mon Sep 17 00:00:00 2001 From: Ian Ward Date: Mon, 14 Oct 2024 13:20:51 -0400 Subject: [PATCH 4/7] [#8308] consistent draft badge style --- ckan/templates/package/read.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/templates/package/read.html b/ckan/templates/package/read.html index 22aa51c5709..596e0e853b3 100644 --- a/ckan/templates/package/read.html +++ b/ckan/templates/package/read.html @@ -13,10 +13,10 @@

{% block page_heading %} {{ h.dataset_display_name(pkg) }} {% if pkg.state.startswith('draft') %} - [{{ _('Draft') }}] + {{ _('Draft') }} {% endif %} {% if pkg.state == 'deleted' %} - [{{ _('Deleted') }}] + {{ _('Deleted') }} {% endif %} {% endblock %}

From f5d55a60d1f73b9f5758d88d7bf5c755081aebd9 Mon Sep 17 00:00:00 2001 From: Ian Ward Date: Mon, 14 Oct 2024 13:58:10 -0400 Subject: [PATCH 5/7] [#8308] changes, lint, allow no resources in form --- changes/8308.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/8308.feature diff --git a/changes/8308.feature b/changes/8308.feature new file mode 100644 index 00000000000..0f5c84651df --- /dev/null +++ b/changes/8308.feature @@ -0,0 +1 @@ +Publish buttons for draft datasets, consistent edit UI and error handling From fe22f07b1fbae0593003a81caccc174bf6d39014 Mon Sep 17 00:00:00 2001 From: Ian Ward Date: Mon, 14 Oct 2024 14:50:15 -0400 Subject: [PATCH 6/7] [#8308] show resources nav on new resource page --- ckan/templates/package/new_resource.html | 1 + .../templates/package/snippets/resources.html | 6 ++++ ckan/views/resource.py | 30 +++++++------------ 3 files changed, 17 insertions(+), 20 deletions(-) diff --git a/ckan/templates/package/new_resource.html b/ckan/templates/package/new_resource.html index 370df275a36..881c623bb7f 100644 --- a/ckan/templates/package/new_resource.html +++ b/ckan/templates/package/new_resource.html @@ -16,6 +16,7 @@ {% block secondary_content %} {% snippet 'package/snippets/resource_help.html' %} + {% snippet 'package/snippets/resources.html', pkg=pkg %} {% endblock %} {% block scripts %} diff --git a/ckan/templates/package/snippets/resources.html b/ckan/templates/package/snippets/resources.html index 7e91ed5090f..bd0e86eb1da 100644 --- a/ckan/templates/package/snippets/resources.html +++ b/ckan/templates/package/snippets/resources.html @@ -32,3 +32,9 @@

{{ _("Resources") }}

{% endblock %} {% endif %} + +{% if can_edit and active and not is_activity_archive %} +
+ {% link_for _('Add new resource'), named_route=pkg.type ~ '_resource.new', id=pkg.name, class_='btn btn-light btn-sm', icon='plus' %} +
+{% endif %} diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 00c84e859de..1074094c1e0 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -4,7 +4,7 @@ import cgi import json import logging -from typing import Any, cast, Optional, Union +from typing import Any, Optional, Union, cast from werkzeug.wrappers.response import Response as WerkzeugResponse import flask @@ -25,7 +25,7 @@ _get_pkg_template, _get_package_type, _setup_template_variables ) -from ckan.types import Context, Response +from ckan.types import Context, Response, ErrorDict Blueprint = flask.Blueprint NotFound = logic.NotFound @@ -217,9 +217,11 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: if save_action == u'go-dataset': # go to final stage of adddataset return h.redirect_to(u'{}.edit'.format(package_type), id=id) - # see if we have added any resources try: - data_dict = get_action(u'package_show')(context, {u'id': id}) + get_action('package_patch')( + logic.fresh_context(context), + {'id': id, 'state': 'active'} + ) except NotAuthorized: return base.abort(403, _(u'Unauthorized to update dataset')) except NotFound: @@ -227,22 +229,9 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: 404, _(u'The dataset {id} could not be found.').format(id=id) ) - if not len(data_dict[u'resources']): - # no data so keep on page - msg = _(u'You must add at least one data resource') - # On new templates do not use flash message - - errors: dict[str, Any] = {} - error_summary = {_(u'Error'): msg} - return self.get(package_type, id, data, errors, error_summary) - - try: - get_action('package_patch')( - logic.fresh_context(context), - {'id': id, 'state': 'active'} - ) except ValidationError as e: - errors = e.error_dict.get('resources',[{}])[-1] + errors = cast( + "list[ErrorDict]", e.error_dict.get('resources', [{}]))[-1] error_summary = e.error_summary return self.get(package_type, id, data, errors, error_summary) return h.redirect_to(u'{}.read'.format(package_type), id=id) @@ -275,7 +264,8 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: {'id': id, 'state': 'active'} ) except ValidationError as e: - errors = e.error_dict.get('resources',[{}])[-1] + errors = cast( + "list[ErrorDict]", e.error_dict.get('resources', [{}]))[-1] error_summary = e.error_summary return self.get(package_type, id, data, errors, error_summary) return h.redirect_to(u'{}.read'.format(package_type), id=id) From 5e3e86ccdd7bd3ab29ad600ef9d165b68fde7d55 Mon Sep 17 00:00:00 2001 From: Ian Ward Date: Tue, 15 Oct 2024 08:49:52 -0400 Subject: [PATCH 7/7] [#8308] empty resource add another error --- ckan/tests/controllers/test_package.py | 10 +++++----- ckan/views/resource.py | 8 ++++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/ckan/tests/controllers/test_package.py b/ckan/tests/controllers/test_package.py index b1991e32069..da5ab69e851 100644 --- a/ckan/tests/controllers/test_package.py +++ b/ckan/tests/controllers/test_package.py @@ -142,9 +142,9 @@ def test_first_page_creates_draft_package(self, app, user): def test_resource_required(self, app, user): url = url_for("dataset.new") - name = "one-resource-required" - env = {"Authorization": user["token"]} - response = app.post(url, extra_environ=env, data={ + name = "resource-data-required" + headers = {"Authorization": user["token"]} + response = app.post(url, headers=headers, data={ "name": name, "save": "", "_ckan_phase": 1 @@ -153,9 +153,9 @@ def test_resource_required(self, app, user): response = app.post(location, extra_environ=env, data={ "id": "", "url": "", - "save": "go-metadata", + "save": "again", }) - assert "You must add at least one data resource" in response + assert "No resource data entered" in response def test_complete_package_with_one_resource(self, app, user): url = url_for("dataset.new") diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 1074094c1e0..9125b9b286e 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -213,8 +213,12 @@ def post(self, package_type: str, id: str) -> Union[str, Response]: data_provided = True break - if not data_provided and save_action != u"go-dataset-complete": - if save_action == u'go-dataset': + if not data_provided and save_action != "go-dataset-complete": + if save_action == 'again': + errors: dict[str, Any] = {} + error_summary = {_('Error'): _('No resource data entered')} + return self.get(package_type, id, data, errors, error_summary) + if save_action == 'go-dataset': # go to final stage of adddataset return h.redirect_to(u'{}.edit'.format(package_type), id=id) try: