From ea74e42f6be4bc89a3ad89f367a47ce0bcd2b761 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 15:38:51 +0100 Subject: [PATCH 1/7] [#5152] Clear the upload and url_type fields if there is a ValidationError Also add a custom error that we can use to show a warning in the template that users need to resubmit the file --- ckan/views/resource.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ckan/views/resource.py b/ckan/views/resource.py index 293c30f8a8c..d3cceafc65b 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -243,6 +243,10 @@ def post(self, package_type, id): except ValidationError as e: errors = e.error_dict error_summary = e.error_summary + if data.get('url_type') == 'upload' and data.get('url'): + data['url'] = '' + data['url_type'] = '' + errors['previous_upload'] = True return self.get(package_type, id, data, errors, error_summary) except NotAuthorized: return base.abort(403, _(u'Unauthorized to create a resource')) From 095b2001d390556cae834fdfbfd6fde6658f8330 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 15:43:54 +0100 Subject: [PATCH 2/7] [#5152] Show a warning if there was an upload and validation errors The previously selected File is lost and it's easy to miss it, so add a little warning under the File button warning users to resubmit it --- ckan/public/base/css/fuchsia.css | 57 ++++++++++++------- ckan/public/base/css/green.css | 57 ++++++++++++------- ckan/public/base/css/main.css | 50 +++++++++------- ckan/public/base/css/maroon.css | 57 ++++++++++++------- ckan/public/base/css/red.css | 57 ++++++++++++------- .../base/javascript/modules/image-upload.js | 9 ++- ckan/public/base/less/forms.less | 6 ++ ckan/templates/macros/form.html | 16 +++++- .../package/snippets/resource_form.html | 4 +- 9 files changed, 198 insertions(+), 115 deletions(-) diff --git a/ckan/public/base/css/fuchsia.css b/ckan/public/base/css/fuchsia.css index 9353e542365..c0e2f7fd3c3 100644 --- a/ckan/public/base/css/fuchsia.css +++ b/ckan/public/base/css/fuchsia.css @@ -8150,6 +8150,11 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { .js .image-upload .btn-remove-url .icon-remove { margin-right: 0; } +.js .image-upload .error-inline { + margin-top: 5px; + margin-left: 2px; + font-weight: bold; +} .add-member-form .control-label { display: block; } @@ -8481,6 +8486,29 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { #activity-archive-notice { clear: both; } +/* diff styles */ +table.diff { + font-family: Courier; + border: medium; +} +.diff_header { + background-color: #e0e0e0; +} +td.diff_header { + text-align: right; +} +.diff_next { + background-color: #c0c0c0; +} +.diff_add { + background-color: #aaffaa; +} +.diff_chg { + background-color: #ffff77; +} +.diff_sub { + background-color: #ffaaaa; +} .search-form { margin-bottom: 20px; padding-bottom: 25px; @@ -10366,6 +10394,13 @@ h4 small { .activity .item.follow-group .icon { background-color: #8ba669; } +.select-time { + width: 250px; + display: inline; +} +br.line-height2 { + line-height: 2; +} .dropdown:hover .dropdown-menu { display: block; } @@ -10760,25 +10795,3 @@ iframe { .reduced-padding { padding: 3px 5px; } -table.diff { - font-family:Courier; - border:medium; -} -.diff_header { - background-color:#e0e0e0; -} -td.diff_header { - text-align:right; -} -.diff_next { - background-color:#c0c0c0; -} -.diff_add { - background-color:#aaffaa; -} -.diff_chg { - background-color:#ffff77; -} -.diff_sub { - background-color:#ffaaaa; -} diff --git a/ckan/public/base/css/green.css b/ckan/public/base/css/green.css index 13a38aacf8b..6042ab9eeaa 100644 --- a/ckan/public/base/css/green.css +++ b/ckan/public/base/css/green.css @@ -8150,6 +8150,11 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { .js .image-upload .btn-remove-url .icon-remove { margin-right: 0; } +.js .image-upload .error-inline { + margin-top: 5px; + margin-left: 2px; + font-weight: bold; +} .add-member-form .control-label { display: block; } @@ -8481,6 +8486,29 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { #activity-archive-notice { clear: both; } +/* diff styles */ +table.diff { + font-family: Courier; + border: medium; +} +.diff_header { + background-color: #e0e0e0; +} +td.diff_header { + text-align: right; +} +.diff_next { + background-color: #c0c0c0; +} +.diff_add { + background-color: #aaffaa; +} +.diff_chg { + background-color: #ffff77; +} +.diff_sub { + background-color: #ffaaaa; +} .search-form { margin-bottom: 20px; padding-bottom: 25px; @@ -10366,6 +10394,13 @@ h4 small { .activity .item.follow-group .icon { background-color: #8ba669; } +.select-time { + width: 250px; + display: inline; +} +br.line-height2 { + line-height: 2; +} .dropdown:hover .dropdown-menu { display: block; } @@ -10760,25 +10795,3 @@ iframe { .reduced-padding { padding: 3px 5px; } -table.diff { - font-family:Courier; - border:medium; -} -.diff_header { - background-color:#e0e0e0; -} -td.diff_header { - text-align:right; -} -.diff_next { - background-color:#c0c0c0; -} -.diff_add { - background-color:#aaffaa; -} -.diff_chg { - background-color:#ffff77; -} -.diff_sub { - background-color:#ffaaaa; -} diff --git a/ckan/public/base/css/main.css b/ckan/public/base/css/main.css index 2c670a61371..eb978223865 100644 --- a/ckan/public/base/css/main.css +++ b/ckan/public/base/css/main.css @@ -8150,6 +8150,11 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { .js .image-upload .btn-remove-url .icon-remove { margin-right: 0; } +.js .image-upload .error-inline { + margin-top: 5px; + margin-left: 2px; + font-weight: bold; +} .add-member-form .control-label { display: block; } @@ -8481,6 +8486,29 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { #activity-archive-notice { clear: both; } +/* diff styles */ +table.diff { + font-family: Courier; + border: medium; +} +.diff_header { + background-color: #e0e0e0; +} +td.diff_header { + text-align: right; +} +.diff_next { + background-color: #c0c0c0; +} +.diff_add { + background-color: #aaffaa; +} +.diff_chg { + background-color: #ffff77; +} +.diff_sub { + background-color: #ffaaaa; +} .search-form { margin-bottom: 20px; padding-bottom: 25px; @@ -10767,25 +10795,3 @@ iframe { .reduced-padding { padding: 3px 5px; } -table.diff { - font-family:Courier; - border:medium; -} -.diff_header { - background-color:#e0e0e0; -} -td.diff_header { - text-align:right; -} -.diff_next { - background-color:#c0c0c0; -} -.diff_add { - background-color:#aaffaa; -} -.diff_chg { - background-color:#ffff77; -} -.diff_sub { - background-color:#ffaaaa; -} diff --git a/ckan/public/base/css/maroon.css b/ckan/public/base/css/maroon.css index 49d29a3d18f..a4ac0d0b4d2 100644 --- a/ckan/public/base/css/maroon.css +++ b/ckan/public/base/css/maroon.css @@ -8150,6 +8150,11 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { .js .image-upload .btn-remove-url .icon-remove { margin-right: 0; } +.js .image-upload .error-inline { + margin-top: 5px; + margin-left: 2px; + font-weight: bold; +} .add-member-form .control-label { display: block; } @@ -8481,6 +8486,29 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { #activity-archive-notice { clear: both; } +/* diff styles */ +table.diff { + font-family: Courier; + border: medium; +} +.diff_header { + background-color: #e0e0e0; +} +td.diff_header { + text-align: right; +} +.diff_next { + background-color: #c0c0c0; +} +.diff_add { + background-color: #aaffaa; +} +.diff_chg { + background-color: #ffff77; +} +.diff_sub { + background-color: #ffaaaa; +} .search-form { margin-bottom: 20px; padding-bottom: 25px; @@ -10366,6 +10394,13 @@ h4 small { .activity .item.follow-group .icon { background-color: #8ba669; } +.select-time { + width: 250px; + display: inline; +} +br.line-height2 { + line-height: 2; +} .dropdown:hover .dropdown-menu { display: block; } @@ -10760,25 +10795,3 @@ iframe { .reduced-padding { padding: 3px 5px; } -table.diff { - font-family:Courier; - border:medium; -} -.diff_header { - background-color:#e0e0e0; -} -td.diff_header { - text-align:right; -} -.diff_next { - background-color:#c0c0c0; -} -.diff_add { - background-color:#aaffaa; -} -.diff_chg { - background-color:#ffff77; -} -.diff_sub { - background-color:#ffaaaa; -} diff --git a/ckan/public/base/css/red.css b/ckan/public/base/css/red.css index 3c0a73f413e..e4aeeba39d4 100644 --- a/ckan/public/base/css/red.css +++ b/ckan/public/base/css/red.css @@ -8150,6 +8150,11 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { .js .image-upload .btn-remove-url .icon-remove { margin-right: 0; } +.js .image-upload .error-inline { + margin-top: 5px; + margin-left: 2px; + font-weight: bold; +} .add-member-form .control-label { display: block; } @@ -8481,6 +8486,29 @@ fieldset[disabled] .control-custom.disabled .checkbox.btn.focus { #activity-archive-notice { clear: both; } +/* diff styles */ +table.diff { + font-family: Courier; + border: medium; +} +.diff_header { + background-color: #e0e0e0; +} +td.diff_header { + text-align: right; +} +.diff_next { + background-color: #c0c0c0; +} +.diff_add { + background-color: #aaffaa; +} +.diff_chg { + background-color: #ffff77; +} +.diff_sub { + background-color: #ffaaaa; +} .search-form { margin-bottom: 20px; padding-bottom: 25px; @@ -10366,6 +10394,13 @@ h4 small { .activity .item.follow-group .icon { background-color: #8ba669; } +.select-time { + width: 250px; + display: inline; +} +br.line-height2 { + line-height: 2; +} .dropdown:hover .dropdown-menu { display: block; } @@ -10760,25 +10795,3 @@ iframe { .reduced-padding { padding: 3px 5px; } -table.diff { - font-family:Courier; - border:medium; -} -.diff_header { - background-color:#e0e0e0; -} -td.diff_header { - text-align:right; -} -.diff_next { - background-color:#c0c0c0; -} -.diff_add { - background-color:#aaffaa; -} -.diff_chg { - background-color:#ffff77; -} -.diff_sub { - background-color:#ffaaaa; -} diff --git a/ckan/public/base/javascript/modules/image-upload.js b/ckan/public/base/javascript/modules/image-upload.js index 49255b711d5..9437305632b 100644 --- a/ckan/public/base/javascript/modules/image-upload.js +++ b/ckan/public/base/javascript/modules/image-upload.js @@ -11,7 +11,8 @@ this.ckan.module('image-upload', function($) { field_url: 'image_url', field_clear: 'clear_upload', field_name: 'name', - upload_label: '' + upload_label: '', + previous_upload: false }, /* Should be changed to true if user modifies resource's name @@ -43,6 +44,7 @@ this.ckan.module('image-upload', function($) { this.label_location = $('label[for="field-image-url"]'); // determines if the resource is a data resource this.is_data_resource = (this.options.field_url === 'url') && (this.options.field_upload === 'upload'); + this.previousUpload = this.options.previous_upload; // Is there a clear checkbox on the form already? var checkbox = $(field_clear, this.el); @@ -68,6 +70,11 @@ this.ckan.module('image-upload', function($) { this._('Upload') + '') .insertAfter(this.input); + if (this.previousUpload) { + $('
' + + this._('Please select the file to upload again') + '
').appendTo(this.field_image); + } + // Button for resetting the form when there is a URL set var removeText = this._('Remove'); $('' diff --git a/ckan/public/base/less/forms.less b/ckan/public/base/less/forms.less index 79cc069b63f..41c33932b31 100644 --- a/ckan/public/base/less/forms.less +++ b/ckan/public/base/less/forms.less @@ -752,6 +752,12 @@ select[data-module="autocomplete"] { margin-right: 0; } } + + .error-inline { + margin-top: 5px; + margin-left: 2px; + font-weight: bold; + } } .add-member-form .control-label { diff --git a/ckan/templates/macros/form.html b/ckan/templates/macros/form.html index dd9a3fef6ff..217904ef2b0 100644 --- a/ckan/templates/macros/form.html +++ b/ckan/templates/macros/form.html @@ -418,14 +418,24 @@ #} {% macro image_upload(data, errors, field_url='image_url', field_upload='image_upload', field_clear='clear_upload', is_url=false, is_upload=false, is_upload_enabled=false, placeholder=false, - url_label='', upload_label='', field_name='image_url') %} + url_label='', upload_label='', field_name='image_url', previous_upload=false) %} {% set placeholder = placeholder if placeholder else _('http://example.com/my-image.jpg') %} {% set url_label = url_label or _('Image URL') %} {% set upload_label = upload_label or _('Image') %} {% if is_upload_enabled %} -
+ + +
{% endif %} diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index 7edd1e40e8b..bf7a917e194 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -23,7 +23,9 @@ {% set is_upload = (data.url_type == 'upload') %} {{ form.image_upload(data, errors, field_url='url', field_upload='upload', field_clear='clear_upload', is_upload_enabled=h.uploads_enabled(), is_url=data.url and not is_upload, is_upload=is_upload, - upload_label=_('Data'), url_label=_('URL'), placeholder=_('http://example.com/external-data.csv'), field_name='name') }} + upload_label=_('Data'), url_label=_('URL'), placeholder=_('http://example.com/external-data.csv'), field_name='name', + previous_upload=(errors['previous_upload'] == True) + ) }} {% endblock %} {% block basic_fields_name %} From 203203c549f404b4b6c5a6398d644f266c0d7da9 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 17:38:06 +0100 Subject: [PATCH 3/7] [#5152] Coding standards --- ckan/views/resource.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ckan/views/resource.py b/ckan/views/resource.py index d3cceafc65b..abfd9737382 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -243,10 +243,10 @@ def post(self, package_type, id): except ValidationError as e: errors = e.error_dict error_summary = e.error_summary - if data.get('url_type') == 'upload' and data.get('url'): - data['url'] = '' - data['url_type'] = '' - errors['previous_upload'] = True + if data.get(u'url_type') == u'upload' and data.get('url'): + data[u'url'] = u'' + data[u'url_type'] = u'' + errors[u'previous_upload'] = True return self.get(package_type, id, data, errors, error_summary) except NotAuthorized: return base.abort(403, _(u'Unauthorized to create a resource')) From 6a3f75349593c052d6fe8c740528d75ad046e041 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 18:00:00 +0100 Subject: [PATCH 4/7] [#5152] Yet another unprefixed literal --- ckan/views/resource.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/views/resource.py b/ckan/views/resource.py index abfd9737382..a2f141212fd 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -243,7 +243,7 @@ def post(self, package_type, id): except ValidationError as e: errors = e.error_dict error_summary = e.error_summary - if data.get(u'url_type') == u'upload' and data.get('url'): + if data.get(u'url_type') == u'upload' and data.get(u'url'): data[u'url'] = u'' data[u'url_type'] = u'' errors[u'previous_upload'] = True From c20dd8bc2653cc8afc21eccce85420be72e474a6 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 18:21:12 +0100 Subject: [PATCH 5/7] [#5152] Use data to pass the previous_upload var Otherwise when using errors it is shown on the error summary on top when using scheming --- ckan/templates/package/snippets/resource_form.html | 2 +- ckan/views/resource.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index bf7a917e194..48e82cff523 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -24,7 +24,7 @@ {{ form.image_upload(data, errors, field_url='url', field_upload='upload', field_clear='clear_upload', is_upload_enabled=h.uploads_enabled(), is_url=data.url and not is_upload, is_upload=is_upload, upload_label=_('Data'), url_label=_('URL'), placeholder=_('http://example.com/external-data.csv'), field_name='name', - previous_upload=(errors['previous_upload'] == True) + previous_upload=(data['previous_upload'] == True) ) }} {% endblock %} diff --git a/ckan/views/resource.py b/ckan/views/resource.py index a2f141212fd..274a648d8b6 100644 --- a/ckan/views/resource.py +++ b/ckan/views/resource.py @@ -246,7 +246,7 @@ def post(self, package_type, id): if data.get(u'url_type') == u'upload' and data.get(u'url'): data[u'url'] = u'' data[u'url_type'] = u'' - errors[u'previous_upload'] = True + data[u'previous_upload'] = True return self.get(package_type, id, data, errors, error_summary) except NotAuthorized: return base.abort(403, _(u'Unauthorized to create a resource')) From 5166a3ae0fd39956fee98cfef10c48dab1d0980d Mon Sep 17 00:00:00 2001 From: amercader Date: Sun, 12 Jan 2020 17:09:16 +0100 Subject: [PATCH 6/7] [#5152] [#5152] Simplify templates params for better compatibility with scheming --- ckan/templates/macros/form.html | 3 ++- ckan/templates/package/snippets/resource_form.html | 4 +--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/ckan/templates/macros/form.html b/ckan/templates/macros/form.html index 217904ef2b0..ff01a076c02 100644 --- a/ckan/templates/macros/form.html +++ b/ckan/templates/macros/form.html @@ -418,10 +418,11 @@ #} {% macro image_upload(data, errors, field_url='image_url', field_upload='image_upload', field_clear='clear_upload', is_url=false, is_upload=false, is_upload_enabled=false, placeholder=false, - url_label='', upload_label='', field_name='image_url', previous_upload=false) %} + url_label='', upload_label='', field_name='image_url') %} {% set placeholder = placeholder if placeholder else _('http://example.com/my-image.jpg') %} {% set url_label = url_label or _('Image URL') %} {% set upload_label = upload_label or _('Image') %} + {% set previous_upload = data['previous_upload'] %} {% if is_upload_enabled %} diff --git a/ckan/templates/package/snippets/resource_form.html b/ckan/templates/package/snippets/resource_form.html index 48e82cff523..7edd1e40e8b 100644 --- a/ckan/templates/package/snippets/resource_form.html +++ b/ckan/templates/package/snippets/resource_form.html @@ -23,9 +23,7 @@ {% set is_upload = (data.url_type == 'upload') %} {{ form.image_upload(data, errors, field_url='url', field_upload='upload', field_clear='clear_upload', is_upload_enabled=h.uploads_enabled(), is_url=data.url and not is_upload, is_upload=is_upload, - upload_label=_('Data'), url_label=_('URL'), placeholder=_('http://example.com/external-data.csv'), field_name='name', - previous_upload=(data['previous_upload'] == True) - ) }} + upload_label=_('Data'), url_label=_('URL'), placeholder=_('http://example.com/external-data.csv'), field_name='name') }} {% endblock %} {% block basic_fields_name %} From db0ef6d7f0470bf257f67d98125e438632eec87b Mon Sep 17 00:00:00 2001 From: amercader Date: Mon, 13 Jan 2020 13:10:04 +0100 Subject: [PATCH 7/7] [#5152] Fix fragile test --- ckan/tests/controllers/test_user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/tests/controllers/test_user.py b/ckan/tests/controllers/test_user.py index 71e25741916..05c47e98efc 100644 --- a/ckan/tests/controllers/test_user.py +++ b/ckan/tests/controllers/test_user.py @@ -850,7 +850,7 @@ def test_request_reset_when_duplicate_emails(self, send_reset_link, app): emailed_users = [ call[0][0].name for call in send_reset_link.call_args_list ] - assert emailed_users == [user_a["name"], user_b["name"]] + assert sorted(emailed_users) == sorted([user_a["name"], user_b["name"]]) def test_request_reset_without_param(self, app):