From 28e51040651b7ec23210b7aa1ffa54e0fd1be580 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 08 2015 11:23:06 +0000 Subject: [PATCH 1/10] Change the PullRequest's status from a Boolean to a Text restricted at the DB level --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 77cb7e6..ee0203d 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -89,6 +89,15 @@ def create_default_status(session, acls=None): session.rollback() ERROR_LOG.debug('Status %s could not be added', ticket_stat) + for status in ['Open', 'Closed', 'Merged']: + pr_stat = StatusPullRequest(status=status) + session.add(pr_stat) + try: + session.commit() + except SQLAlchemyError: # pragma: no cover + session.rollback() + ERROR_LOG.debug('Status %s could not be added', pr_stat) + for grptype in ['user', 'admin']: grp_type = PagureGroupType(group_type=grptype) session.add(grp_type) @@ -122,6 +131,17 @@ class StatusIssue(BASE): status = sa.Column(sa.Text, nullable=False, unique=True) +class StatusPullRequest(BASE): + """ Stores the status a pull-request can have. + + Table -- status_issue + """ + __tablename__ = 'status_pull_requests' + + id = sa.Column(sa.Integer, primary_key=True) + status = sa.Column(sa.Text, nullable=False, unique=True) + + class User(BASE): """ Stores information about users. @@ -672,7 +692,12 @@ class PullRequest(BASE): name='merge_status_enum'), nullable=True) - status = sa.Column(sa.Boolean, nullable=False, default=True) + status = sa.Column( + sa.Text, + sa.ForeignKey( + 'status_pull_requests.status', onupdate='CASCADE'), + default='Open', + nullable=False) date_created = sa.Column(sa.DateTime, nullable=False, default=datetime.datetime.utcnow) From 5e7392b48d5ee49fe5374505362d70db3c8f7f9c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 08 2015 11:23:33 +0000 Subject: [PATCH 2/10] Add an alembic migration script to update the pull-request's status --- diff --git a/alembic/versions/298891e63039_change_the_status_of_pull_requests.py b/alembic/versions/298891e63039_change_the_status_of_pull_requests.py new file mode 100644 index 0000000..beb05b9 --- /dev/null +++ b/alembic/versions/298891e63039_change_the_status_of_pull_requests.py @@ -0,0 +1,60 @@ +"""Change the status of pull_requests + + +Revision ID: 298891e63039 +Revises: 3c25e14b855b +Create Date: 2015-06-08 13:06:11.938966 + +""" + +# revision identifiers, used by Alembic. +revision = '298891e63039' +down_revision = '3c25e14b855b' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ''' Adjust the status column of the pull_requests table. + ''' + op.add_column( + 'pull_requests', + sa.Column( + '_status', sa.Text, + sa.ForeignKey( + 'status_pull_requests.status', onupdate='CASCADE'), + default='Open', + nullable=True) + ) + + op.execute('''UPDATE "pull_requests" ''' + '''SET _status='Open' WHERE status=TRUE;''') + op.execute('''UPDATE "pull_requests" ''' + '''SET _status='Merged' WHERE status=FALSE;''') + + op.drop_column('pull_requests', 'status') + op.alter_column( + 'pull_requests', + column_name='_status', new_column_name='status', + nullable=False, existing_nullable=True) + + +def downgrade(): + ''' Revert the status column of the pull_requests table. + ''' + op.add_column( + 'pull_requests', + sa.Column( + '_status', sa.Boolean, default=True, nullable=True) + ) + op.execute('''UPDATE "pull_requests" ''' + '''SET _status=TRUE WHERE status='Open';''') + op.execute('''UPDATE "pull_requests" ''' + '''SET _status=FALSE WHERE status!='Open';''') + + op.drop_column('pull_requests', 'status') + op.alter_column( + 'pull_requests', + column_name='_status', new_column_name='status', + nullable=False, existing_nullable=True) From 5991ef753d03fa0fc2208438c9445204e65c5d8e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 08 2015 11:40:55 +0000 Subject: [PATCH 3/10] Replace boolean checks by checking for the string allowed --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 34d0e41..d4e352d 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -43,9 +43,9 @@ - {% elif pull_request and pull_request.status == False %} + {% elif pull_request and pull_request.status != 'Open' %}
  • - Merged + {{ pull_request.status }}
    • {% endif %}
  • diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 5872f16..a3ce3d6 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -45,7 +45,7 @@ def _get_parent_repo_path(repo): def request_pulls(repo, username=None): """ Request pulling the changes from the fork into the project. """ - status = flask.request.args.get('status', True) + status = flask.request.args.get('status', 'Open') assignee = flask.request.args.get('assignee', None) author = flask.request.args.get('author', None) @@ -131,7 +131,7 @@ def request_pull(repo, requestid, username=None): diff_commits = [] diff = None # Closed pull-request - if request.status is False: + if request.status != 'Open': commitid = request.commit_stop for commit in repo_obj.walk(commitid, pygit2.GIT_SORT_TIME): diff_commits.append(commit) @@ -208,7 +208,7 @@ def request_pull_patch(repo, requestid, username=None): commitid = branch.get_object().hex diff_commits = [] - if request.status is False: + if request.status != 'Open': commitid = request.commit_stop for commit in repo_obj.walk(commitid, pygit2.GIT_SORT_TIME): diff_commits.append(commit) @@ -514,7 +514,7 @@ def set_assignee_requests(repo, requestid, username=None): if not request: flask.abort(404, 'Pull-request not found') - if not request.status: + if request.status != 'Open': flask.abort(403, 'Pull-request closed') form = pagure.forms.AddUserForm() From f86685e6991a0d69758b22438a642609feeb28c2 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 08 2015 11:41:27 +0000 Subject: [PATCH 4/10] Adjust close_pull_request to the new PR's status allowed --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 5985627..e29ab52 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -1470,7 +1470,10 @@ def close_pull_request(session, request, user, requestfolder, merged=True): ''' user_obj = __get_user(session, user) - request.status = False + if merged is True: + request.status = 'Merged' + else: + request.status = 'Closed' session.add(request) session.flush() From 127606ab4aed2ee7d545d550bf0948c529602ad6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 08 2015 11:41:49 +0000 Subject: [PATCH 5/10] Adjust new_pull_request to default to the correct status allowed --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index e29ab52..1272235 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -866,7 +866,7 @@ def drop_issue(session, issue, user, ticketfolder): def new_pull_request(session, repo_from, branch_from, repo_to, branch_to, title, user, requestfolder, requestuid=None, requestid=None, - status=True, notify=True): + status='Open', notify=True): ''' Create a new pull request on the specified repo. ''' user_obj = __get_user(session, user) From c0f8df3d3f033a2ee7e0e3ad1c2ec6972576f023 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 08 2015 11:42:01 +0000 Subject: [PATCH 6/10] Adjust search_pull_requests to support searching for new and old status type The old status type was a boolean: True = Open, False = Closed/Merged. So we now support searching for the provided status, or with a boolean --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 1272235..72206f8 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -1414,9 +1414,19 @@ def search_pull_requests( ) if status is not None: - query = query.filter( - model.PullRequest.status == status - ) + if isinstance(status, bool): + if status: + query = query.filter( + model.PullRequest.status == 'Open' + ) + else: + query = query.filter( + model.PullRequest.status != 'Open' + ) + else: + query = query.filter( + model.PullRequest.status == status + ) if assignee is not None: if str(assignee).lower() not in ['false', '0', 'true', '1']: From 01a29aeefd0ded672c561add0d1b8d87e404e4c5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 08 2015 11:43:14 +0000 Subject: [PATCH 7/10] Adjust unit-tests for the change in status representation of the PR --- diff --git a/tests/test_progit_flask_api_fork.py b/tests/test_progit_flask_api_fork.py index 4f1423d..8bbe951 100644 --- a/tests/test_progit_flask_api_fork.py +++ b/tests/test_progit_flask_api_fork.py @@ -131,7 +131,7 @@ class PagureFlaskApiForktests(tests.Modeltests): "name": "pingou" } }, - "status": True, + "status": 'Open', "title": "test pull-request", "uid": "1431414800", "user": { @@ -247,7 +247,7 @@ class PagureFlaskApiForktests(tests.Modeltests): "name": "pingou" } }, - "status": True, + "status": 'Open', "title": "test pull-request", "uid": "1431414800", "user": { From 6afa3130c7dda02637acb789e8e4e4152773bd88 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jun 08 2015 11:51:33 +0000 Subject: [PATCH 8/10] Adjust the list of pull-requests returned based on the status asked Specifying ?status=0 or ?status=False will return all closed or merged pull-request. Specifying either ?status=Closed or ?status=Merged will returned what is asked Specifying ?status=open or nothing, will returned the opened pull-requests --- diff --git a/pagure/templates/requests.html b/pagure/templates/requests.html index 96e7b5c..a814cf5 100644 --- a/pagure/templates/requests.html +++ b/pagure/templates/requests.html @@ -8,13 +8,14 @@

    - {% if status and (status == False or status|lower == 'closed') %} - Closed {% endif -%} + {% if status|lower != 'open' and status|lower != 'false' %} + {{ status }} {% elif status|lower != 'open' -%} + Closed/Merged {% endif -%} Pull-requests ({{ requests|count }})

    - {% if status and not (status == False or status|lower == 'closed') %} + {% if status|lower == 'open' %} + repo=repo.name) }}?status=0"> ({{ oth_requests }} Closed) {% else %} Fork is empty, there are no commits to ' 'request pulling
    • ', output.data) - output = self.app.get('/test/new_issue') + output = self.app.get('/fork/foo/test/new_issue') csrf_token = output.data.split( 'name="csrf_token" type="hidden" value="')[1].split('">')[0]