From e9adf793e5eb3989e23b9c152d5898bb1adb015d Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 18 Oct 2019 19:12:54 +0100 Subject: [PATCH 01/14] Fix resource.url being saved as "___" Fixes #5031 werkzeug upgrade 0.14 to 0.15 means that when you submit the resource form with a resource url (not an upload), and the uploader is enabled, then the 'upload' parameter value changes from being u'' to: FileStorage(filename=u''). This half-confused ResourceUpload into thinking that it should upload something and it stored the url as '___' (!). This is fixed by essentially testing bool(resource['upload']) because bool(FileStorage(filename=u'')) == False. I was unable to test this using webtest - it didn't submit the form like my real browser does. So I just add a test for ResourceUpload that shows the issue and prevents it regressing in future. --- ckan/lib/uploader.py | 3 +- ckan/tests/controllers/test_package.py | 24 +------ ckan/tests/lib/test_uploader.py | 89 ++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 24 deletions(-) create mode 100644 ckan/tests/lib/test_uploader.py diff --git a/ckan/lib/uploader.py b/ckan/lib/uploader.py index fee5a7ac0a2..15dded290c1 100644 --- a/ckan/lib/uploader.py +++ b/ckan/lib/uploader.py @@ -212,7 +212,8 @@ def __init__(self, resource): if url and config_mimetype_guess == 'file_ext': self.mimetype = mimetypes.guess_type(url)[0] - if isinstance(upload_field_storage, ALLOWED_UPLOAD_TYPES): + if upload_field_storage and \ + isinstance(upload_field_storage, ALLOWED_UPLOAD_TYPES): self.filesize = 0 # bytes self.filename = upload_field_storage.filename diff --git a/ckan/tests/controllers/test_package.py b/ckan/tests/controllers/test_package.py index d469540538e..7353e6f04ac 100644 --- a/ckan/tests/controllers/test_package.py +++ b/ckan/tests/controllers/test_package.py @@ -162,29 +162,7 @@ def test_complete_package_with_two_resources(self): assert_equal(pkg.resources[1].url, u'http://example.com/resource1') assert_equal(pkg.state, 'active') - # def test_resource_uploads(self): - # app = self._get_test_app() - # env, response = _get_package_new_page(app) - # form = response.forms['dataset-edit'] - # form['name'] = u'complete-package-with-two-resources' - - # response = submit_and_follow(app, form, env, 'save') - # form = response.forms['resource-edit'] - # form['upload'] = ('README.rst', b'data') - - # response = submit_and_follow(app, form, env, 'save', 'go-metadata') - # pkg = model.Package.by_name(u'complete-package-with-two-resources') - # assert_equal(pkg.resources[0].url_type, u'upload') - # assert_equal(pkg.state, 'active') - # response = app.get( - # url_for( - # controller='package', - # action='resource_download', - # id=pkg.id, - # resource_id=pkg.resources[0].id - # ), - # ) - # assert_equal('data', response.body) + # resource upload is tested in TestExampleIUploaderPlugin def test_previous_button_works(self): app = self._get_test_app() diff --git a/ckan/tests/lib/test_uploader.py b/ckan/tests/lib/test_uploader.py new file mode 100644 index 00000000000..3cf98f61475 --- /dev/null +++ b/ckan/tests/lib/test_uploader.py @@ -0,0 +1,89 @@ +import mock +import tempfile +import __builtin__ as builtins +import flask +import paste.fileapp +import cStringIO + +from werkzeug.datastructures import FileStorage +from pyfakefs import fake_filesystem +from nose.tools import assert_equal as eq_ + +from ckan.common import config +import ckan.lib.uploader +from ckan.lib.uploader import ResourceUpload +from ckanext.example_iuploader.test.test_plugin import mock_open_if_open_fails + + +fs = fake_filesystem.FakeFilesystem() +fake_os = fake_filesystem.FakeOsModule(fs) + + +class TestInitResourceUpload(object): + @mock.patch.object(ckan.lib.uploader, 'os', fake_os) + @mock.patch.object(builtins, 'open', side_effect=mock_open_if_open_fails) + @mock.patch.object(flask, 'send_file', side_effect=[b'DATA']) + @mock.patch.object(paste.fileapp, 'os', fake_os) + @mock.patch.object(config['pylons.h'], 'uploads_enabled', + return_value=True) + @mock.patch.object(ckan.lib.uploader, '_storage_path', new='/doesnt_exist') + def test_resource_without_upload_with_old_werkzeug( + self, mock_uploads_enabled, mock_open, send_file): + # this test data is based on real observation using a browser + # and werkzeug 0.14.1 + res = {'clear_upload': u'true', + 'format': u'CSV', + 'url': u'https://example.com/data.csv', + 'description': u'', + 'upload': u'', + u'package_id': u'dataset1', + 'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', + 'name': u'data.csv'} + res_upload = ResourceUpload(res) + + eq_(res_upload.filename, None) + + @mock.patch.object(ckan.lib.uploader, 'os', fake_os) + @mock.patch.object(builtins, 'open', side_effect=mock_open_if_open_fails) + @mock.patch.object(flask, 'send_file', side_effect=[b'DATA']) + @mock.patch.object(paste.fileapp, 'os', fake_os) + @mock.patch.object(config['pylons.h'], 'uploads_enabled', + return_value=True) + @mock.patch.object(ckan.lib.uploader, '_storage_path', new='/doesnt_exist') + def test_resource_without_upload( + self, mock_uploads_enabled, mock_open, send_file): + # this test data is based on real observation using a browser + res = {'clear_upload': u'true', + 'format': u'PNG', + 'url': u'https://example.com/data.csv', + 'description': u'', + 'upload': FileStorage(filename=u''), + u'package_id': u'dataset1', + 'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', + 'name': u'data.csv'} + res_upload = ResourceUpload(res) + + eq_(res_upload.filename, None) + + @mock.patch.object(ckan.lib.uploader, 'os', fake_os) + @mock.patch.object(builtins, 'open', side_effect=mock_open_if_open_fails) + @mock.patch.object(flask, 'send_file', side_effect=[b'DATA']) + @mock.patch.object(paste.fileapp, 'os', fake_os) + @mock.patch.object(config['pylons.h'], 'uploads_enabled', + return_value=True) + @mock.patch.object(ckan.lib.uploader, '_storage_path', new='/doesnt_exist') + def test_resource_with_upload( + self, mock_uploads_enabled, mock_open, send_file): + # this test data is based on real observation using a browser + res = {'clear_upload': u'', + 'format': u'PNG', + 'url': u'https://example.com/data.csv', + 'description': u'', + 'upload': FileStorage(filename=u'data.csv', content_type='CSV'), + u'package_id': u'dataset1', + 'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', + 'name': u'data.csv'} + res_upload = ResourceUpload(res) + + eq_(res_upload.filesize, 0) + eq_(res_upload.filename, 'data.csv') From 856a9a1e5e1f2ea6b2094eb4ce8bf56d5c45f833 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 12:07:12 +0100 Subject: [PATCH 02/14] [#5031] Use bool in upload field check --- ckan/lib/uploader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/lib/uploader.py b/ckan/lib/uploader.py index 5337e293ea0..29122019d21 100644 --- a/ckan/lib/uploader.py +++ b/ckan/lib/uploader.py @@ -214,7 +214,7 @@ def __init__(self, resource): if url and config_mimetype_guess == 'file_ext': self.mimetype = mimetypes.guess_type(url)[0] - if upload_field_storage and \ + if bool(upload_field_storage) and \ isinstance(upload_field_storage, ALLOWED_UPLOAD_TYPES): self.filesize = 0 # bytes From d1553c743da0c54b1a0cdafb44e52880d5c5fa89 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 12:07:49 +0100 Subject: [PATCH 03/14] [#5031] Use Werkzeug based fake storage object in tests --- ckan/tests/logic/action/test_update.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/ckan/tests/logic/action/test_update.py b/ckan/tests/logic/action/test_update.py index 982eee266c7..163e88e0542 100644 --- a/ckan/tests/logic/action/test_update.py +++ b/ckan/tests/logic/action/test_update.py @@ -6,6 +6,8 @@ import mock import pytest +from werkzeug.datastructures import FileStorage as FlaskFileStorage + import ckan import ckan.lib.app_globals as app_globals import ckan.logic as logic @@ -810,19 +812,15 @@ def test_calling_with_only_id_doesnt_update_anything(self): @pytest.mark.ckan_config("ckan.plugins", "image_view recline_view") @pytest.mark.usefixtures("clean_db", "with_plugins") class TestResourceUpdate(object): - import cgi - class FakeFileStorage(cgi.FieldStorage): - def __init__(self, fp, filename): - self.file = fp + class FakeFileStorage(FlaskFileStorage): + content_type = None + + def __init__(self, stream, filename): + self.stream = stream self.filename = filename self.name = "upload" - def setup(self): - import ckan.model as model - - model.repo.rebuild_db() - def test_url_only(self): dataset = factories.Dataset() resource = factories.Resource(package=dataset, url="http://first") From 6755367836d9090513961752a828715cca4c51f4 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 12:08:11 +0100 Subject: [PATCH 04/14] [#5031] Coding standards --- ckan/tests/lib/test_uploader.py | 81 +++++++++++++++++---------------- 1 file changed, 41 insertions(+), 40 deletions(-) diff --git a/ckan/tests/lib/test_uploader.py b/ckan/tests/lib/test_uploader.py index 3cf98f61475..b4117202939 100644 --- a/ckan/tests/lib/test_uploader.py +++ b/ckan/tests/lib/test_uploader.py @@ -1,3 +1,4 @@ +# encoding: utf-8 import mock import tempfile import __builtin__ as builtins @@ -20,70 +21,70 @@ class TestInitResourceUpload(object): - @mock.patch.object(ckan.lib.uploader, 'os', fake_os) - @mock.patch.object(builtins, 'open', side_effect=mock_open_if_open_fails) - @mock.patch.object(flask, 'send_file', side_effect=[b'DATA']) - @mock.patch.object(paste.fileapp, 'os', fake_os) - @mock.patch.object(config['pylons.h'], 'uploads_enabled', + @mock.patch.object(ckan.lib.uploader, u'os', fake_os) + @mock.patch.object(builtins, u'open', side_effect=mock_open_if_open_fails) + @mock.patch.object(flask, u'send_file', side_effect=[b'DATA']) + @mock.patch.object(paste.fileapp, u'os', fake_os) + @mock.patch.object(config[u'pylons.h'], u'uploads_enabled', return_value=True) - @mock.patch.object(ckan.lib.uploader, '_storage_path', new='/doesnt_exist') + @mock.patch.object(ckan.lib.uploader, u'_storage_path', new=u'/doesnt_exist') def test_resource_without_upload_with_old_werkzeug( self, mock_uploads_enabled, mock_open, send_file): # this test data is based on real observation using a browser # and werkzeug 0.14.1 - res = {'clear_upload': u'true', - 'format': u'CSV', - 'url': u'https://example.com/data.csv', - 'description': u'', - 'upload': u'', + res = {u'clear_upload': u'true', + u'format': u'CSV', + u'url': u'https://example.com/data.csv', + u'description': u'', + u'upload': u'', u'package_id': u'dataset1', - 'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', - 'name': u'data.csv'} + u'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', + u'name': u'data.csv'} res_upload = ResourceUpload(res) eq_(res_upload.filename, None) - @mock.patch.object(ckan.lib.uploader, 'os', fake_os) - @mock.patch.object(builtins, 'open', side_effect=mock_open_if_open_fails) - @mock.patch.object(flask, 'send_file', side_effect=[b'DATA']) - @mock.patch.object(paste.fileapp, 'os', fake_os) - @mock.patch.object(config['pylons.h'], 'uploads_enabled', + @mock.patch.object(ckan.lib.uploader, u'os', fake_os) + @mock.patch.object(builtins, u'open', side_effect=mock_open_if_open_fails) + @mock.patch.object(flask, u'send_file', side_effect=[b'DATA']) + @mock.patch.object(paste.fileapp, u'os', fake_os) + @mock.patch.object(config[u'pylons.h'], u'uploads_enabled', return_value=True) - @mock.patch.object(ckan.lib.uploader, '_storage_path', new='/doesnt_exist') + @mock.patch.object(ckan.lib.uploader, u'_storage_path', new=u'/doesnt_exist') def test_resource_without_upload( self, mock_uploads_enabled, mock_open, send_file): # this test data is based on real observation using a browser - res = {'clear_upload': u'true', - 'format': u'PNG', - 'url': u'https://example.com/data.csv', - 'description': u'', - 'upload': FileStorage(filename=u''), + res = {u'clear_upload': u'true', + u'format': u'PNG', + u'url': u'https://example.com/data.csv', + u'description': u'', + u'upload': FileStorage(filename=u''), u'package_id': u'dataset1', - 'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', - 'name': u'data.csv'} + u'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', + u'name': u'data.csv'} res_upload = ResourceUpload(res) eq_(res_upload.filename, None) - @mock.patch.object(ckan.lib.uploader, 'os', fake_os) - @mock.patch.object(builtins, 'open', side_effect=mock_open_if_open_fails) - @mock.patch.object(flask, 'send_file', side_effect=[b'DATA']) - @mock.patch.object(paste.fileapp, 'os', fake_os) - @mock.patch.object(config['pylons.h'], 'uploads_enabled', + @mock.patch.object(ckan.lib.uploader, u'os', fake_os) + @mock.patch.object(builtins, u'open', side_effect=mock_open_if_open_fails) + @mock.patch.object(flask, u'send_file', side_effect=[b'DATA']) + @mock.patch.object(paste.fileapp, u'os', fake_os) + @mock.patch.object(config[u'pylons.h'], u'uploads_enabled', return_value=True) - @mock.patch.object(ckan.lib.uploader, '_storage_path', new='/doesnt_exist') + @mock.patch.object(ckan.lib.uploader, u'_storage_path', new=u'/doesnt_exist') def test_resource_with_upload( self, mock_uploads_enabled, mock_open, send_file): # this test data is based on real observation using a browser - res = {'clear_upload': u'', - 'format': u'PNG', - 'url': u'https://example.com/data.csv', - 'description': u'', - 'upload': FileStorage(filename=u'data.csv', content_type='CSV'), + res = {u'clear_upload': u'', + u'format': u'PNG', + u'url': u'https://example.com/data.csv', + u'description': u'', + u'upload': FileStorage(filename=u'data.csv', content_type=u'CSV'), u'package_id': u'dataset1', - 'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', - 'name': u'data.csv'} + u'id': u'8a3a874e-5ee1-4e43-bdaf-e2569cf72344', + u'name': u'data.csv'} res_upload = ResourceUpload(res) eq_(res_upload.filesize, 0) - eq_(res_upload.filename, 'data.csv') + eq_(res_upload.filename, u'data.csv') From eca8887bece5c14eb08f43d4bd527b63b204b2af Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 12:16:40 +0100 Subject: [PATCH 05/14] Fix asserts after wrong merge --- ckan/tests/controllers/test_package.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ckan/tests/controllers/test_package.py b/ckan/tests/controllers/test_package.py index 126076deb6e..0d9363e265a 100644 --- a/ckan/tests/controllers/test_package.py +++ b/ckan/tests/controllers/test_package.py @@ -134,9 +134,9 @@ def test_complete_package_with_two_resources(self, app): submit_and_follow(app, form, env, 'save', 'go-metadata') pkg = model.Package.by_name(u'complete-package-with-two-resources') - assert_equal(pkg.resources[0].url, u'http://example.com/resource0') - assert_equal(pkg.resources[1].url, u'http://example.com/resource1') - assert_equal(pkg.state, 'active') + assert pkg.resources[0].url == u'http://example.com/resource0' + assert pkg.resources[1].url == u'http://example.com/resource1' + assert pkg.state == 'active' # resource upload is tested in TestExampleIUploaderPlugin From f22f01d989b1b9fd87720140bb13aab3e3a7602c Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 13:38:33 +0100 Subject: [PATCH 06/14] [#5031] [#5031] More tests fixes --- ckan/tests/controllers/test_package.py | 3 +-- ckan/tests/logic/action/test_create.py | 12 ++++++++---- ckan/tests/logic/action/test_update.py | 19 ++++++++++--------- 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/ckan/tests/controllers/test_package.py b/ckan/tests/controllers/test_package.py index 0d9363e265a..123e663bf0d 100644 --- a/ckan/tests/controllers/test_package.py +++ b/ckan/tests/controllers/test_package.py @@ -140,8 +140,7 @@ def test_complete_package_with_two_resources(self, app): # resource upload is tested in TestExampleIUploaderPlugin - def test_previous_button_works(self): - app = self._get_test_app() + def test_previous_button_works(self, app): env, response = _get_package_new_page(app) form = response.forms["dataset-edit"] form["name"] = u"previous-button-works" diff --git a/ckan/tests/logic/action/test_create.py b/ckan/tests/logic/action/test_create.py index 98293ec3f1a..82bc1992eff 100644 --- a/ckan/tests/logic/action/test_create.py +++ b/ckan/tests/logic/action/test_create.py @@ -8,6 +8,8 @@ import mock import pytest +from werkzeug.datastructures import FileStorage as FlaskFileStorage + import ckan import ckan.logic as logic import ckan.model as model @@ -26,9 +28,11 @@ fake_open = fake_filesystem.FakeFileOpen(fs) -class FakeFileStorage(cgi.FieldStorage): - def __init__(self, fp, filename): - self.file = fp +class FakeFileStorage(FlaskFileStorage): + content_type = None + + def __init__(self, stream, filename): + self.stream = stream self.filename = filename self.name = "upload" @@ -535,7 +539,7 @@ def test_mimetype_by_upload_by_file(self, mock_open): NAZKO,1C08,1070,2016/01/05,20,31,,76,16,JAN-01,41 """ ) - test_resource = FakeFileStorage(test_file, "") + test_resource = FakeFileStorage(test_file, "test.csv") context = {} params = { diff --git a/ckan/tests/logic/action/test_update.py b/ckan/tests/logic/action/test_update.py index 163e88e0542..b30033b3c34 100644 --- a/ckan/tests/logic/action/test_update.py +++ b/ckan/tests/logic/action/test_update.py @@ -26,6 +26,15 @@ fake_open = fake_filesystem.FakeFileOpen(fs) +class FakeFileStorage(FlaskFileStorage): + content_type = None + + def __init__(self, stream, filename): + self.stream = stream + self.filename = filename + self.name = "upload" + + def mock_open_if_open_fails(*args, **kwargs): try: return real_open(*args, **kwargs) @@ -813,14 +822,6 @@ def test_calling_with_only_id_doesnt_update_anything(self): @pytest.mark.usefixtures("clean_db", "with_plugins") class TestResourceUpdate(object): - class FakeFileStorage(FlaskFileStorage): - content_type = None - - def __init__(self, stream, filename): - self.stream = stream - self.filename = filename - self.name = "upload" - def test_url_only(self): dataset = factories.Dataset() resource = factories.Resource(package=dataset, url="http://first") @@ -1072,7 +1073,7 @@ def test_mimetype_by_upload_by_file(self, mock_open): NAZKO,1C08,1070,2016/01/05,20,31,,76,16,JAN-01,41 """ ) - update_resource = TestResourceUpdate.FakeFileStorage( + update_resource = FakeFileStorage( update_file, "update_test" ) From e26b3b8bf5bd9f688567d95151fe94cc4184cab4 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 14:37:36 +0100 Subject: [PATCH 07/14] [#5031] Fix class reference --- ckan/tests/logic/action/test_update.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ckan/tests/logic/action/test_update.py b/ckan/tests/logic/action/test_update.py index b30033b3c34..1dbfeadad1a 100644 --- a/ckan/tests/logic/action/test_update.py +++ b/ckan/tests/logic/action/test_update.py @@ -1123,7 +1123,7 @@ def test_mimetype_by_upload_by_filename(self, mock_open): } """ ) - test_resource = TestResourceUpdate.FakeFileStorage( + test_resource = FakeFileStorage( test_file, "test.json" ) dataset = factories.Dataset() @@ -1146,7 +1146,7 @@ def test_mimetype_by_upload_by_filename(self, mock_open): NAZKO,1C08,1070,2016/01/05,20,31,,76,16,JAN-01,41 """ ) - update_resource = TestResourceUpdate.FakeFileStorage( + update_resource = FakeFileStorage( update_file, "update_test.csv" ) @@ -1218,7 +1218,7 @@ def test_size_of_resource_by_upload(self, mock_open): } """ ) - test_resource = TestResourceUpdate.FakeFileStorage( + test_resource = FakeFileStorage( test_file, "test.json" ) dataset = factories.Dataset() @@ -1241,7 +1241,7 @@ def test_size_of_resource_by_upload(self, mock_open): NAZKO,1C08,1070,2016/01/05,20,31,,76,16,JAN-01,41 """ ) - update_resource = TestResourceUpdate.FakeFileStorage( + update_resource = FakeFileStorage( update_file, "update_test.csv" ) From ea74e42f6be4bc89a3ad89f367a47ce0bcd2b761 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 10 Jan 2020 15:38:51 +0100 Subject: [PATCH 08/14] [#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 09/14] [#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 10/14] [#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 11/14] [#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 12/14] [#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 13/14] [#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 14/14] [#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):