From 1427744328fcec67a7efd342d7bb75d7a022df2e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 1/14] Add the possibility for comments to be just notification Notifications are comments that are show differently and cannot be edited. They are made by the system under the name of the user who opened the PR. --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index ab6a98a..e5d7efe 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -971,6 +971,7 @@ class PullRequestComment(BASE): sa.Integer, sa.ForeignKey('pull_request_comments.id', onupdate='CASCADE'), nullable=True) + notification = sa.Column(sa.Boolean, default=False, nullable=False) edited_on = sa.Column(sa.DateTime, nullable=True) editor_id = sa.Column( sa.Integer, @@ -1021,6 +1022,7 @@ class PullRequestComment(BASE): 'user': self.user.to_json(public=public), 'edited_on': self.edited_on.strftime('%s') if self.edited_on else None, 'editor': self.editor.to_json(public=public) if self.editor_id else None, + 'notification': self.notification, } From 7464136d161837ef04acac25b4c1451bdd5bf594 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 2/14] Fix the alembic migration to work from a checkout --- diff --git a/alembic/versions/1b6d7dc5600a_versioning_passwords.py b/alembic/versions/1b6d7dc5600a_versioning_passwords.py index f1cd105..e1c0a13 100644 --- a/alembic/versions/1b6d7dc5600a_versioning_passwords.py +++ b/alembic/versions/1b6d7dc5600a_versioning_passwords.py @@ -13,7 +13,13 @@ down_revision = '3b441ef4e928' from alembic import op import sqlalchemy as sa import sqlalchemy.orm -from pagure.lib import model + +try: + from pagure.lib import model +except ImportError: + import sys + sys.path.insert(0, '.') + from pagure.lib import model def upgrade(): From c4361a7813cb5920de679807c32fc1d6efafab94 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 3/14] Add an alembic migration script adding the notification field --- diff --git a/alembic/versions/58e60d869326_add_notification_bool_to_pr.py b/alembic/versions/58e60d869326_add_notification_bool_to_pr.py new file mode 100644 index 0000000..164b730 --- /dev/null +++ b/alembic/versions/58e60d869326_add_notification_bool_to_pr.py @@ -0,0 +1,33 @@ +"""add notification bool to PR + +Revision ID: 58e60d869326 +Revises: 1b6d7dc5600a +Create Date: 2016-02-12 12:39:07.839530 + +""" + +# revision identifiers, used by Alembic. +revision = '58e60d869326' +down_revision = '1b6d7dc5600a' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ''' Add the column merge_status to the table projects. + ''' + op.add_column( + 'pull_request_comments', + sa.Column('notification', sa.Boolean, default=False, nullable=True) + ) + op.execute('''UPDATE "pull_request_comments" SET notification=False;''') + op.alter_column( + 'pull_request_comments', 'notification', + nullable=False, existing_nullable=True) + + +def downgrade(): + ''' Remove the column merge_status from the table projects. + ''' + op.drop_column('pull_request_comments', 'notification') From 81cbe0e62d90ffc3368fe1765b9c4ff67a6648a4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 4/14] Expand pagure.lib.add_pull_request_comment to support adding notifications --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index d81aa9f..9c4927c 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -790,7 +790,8 @@ def add_group_to_project(session, project, new_group, user): def add_pull_request_comment(session, request, commit, filename, row, - comment, user, requestfolder, notify=True): + comment, user, requestfolder, notify=True, + notification=False): ''' Add a comment to a pull-request. ''' user_obj = __get_user(session, user) @@ -801,6 +802,7 @@ def add_pull_request_comment(session, request, commit, filename, row, line=row, comment=comment, user_id=user_obj.id, + notification=notification, ) session.add(pr_comment) # Make sure we won't have SQLAlchemy error before we continue From fa1dee59620b22e6477e98aa9073d5e5b7172b3a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 5/14] When a PR gets updated, add a notification saying so --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index a2d22cb..7240c03 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1149,13 +1149,25 @@ def diff_pull_request( if request.status and diff_commits: first_commit = repo_obj[diff_commits[-1].oid.hex] # Check if we can still rely on the merge_status + verb = 'updated' if request.commit_start != first_commit.oid.hex or\ request.commit_stop != diff_commits[0].oid.hex: request.merge_status = None + if request.commit_start != first_commit.oid.hex: + verb = 'rebased' request.commit_start = first_commit.oid.hex request.commit_stop = diff_commits[0].oid.hex session.add(request) session.commit() + if not request.merge_status: + pagure.lib.add_pull_request_comment( + session, request, + commit=None, filename=None, row=None, + comment='Pull-Request has been %s' % verb, + user=request.user.username, + requestfolder=requestfolder, + notify=False, notification=True + ) pagure.lib.git.update_git( request, repo=request.project, repofolder=requestfolder) From c6e91b82a481d90ab3f146aaf2098c12ba2d3778 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 6/14] Show notification in the PR page --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index f12e607..9a1f794 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -306,10 +306,7 @@ onsubmit="return try_async_comment(this, null)"> {% for comment in pull_request.comments %} - {% if not comment.commit_id %} - {{ show_comment(comment, comment.id, repo, username, - requestid, form, repo_admin) }} - {% else %} + {% if comment.commit_id %}
@@ -322,6 +319,20 @@
+ {% elif comment.notification %} +
+
+
+ {{ comment.comment }} +
+ {{ + comment.date_created | humanize}}
+
+
+
+ {% else %} + {{ show_comment(comment, comment.id, repo, username, + requestid, form, repo_admin) }} {% endif %} {% endfor %} {{ mergeform.csrf_token }} From 9af6f144953f90f743f6867f5532fb6bf65a0cae Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 7/14] Add a notification upon closing PR --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 9c4927c..be48a18 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -1816,6 +1816,16 @@ def close_pull_request(session, request, user, requestfolder, merged=True): pagure.lib.git.update_git( request, repo=request.project, repofolder=requestfolder) + pagure.lib.add_pull_request_comment( + session, request, + commit=None, filename=None, row=None, + comment='Pull-Request has been %s by %s' % ( + request.status.lower(), user), + user=user, + requestfolder=requestfolder, + notify=False, notification=True + ) + pagure.lib.notify.log( request.project, topic='pull-request.closed', From df1023a5913675819ed56ce46d37c63a016359f6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 8/14] Change the trash icon to an X for the button to close a PR The trash icon is for delete while closing the PR isn't deleting it it's closing it. --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 9a1f794..7f3b8e9 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -48,7 +48,9 @@ (repo_admin or g.fas_user.username == pull_request.user.username) %} {{ mergeform.csrf_token }} + class="btn btn-danger btn-sm" title="Close PR without merging it"> + + {% endif %} {% if pull_request.status == 'Open' and authenticated and From a310ad58dc4867fdc70a14a2b70dff2047a7e535 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 9/14] Just some small indentation fixes --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 7f3b8e9..2b4f225 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -62,12 +62,17 @@
- Proposed {{ pull_request.date_created |humanize }} - by {{ pull_request.user.default_email | avatar(16) | safe }} {{ pull_request.user.user }} + Proposed {{ pull_request.date_created |humanize }} + by {{ pull_request.user.default_email | avatar(16) | safe + }} {{ pull_request.user.user }}
From - {{ pull_request.project_from.fullname or pull_request.remote_git}} + {{ + pull_request.project_from.fullname or pull_request.remote_git + }} {% if pull_request %} -
  • Comments
    • +
  • + + Comments + +
    • {%endif%}
  • - + Files Changed  {{ diff|length if diff else 0}} From 21dd347f942d33dd668f91c02f8115d1098d3395 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 10/14] Add unit-tests checking if the notification is added when a PR is closed/merged --- diff --git a/tests/test_pagure_flask_ui_fork.py b/tests/test_pagure_flask_ui_fork.py index f36b735..55c88d4 100644 --- a/tests/test_pagure_flask_ui_fork.py +++ b/tests/test_pagure_flask_ui_fork.py @@ -408,6 +408,13 @@ class PagureFlaskForktests(tests.Modeltests): ] ) + # Check if the closing notification was added + output = self.app.get('/test/pull-request/1') + self.assertIn( + '
    \n ' + 'Pull-Request has been merged by pingou\n ' + '
    ', output.data) + @patch('pagure.lib.notify.send_email') def test_merge_request_pull_merge(self, send_email): """ Test the merge_request_pull endpoint with a merge PR. """ @@ -442,6 +449,13 @@ class PagureFlaskForktests(tests.Modeltests): '\n Changes merged!', output.data) + # Check if the closing notification was added + output = self.app.get('/test/pull-request/1') + self.assertIn( + '
    \n ' + 'Pull-Request has been merged by pingou\n ' + '
    ', output.data) + @patch('pagure.lib.notify.send_email') def test_merge_request_pull_conflicts(self, send_email): """ Test the merge_request_pull endpoint with a conflicting PR. """ @@ -514,6 +528,13 @@ class PagureFlaskForktests(tests.Modeltests): '\n Nothing to do, changes ' 'were already merged', output.data) + # Check if the closing notification was added + output = self.app.get('/test/pull-request/1') + self.assertIn( + '
    \n ' + 'Pull-Request has been merged by pingou\n ' + '
    ', output.data) + @patch('pagure.lib.notify.send_email') def test_request_pull_close(self, send_email): """ Test the request_pull endpoint with a closed PR. """ @@ -1792,6 +1813,13 @@ index 0000000..2a552bb 'A commit on branch feature', ]) + # Check if the closing notification was added + output = self.app.get('/test/pull-request/1') + self.assertIn( + '
    \n ' + 'Pull-Request has been merged by pingou\n ' + '
    ', output.data) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureFlaskForktests) From 13df245fe818e70b31210bdd186884eff2590107 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 11/14] Drop trailing spaces --- diff --git a/pagure/templates/commit.html b/pagure/templates/commit.html index 7598439..1a4643b 100644 --- a/pagure/templates/commit.html +++ b/pagure/templates/commit.html @@ -41,9 +41,9 @@ {{commit.commit_time | humanize}} {% else %} {{diff|count}} file{{'s' if diff|count > 1}} Authored by {{ commit.author | author2user |safe }} - {{commit.commit_time | humanize}}, + {{commit.commit_time | humanize}}, Committed by {{ commit.committer | author2user |safe }} - {{commit.commit_time | humanize}}, + {{commit.commit_time | humanize}}, {% endif%}
    From 2767e0d47b83234508108f34b5e11d804abf4d58 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 12/14] Let's remove the ending < or > when viewing a commit --- diff --git a/pagure/ui/filters.py b/pagure/ui/filters.py index bdd8966..d9201fe 100644 --- a/pagure/ui/filters.py +++ b/pagure/ui/filters.py @@ -294,13 +294,16 @@ def patch_to_diff(patch): hunk.old_start, hunk.old_lines, hunk.new_start, hunk.new_lines) for line in hunk.lines: if hasattr(line, 'content'): - content = content + line.origin + ' '+ line.content + origin = line.origin + if line.origin in ['<', '>']: + origin = '' + content = content + origin + ' '+ line.content else: # Avoid situation where at the end of a file we get: # + foo< # \ No newline at end of file if line[0] == '<': - line = ('',line[1]) + line = ('', line[1]) content = content + ' '.join(line) return content From 15f4dd90d2f6c2463ccbc37c3069b98049a57786 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 13/14] Let's ignore the '=' signs as well when viewing a commit --- diff --git a/pagure/ui/filters.py b/pagure/ui/filters.py index d9201fe..f64cfb6 100644 --- a/pagure/ui/filters.py +++ b/pagure/ui/filters.py @@ -295,14 +295,14 @@ def patch_to_diff(patch): for line in hunk.lines: if hasattr(line, 'content'): origin = line.origin - if line.origin in ['<', '>']: + if line.origin in ['<', '>', '=']: origin = '' content = content + origin + ' '+ line.content else: # Avoid situation where at the end of a file we get: # + foo< # \ No newline at end of file - if line[0] == '<': + if line[0] in ['<', '>', '=']: line = ('', line[1]) content = content + ' '.join(line) From 5422b011759c860157663061f297a1cdb2cb7750 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 19 2016 22:08:25 +0000 Subject: [PATCH 14/14] Let's be explicit about what we want the merge_status to be --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 7240c03..ef4f21c 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1159,7 +1159,7 @@ def diff_pull_request( request.commit_stop = diff_commits[0].oid.hex session.add(request) session.commit() - if not request.merge_status: + if request.merge_status is None: pagure.lib.add_pull_request_comment( session, request, commit=None, filename=None, row=None,