From 413a97aa4eea6d91e607fd79561c07770debe9dc Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 24 2017 08:35:41 +0000 Subject: [PATCH 1/7] Add the tags_pull_requests table and its alembic migration This also adds the relation so we can access tags directly from the pull-request object as we do for issues. Signed-off-by: Pierre-Yves Chibon --- diff --git a/alembic/versions/a13967424130_add_pr_tags_table.py b/alembic/versions/a13967424130_add_pr_tags_table.py new file mode 100644 index 0000000..7b30c49 --- /dev/null +++ b/alembic/versions/a13967424130_add_pr_tags_table.py @@ -0,0 +1,48 @@ +"""Add PR tags table + +Revision ID: a13967424130 +Revises: 01e58ee9eccb +Create Date: 2017-11-05 16:56:01.164976 + +""" + +import datetime + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = 'a13967424130' +down_revision = '01e58ee9eccb' + + +def upgrade(): + """ Create the tags_pull_requests to store the tags of pull-requests. + """ + op.create_table( + 'tags_pull_requests', + sa.Column( + 'tag_id', + sa.Integer, + sa.ForeignKey( + 'tags_colored.id', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True), + sa.Column( + 'request_uid', + sa.String(32), + sa.ForeignKey( + 'pull_requests.uid', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True), + sa.Column( + 'date_created', + sa.DateTime, + nullable=False, + default=datetime.datetime.utcnow), + ) + + +def downgrade(): + """ Delete the tags_pull_requests table. """ + op.drop_table('tags_pull_requests') diff --git a/pagure/lib/model.py b/pagure/lib/model.py index cb78982..40b1440 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1724,6 +1724,14 @@ class PullRequest(BASE): closed_by = relation('User', foreign_keys=[closed_by_id], remote_side=[User.id], backref='closed_requests') + tags = relation( + "TagColored", + secondary="tags_pull_requests", + primaryjoin="pull_requests.c.uid==tags_pull_requests.c.request_uid", + secondaryjoin="tags_pull_requests.c.tag_id==tags_colored.c.id", + viewonly=True + ) + def __repr__(self): return 'PullRequest(%s, project:%s, user:%s, title:%s)' % ( self.id, self.project.name, self.user.user, self.title @@ -1742,6 +1750,11 @@ class PullRequest(BASE): return '%s-pull-request-%s' % (self.project.name, self.uid) @property + def tags_text(self): + ''' Return the list of tags in a simple text form. ''' + return [tag.tag for tag in self.tags] + + @property def discussion(self): ''' Return the list of comments related to the pull-request itself, ie: not related to a specific commit. @@ -2093,6 +2106,46 @@ class CommitFlag(BASE): return output +class TagPullRequest(BASE): + """ Stores the tag associated with an pull-request. + + Table -- tags_pull_requests + """ + + __tablename__ = 'tags_pull_requests' + + tag_id = sa.Column( + sa.Integer, + sa.ForeignKey( + 'tags_colored.id', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True) + request_uid = sa.Column( + sa.String(32), + sa.ForeignKey( + 'pull_requests.uid', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True) + date_created = sa.Column(sa.DateTime, nullable=False, + default=datetime.datetime.utcnow) + + pull_request = relation( + 'PullRequest', + foreign_keys=[request_uid], + remote_side=[PullRequest.uid], + backref=backref( + 'tags_pr_colored', cascade="delete, delete-orphan" + ) + ) + tag = relation( + 'TagColored', foreign_keys=[tag_id], remote_side=[TagColored.id], + ) + + def __repr__(self): + return 'TagPullRequest(PR:%s, tag:%s)' % ( + self.pull_request.id, self.tag) + + class PagureGroupType(BASE): """ A list of the type a group can have definition. From 1ab5757fde10c01338f35c4eabd5bb98e19c4d6f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 24 2017 08:35:42 +0000 Subject: [PATCH 2/7] Add a private column to the pull_requests table This makes the PR object closer to the Issue one allowing to re-share more code and having private PRs is something that has already been asked (cf ticket #1893) for pagure so this is laying ground for it. Signed-off-by: Pierre-Yves Chibon --- diff --git a/alembic/versions/47f5fab6f46a_private_pull_request.py b/alembic/versions/47f5fab6f46a_private_pull_request.py new file mode 100644 index 0000000..37e41c5 --- /dev/null +++ b/alembic/versions/47f5fab6f46a_private_pull_request.py @@ -0,0 +1,36 @@ +"""private pull-request + +Revision ID: 47f5fab6f46a +Revises: a13967424130 +Create Date: 2017-11-06 11:37:57.460886 + +""" + + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = '47f5fab6f46a' +down_revision = 'a13967424130' + +def upgrade(): + ''' Add a private column in the pull_requests table + ''' + op.add_column( + 'pull_requests', + sa.Column('private', sa.Boolean, nullable=True, default=False) + ) + op.execute('''UPDATE "pull_requests" ''' + '''SET private=False;''') + + op.alter_column( + 'pull_requests', + column_name='private', + nullable=False, existing_nullable=True) + + +def downgrade(): + ''' Remove the private column + ''' + op.drop_column('pull_requests', 'private') diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 40b1440..9827b73 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1680,6 +1680,9 @@ class PullRequest(BASE): ), nullable=True) + # While present this column isn't used anywhere yet + private = sa.Column(sa.Boolean, nullable=False, default=False) + status = sa.Column( sa.String(255), sa.ForeignKey( From ef1fe1a5158da393189913bb449b20ecb349857f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 24 2017 08:35:42 +0000 Subject: [PATCH 3/7] Make the methods to tag/untag an object work on Issue and PR objects This allows us to reuse most of the code or at least be consistent in our path. There are still some differences between issues, pull-requests and projects but the code should be able to handle all of these fine now. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index d121b3d..3eba96e 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -791,10 +791,10 @@ def api_change_status_issue(repo, issueid, username=None, namespace=None): if message: pagure.lib.add_metadata_update_notif( session=SESSION, - issue=issue, + obj=issue, messages=message, user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] + gitfolder=APP.config['TICKETS_FOLDER'] ) except pagure.exceptions.PagureException as err: raise pagure.exceptions.APIError( @@ -895,10 +895,10 @@ def api_change_milestone_issue(repo, issueid, username=None, namespace=None): if message: pagure.lib.add_metadata_update_notif( session=SESSION, - issue=issue, + obj=issue, messages=message, user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] + gitfolder=APP.config['TICKETS_FOLDER'] ) except pagure.exceptions.PagureException as err: raise pagure.exceptions.APIError( @@ -1065,10 +1065,10 @@ def api_assign_issue(repo, issueid, username=None, namespace=None): if message: pagure.lib.add_metadata_update_notif( session=SESSION, - issue=issue, + obj=issue, messages=message, user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] + gitfolder=APP.config['TICKETS_FOLDER'] ) output['message'] = message else: @@ -1247,10 +1247,10 @@ def api_update_custom_field( output['message'] = message pagure.lib.add_metadata_update_notif( session=SESSION, - issue=issue, + obj=issue, messages=message, user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] + gitfolder=APP.config['TICKETS_FOLDER'] ) else: output['message'] = 'No changes' @@ -1371,10 +1371,10 @@ def api_update_custom_fields( output['messages'].append({key.name: message}) pagure.lib.add_metadata_update_notif( session=SESSION, - issue=issue, + obj=issue, messages=message, user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] + gitfolder=APP.config['TICKETS_FOLDER'] ) else: output['messages'].append({key.name: 'No changes'}) diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 6f4e8c3..a8bcc17 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -384,7 +384,7 @@ def add_issue_comment(session, issue, comment, user, ticketfolder, return 'Comment added' -def add_tag_obj(session, obj, tags, user, ticketfolder): +def add_tag_obj(session, obj, tags, user, gitfolder): ''' Add a tag to an object (either an issue or a project). ''' user_obj = get_user(session, user) @@ -426,10 +426,17 @@ def add_tag_obj(session, obj, tags, user, ticketfolder): session.add(tagobj) session.flush() - dbobjtag = model.TagIssueColored( - issue_uid=obj.uid, - tag_id=tagobj.id - ) + if obj.isa == 'issue': + dbobjtag = model.TagIssueColored( + issue_uid=obj.uid, + tag_id=tagobj.id + ) + else: + dbobjtag = model.TagPullRequest( + request_uid=obj.uid, + tag_id=tagobj.id + ) + added_tags_color.append(tagobj.tag_color) session.add(dbobjtag) @@ -439,7 +446,7 @@ def add_tag_obj(session, obj, tags, user, ticketfolder): if isinstance(obj, model.Issue): pagure.lib.git.update_git( - obj, repo=obj.project, repofolder=ticketfolder) + obj, repo=obj.project, repofolder=gitfolder) if not obj.private: pagure.lib.notify.log( @@ -462,9 +469,35 @@ def add_tag_obj(session, obj, tags, user, ticketfolder): 'added_tags_color': added_tags_color, } )) + elif isinstance(obj, model.PullRequest): + pagure.lib.git.update_git( + obj, repo=obj.project, repofolder=gitfolder) + + if not obj.private: + pagure.lib.notify.log( + obj.project, + topic='pull-request.tag.added', + msg=dict( + pull_request=obj.to_json(public=True), + project=obj.project.to_json(public=True), + tags=added_tags, + agent=user_obj.username, + ), + redis=REDIS, + ) + + # Send notification for the event-source server + if REDIS and not obj.project.private: + REDIS.publish('pagure.%s' % obj.uid, json.dumps( + { + 'added_tags': added_tags, + 'added_tags_color': added_tags_color, + } + )) if added_tags: - return 'Issue tagged with: %s' % ', '.join(added_tags) + return '%s tagged with: %s' % ( + obj.isa.capitalize(), ', '.join(added_tags)) else: return 'Nothing to add' @@ -720,7 +753,7 @@ def remove_issue_dependency( [str(id) for id in parent_del]) -def remove_tags(session, project, tags, ticketfolder, user): +def remove_tags(session, project, tags, gitfolder, user): ''' Removes the specified tag of a project. ''' user_obj = get_user(session, user) @@ -751,7 +784,7 @@ def remove_tags(session, project, tags, ticketfolder, user): tag = issue_tag.tag session.delete(issue_tag) pagure.lib.git.update_git( - issue, repo=issue.project, repofolder=ticketfolder) + issue, repo=issue.project, repofolder=gitfolder) pagure.lib.notify.log( project, @@ -767,7 +800,7 @@ def remove_tags(session, project, tags, ticketfolder, user): return msgs -def remove_tags_obj(session, obj, tags, ticketfolder, user): +def remove_tags_obj(session, obj, tags, gitfolder, user): ''' Removes the specified tag(s) of a given object. ''' user_obj = get_user(session, user) @@ -781,16 +814,22 @@ def remove_tags_obj(session, obj, tags, ticketfolder, user): tag = objtag.tag removed_tags.append(tag) session.delete(objtag) - else: + elif obj.isa == 'issue': for objtag in obj.tags_issues_colored: if objtag.tag.tag in tags: tag = objtag.tag.tag removed_tags.append(tag) session.delete(objtag) + elif obj.isa == 'pull-request': + for objtag in obj.tags_pr_colored: + if objtag.tag.tag in tags: + tag = objtag.tag.tag + removed_tags.append(tag) + session.delete(objtag) if isinstance(obj, model.Issue): pagure.lib.git.update_git( - obj, repo=obj.project, repofolder=ticketfolder) + obj, repo=obj.project, repofolder=gitfolder) pagure.lib.notify.log( obj.project, @@ -808,8 +847,29 @@ def remove_tags_obj(session, obj, tags, ticketfolder, user): if REDIS and not obj.project.private: REDIS.publish('pagure.%s' % obj.uid, json.dumps( {'removed_tags': removed_tags})) + elif isinstance(obj, model.PullRequest): + pagure.lib.git.update_git( + obj, repo=obj.project, repofolder=gitfolder) + + pagure.lib.notify.log( + obj.project, + topic='pull-request.tag.removed', + msg=dict( + pull_request=obj.to_json(public=True), + project=obj.project.to_json(public=True), + tags=removed_tags, + agent=user_obj.username, + ), + redis=REDIS, + ) + + # Send notification for the event-source server + if REDIS and not obj.project.private: + REDIS.publish('pagure.%s' % obj.uid, json.dumps( + {'removed_tags': removed_tags})) - return 'Issue **un**tagged with: %s' % ', '.join(removed_tags) + return '%s **un**tagged with: %s' % ( + obj.isa.capitalize(), ', '.join(removed_tags)) def edit_issue_tags( @@ -3001,7 +3061,7 @@ def avatar_url_from_email(email, size=64, default='retro', dns=False): hashhex, query) -def update_tags(session, obj, tags, username, ticketfolder): +def update_tags(session, obj, tags, username, gitfolder): """ Update the tags of a specified object (adding or removing them). This object can be either an issue or a project. @@ -3018,9 +3078,10 @@ def update_tags(session, obj, tags, username, ticketfolder): obj=obj, tags=toadd, user=username, - ticketfolder=ticketfolder, + gitfolder=gitfolder, ) - messages.append('Issue tagged with: %s' % ', '.join(sorted(toadd))) + messages.append('%s tagged with: %s' % ( + obj.isa.capitalize(), ', '.join(sorted(toadd)))) if torm: remove_tags_obj( @@ -3028,10 +3089,10 @@ def update_tags(session, obj, tags, username, ticketfolder): obj=obj, tags=torm, user=username, - ticketfolder=ticketfolder, + gitfolder=gitfolder, ) - messages.append('Issue **un**tagged with: %s' % ', '.join( - sorted(torm))) + messages.append('%s **un**tagged with: %s' % ( + obj.isa.capitalize(), ', '.join(sorted(torm)))) session.commit() @@ -4366,7 +4427,7 @@ def get_active_milestones(session, project): return sorted([item[0] for item in query.distinct()]) -def add_metadata_update_notif(session, issue, messages, user, ticketfolder): +def add_metadata_update_notif(session, obj, messages, user, gitfolder): ''' Add a notification to the specified issue with the given messages which should reflect changes made to the meta-data of the issue. ''' @@ -4381,16 +4442,25 @@ def add_metadata_update_notif(session, issue, messages, user, ticketfolder): user_obj = get_user(session, user) user_id = user_obj.id - issue_comment = model.IssueComment( - issue_uid=issue.uid, - comment='**Metadata Update from @%s**:\n- %s' % ( - user, '\n- '.join(sorted(messages))), - user_id=user_id, - notification=True, - ) - issue.last_updated = datetime.datetime.utcnow() - session.add(issue) - session.add(issue_comment) + if obj.isa == 'issue': + obj_comment = model.IssueComment( + issue_uid=obj.uid, + comment='**Metadata Update from @%s**:\n- %s' % ( + user, '\n- '.join(sorted(messages))), + user_id=user_id, + notification=True, + ) + elif obj.isa == 'pull-request': + obj_comment = model.PullRequestComment( + pull_request_uid=obj.uid, + comment='**Metadata Update from @%s**:\n- %s' % ( + user, '\n- '.join(sorted(messages))), + user_id=user_id, + notification=True, + ) + obj.last_updated = datetime.datetime.utcnow() + session.add(obj) + session.add(obj_comment) # Make sure we won't have SQLAlchemy error before we continue session.commit() diff --git a/pagure/lib/git.py b/pagure/lib/git.py index f5e98a5..1fdcb21 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -433,7 +433,7 @@ def get_project_from_json( if tags: pagure.lib.add_tag_obj( session, project, tags=tags, user=user.username, - ticketfolder=None) + gitfolder=None) return project @@ -620,7 +620,7 @@ def update_ticket_from_git( # Update tags tags = json_data.get('tags', []) msgs = pagure.lib.update_tags( - session, issue, tags, username=user.user, ticketfolder=None) + session, issue, tags, username=user.user, gitfolder=None) if msgs: messages.extend(msgs) @@ -668,10 +668,10 @@ def update_ticket_from_git( if messages: pagure.lib.add_metadata_update_notif( session=session, - issue=issue, + obj=issue, messages=messages, user=agent.username, - ticketfolder=None + gitfolder=None ) session.commit() diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index f6395fd..1439581 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -235,7 +235,7 @@ def update_issue(repo, issueid, username=None, namespace=None): msgs = pagure.lib.update_tags( SESSION, issue, tags, username=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'], + gitfolder=APP.config['TICKETS_FOLDER'], ) messages = messages.union(set(msgs)) @@ -340,10 +340,10 @@ def update_issue(repo, issueid, username=None, namespace=None): not_needed = set(['Comment added', 'Updated comment']) pagure.lib.add_metadata_update_notif( session=SESSION, - issue=issue, + obj=issue, messages=messages - not_needed, user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] + gitfolder=APP.config['TICKETS_FOLDER'] ) messages.add('Metadata fields updated') @@ -567,7 +567,7 @@ def remove_tag(repo, username=None, namespace=None): msgs = pagure.lib.remove_tags( SESSION, repo, tags, user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] + gitfolder=APP.config['TICKETS_FOLDER'] ) try: @@ -1200,10 +1200,10 @@ def edit_issue(repo, issueid, username=None, namespace=None): if messages: pagure.lib.add_metadata_update_notif( session=SESSION, - issue=issue, + obj=issue, messages=messages, user=flask.g.fas_user.username, - ticketfolder=APP.config['TICKETS_FOLDER'] + gitfolder=APP.config['TICKETS_FOLDER'] ) # If there is a file attached, attach it. diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 5dfc70c..eb4e2f9 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -1151,7 +1151,7 @@ def update_project(repo, username=None, namespace=None): SESSION, repo, tags=[t.strip() for t in form.tags.data.split(',')], username=flask.g.fas_user.username, - ticketfolder=None, + gitfolder=None, ) SESSION.add(repo) SESSION.commit() diff --git a/tests/test_pagure_flask_api_project.py b/tests/test_pagure_flask_api_project.py index ab7fe80..6d19539 100644 --- a/tests/test_pagure_flask_api_project.py +++ b/tests/test_pagure_flask_api_project.py @@ -351,7 +351,7 @@ class PagureFlaskApiProjecttests(tests.Modeltests): output = pagure.lib.update_tags( self.session, repo, 'infra', 'pingou', None) - self.assertEqual(output, ['Issue tagged with: infra']) + self.assertEqual(output, ['Project tagged with: infra']) # Check after adding repo = pagure.get_authorized_project(self.session, 'test') @@ -801,8 +801,8 @@ class PagureFlaskApiProjecttests(tests.Modeltests): # Adding a tag output = pagure.lib.update_tags( self.session, repo, 'infra', 'pingou', - ticketfolder=None) - self.assertEqual(output, ['Issue tagged with: infra']) + gitfolder=None) + self.assertEqual(output, ['Project tagged with: infra']) # Check after adding repo = pagure.get_authorized_project(self.session, 'test') @@ -871,8 +871,8 @@ class PagureFlaskApiProjecttests(tests.Modeltests): # Adding a tag output = pagure.lib.update_tags( self.session, repo, 'infra', 'pingou', - ticketfolder=None) - self.assertEqual(output, ['Issue tagged with: infra']) + gitfolder=None) + self.assertEqual(output, ['Project tagged with: infra']) # Check after adding repo = pagure.get_authorized_project(self.session, 'test') @@ -967,8 +967,8 @@ class PagureFlaskApiProjecttests(tests.Modeltests): # Adding a tag output = pagure.lib.update_tags( self.session, repo, 'infra', 'pingou', - ticketfolder=None) - self.assertEqual(output, ['Issue tagged with: infra']) + gitfolder=None) + self.assertEqual(output, ['Project tagged with: infra']) # Check after adding repo = pagure.get_authorized_project(self.session, 'test') diff --git a/tests/test_pagure_flask_api_ui_private_repo.py b/tests/test_pagure_flask_api_ui_private_repo.py index f5ad6c9..16586c0 100644 --- a/tests/test_pagure_flask_api_ui_private_repo.py +++ b/tests/test_pagure_flask_api_ui_private_repo.py @@ -986,8 +986,8 @@ class PagurePrivateRepotest(tests.Modeltests): # Adding a tag output = pagure.lib.update_tags( self.session, repo, 'infra', 'pingou', - ticketfolder=None) - self.assertEqual(output, ['Issue tagged with: infra']) + gitfolder=None) + self.assertEqual(output, ['Project tagged with: infra']) # Check after adding repo = pagure.lib._get_project(self.session, 'test4') diff --git a/tests/test_pagure_flask_dump_load_ticket.py b/tests/test_pagure_flask_dump_load_ticket.py index fde473f..e11ce4b 100644 --- a/tests/test_pagure_flask_dump_load_ticket.py +++ b/tests/test_pagure_flask_dump_load_ticket.py @@ -133,7 +133,7 @@ class PagureFlaskDumpLoadTicketTests(tests.Modeltests): obj=issue, tags=[' feature ', 'future '], user='pingou', - ticketfolder=repopath, + gitfolder=repopath, ) self.session.commit() self.assertEqual(msg, 'Issue tagged with: feature, future') diff --git a/tests/test_pagure_flask_ui_issues.py b/tests/test_pagure_flask_ui_issues.py index 27526c6..2111f0b 100644 --- a/tests/test_pagure_flask_ui_issues.py +++ b/tests/test_pagure_flask_ui_issues.py @@ -2670,7 +2670,7 @@ class PagureFlaskIssuestests(tests.Modeltests): obj=issue, tags='tag1', user='pingou', - ticketfolder=None) + gitfolder=None) self.session.commit() self.assertEqual(msg, 'Issue tagged with: tag1') @@ -2772,7 +2772,7 @@ class PagureFlaskIssuestests(tests.Modeltests): obj=issue, tags='tag1', user='pingou', - ticketfolder=None) + gitfolder=None) self.session.commit() self.assertEqual(msg, 'Issue tagged with: tag1') diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py index 5e016a3..553dfef 100644 --- a/tests/test_pagure_flask_ui_repo.py +++ b/tests/test_pagure_flask_ui_repo.py @@ -3548,7 +3548,7 @@ index 0000000..fb7093d obj=issue, tags='tag1', user='pingou', - ticketfolder=None) + gitfolder=None) self.session.commit() self.assertEqual(msg, 'Issue tagged with: tag1') diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 57409b5..a7a5d73 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -993,7 +993,7 @@ class PagureLibtests(tests.Modeltests): obj=issue, tags='tag1', user='pingou', - ticketfolder=None) + gitfolder=None) self.session.commit() self.assertEqual(msg, 'Issue tagged with: tag1') @@ -1012,7 +1012,7 @@ class PagureLibtests(tests.Modeltests): obj=issue, tags='tag1', user='pingou', - ticketfolder=None) + gitfolder=None) self.session.commit() self.assertEqual(msg, 'Nothing to add') @@ -1040,14 +1040,14 @@ class PagureLibtests(tests.Modeltests): project=repo, tags='foo', user='pingou', - ticketfolder=None) + gitfolder=None) msgs = pagure.lib.remove_tags( session=self.session, project=repo, tags='tag1', user='pingou', - ticketfolder=None) + gitfolder=None) self.assertEqual(msgs, ['Issue **un**tagged with: tag1']) @@ -1066,7 +1066,7 @@ class PagureLibtests(tests.Modeltests): obj=issue, tags='tag1', user='pingou', - ticketfolder=None) + gitfolder=None) self.assertEqual(msgs, 'Issue **un**tagged with: tag1') @patch('pagure.lib.git.update_git') @@ -1084,8 +1084,8 @@ class PagureLibtests(tests.Modeltests): self.session, repo, tags=['pagure', 'test'], user='pingou', - ticketfolder=None) - self.assertEqual(msg, 'Issue tagged with: pagure, test') + gitfolder=None) + self.assertEqual(msg, 'Project tagged with: pagure, test') self.session.commit() # Check the tags @@ -1098,8 +1098,8 @@ class PagureLibtests(tests.Modeltests): obj=repo, tags='test', user='pingou', - ticketfolder=None) - self.assertEqual(msgs, 'Issue **un**tagged with: test') + gitfolder=None) + self.assertEqual(msgs, 'Project **un**tagged with: test') self.session.commit() # Check the tags @@ -1179,7 +1179,7 @@ class PagureLibtests(tests.Modeltests): obj=issue, tags='tag3', user='pingou', - ticketfolder=None) + gitfolder=None) self.session.commit() self.assertEqual(msg, 'Issue tagged with: tag3') self.assertEqual([tag.tag for tag in issue.tags], ['tag2', 'tag3']) @@ -2026,7 +2026,7 @@ class PagureLibtests(tests.Modeltests): obj=issue, tags='tag1', user='pingou', - ticketfolder=None) + gitfolder=None) self.session.commit() self.assertEqual(msg, 'Issue tagged with: tag1') @@ -2036,7 +2036,7 @@ class PagureLibtests(tests.Modeltests): obj=issue, tags='tag2', user='pingou', - ticketfolder=None) + gitfolder=None) self.session.commit() self.assertEqual(msg, 'Issue tagged with: tag2') @@ -2895,7 +2895,7 @@ class PagureLibtests(tests.Modeltests): self.assertEqual(issue.tags_text, []) messages = pagure.lib.update_tags( - self.session, issue, 'tag', 'pingou', ticketfolder=None) + self.session, issue, 'tag', 'pingou', gitfolder=None) self.assertEqual(messages, ['Issue tagged with: tag']) # after @@ -2909,7 +2909,7 @@ class PagureLibtests(tests.Modeltests): # Replace the tag by two others messages = pagure.lib.update_tags( self.session, issue, ['tag2', 'tag3'], 'pingou', - ticketfolder=None) + gitfolder=None) self.assertEqual( messages, [ 'Issue tagged with: tag2, tag3', diff --git a/tests/test_pagure_lib_drop_issue.py b/tests/test_pagure_lib_drop_issue.py index abc9965..4c5bb4b 100644 --- a/tests/test_pagure_lib_drop_issue.py +++ b/tests/test_pagure_lib_drop_issue.py @@ -120,7 +120,7 @@ class PagureLibDropIssuetests(tests.Modeltests): issue, tags=['red'], username='pingou', - ticketfolder=None, + gitfolder=None, ) self.session.commit() @@ -164,7 +164,7 @@ class PagureLibDropIssuetests(tests.Modeltests): issue, tags=['red'], username='pingou', - ticketfolder=None, + gitfolder=None, ) self.session.commit() self.assertEqual(msgs, ['Issue tagged with: red']) @@ -175,7 +175,7 @@ class PagureLibDropIssuetests(tests.Modeltests): issue, tags=['red'], username='pingou', - ticketfolder=None, + gitfolder=None, ) self.session.commit() self.assertEqual(msgs, ['Issue tagged with: red']) From 401c1136ce5a974ad93ee3760cee0efdd41c124f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 24 2017 08:35:42 +0000 Subject: [PATCH 4/7] Refresh the metadata git repo when adding a notification to an issue/PR Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index a8bcc17..308f2fb 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -4464,6 +4464,10 @@ def add_metadata_update_notif(session, obj, messages, user, gitfolder): # Make sure we won't have SQLAlchemy error before we continue session.commit() + if gitfolder: + pagure.lib.git.update_git( + obj, repo=obj.project, repofolder=gitfolder) + def tokenize_search_string(pattern): """This function tokenizes search patterns into key:value and rest. From c292442d75f8c88ff65f3220ff07b3ea15460e43 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 24 2017 08:35:42 +0000 Subject: [PATCH 5/7] Allow tagging PR using the same tag list as used for issues. This will allow projects to use tags on PR (tags which here can be updated by the person who opened a PR even if they are not committers). Fixes #2442 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 1c59530..d6ef1fe 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -707,8 +707,9 @@
- {% if authenticated and mergeform and pull_request.status == 'Open' and g.repo_committer %} -
- -
- {{ pull_request.assignee.username or 'Unassigned' }} -
- - {% if authenticated and mergeform and pull_request.status == 'Open' and g.repo_committer %} +
+ +
+ {{ pull_request.assignee.username or 'Unassigned' }} +
+
+ + {% if authenticated and ( + g.repo_user + or g.fas_user.username == pull_request.user.user) %} +
+ + +
+ {% endif%} + +
+ +

+ {% for tag in pull_request.tags %} + + {{ tag.tag }} + + {% endfor %} +

+
+ + {% if authenticated and mergeform and pull_request.status == 'Open' + and (g.repo_committer + or g.fas_user.username == pull_request.user.user) %} Cancel
@@ -1444,6 +1477,23 @@ $(document).ready(function () { $("#comment").atwho(issueAndPrConfig); $("#initial_comment").atwho(issueAndPrConfig); }); + + var available_tags = []; + {%for tog in tag_list %} + available_tags.push("{{tog.tag}}"); + {%endfor%} + var items = available_tags.map(function(x) { return { item: x }; }); + + $('#tag').selectize({ + delimiter: ',', + options: items, + persist: false, + create: false, + labelField: "item", + valueField: "item", + searchField: ["item"], + }); + } ); $(window).on('hashchange', updateHighlight); diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index b3de389..b7f0afe 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -255,6 +255,7 @@ def request_pull(repo, requestid, username=None, namespace=None): diff=diff, mergeform=form, subscribers=pagure.lib.get_watch_list(SESSION, request), + tag_list=pagure.lib.get_tags_of_project(SESSION, repo), ) @@ -896,19 +897,19 @@ def refresh_request_pull(repo, requestid, username=None, namespace=None): @APP.route( - '//pull-request//assign', methods=['POST']) + '//pull-request//update', methods=['POST']) @APP.route( - '///pull-request//assign', + '///pull-request//update', methods=['POST']) @APP.route( - '/fork///pull-request//assign', + '/fork///pull-request//update', methods=['POST']) @APP.route( - '/fork////pull-request//assign', + '/fork////pull-request//update', methods=['POST']) @login_required -def set_assignee_requests(repo, requestid, username=None, namespace=None): - ''' Assign a pull-request. ''' +def update_pull_requests(repo, requestid, username=None, namespace=None): + ''' Update the metadata of a pull-request. ''' repo = flask.g.repo if not repo.settings.get('pull_requests', True): @@ -923,22 +924,55 @@ def set_assignee_requests(repo, requestid, username=None, namespace=None): if request.status != 'Open': flask.abort(403, 'Pull-request closed') - if not flask.g.repo_committer: - flask.abort(403, 'You are not allowed to assign this pull-request') + if not flask.g.repo_committer \ + and flask.g.fas_user.username != request.user.username: + flask.abort(403, 'You are not allowed to update this pull-request') form = pagure.forms.ConfirmationForm() if form.validate_on_submit(): + tags = [ + tag.strip() + for tag in flask.request.form.get('tag', '').strip().split(',') + if tag.strip()] + + messages = set() try: - # Assign or update assignee of the ticket - message = pagure.lib.add_pull_request_assignee( - SESSION, - request=request, - assignee=flask.request.form.get('user', '').strip() or None, - user=flask.g.fas_user.username, - requestfolder=APP.config['REQUESTS_FOLDER'],) - if message: + # Adjust (add/remove) tags + msgs = pagure.lib.update_tags( + SESSION, request, tags, + username=flask.g.fas_user.username, + gitfolder=APP.config['TICKETS_FOLDER'], + ) + messages = messages.union(set(msgs)) + + if flask.g.repo_committer: + # Assign or update assignee of the ticket + msg = pagure.lib.add_pull_request_assignee( + SESSION, + request=request, + assignee=flask.request.form.get( + 'user', '').strip() or None, + user=flask.g.fas_user.username, + requestfolder=APP.config['REQUESTS_FOLDER'], + ) + if msg: + messages.add(msg) + + if messages: + # Add the comment for field updates: + not_needed = set(['Comment added', 'Updated comment']) + pagure.lib.add_metadata_update_notif( + session=SESSION, + obj=request, + messages=messages - not_needed, + user=flask.g.fas_user.username, + gitfolder=APP.config['REQUESTS_FOLDER'] + ) + messages.add('Metadata fields updated') + SESSION.commit() - flask.flash(message) + for message in messages: + flask.flash(message) except pagure.exceptions.PagureException as err: SESSION.rollback() flask.flash(err.message, 'error') diff --git a/tests/test_pagure_flask_ui_fork.py b/tests/test_pagure_flask_ui_fork.py index 990c9f5..b7717bd 100644 --- a/tests/test_pagure_flask_ui_fork.py +++ b/tests/test_pagure_flask_ui_fork.py @@ -20,7 +20,7 @@ import time import os import pygit2 -from mock import patch +from mock import patch, MagicMock sys.path.insert(0, os.path.join(os.path.dirname( os.path.abspath(__file__)), '..')) @@ -1240,10 +1240,10 @@ index 0000000..2a552bb '\n Pull request canceled!', output.data) - @patch('pagure.lib.notify.send_email') - def test_set_assignee_requests(self, send_email): - """ Test the set_assignee_requests endpoint. """ - send_email.return_value = True + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_update_pull_requests_assign(self): + """ Test the update_pull_requests endpoint when assigning a PR. + """ tests.create_projects(self.session) tests.create_projects_git( @@ -1254,15 +1254,15 @@ index 0000000..2a552bb user.username = 'pingou' with tests.user_set(pagure.APP, user): # No such project - output = self.app.post('/foo/pull-request/1/assign') + output = self.app.post('/foo/pull-request/1/update') self.assertEqual(output.status_code, 404) - output = self.app.post('/test/pull-request/100/assign') + output = self.app.post('/test/pull-request/100/update') self.assertEqual(output.status_code, 404) # Invalid input output = self.app.post( - '/test/pull-request/1/assign', follow_redirects=True) + '/test/pull-request/1/update', follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( 'PR#1: PR from the feature branch - test\n - ' @@ -1285,7 +1285,7 @@ index 0000000..2a552bb # No CSRF output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( @@ -1305,7 +1305,7 @@ index 0000000..2a552bb } output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( @@ -1327,14 +1327,14 @@ index 0000000..2a552bb user.username = 'foo' with tests.user_set(pagure.APP, user): output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 403) user.username = 'pingou' with tests.user_set(pagure.APP, user): output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( @@ -1356,7 +1356,145 @@ index 0000000..2a552bb self.session.commit() output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 403) + + # Project w/o pull-request + repo = pagure.get_authorized_project(self.session, 'test') + settings = repo.settings + settings['pull_requests'] = False + repo.settings = settings + self.session.add(repo) + self.session.commit() + + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 404) + + + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_update_pull_requests_tag(self): + """ Test the update_pull_requests endpoint when tagging a PR. + """ + + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'requests'), bare=True) + self.set_up_git_repo(new_project=None, branch_from='feature') + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + output = self.app.get('/test/pull-request/1') + self.assertEqual(output.status_code, 200) + + csrf_token = self.get_csrf(output=output) + + data = { + 'tag': 'black', + } + + # No CSRF + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + '<title>PR#1: PR from the feature branch - test\n - ' + 'Pagure', output.data) + self.assertIn( + '

PR#1\n' + ' PR from the feature branch\n', output.data) + self.assertNotIn( + '\n Request assigned', + output.data) + + # Tag the PR + data = { + 'csrf_token': csrf_token, + 'tag': 'black', + } + + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'PR#1: PR from the feature branch - test\n - ' + 'Pagure', output.data) + self.assertIn( + '

PR#1\n' + ' PR from the feature branch\n', output.data) + self.assertIn( + '\n Pull-request tagged with: black', + output.data) + self.assertIn( + 'title="comma separated list of tags"\n ' + 'value="black" />', output.data) + + # Try as another user + user.username = 'foo' + with tests.user_set(pagure.APP, user): + # Tag the PR + data = { + 'csrf_token': csrf_token, + 'tag': 'blue, yellow', + } + + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 403) + + # Make the PR be from foo + repo = pagure.get_authorized_project(self.session, 'test') + req = repo.requests[0] + req.user_id = 2 + self.session.add(req) + self.session.commit() + + # Re-try to tag the PR + data = { + 'csrf_token': csrf_token, + 'tag': 'blue, yellow', + } + + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'PR#1: PR from the feature branch - test\n - ' + 'Pagure', output.data) + self.assertIn( + '

PR#1\n' + ' PR from the feature branch\n', output.data) + self.assertIn( + '\n ' + 'Pull-request **un**tagged with: black', + output.data) + self.assertIn( + '\n ' + 'Pull-request tagged with: blue, yellow', + output.data) + self.assertIn( + 'title="comma separated list of tags"\n ' + 'value="blue,yellow" />', output.data) + + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + # Pull-Request closed + repo = pagure.get_authorized_project(self.session, 'test') + req = repo.requests[0] + req.status = 'Closed' + req.closed_by_in = 1 + self.session.add(req) + self.session.commit() + + output = self.app.post( + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 403) @@ -1369,7 +1507,7 @@ index 0000000..2a552bb self.session.commit() output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 404) From 8312e7ee3c80f39b0710e9cc552a4da20797ab11 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 24 2017 08:35:42 +0000 Subject: [PATCH 6/7] Do not update git is no folder is provided and small style fix Checking if a git folder is provided before creating the task, this allows speeding up the tests. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 1fdcb21..4751b60 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -132,6 +132,9 @@ def generate_gitolite_acls(project=None, group=None): def update_git(obj, repo, repofolder): """ Schedules an update_repo task after determining arguments. """ + if not repofolder: + return None + ticketuid = None requestuid = None if obj.isa == 'issue': @@ -152,8 +155,8 @@ def update_git(obj, repo, repofolder): def _maybe_wait(result): """ Function to patch if one wants to wait for finish. - This function should only ever be overridden by a few tests that depend on - counting and very precise timing. """ + This function should only ever be overridden by a few tests that depend + on counting and very precise timing. """ pass From 2b23a166b0f19c5616dd3edcf7ee47ac2612f492 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 24 2017 08:35:42 +0000 Subject: [PATCH 7/7] Fix tests for a typo fixed earlier Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_api_project.py b/tests/test_pagure_flask_api_project.py index 6d19539..9367556 100644 --- a/tests/test_pagure_flask_api_project.py +++ b/tests/test_pagure_flask_api_project.py @@ -2755,7 +2755,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): self.assertEqual(output.status_code, 400) data = json.loads(output.data) expected_output = { - "error": "Invalid or incomplete input submited", + "error": "Invalid or incomplete input submitted", "error_code": "EINVALIDREQ", "errors": { "status": [ @@ -2784,7 +2784,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): self.assertEqual(output.status_code, 400) data = json.loads(output.data) expected_output = { - "error": "Invalid or incomplete input submited", + "error": "Invalid or incomplete input submitted", "error_code": "EINVALIDREQ", "errors": { "username": [ @@ -2813,7 +2813,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): self.assertEqual(output.status_code, 400) data = json.loads(output.data) expected_output = { - "error": "Invalid or incomplete input submited", + "error": "Invalid or incomplete input submitted", "error_code": "EINVALIDREQ", "errors": { "comment": [ @@ -2842,7 +2842,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): self.assertEqual(output.status_code, 400) data = json.loads(output.data) expected_output = { - "error": "Invalid or incomplete input submited", + "error": "Invalid or incomplete input submitted", "error_code": "EINVALIDREQ", "errors": { "url": [ @@ -2900,7 +2900,7 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): { u'errors': {u'status': [u'Not a valid choice']}, u'error_code': u'EINVALIDREQ', - u'error': u'Invalid or incomplete input submited' + u'error': u'Invalid or incomplete input submitted' } )