PR#2484: New API endpoint to modify multiple custom fields. - pagure

pagure

#2484 New API endpoint to modify multiple custom fields.

Merged 6 years ago by pingou. Opened 6 years ago by cverna.

cverna/pagure multi_custom_key_api into master

Fixing comment from the review

Clement Verna • 6 years ago

ee0ae35

Remove duplicated call to pagure.lib.add_metadata_update_notif

Clement Verna • 6 years ago

5ce0bda

Replace common logic with helper function to check link url

Clement Verna • 6 years ago

8413286

New API endpoint to modify multiple custom fields.

Clement Verna • 6 years ago

72ac1c9

pagure/api/issue.py

file modified

+140 -15

		`@@ -122,6 +122,22 @@`
		`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 : 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)`
		`+`
		`+`
		`@API.route('/<repo>/new_issue', methods=['POST'])`
		`@API.route('/<namespace>/<repo>/new_issue', methods=['POST'])`
		`@API.route('/fork/<username>/<repo>/new_issue', methods=['POST'])`
		`@@ -1169,13 +1185,7 @@`
		`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)`
		`@@ -1200,14 +1210,129 @@`
		`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`
		`+`
		`+`
		`+ @API.route('/<repo>/issue/<int:issueid>/custom', methods=['POST'])`
		`+ @API.route(`
		`+ '/<namespace>/<repo>/issue/<int:issueid>/custom',`
		`+ methods=['POST'])`
		`+ @API.route(`
		`+ '/fork/<username>/<repo>/issue/<int:issueid>/custom',`
		`+ methods=['POST'])`
		`+ @API.route(`
		`+ '/fork/<username>/<namespace>/<repo>/issue/<int:issueid>/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/<repo>/issue/<issue id>/custom`
		`+ POST /api/0/<namespace>/<repo>/issue/<issue id>/custom`
		`+`
		`+ ::`
		`+`
		`+ POST /api/0/fork/<username>/<repo>/issue/<issue id>/custom`
		`+ POST /api/0/fork/<username>/<namespace>/<repo>/issue/<issue id>/custom`
		`+`
		`+ Input`
		`+ ^^^^^`
		`+`
		`+ +------------------+---------+--------------+-----------------------------+`
		`+ \| Key \| Type \| Optionality \| Description \|`
		`+ +==================+=========+==============+=============================+`
		+ \| ``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`
		`+ ^^^^^^^^^^^^^^^`
		`+`
		`+ ::`
		`+`
		`+ {`
		`+ "messages": [`
		`+ {`
		`+ "myField" : "Custom field myField adjusted to to do"`
		`+ },`
		`+ {`
		`+ "myField_1": "Custom field myField_1 adjusted test (was: to do)"`
		`+ },`
		`+ {`
		`+ "myField_2": "Custom field myField_1 adjusted to done (was: test)"`
		`+ }`
		`+ ]`
		`+ }`
		`+`
		`+ """ # noqa`
		`+ output = {'messages': []}`
		`+ 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.form`
		`+`
		`+ if not fields:`
		`+ raise pagure.exceptions.APIError(`
		`+ 400, error_code=APIERROR.EINVALIDREQ)`
		`+`
		`+ 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['messages'].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['messages'].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`

tests/test_pagure_flask_api_issue_custom_fields.py

file added

+180

		`@@ -0,0 +1,180 @@`
		`+ """`
		`+ (c) 2017 - Copyright Red Hat Inc`
		`+`
		`+ Authors:`
		`+ Clement Verna <cverna@tutanota.com>`
		`+`
		`+ """`
		`+`
		`+ 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 (EMPTY)`
		`+ payload = {}`
		`+ 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 or incomplete input submited",`
		`+ "error_code": "EINVALIDREQ",`
		`+ }`
		`+ )`
		`+`
		`+ 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 = {'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'}`
		`+`
		`+ # 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 = {'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,`
		`+ {`
		`+ "messages": [`
		`+ {"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 = {'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,`
		`+ {`
		`+ "messages": [`
		`+ {"bugzilla": "Custom field bugzilla adjusted to "`
		`+ "https://bugzilla.redhat.com/1234"},`
		`+ {"reviewstatus": "Custom field reviewstatus adjusted to ack"},`
		`+ {"upstream": "Custom field upstream adjusted to False (was: 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), 3)`
		`+`
		`+ # Reset the value`
		`+ payload = {'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,`
		`+ {`
		`+ "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 False)"},`
		`+ ]`
		`+ }`
		`+ )`
		`+`
		`+`
		`+ if __name__ == '__main__':`
		`+ unittest.main(verbosity=2)`

cverna commented 6 years ago

This commit adds a new endpoint so that a user can send multiple custom fields in one request.

In order to keep backward compatibility I have added a new route /custom and preserved the /custom/field_name route that can be used to edit a single field.

This PR has 3 commits, first one implement the new API, second refactor the common code between the 2 custom field API and the third commit remove a duplicated call to pagure.lib.add_metadata_update_notif

This partially fixes #2057

1 new commit added

Replace common logic with helper function to check link url

6 years ago

rebased

6 years ago

1 new commit added

Remove duplicated call to pagure.lib.add_metadata_update_notif

6 years ago

pingou commented on line 5 of pagure/api/__init__.py 6 years ago

This sounds very close to EINVALIDREQ https://pagure.io/fork/cverna/pagure/blob/multi_custom_key_api/f/pagure/api/__init__.py#_64 no?

pingou commented on line 13 of pagure/api/issue.py 6 years ago

indentation?

pingou commented on line 11 of pagure/api/issue.py 6 years ago

This docblock isn't quite following the style we're using elsewhere :/

pingou commented on line 94 of pagure/api/issue.py 6 years ago

Since this is a dict, should we provide an example on how the dictionary should/could look like?

pingou commented on line 101 of pagure/api/issue.py 6 years ago

I think we're using messages as keys in the other endpoints

pingou commented on line 90 of tests/test_pagure_flask_api_issue_custom_fields.py 6 years ago

I'm not quire seeing the difference with the test just above :(

pingou commented on line 90 of tests/test_pagure_flask_api_issue_custom_fields.py 6 years ago

Oh, I see fields vs field

pingou commented on line 65 of tests/test_pagure_flask_api_issue_custom_fields.py 6 years ago

Let's maybe specify what is incorrect about this request

pingou commented on line 113 of tests/test_pagure_flask_api_issue_custom_fields.py 6 years ago

Is this needed? issue_tracker should be on by default no?

pingou commented on line 149 of tests/test_pagure_flask_api_issue_custom_fields.py 6 years ago

Should we add a test where the value provided for reviewstatus isn't in the list?

pingou commented 6 years ago

Quite a few comments but this is looking good :)

cverna commented on line 5 of pagure/api/__init__.py 6 years ago

Yeah, I was looking for something similar, I ll use EINVALIDREQ.

cverna commented on line 11 of pagure/api/issue.py 6 years ago

OK I ll change it to match the others

cverna commented on line 94 of pagure/api/issue.py 6 years ago

I was thinking about it. Would you put the example in the description ?

cverna commented on line 101 of pagure/api/issue.py 6 years ago

Ok I will change it

cverna commented on line 113 of tests/test_pagure_flask_api_issue_custom_fields.py 6 years ago

Copy/paste :D

cverna commented on line 149 of tests/test_pagure_flask_api_issue_custom_fields.py 6 years ago

Ok, although I am not sure this is validated, I think you can add any value even if it not in the list, I think it is the same for the boolean ( you could add text). I can investigate and raise a new ticket if this is the case

1 new commit added

Fixing comment from the review

6 years ago

4 new commits added

Fixing comment from the review
Remove duplicated call to pagure.lib.add_metadata_update_notif
Replace common logic with helper function to check link url
New API endpoint to modify multiple custom fields.

6 years ago

rebased

6 years ago

cverna commented 6 years ago

I should have fixed all the comments. It is possible to add any value in the list field, there is no validation. I ll raise a ticket for this

4 new commits added

Fixing comment from the review
Remove duplicated call to pagure.lib.add_metadata_update_notif
Replace common logic with helper function to check link url
New API endpoint to modify multiple custom fields.

6 years ago

rebased

6 years ago

pingou commented on line 9 of pagure/api/issue.py 6 years ago

Not using the same style as the rest of the project here :(

pingou commented 6 years ago

Tests are failing (flake8)

pingou commented 6 years ago

So two comments, otherwise :thumbsup: for me

cverna commented on line 9 of pagure/api/issue.py 6 years ago

I followed the same style as the rest of the file :( https://pagure.io/pagure/blob/master/f/pagure/api/issue.py#_30

I don't mind changing the style but then it will be inconsistent in the file.

pingou commented on line 9 of pagure/api/issue.py 6 years ago

You're using ::param <key> : while the file users :param <key>: :)

cverna commented on line 9 of pagure/api/issue.py 6 years ago

Oops I was sure I fixed that :)

rebased

6 years ago

rebased

6 years ago

cverna commented 6 years ago

Ok I have update the docstring. I run the test on my side and no flake8 failure :(. Could double check and if it still fails tell me where it is failling ?

rebased

6 years ago

cverna commented 6 years ago

@pingou OK, I have rebased, run the tests and it looks good on my side.

rebased

6 years ago