From d8c24159926ef5a6b1ddc66b65a14bcd4d844572 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 07 2017 09:53:02 +0000 Subject: [PATCH 1/7] Add support for uploading multiple files to an issue at once Fixes https://pagure.io/pagure/issue/2315 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index c0a6313..c0ce5bb 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -2511,7 +2511,7 @@ def add_attachment(repo, issue, attachmentfolder, user, filename, filestream): hashlib.sha256(filestream.read()).hexdigest(), werkzeug.secure_filename(filename) ) - filedir = os.path.join(attachmentfolder, repo.fullname) + filedir = os.path.join(attachmentfolder, repo.fullname, 'files') filepath = os.path.join(filedir, filename) if os.path.exists(filepath): diff --git a/pagure/static/upload.js b/pagure/static/upload.js index 387669b..6c9c251 100644 --- a/pagure/static/upload.js +++ b/pagure/static/upload.js @@ -63,16 +63,17 @@ function doUpload(csrf_token, files) { return; } else { - console.log(data); var _txt = $("#comment").val(); if (_txt) { _txt += '\n'; } - $("#comment").val( - _txt - + '[![' + data.filename + '](' + data.filelocation + ')](' - + data.filelocation + ')' - ) + var _urls = ''; + for (var i = 0, ie = data.filenames.length; i < ie; i++) { + _urls += '[![' + data.filenames[i] + '](' + + data.filelocations[i] + ')](' + + data.filelocations[i] + ')' + } + $("#comment").val(_txt + _urls) } setTimeout( function(){ diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 9c897b4..deb93c9 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -1249,26 +1249,33 @@ def upload_issue(repo, issueid, username=None, namespace=None): form = pagure.forms.UploadFileForm() if form.validate_on_submit(): - filestream = flask.request.files['filestream'] - new_filename = pagure.lib.add_attachment( - repo=repo, - issue=issue, - attachmentfolder=APP.config['ATTACHMENTS_FOLDER'], - user=user_obj, - filename=filestream.filename, - filestream=filestream.stream, - ) + filenames = [] + for filestream in flask.request.files.getlist('filestream'): + filenames.append(filestream.filename) + new_filename = pagure.lib.add_attachment( + repo=repo, + issue=issue, + attachmentfolder=APP.config['ATTACHMENTS_FOLDER'], + user=user_obj, + filename=filestream.filename, + filestream=filestream.stream, + ) return flask.jsonify({ 'output': 'ok', - 'filename': new_filename.split('-', 1)[1], - 'filelocation': flask.url_for( - 'view_issue_raw_file', - repo=repo.name, - username=username, - namespace=repo.namespace, - filename=new_filename, - ) + 'filenames': [ + filename.split('-', 1)[1] + for filename in filenames], + 'filelocations': [ + flask.url_for( + 'view_issue_raw_file', + repo=repo.name, + username=username, + namespace=repo.namespace, + filename=new_filename, + ) + for new_filename in filenames + ] }) else: return flask.jsonify({'output': 'notok'}) From 1cb715ee2041638ea0b0e67592407d4f1ef3a97d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 07 2017 09:53:02 +0000 Subject: [PATCH 2/7] Fix typo in the logging Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index c0ce5bb..778f463 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -2503,7 +2503,7 @@ def reset_status_pull_request(session, project): def add_attachment(repo, issue, attachmentfolder, user, filename, filestream): ''' Add a file to the attachments folder of repo and update git. ''' _log.info( - 'Addinf file: %s to the git repo: %s', + 'Adding file: %s to the git repo: %s', repo.path, werkzeug.secure_filename(filename)) # Prefix the filename with a timestamp: From 3887318d8f62d5cea5d3ff9c6e87c320d69a458c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 07 2017 09:53:02 +0000 Subject: [PATCH 3/7] Fixes path to the files uploaded or linked to Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 6d79166..55b36fa 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -871,7 +871,7 @@ def _add_file_to_git(repo, issue, attachmentfolder, ticketfolder, user, os.mkdir(folder_path) # Copy from attachments directory - src = os.path.join(attachmentfolder, repo.fullname, filename) + src = os.path.join(attachmentfolder, repo.fullname, 'files', filename) shutil.copyfile(src, file_path) # Retrieve the list of files that changed diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index deb93c9..e5337c9 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -1251,7 +1251,6 @@ def upload_issue(repo, issueid, username=None, namespace=None): if form.validate_on_submit(): filenames = [] for filestream in flask.request.files.getlist('filestream'): - filenames.append(filestream.filename) new_filename = pagure.lib.add_attachment( repo=repo, issue=issue, @@ -1260,6 +1259,7 @@ def upload_issue(repo, issueid, username=None, namespace=None): filename=filestream.filename, filestream=filestream.stream, ) + filenames.append(new_filename) return flask.jsonify({ 'output': 'ok', @@ -1272,7 +1272,7 @@ def upload_issue(repo, issueid, username=None, namespace=None): repo=repo.name, username=username, namespace=repo.namespace, - filename=new_filename, + filename='files/%s' % new_filename, ) for new_filename in filenames ] From 8be363bef5c5ea30925a7876cc49afc6333658f8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 07 2017 09:53:02 +0000 Subject: [PATCH 4/7] Add some more logging to the tasks performed by the worker Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index dbbcf98..411dee0 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -28,6 +28,7 @@ from pagure import APP import pagure.lib import pagure.lib.git +logging.config.dictConfig(APP.config.get('LOGGING') or {'version': 1}) _log = logging.getLogger(__name__) @@ -359,7 +360,9 @@ def merge_pull_request(name, namespace, user, requestid, user_merger): with project.lock('WORKER'): request = pagure.lib.search_pull_requests( session, project_id=project.id, requestid=requestid) - + _log.debug( + 'Merging pull-request: %/#%s', request.project.fullname, + request.id) pagure.lib.git.merge_pull_request( session, request, user_merger, APP.config['REQUESTS_FOLDER']) @@ -379,9 +382,16 @@ def add_file_to_git(name, namespace, user, user_attacher, issueuid, filename): issue = pagure.lib.get_issue_by_uid(session, issueuid) user_attacher = pagure.lib.search_user(session, username=user_attacher) + from_folder = APP.config['ATTACHMENTS_FOLDER'] + to_folder = APP.config['TICKETS_FOLDER'] + _log.info( + 'Adding file %s from %s to %s', filename, from_folder, to_folder) pagure.lib.git._add_file_to_git( - project, issue, APP.config['ATTACHMENTS_FOLDER'], - APP.config['TICKETS_FOLDER'], user_attacher, filename) + project, issue, + from_folder, + to_folder, + user_attacher, + filename) session.remove() gc_clean() From d265db336e6cd3b49004829fcc08ca08f2177af9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 07 2017 09:53:02 +0000 Subject: [PATCH 5/7] Adjust the default logging config to the one run in prod Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/default_config.py b/pagure/default_config.py index fca3695..21b0318 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -305,8 +305,8 @@ LOGGING = { 'loggers': { 'pagure': { 'handlers': ['console'], - 'level': 'INFO', - 'propagate': False + 'level': 'DEBUG', + 'propagate': True }, 'flask': { 'handlers': ['console'], @@ -318,5 +318,15 @@ LOGGING = { 'level': 'WARN', 'propagate': False }, + 'binaryornot': { + 'handlers': ['console'], + 'level': 'WARN', + 'propagate': True + }, + 'pagure.lib.encoding_utils': { + 'handlers': ['console'], + 'level': 'WARN', + 'propagate': False + }, } } From e85e3283f28ba6b851062441574caf70d2f6c5c4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 07 2017 09:53:02 +0000 Subject: [PATCH 6/7] Log when we're pushing to the remote --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 55b36fa..c1f11aa 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -916,6 +916,7 @@ def _add_file_to_git(repo, issue, attachmentfolder, ticketfolder, user, master_ref = new_repo.lookup_reference('HEAD').resolve() refname = '%s:%s' % (master_ref.name, master_ref.name) + _log.info('Pushing to %s: %s', ori_remote.name, refname) PagureRepo.push(ori_remote, refname) # Remove the clone From 766620b3af4e9f4f2d1e3fa255163d7a39ad816d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 07 2017 10:15:46 +0000 Subject: [PATCH 7/7] Fix and improve testing uploading a file to a ticket This commit adds unit-tests for uploading multiple files at once in the UI as well as rework how the testing for single upload is done so that it is tested even when pyclamd is not installed/available. Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_issues.py b/tests/test_pagure_flask_ui_issues.py index 972f28f..48212f5 100644 --- a/tests/test_pagure_flask_ui_issues.py +++ b/tests/test_pagure_flask_ui_issues.py @@ -42,9 +42,6 @@ class PagureFlaskIssuestests(tests.Modeltests): super(PagureFlaskIssuestests, self).setUp() pagure.APP.config['TESTING'] = True - # TODO: Figure a way to enable this test on jenkins - if not os.environ.get('BUILD_ID'): - pagure.APP.config['VIRUS_SCAN_ATTACHMENTS'] = True pagure.SESSION = self.session pagure.ui.SESSION = self.session pagure.ui.app.SESSION = self.session @@ -1571,8 +1568,6 @@ class PagureFlaskIssuestests(tests.Modeltests): @patch('pagure.lib.notify.send_email') def test_upload_issue(self, p_send_email, p_ugt): """ Test the upload_issue endpoint. """ - if not pyclamd: - raise SkipTest() p_send_email.return_value = True p_ugt.return_value = True @@ -1628,6 +1623,75 @@ class PagureFlaskIssuestests(tests.Modeltests): exp = {'output': 'notok'} self.assertDictEqual(json_data, exp) + # Attach a file to a ticket + with open(os.path.join(tests.HERE, 'placebo.png'), 'rb') as stream: + data = { + 'csrf_token': csrf_token, + 'filestream': stream, + 'enctype': 'multipart/form-data', + } + output = self.app.post( + '/test/issue/1/upload', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + json_data = json.loads(output.data) + + folder = os.path.dirname( + os.path.abspath(__file__))[1:].replace('/', '_') + exp = { + 'filelocations': [ + '/test/issue/raw/files/8a06845923010b27bfd8' + 'e7e75acff7badc40d1021b4994e01f5e11ca40bc3a' + 'be-%s_placebo.png' % folder + ], + 'filenames': ['%s_placebo.png' % folder], + 'output': 'ok' + } + self.assertDictEqual(json_data, exp) + + # Project w/o issue tracker + repo = pagure.get_authorized_project(self.session, 'test') + repo.settings = {'issue_tracker': False} + self.session.add(repo) + self.session.commit() + + with tests.user_set(pagure.APP, user): + output = self.app.post('/test/issue/1/upload') + self.assertEqual(output.status_code, 404) + + @patch.dict('pagure.APP.config', {'PR_ONLY': True}) + @patch('pagure.lib.git.update_git') + @patch('pagure.lib.notify.send_email') + def test_upload_issue_virus(self, p_send_email, p_ugt): + """ Test the upload_issue endpoint. """ + if not pyclamd: + raise SkipTest() + p_send_email.return_value = True + p_ugt.return_value = True + + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'repos'), bare=True) + tests.create_projects_git( + os.path.join(self.path, 'tickets'), bare=True) + + # Create issues to play with + repo = pagure.get_authorized_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ticketfolder=None + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + csrf_token = self.get_csrf() + # TODO: Figure a way to enable this test on jenkins # Try to attach a virus if not os.environ.get('BUILD_ID'): @@ -1649,15 +1713,47 @@ class PagureFlaskIssuestests(tests.Modeltests): } self.assertDictEqual(json_data, exp) - # Attach a file to a ticket + @patch('pagure.lib.git.update_git') + @patch('pagure.lib.notify.send_email') + def test_upload_issue_two_files(self, p_send_email, p_ugt): + """ Test the upload_issue endpoint with two files. """ + p_send_email.return_value = True + p_ugt.return_value = True + + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'repos'), bare=True) + tests.create_projects_git( + os.path.join(self.path, 'tickets'), bare=True) + + # Create issues to play with + repo = pagure.get_authorized_project(self.session, 'test') + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ticketfolder=None + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + csrf_token = self.get_csrf() + + # Attach two files to a ticket with open(os.path.join(tests.HERE, 'placebo.png'), 'rb') as stream: - data = { - 'csrf_token': csrf_token, - 'filestream': stream, - 'enctype': 'multipart/form-data', - } - output = self.app.post( - '/test/issue/1/upload', data=data, follow_redirects=True) + with open(os.path.join(tests.HERE, 'placebo.png'), 'rb') as stream2: + data = { + 'csrf_token': csrf_token, + 'filestream': [stream, stream2], + 'enctype': 'multipart/form-data', + } + output = self.app.post( + '/test/issue/1/upload', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) json_data = json.loads(output.data) @@ -1665,23 +1761,21 @@ class PagureFlaskIssuestests(tests.Modeltests): os.path.abspath(__file__))[1:].replace('/', '_') exp = { 'output': 'ok', - 'filelocation': '/test/issue/raw/8a06845923010b27bfd8' - 'e7e75acff7badc40d1021b4994e01f5e11ca40bc3a' - 'be-%s_placebo.png' % folder, - 'filename': '%s_placebo.png' % folder, + 'filelocations': [ + '/test/issue/raw/files/8a06845923010b27bfd8' + 'e7e75acff7badc40d1021b4994e01f5e11ca40bc3a' + 'be-%s_placebo.png' % folder, + '/test/issue/raw/files/8a06845923010b27bfd8' + 'e7e75acff7badc40d1021b4994e01f5e11ca40bc3a' + 'be-%s_placebo.png' % folder, + ], + 'filenames': [ + '%s_placebo.png' % folder, + '%s_placebo.png' % folder + ], } self.assertDictEqual(json_data, exp) - # Project w/o issue tracker - repo = pagure.get_authorized_project(self.session, 'test') - repo.settings = {'issue_tracker': False} - self.session.add(repo) - self.session.commit() - - with tests.user_set(pagure.APP, user): - output = self.app.post('/test/issue/1/upload') - self.assertEqual(output.status_code, 404) - def test_view_issue_raw_file_empty(self): """ Test the view_issue_raw_file endpoint. """ # Create the project and git repos