From 72ac1c9a9b79759663375f8f4446e99c6d3c1e05 Mon Sep 17 00:00:00 2001 From: Clement Verna Date: Aug 21 2017 09:06:04 +0000 Subject: [PATCH 1/4] New API endpoint to modify multiple custom fields. This commit adds a new endpoint so that a user can send multiple custom fields in one request. Signed-off-by: Clement Verna --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 4c5f595..f2699e7 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -89,6 +89,7 @@ class APIERROR(enum.Enum): ENOTMAINADMIN = 'Only the main admin can set the main admin of a project' EMODIFYPROJECTNOTALLOWED = 'You are not allowed to modify this project' EINVALIDPERPAGEVALUE = 'The per_page value must be between 1 and 100' + EINVALIDCUSTOMFIELDS = 'This request format is invalid' def get_authorized_api_project(SESSION, repo, user=None, namespace=None): diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 1768f56..d5d0232 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -122,6 +122,21 @@ def _check_ticket_access(issue): 403, error_code=APIERROR.EISSUENOTALLOWED) +def _check_link_custom_field(field, links): + """Check if the value provided in the link custom field + is a link. + ::param field (pagure.lib.model.IssueKeys) : The issue custom field key object. + ::param links (str): Value of the custom field. + ::raises pagure.exceptions.APIERROR.EINVALIDISSUEFIELD_LINK when invalid. + """ + if field.key_type == 'link': + links = links.split(',') + for link in links: + link = link.replace(' ', '') + if not urlpattern.match(link): + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EINVALIDISSUEFIELD_LINK) + @API.route('//new_issue', methods=['POST']) @API.route('///new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) @@ -1211,3 +1226,115 @@ def api_update_custom_field( jsonout = flask.jsonify(output) return jsonout + + +@API.route('//issue//custom', methods=['POST']) +@API.route( + '///issue//custom', + methods=['POST']) +@API.route( + '/fork///issue//custom', + methods=['POST']) +@API.route( + '/fork////issue//custom', + methods=['POST']) +@api_login_required(acls=['issue_update_custom_fields', 'issue_update']) +@api_method +def api_update_custom_fields( + repo, issueid, username=None, namespace=None): + """ + Update custom fields + -------------------- + Update or reset the content of a collection of custom fields + associated to an issue. + + :: + + POST /api/0//issue//custom + POST /api/0///issue//custom + + :: + + POST /api/0/fork///issue//custom + POST /api/0/fork////issue//custom + + Input + ^^^^^ + + +------------------+---------+--------------+-----------------------------+ + | Key | Type | Optionality | Description | + +==================+=========+==============+=============================+ + | ``fields`` | dict | Mandatory | A dictionary with the field | + | | | | name as key and the value | + +------------------+---------+--------------+-----------------------------+ + + Sample response + ^^^^^^^^^^^^^^^ + + :: + + { + "fields": [ + { + "myField" : "Custom field myField adjusted to test (was: to do)" + }, + { + "myField_1": "Custom field myField_1 adjusted to done (was: test)" + } + ] + } + + """ # noqa + output = {'fields': []} + repo = _get_repo(repo, username, namespace) + _check_issue_tracker(repo) + _check_token(repo) + + issue = _get_issue(repo, issueid) + _check_ticket_access(issue) + + fields = flask.request.get_json(force=True, silent=True) + + if fields is None or fields.get('fields') is None: + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EINVALIDCUSTOMFIELDS) + + fields = fields.get('fields') + + repo_fields = {k.name: k for k in repo.issue_keys} + + if not all(key in repo_fields.keys() for key in fields.keys()): + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EINVALIDISSUEFIELD) + + for field in fields: + key = repo_fields[field] + value = fields.get(key.name) + if value: + _check_link_custom_field(key, value) + try: + message = pagure.lib.set_custom_key_value( + SESSION, issue, key, value) + + SESSION.commit() + if message: + output['fields'].append({key.name: message}) + pagure.lib.add_metadata_update_notif( + session=SESSION, + issue=issue, + messages=message, + user=flask.g.fas_user.username, + ticketfolder=APP.config['TICKETS_FOLDER'] + ) + else: + output['fields'].append({key.name: 'No changes'}) + except pagure.exceptions.PagureException as err: + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.ENOCODE, error=str(err)) + except SQLAlchemyError as err: # pragma: no cover + print err + SESSION.rollback() + raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR) + + jsonout = flask.jsonify(output) + return jsonout diff --git a/tests/test_pagure_flask_api_issue_custom_fields.py b/tests/test_pagure_flask_api_issue_custom_fields.py new file mode 100644 index 0000000..7abbc4e --- /dev/null +++ b/tests/test_pagure_flask_api_issue_custom_fields.py @@ -0,0 +1,195 @@ +""" + (c) 2017 - Copyright Red Hat Inc + + Authors: + Clement Verna + +""" + + +import unittest +import sys +import os +import json + + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure # noqa: E402 +import pagure.lib # noqa: E402 +import tests # noqa: E402 + + +class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): + """ Tests for the flask API of pagure for issue's custom fields """ + def setUp(self): + """ Set up the environnment, ran before every tests. """ + self.maxDiff = None + super(PagureFlaskApiCustomFieldIssuetests, self).setUp() + + pagure.APP.config['TESTING'] = True + pagure.SESSION = self.session + pagure.api.SESSION = self.session + pagure.api.issue.SESSION = self.session + pagure.lib.SESSION = self.session + + pagure.APP.config['TICKETS_FOLDER'] = None + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'tickets')) + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + + # Create normal issue + repo = pagure.get_authorized_project(self.session, 'test') + pagure.lib.new_issue( + session=self.session, + repo=repo, + title='Test issue #1', + content='We should work on this', + user='pingou', + ticketfolder=None, + private=False, + ) + self.session.commit() + + def test_api_update_custom_field_bad_request(self): + """ Test the api_update_custom_field method of the flask api. + This test that a badly form request returns the correct error. + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Request is not formated correctly + payload = json.dumps({'field': + {'foo': 'bar'}}) + output = self.app.post( + '/api/0/test/issue/1/custom', headers=headers, data=payload) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "This request format is invalid", + "error_code": "EINVALIDCUSTOMFIELDS", + } + ) + + def test_api_update_custom_field_wrong_field(self): + """ Test the api_update_custom_field method of the flask api. + This test that an invalid field retruns the correct error. + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + # Project does not have this custom field + payload = json.dumps({'fields': + {'foo': 'bar'}}) + output = self.app.post( + '/api/0/test/issue/1/custom', headers=headers, data=payload) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "error": "Invalid custom field submitted", + "error_code": "EINVALIDISSUEFIELD", + } + ) + + def test_api_update_custom_field(self): + """ Test the api_update_custom_field method of the flask api. + This test the successful requests scenarii. + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + + repo = pagure.get_authorized_project(self.session, 'test') + settings = repo.settings + settings['issue_tracker'] = True + repo.settings = settings + self.session.add(repo) + self.session.commit() + + # Set some custom fields + repo = pagure.get_authorized_project(self.session, 'test') + msg = pagure.lib.set_custom_key_fields( + self.session, repo, + ['bugzilla', 'upstream', 'reviewstatus'], + ['link', 'boolean', 'list'], + ['unused data for non-list type', '', 'ack', 'nack', 'needs review'], + [None, None, None]) + self.session.commit() + self.assertEqual(msg, 'List of custom fields updated') + + payload = json.dumps({'fields': + {'bugzilla': '', 'upstream': True}}) + output = self.app.post( + '/api/0/test/issue/1/custom', headers=headers, data=payload) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "fields": [ + {"bugzilla": "No changes"}, + {"upstream": "Custom field upstream adjusted to True"}, + ] + } + ) + + repo = pagure.get_authorized_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(len(issue.other_fields), 1) + + payload = json.dumps({'fields': + {'bugzilla': 'https://bugzilla.redhat.com/1234', + 'upstream': False, + 'reviewstatus': 'ack'}}) + output = self.app.post( + '/api/0/test/issue/1/custom', headers=headers, + data=payload) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "fields": [ + {"bugzilla": "Custom field bugzilla adjusted to " + "https://bugzilla.redhat.com/1234"}, + {"reviewstatus": "Custom field reviewstatus adjusted to ack"}, + {"upstream": "Custom field upstream reset (from 1)"}, + + ] + } + ) + + repo = pagure.get_authorized_project(self.session, 'test') + issue = pagure.lib.search_issues(self.session, repo, issueid=1) + self.assertEqual(len(issue.other_fields), 3) + + # Reset the value + payload = json.dumps({'fields': + {'bugzilla': '', + 'upstream': '', + 'reviewstatus': ''}}) + output = self.app.post( + '/api/0/test/issue/1/custom', headers=headers, + data=payload) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + self.assertDictEqual( + data, + { + "fields": [ + {"bugzilla": "Custom field bugzilla reset " + "(from https://bugzilla.redhat.com/1234)"}, + {"reviewstatus": "Custom field reviewstatus reset (from ack)"}, + {"upstream": "Custom field upstream reset (from 0)"}, + ] + } + ) + + +if __name__ == '__main__': + unittest.main(verbosity=2) From 8413286b1241cd4fd3763c6890780a02760cd175 Mon Sep 17 00:00:00 2001 From: Clement Verna Date: Aug 21 2017 09:06:04 +0000 Subject: [PATCH 2/4] Replace common logic with helper function to check link url Signed-off-by: Clement Verna --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index d5d0232..38f8198 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -137,6 +137,7 @@ def _check_link_custom_field(field, links): raise pagure.exceptions.APIError( 400, error_code=APIERROR.EINVALIDISSUEFIELD_LINK) + @API.route('//new_issue', methods=['POST']) @API.route('///new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) @@ -1184,13 +1185,7 @@ def api_update_custom_field( key = fields[field] value = flask.request.form.get('value') if value: - if key.key_type == 'link': - links = value.split(',') - for link in links: - link = link.replace(' ', '') - if not urlpattern.match(link): - raise pagure.exceptions.APIError( - 400, error_code=APIERROR.EINVALIDISSUEFIELD_LINK) + _check_link_custom_field(key, value) try: message = pagure.lib.set_custom_key_value( SESSION, issue, key, value) From 5ce0bdaf0a488af5a204db781ea41385cc5f2ede Mon Sep 17 00:00:00 2001 From: Clement Verna Date: Aug 21 2017 09:06:04 +0000 Subject: [PATCH 3/4] Remove duplicated call to pagure.lib.add_metadata_update_notif Signed-off-by: Clement Verna --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 38f8198..3333e4e 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -1210,15 +1210,6 @@ def api_update_custom_field( SESSION.rollback() raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR) - if message: - pagure.lib.add_metadata_update_notif( - session=SESSION, - issue=issue, - messages=message, - user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] - ) - jsonout = flask.jsonify(output) return jsonout From ee0ae35377f0830a7cd7aa44b475186bc29a935f Mon Sep 17 00:00:00 2001 From: Clement Verna Date: Aug 21 2017 09:06:04 +0000 Subject: [PATCH 4/4] Fixing comment from the review Signed-off-by: Clement Verna --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index f2699e7..4c5f595 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -89,7 +89,6 @@ class APIERROR(enum.Enum): ENOTMAINADMIN = 'Only the main admin can set the main admin of a project' EMODIFYPROJECTNOTALLOWED = 'You are not allowed to modify this project' EINVALIDPERPAGEVALUE = 'The per_page value must be between 1 and 100' - EINVALIDCUSTOMFIELDS = 'This request format is invalid' def get_authorized_api_project(SESSION, repo, user=None, namespace=None): diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 3333e4e..577ccef 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -125,17 +125,17 @@ def _check_ticket_access(issue): def _check_link_custom_field(field, links): """Check if the value provided in the link custom field is a link. - ::param field (pagure.lib.model.IssueKeys) : The issue custom field key object. - ::param links (str): Value of the custom field. - ::raises pagure.exceptions.APIERROR.EINVALIDISSUEFIELD_LINK when invalid. + :param field : The issue custom field key object. + :param links : Value of the custom field. + :raises pagure.exceptions.APIERROR when invalid. """ if field.key_type == 'link': - links = links.split(',') - for link in links: - link = link.replace(' ', '') - if not urlpattern.match(link): - raise pagure.exceptions.APIError( - 400, error_code=APIERROR.EINVALIDISSUEFIELD_LINK) + links = links.split(',') + for link in links: + link = link.replace(' ', '') + if not urlpattern.match(link): + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.EINVALIDISSUEFIELD_LINK) @API.route('//new_issue', methods=['POST']) @@ -1250,28 +1250,42 @@ def api_update_custom_fields( +------------------+---------+--------------+-----------------------------+ | Key | Type | Optionality | Description | +==================+=========+==============+=============================+ - | ``fields`` | dict | Mandatory | A dictionary with the field | + | ``myfields`` | dict | Mandatory | A dictionary with the fields| | | | | name as key and the value | +------------------+---------+--------------+-----------------------------+ + Sample payload + ^^^^^^^^^^^^^^ + + :: + + { + "myField": "to do", + "myField_1": "test", + "myField_2": "done", + } + Sample response ^^^^^^^^^^^^^^^ :: { - "fields": [ + "messages": [ + { + "myField" : "Custom field myField adjusted to to do" + }, { - "myField" : "Custom field myField adjusted to test (was: to do)" + "myField_1": "Custom field myField_1 adjusted test (was: to do)" }, { - "myField_1": "Custom field myField_1 adjusted to done (was: test)" + "myField_2": "Custom field myField_1 adjusted to done (was: test)" } ] } """ # noqa - output = {'fields': []} + output = {'messages': []} repo = _get_repo(repo, username, namespace) _check_issue_tracker(repo) _check_token(repo) @@ -1279,13 +1293,11 @@ def api_update_custom_fields( issue = _get_issue(repo, issueid) _check_ticket_access(issue) - fields = flask.request.get_json(force=True, silent=True) + fields = flask.request.form - if fields is None or fields.get('fields') is None: + if not fields: raise pagure.exceptions.APIError( - 400, error_code=APIERROR.EINVALIDCUSTOMFIELDS) - - fields = fields.get('fields') + 400, error_code=APIERROR.EINVALIDREQ) repo_fields = {k.name: k for k in repo.issue_keys} @@ -1304,7 +1316,7 @@ def api_update_custom_fields( SESSION.commit() if message: - output['fields'].append({key.name: message}) + output['messages'].append({key.name: message}) pagure.lib.add_metadata_update_notif( session=SESSION, issue=issue, @@ -1313,7 +1325,7 @@ def api_update_custom_fields( ticketfolder=APP.config['TICKETS_FOLDER'] ) else: - output['fields'].append({key.name: 'No changes'}) + output['messages'].append({key.name: 'No changes'}) except pagure.exceptions.PagureException as err: raise pagure.exceptions.APIError( 400, error_code=APIERROR.ENOCODE, error=str(err)) diff --git a/tests/test_pagure_flask_api_issue_custom_fields.py b/tests/test_pagure_flask_api_issue_custom_fields.py index 7abbc4e..e0f1e02 100644 --- a/tests/test_pagure_flask_api_issue_custom_fields.py +++ b/tests/test_pagure_flask_api_issue_custom_fields.py @@ -6,7 +6,6 @@ """ - import unittest import sys import os @@ -61,9 +60,8 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): headers = {'Authorization': 'token aaabbbcccddd'} - # Request is not formated correctly - payload = json.dumps({'field': - {'foo': 'bar'}}) + # Request is not formated correctly (EMPTY) + payload = {} output = self.app.post( '/api/0/test/issue/1/custom', headers=headers, data=payload) self.assertEqual(output.status_code, 400) @@ -71,8 +69,8 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): self.assertDictEqual( data, { - "error": "This request format is invalid", - "error_code": "EINVALIDCUSTOMFIELDS", + "error": "Invalid or incomplete input submited", + "error_code": "EINVALIDREQ", } ) @@ -83,8 +81,7 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): headers = {'Authorization': 'token aaabbbcccddd'} # Project does not have this custom field - payload = json.dumps({'fields': - {'foo': 'bar'}}) + payload = {'foo': 'bar'} output = self.app.post( '/api/0/test/issue/1/custom', headers=headers, data=payload) self.assertEqual(output.status_code, 400) @@ -104,13 +101,6 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): headers = {'Authorization': 'token aaabbbcccddd'} - repo = pagure.get_authorized_project(self.session, 'test') - settings = repo.settings - settings['issue_tracker'] = True - repo.settings = settings - self.session.add(repo) - self.session.commit() - # Set some custom fields repo = pagure.get_authorized_project(self.session, 'test') msg = pagure.lib.set_custom_key_fields( @@ -122,8 +112,7 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): self.session.commit() self.assertEqual(msg, 'List of custom fields updated') - payload = json.dumps({'fields': - {'bugzilla': '', 'upstream': True}}) + payload = {'bugzilla': '', 'upstream': True} output = self.app.post( '/api/0/test/issue/1/custom', headers=headers, data=payload) self.assertEqual(output.status_code, 200) @@ -131,7 +120,7 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): self.assertDictEqual( data, { - "fields": [ + "messages": [ {"bugzilla": "No changes"}, {"upstream": "Custom field upstream adjusted to True"}, ] @@ -142,10 +131,9 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): issue = pagure.lib.search_issues(self.session, repo, issueid=1) self.assertEqual(len(issue.other_fields), 1) - payload = json.dumps({'fields': - {'bugzilla': 'https://bugzilla.redhat.com/1234', - 'upstream': False, - 'reviewstatus': 'ack'}}) + payload = {'bugzilla': 'https://bugzilla.redhat.com/1234', + 'upstream': False, + 'reviewstatus': 'ack'} output = self.app.post( '/api/0/test/issue/1/custom', headers=headers, data=payload) @@ -154,11 +142,11 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): self.assertDictEqual( data, { - "fields": [ + "messages": [ {"bugzilla": "Custom field bugzilla adjusted to " "https://bugzilla.redhat.com/1234"}, {"reviewstatus": "Custom field reviewstatus adjusted to ack"}, - {"upstream": "Custom field upstream reset (from 1)"}, + {"upstream": "Custom field upstream adjusted to False (was: True)"}, ] } @@ -169,10 +157,7 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): self.assertEqual(len(issue.other_fields), 3) # Reset the value - payload = json.dumps({'fields': - {'bugzilla': '', - 'upstream': '', - 'reviewstatus': ''}}) + payload = {'bugzilla': '', 'upstream': '', 'reviewstatus': ''} output = self.app.post( '/api/0/test/issue/1/custom', headers=headers, data=payload) @@ -181,11 +166,11 @@ class PagureFlaskApiCustomFieldIssuetests(tests.Modeltests): self.assertDictEqual( data, { - "fields": [ + "messages": [ {"bugzilla": "Custom field bugzilla reset " "(from https://bugzilla.redhat.com/1234)"}, {"reviewstatus": "Custom field reviewstatus reset (from ack)"}, - {"upstream": "Custom field upstream reset (from 0)"}, + {"upstream": "Custom field upstream reset (from False)"}, ] } )