From abcbf77ee5fc52b19691733804999732392855cc Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 22 2017 16:45:23 +0000 Subject: Add a status field to flags This status is used in addition to the percentage making it optional. For flags on commit the status is mandatory, on pull-requests it is optional and will be inferred from the percentage specified (or not). This keeps the backward compatibility on the API endpoint to flag PR while enforcing the new behavior for the new API endpoint for commits. Signed-off-by: Pierre-Yves Chibon --- diff --git a/alembic/versions/2b626a16542e_commit_flag.py b/alembic/versions/2b626a16542e_commit_flag.py index 503cff0..20ab714 100644 --- a/alembic/versions/2b626a16542e_commit_flag.py +++ b/alembic/versions/2b626a16542e_commit_flag.py @@ -38,8 +38,9 @@ def upgrade(): 'user_id', sa.Integer, sa.ForeignKey('users.id', onupdate='CASCADE'), nullable=False, index=True), + sa.Column('status', sa.String(32), nullable=False), sa.Column('username', sa.Text(), nullable=False), - sa.Column('percent', sa.Integer(), nullable=False), + sa.Column('percent', sa.Integer(), nullable=True), sa.Column('comment', sa.Text(), nullable=False), sa.Column('url', sa.Text(), nullable=False), sa.Column( diff --git a/alembic/versions/6119fbbcc8e9_migrate_current_flag.py b/alembic/versions/6119fbbcc8e9_migrate_current_flag.py new file mode 100644 index 0000000..8274738 --- /dev/null +++ b/alembic/versions/6119fbbcc8e9_migrate_current_flag.py @@ -0,0 +1,42 @@ +"""Migrate current flag + +Revision ID: 6119fbbcc8e9 +Revises: 2b626a16542e +Create Date: 2017-11-16 15:11:28.199971 + +""" + +# revision identifiers, used by Alembic. +revision = '6119fbbcc8e9' +down_revision = '2b626a16542e' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + """ Add the status column to pull_request_flags and migrate the data. + """ + op.add_column( + 'pull_request_flags', + sa.Column('status', sa.String(32), nullable=True) + ) + op.execute( + 'UPDATE pull_request_flags SET status=\'success\' ' + 'WHERE percent in (100, \'100\')') + op.execute( + 'UPDATE pull_request_flags SET status=\'failure\' ' + 'WHERE percent not in (100, \'100\')') + op.alter_column( + 'pull_request_flags', 'status', + nullable=False, existing_nullable=True) + + +def downgrade(): + """ Drop the status column in pull_request_flags. + + We can't undo the change to the status column since it may now + contain empty rows. + + """ + op.drop_column('pull_request_flags', 'status') diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 89d495f..332f1e0 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -611,14 +611,6 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): | | | | presented to users | | | | | on the pull request page | +---------------+---------+--------------+-----------------------------+ - | ``percent`` | int | Mandatory | | A percentage of | - | | | | completion compared to | - | | | | the goal. The percentage | - | | | | also determine the | - | | | | background color of the | - | | | | flag on the pull-request | - | | | | page | - +---------------+---------+--------------+-----------------------------+ | ``comment`` | string | Mandatory | | A short message | | | | | summarizing the | | | | | presented results | @@ -626,6 +618,25 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): | ``url`` | string | Mandatory | | A URL to the result | | | | | of this flag | +---------------+---------+--------------+-----------------------------+ + | ``status`` | string | Optional | | The status of the task, | + | | | | can be any of: success, | + | | | | failure, error, pending, | + | | | | canceled | + | | | | If not provided it will be| + | | | | set to ``success`` if | + | | | | percent is higher than 0, | + | | | | ``failure`` if it is 0 and| + | | | | ``pending`` if percent is | + | | | | not specified | + +---------------+---------+--------------+-----------------------------+ + | ``percent`` | int | Optional | | A percentage of | + | | | | completion compared to | + | | | | the goal. The percentage | + | | | | also determine the | + | | | | background color of the | + | | | | flag on the pull-request | + | | | | page | + +---------------+---------+--------------+-----------------------------+ | ``uid`` | string | Optional | | A unique identifier used | | | | | to identify a flag on a | | | | | pull-request. If the | @@ -638,9 +649,6 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): | | | | characters. Default: an | | | | | auto generated UID | +---------------+---------+--------------+-----------------------------+ - | ``commit`` | string | Optional | | The hash of the commit | - | | | | you use | - +---------------+---------+--------------+-----------------------------+ Sample response ^^^^^^^^^^^^^^^ @@ -653,6 +661,7 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): "date_created": "1510742565", "percent": 0, "pull_request_uid": "62b49f00d489452994de5010565fab81", + "status": "error", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { "default_email": "bar@pingou.com", @@ -672,6 +681,7 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): "date_created": "1510742565", "percent": 0, "pull_request_uid": "62b49f00d489452994de5010565fab81", + "status": "error", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { "default_email": "bar@pingou.com", @@ -707,19 +717,30 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): if not request: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) - form = pagure.forms.AddPullRequestFlagForm(csrf_enabled=False) + if 'status' in flask.request.form: + form = pagure.forms.AddPullRequestFlagForm(csrf_enabled=False) + else: + form = pagure.forms.AddPullRequestFlagFormV1(csrf_enabled=False) if form.validate_on_submit(): username = form.username.data - percent = form.percent.data + percent = form.percent.data.strip() or None comment = form.comment.data.strip() url = form.url.data.strip() uid = form.uid.data.strip() if form.uid.data else None + if 'status' in flask.request.form: + status = form.status.data.strip() + else: + if percent is None: + status = 'pending' + else: + status = 'success' if percent != '0' else 'failure' try: # New Flag message, uid = pagure.lib.add_pull_request_flag( SESSION, request=request, username=username, + status=status, percent=percent, comment=comment, url=url, diff --git a/pagure/api/project.py b/pagure/api/project.py index 0346c69..f527a75 100644 --- a/pagure/api/project.py +++ b/pagure/api/project.py @@ -1303,15 +1303,7 @@ def api_commit_add_flag(repo, commit_hash, username=None, namespace=None): | ``username`` | string | Mandatory | | The name of the | | | | | application to be | | | | | presented to users | - | | | | on the pull request page | - +---------------+---------+--------------+-----------------------------+ - | ``percent`` | int | Mandatory | | A percentage of | - | | | | completion compared to | - | | | | the goal. The percentage | - | | | | also determine the | - | | | | background color of the | - | | | | flag on the pull-request | - | | | | page | + | | | | on the commit pages | +---------------+---------+--------------+-----------------------------+ | ``comment`` | string | Mandatory | | A short message | | | | | summarizing the | @@ -1320,9 +1312,21 @@ def api_commit_add_flag(repo, commit_hash, username=None, namespace=None): | ``url`` | string | Mandatory | | A URL to the result | | | | | of this flag | +---------------+---------+--------------+-----------------------------+ + | ``status`` | string | Mandatory | | The status of the task, | + | | | | can be any of: success, | + | | | | failure, error, pending, | + | | | | canceled | + +---------------+---------+--------------+-----------------------------+ + | ``percent`` | int | Optional | | A percentage of | + | | | | completion compared to | + | | | | the goal. The percentage | + | | | | also determine the | + | | | | background color of the | + | | | | flag on the pages | + +---------------+---------+--------------+-----------------------------+ | ``uid`` | string | Optional | | A unique identifier used | - | | | | to identify a flag on a | - | | | | pull-request. If the | + | | | | to identify a flag across | + | | | | all projects. If the | | | | | provided UID matches an | | | | | existing one, then the | | | | | API call will update the | @@ -1345,6 +1349,7 @@ def api_commit_add_flag(repo, commit_hash, username=None, namespace=None): "commit_hash": "62b49f00d489452994de5010565fab81", "date_created": "1510742565", "percent": 100, + "status": "success", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { "default_email": "bar@pingou.com", @@ -1365,6 +1370,7 @@ def api_commit_add_flag(repo, commit_hash, username=None, namespace=None): "commit_hash": "62b49f00d489452994de5010565fab81", "date_created": "1510742565", "percent": 100, + "status": "success", "url": "http://jenkins.cloud.fedoraproject.org/", "user": { "default_email": "bar@pingou.com", @@ -1403,10 +1409,11 @@ def api_commit_add_flag(repo, commit_hash, username=None, namespace=None): form = pagure.forms.AddPullRequestFlagForm(csrf_enabled=False) if form.validate_on_submit(): username = form.username.data - percent = form.percent.data + percent = form.percent.data.strip() or None comment = form.comment.data.strip() url = form.url.data.strip() uid = form.uid.data.strip() if form.uid.data else None + status = form.status.data.strip() try: # New Flag message, uid = pagure.lib.add_commit_flag( @@ -1416,6 +1423,7 @@ def api_commit_add_flag(repo, commit_hash, username=None, namespace=None): username=username, percent=percent, comment=comment, + status=status, url=url, uid=uid, user=flask.g.fas_user.username, diff --git a/pagure/forms.py b/pagure/forms.py index c19e735..3f7fd3e 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -471,12 +471,13 @@ class AddPullRequestCommentForm(PagureForm): ) -class AddPullRequestFlagForm(PagureForm): - ''' Form to add a flag to a pull-request. ''' +class AddPullRequestFlagFormV1(PagureForm): + ''' Form to add a flag to a pull-request or commit. ''' username = wtforms.TextField( 'Username', [wtforms.validators.Required()]) percent = wtforms.TextField( - 'Percentage of completion', [wtforms.validators.Required()]) + 'Percentage of completion', + [wtforms.validators.optional()]) comment = wtforms.TextAreaField( 'Comment', [wtforms.validators.Required()]) url = wtforms.TextField( @@ -485,6 +486,21 @@ class AddPullRequestFlagForm(PagureForm): 'UID', [wtforms.validators.optional()]) +class AddPullRequestFlagForm(AddPullRequestFlagFormV1): + ''' Form to add a flag to a pull-request or commit. ''' + status = wtforms.SelectField( + 'status', + [wtforms.validators.Required()], + choices=[ + ('success', 'success'), + ('failure', 'failure'), + ('error', 'error'), + ('canceled', 'canceled'), + ('pending', 'pending'), + ], + ) + + class UserSettingsForm(PagureForm): ''' Form to create or edit project. ''' ssh_key = wtforms.TextAreaField( diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 54ef5f8..703d313 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -1276,7 +1276,7 @@ def edit_comment(session, parent, comment, user, def add_pull_request_flag(session, request, username, percent, comment, url, - uid, user, token, requestfolder): + status, uid, user, token, requestfolder): ''' Add a flag to a pull-request. ''' user_obj = get_user(session, user) @@ -1285,6 +1285,7 @@ def add_pull_request_flag(session, request, username, percent, comment, url, if pr_flag: action = 'updated' pr_flag.comment = comment + pr_flag.status = status pr_flag.percent = percent pr_flag.url = url else: @@ -1294,6 +1295,7 @@ def add_pull_request_flag(session, request, username, percent, comment, url, username=username, percent=percent, comment=comment, + status=status, url=url, user_id=user_obj.id, token_id=token, @@ -1331,6 +1333,7 @@ def add_commit_flag( action = 'updated' c_flag.comment = comment c_flag.percent = percent + c_flag.status = status c_flag.url = url else: c_flag = model.CommitFlag( @@ -1338,6 +1341,7 @@ def add_commit_flag( project_id=repo.id, commit_hash=commit_hash, username=username, + status=status, percent=percent, comment=comment, url=url, diff --git a/pagure/lib/model.py b/pagure/lib/model.py index e137112..59aafc5 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1959,6 +1959,9 @@ class PullRequestFlag(BASE): 'tokens.id', ), nullable=True) + status = sa.Column( + sa.String(32), + nullable=False) user_id = sa.Column( sa.Integer, sa.ForeignKey( @@ -1971,7 +1974,7 @@ class PullRequestFlag(BASE): nullable=False) percent = sa.Column( sa.Integer(), - nullable=False) + nullable=True) comment = sa.Column( sa.Text(), nullable=False) @@ -2005,6 +2008,7 @@ class PullRequestFlag(BASE): 'username': self.username, 'percent': self.percent, 'comment': self.comment, + 'status': self.status, 'url': self.url, 'date_created': self.date_created.strftime('%s'), 'user': self.user.to_json(public=public), @@ -2042,12 +2046,15 @@ class CommitFlag(BASE): nullable=False, index=True) uid = sa.Column(sa.String(32), unique=True, nullable=False) + status = sa.Column( + sa.String(32), + nullable=False) username = sa.Column( sa.Text(), nullable=False) percent = sa.Column( sa.Integer(), - nullable=False) + nullable=True) comment = sa.Column( sa.Text(), nullable=False) @@ -2073,6 +2080,7 @@ class CommitFlag(BASE): 'username': self.username, 'percent': self.percent, 'comment': self.comment, + 'status': self.status, 'url': self.url, 'date_created': self.date_created.strftime('%s'), 'user': self.user.to_json(public=public), diff --git a/tests/test_pagure_flask_api_fork.py b/tests/test_pagure_flask_api_fork.py index 8e52ff9..5bdda48 100644 --- a/tests/test_pagure_flask_api_fork.py +++ b/tests/test_pagure_flask_api_fork.py @@ -1055,8 +1055,7 @@ class PagureFlaskApiForktests(tests.Modeltests): data = { 'username': 'Jenkins', - 'percent': 0, - 'comment': 'Tests failed', + 'comment': 'Tests running', 'url': 'http://jenkins.cloud.fedoraproject.org/', 'uid': 'jenkins_build_pagure_100+seed', } @@ -1072,10 +1071,11 @@ class PagureFlaskApiForktests(tests.Modeltests): data, { u'flag': { - u'comment': u'Tests failed', + u'comment': u'Tests running', u'date_created': u'1510742565', - u'percent': 0, + u'percent': None, u'pull_request_uid': u'62b49f00d489452994de5010565fab81', + u'status': u'pending', u'url': u'http://jenkins.cloud.fedoraproject.org/', u'user': { u'default_email': u'bar@pingou.com', @@ -1092,10 +1092,10 @@ class PagureFlaskApiForktests(tests.Modeltests): request = pagure.lib.search_pull_requests( self.session, project_id=1, requestid=1) self.assertEqual(len(request.flags), 1) - self.assertEqual(request.flags[0].comment, 'Tests failed') - self.assertEqual(request.flags[0].percent, 0) + self.assertEqual(request.flags[0].comment, 'Tests running') + self.assertEqual(request.flags[0].percent, None) - # Update flag + # Update flag - w/o providing the status data = { 'username': 'Jenkins', 'percent': 100, @@ -1118,6 +1118,7 @@ class PagureFlaskApiForktests(tests.Modeltests): u'date_created': u'1510742565', u'percent': 100, u'pull_request_uid': u'62b49f00d489452994de5010565fab81', + u'status': u'success', u'url': u'http://jenkins.cloud.fedoraproject.org/', u'user': { u'default_email': u'bar@pingou.com', @@ -1243,7 +1244,7 @@ class PagureFlaskApiForktests(tests.Modeltests): 'uid': 'jenkins_build_pagure_100+seed', } - # Valid request + # Valid request - w/o providing the status output = self.app.post( '/api/0/test/pull-request/1/flag', data=data, headers=headers) self.assertEqual(output.status_code, 200) @@ -1258,6 +1259,7 @@ class PagureFlaskApiForktests(tests.Modeltests): u'date_created': u'1510742565', u'percent': 0, u'pull_request_uid': u'62b49f00d489452994de5010565fab81', + u'status': u'failure', u'url': u'http://jenkins.cloud.fedoraproject.org/', u'user': { u'default_email': u'bar@pingou.com', @@ -1284,6 +1286,7 @@ class PagureFlaskApiForktests(tests.Modeltests): 'comment': 'Tests passed', 'url': 'http://jenkins.cloud.fedoraproject.org/', 'uid': 'jenkins_build_pagure_100+seed', + 'status': 'success', } output = self.app.post( @@ -1300,6 +1303,7 @@ class PagureFlaskApiForktests(tests.Modeltests): u'date_created': u'1510742565', u'percent': 100, u'pull_request_uid': u'62b49f00d489452994de5010565fab81', + u'status': u'success', u'url': u'http://jenkins.cloud.fedoraproject.org/', u'user': { u'default_email': u'bar@pingou.com', diff --git a/tests/test_pagure_flask_api_project.py b/tests/test_pagure_flask_api_project.py index 9643dcd..ab7fe80 100644 --- a/tests/test_pagure_flask_api_project.py +++ b/tests/test_pagure_flask_api_project.py @@ -2737,7 +2737,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): tests.create_tokens_acl( self.session, 'aaabbbcccddd', 'commit_flag') - def test_flag_commit_missing_percent(self): + def test_flag_commit_missing_status(self): """ Test flagging a commit with missing precentage. """ repo_obj = pygit2.Repository(self.git_path) commit = repo_obj.revparse_single('HEAD') @@ -2758,8 +2758,8 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): "error": "Invalid or incomplete input submited", "error_code": "EINVALIDREQ", "errors": { - "percent": [ - "This field is required." + "status": [ + "Not a valid choice" ] } } @@ -2776,6 +2776,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): 'comment': 'Tests passed', 'url': 'http://jenkins.cloud.fedoraproject.org/', 'uid': 'jenkins_build_pagure_100+seed', + 'status': 'success', } output = self.app.post( '/api/0/test/c/%s/flag' % commit.oid.hex, @@ -2804,6 +2805,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): 'percent': 100, 'url': 'http://jenkins.cloud.fedoraproject.org/', 'uid': 'jenkins_build_pagure_100+seed', + 'status': 'success', } output = self.app.post( '/api/0/test/c/%s/flag' % commit.oid.hex, @@ -2832,6 +2834,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): 'percent': 100, 'comment': 'Tests passed', 'uid': 'jenkins_build_pagure_100+seed', + 'status': 'success', } output = self.app.post( '/api/0/test/c/%s/flag' % commit.oid.hex, @@ -2874,6 +2877,33 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): } self.assertEqual(data, expected_output) + def test_flag_commit_invalid_status(self): + """ Test flagging a commit with an invalid status. """ + repo_obj = pygit2.Repository(self.git_path) + commit = repo_obj.revparse_single('HEAD') + + headers = {'Authorization': 'token aaabbbcccddd'} + data = { + 'username': 'Jenkins', + 'percent': 100, + 'comment': 'Tests passed', + 'url': 'http://jenkins.cloud.fedoraproject.org/', + 'status': 'foobar', + } + output = self.app.post( + '/api/0/test/c/%s/flag' % commit.oid.hex, + headers=headers, data=data) + self.assertEqual(output.status_code, 400) + data = json.loads(output.data) + self.assertEqual( + data, + { + u'errors': {u'status': [u'Not a valid choice']}, + u'error_code': u'EINVALIDREQ', + u'error': u'Invalid or incomplete input submited' + } + ) + def test_flag_commit_with_uid(self): """ Test flagging a commit with provided uid. """ repo_obj = pygit2.Repository(self.git_path) @@ -2886,6 +2916,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): 'comment': 'Tests passed', 'url': 'http://jenkins.cloud.fedoraproject.org/', 'uid': 'jenkins_build_pagure_100+seed', + 'status': 'success', } output = self.app.post( '/api/0/test/c/%s/flag' % commit.oid.hex, @@ -2900,6 +2931,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): u'commit_hash': u'62b49f00d489452994de5010565fab81', u'date_created': u'1510742565', u'percent': 100, + u'status': 'success', u'url': u'http://jenkins.cloud.fedoraproject.org/', u'user': { u'default_email': u'bar@pingou.com', @@ -2925,6 +2957,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): 'percent': 100, 'comment': 'Tests passed', 'url': 'http://jenkins.cloud.fedoraproject.org/', + 'status': 'success', } output = self.app.post( '/api/0/test/c/%s/flag' % commit.oid.hex, @@ -2944,6 +2977,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): u'commit_hash': u'62b49f00d489452994de5010565fab81', u'date_created': u'1510742565', u'percent': 100, + u'status': 'success', u'url': u'http://jenkins.cloud.fedoraproject.org/', u'user': { u'default_email': u'bar@pingou.com',