diff --git a/changes/198.backport.feature b/changes/198.backport.feature new file mode 100644 index 00000000000..8f84caaedaa --- /dev/null +++ b/changes/198.backport.feature @@ -0,0 +1,5 @@ +- Saving a draft dataset is now called "Publish" and an "Unpublish" button is available +to move an active dataset back to the draft state. +- Consistent display of draft state across dataset pages. +- Error handling is now done on publish, so validators that only apply certain rules to fields for published datasets (e.g. required-only-when-published) will be properly displayed instead of causing a server error. +- templates/package/new_package_form.html content has been merged into its parent template templates/package/snippets/package_form.html and is now marked as deprecated. diff --git a/changes/8308.feature b/changes/8308.feature new file mode 100644 index 00000000000..8f84caaedaa --- /dev/null +++ b/changes/8308.feature @@ -0,0 +1,5 @@ +- Saving a draft dataset is now called "Publish" and an "Unpublish" button is available +to move an active dataset back to the draft state. +- Consistent display of draft state across dataset pages. +- Error handling is now done on publish, so validators that only apply certain rules to fields for published datasets (e.g. required-only-when-published) will be properly displayed instead of causing a server error. +- templates/package/new_package_form.html content has been merged into its parent template templates/package/snippets/package_form.html and is now marked as deprecated. diff --git a/ckan/lib/plugins.py b/ckan/lib/plugins.py index 9f04a6b0ace..d8b477e97bc 100644 --- a/ckan/lib/plugins.py +++ b/ckan/lib/plugins.py @@ -406,7 +406,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..a0868b718fd 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 and may be removed in a future release. -{% 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/new_resource.html b/ckan/templates/package/new_resource.html index ed57a7aafd5..849548fee10 100644 --- a/ckan/templates/package/new_resource.html +++ b/ckan/templates/package/new_resource.html @@ -18,6 +18,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/read.html b/ckan/templates/package/read.html index 457501976ab..f7c47546998 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 %}

diff --git a/ckan/templates/package/snippets/package_form.html b/ckan/templates/package/snippets/package_form.html index 787a259dbd4..fba886dcc38 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 %} {# (canada fork only): add ckan_phase block #} {% block ckan_phase %} @@ -40,12 +42,25 @@

{% 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 publish_button %} + {% if form_style == 'edit' %} + + {% endif %} + {% endblock %} {% block save_button %} - + {% endblock %} {% endblock %} diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index 9fd82f86f36..791bebe1218 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -92,7 +92,7 @@ {% endblock %} {% if stage %} {% block save_button %} - + {% endblock %} {% else %} {% block add_button %} diff --git a/ckan/templates/package/snippets/resources.html b/ckan/templates/package/snippets/resources.html index f5d467120d9..16fe50964fa 100644 --- a/ckan/templates/package/snippets/resources.html +++ b/ckan/templates/package/snippets/resources.html @@ -55,7 +55,7 @@

{{ _("Resources") }}

{% endblock %} {% endif %} -{% if can_edit and not is_activity_archive %}{# (canada fork only): prevent Resource edit/manage links when in activity archive "view" #} +{% 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' %}
diff --git a/ckan/tests/controllers/test_package.py b/ckan/tests/controllers/test_package.py index b1991e32069..c5b41277f22 100644 --- a/ckan/tests/controllers/test_package.py +++ b/ckan/tests/controllers/test_package.py @@ -142,7 +142,7 @@ 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" + name = "resource-data-required" env = {"Authorization": user["token"]} response = app.post(url, extra_environ=env, data={ "name": name, @@ -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/dataset.py b/ckan/views/dataset.py index 9a2153d66b0..d05e28f480b 100644 --- a/ckan/views/dataset.py +++ b/ckan/views/dataset.py @@ -574,10 +574,25 @@ def post(self, package_type: str) -> Union[Response, str]: ) # redirect to add dataset resources - url = h.url_for( - u'{}_resource.new'.format(package_type), - id=pkg_dict[u'name'] - ) + try: + last_added_resource = pkg_dict[u'resources'][-1] + except IndexError: + last_added_resource = None + if last_added_resource and request.form[u'save'] == "go-resources": + url = h.url_for( + u'{}_resource.edit'.format(package_type), + id=pkg_dict.get('id'), + resource_id=last_added_resource.get('id')) + elif request.form[u'save'] == "go-metadata-preview": + url = h.url_for( + u'{}.read'.format(package_type), + id=pkg_dict.get('id') + ) + else: + url = h.url_for( + u'{}_resource.new'.format(package_type), + id=pkg_dict[u'name'] + ) return h.redirect_to(url) # Make sure we don't index this dataset if request.form[u'save'] not in [ @@ -749,6 +764,9 @@ def post(self, package_type: str, id: str) -> Union[Response, str]: del data_dict[u'_ckan_phase'] del data_dict[u'save'] data_dict['id'] = id + if request.form.get('save', None) == 'go-metadata-unpublish': + data_dict['state'] = 'draft' + pkg_dict = get_action(u'package_update')(context, data_dict) return _form_save_redirect( diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 052088a3f5f..cb03b5c2f4f 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 @@ -213,13 +213,19 @@ 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) - # 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,21 +233,11 @@ 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} + except ValidationError as e: + 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) - - # 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') - ) return h.redirect_to(u'{}.read'.format(package_type), id=id) data[u'package_id'] = id @@ -266,12 +262,16 @@ 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')( + logic.fresh_context(context), + {'id': id, 'state': 'active'} + ) + except ValidationError as e: + 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) elif save_action == u'go-dataset': # go to first stage of add dataset