kallithea Changeset - aa0560cfca9b

Changeset - aa0560cfca9b

Parent rev.

Child rev.

[Not reviewed]

default

0 3 0

Mads Kiilerich - 9 years ago 2016-10-24 15:18:51
madski@unity3d.com

pullrequests: when updating a PR, only add and remove the reviewers that actually were added/removed

3 files changed with 28 insertions and 39 deletions:

kallithea/controllers/pullrequests.py

kallithea/model/pull_request.py

kallithea/tests/functional/test_pullrequests.py

0 comments (0 inline, 0 general)

kallithea/controllers/pullrequests.py

➞

Show inline comments

@@ @@ -484,99 +484,102 @@ class PullrequestsController(BaseRepoCon @@
                 category='success')
         raise HTTPFound(location=pull_request.url())
     # pullrequest_post for PR editing
     @LoginRequired()
     @NotAnonymous()
     @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
                                    'repository.admin')
     def post(self, repo_name, pull_request_id):
         pull_request = PullRequest.get_or_404(pull_request_id)
         if pull_request.is_closed():
             raise HTTPForbidden()
         assert pull_request.other_repo.repo_name == repo_name
         #only owner or admin can update it
         owner = pull_request.owner_id == c.authuser.user_id
         repo_admin = h.HasRepoPermissionAny('repository.admin')(c.repo_name)
         if not (h.HasPermissionAny('hg.admin')() or repo_admin or owner):
             raise HTTPForbidden()
         _form = PullRequestPostForm()().to_python(request.POST)
         reviewer_ids = set(int(s) for s in _form['review_members'])
         org_reviewer_ids = set(int(s) for s in _form['org_review_members'])
         current_reviewer_ids = set(prr.user_id for prr in pull_request.reviewers)
         other_added = [User.get(u) for u in current_reviewer_ids - org_reviewer_ids]
         other_removed = [User.get(u) for u in org_reviewer_ids - current_reviewer_ids]
         if other_added:
             h.flash(_('Meanwhile, the following reviewers have been added: %s') %
                     (', '.join(u.username for u in other_added)),
                     category='warning')
         if other_removed:
             h.flash(_('Meanwhile, the following reviewers have been removed: %s') %
                     (', '.join(u.username for u in other_removed)),
                     category='warning')
         if _form['updaterev']:
             return self.create_new_iteration(pull_request,
                                       _form['updaterev'],
                                       _form['pullrequest_title'],
                                       _form['pullrequest_desc'],
                                       reviewer_ids)
         old_description = pull_request.description
         pull_request.title = _form['pullrequest_title']
         pull_request.description = _form['pullrequest_desc'].strip() or _('No description')
         pull_request.owner = User.get_by_username(_form['owner'])
         user = User.get(c.authuser.user_id)
         add_reviewer_ids = reviewer_ids - org_reviewer_ids - current_reviewer_ids
         remove_reviewer_ids = (org_reviewer_ids - reviewer_ids) & current_reviewer_ids
         try:
             PullRequestModel().mention_from_description(user, pull_request, old_description)
             PullRequestModel().update_reviewers(user, pull_request_id, reviewer_ids)
             PullRequestModel().add_reviewers(user, pull_request, add_reviewer_ids)
             PullRequestModel().remove_reviewers(user, pull_request, remove_reviewer_ids)
         except UserInvalidException as u:
             h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
             raise HTTPBadRequest()
         Session().commit()
         h.flash(_('Pull request updated'), category='success')
         raise HTTPFound(location=pull_request.url())
     @LoginRequired()
     @NotAnonymous()
     @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
                                    'repository.admin')
     @jsonify
     def delete(self, repo_name, pull_request_id):
         pull_request = PullRequest.get_or_404(pull_request_id)
         #only owner can delete it !
         if pull_request.owner_id == c.authuser.user_id:
             PullRequestModel().delete(pull_request)
             Session().commit()
             h.flash(_('Successfully deleted pull request'),
                     category='success')
             raise HTTPFound(location=url('my_pullrequests'))
         raise HTTPForbidden()
     @LoginRequired()
     @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
                                    'repository.admin')
     def show(self, repo_name, pull_request_id, extra=None):
         repo_model = RepoModel()
         c.users_array = repo_model.get_users_js()
         c.user_groups_array = repo_model.get_user_groups_js()
         c.pull_request = PullRequest.get_or_404(pull_request_id)
         c.allowed_to_change_status = self._get_is_allowed_change_status(c.pull_request)
         cc_model = ChangesetCommentsModel()
         cs_model = ChangesetStatusModel()
         # pull_requests repo_name we opened it against
         # ie. other_repo must match
         if repo_name != c.pull_request.other_repo.repo_name:
             raise HTTPNotFound
         # load compare data into template context
         c.cs_repo = c.pull_request.org_repo
         (c.cs_ref_type,
          c.cs_ref_name,
          c.cs_rev) = c.pull_request.org_ref.split(':')

kallithea/model/pull_request.py

➞

Show inline comments

@@ @@ -53,157 +53,143 @@ class PullRequestModel(BaseModel): @@
         specified, or if a given ID or username does not match any user.
         """
         for user_spec in seq:
             user = self._get_user(user_spec)
             if user is None or user.username == User.DEFAULT_USER:
                 raise UserInvalidException(user_spec)
             yield user
     def create(self, created_by, org_repo, org_ref, other_repo, other_ref,
                revisions, reviewers, title, description=None):
         from kallithea.model.changeset_status import ChangesetStatusModel
         created_by_user = self._get_user(created_by)
         org_repo = self._get_repo(org_repo)
         other_repo = self._get_repo(other_repo)
         new = PullRequest()
         new.org_repo = org_repo
         new.org_ref = org_ref
         new.other_repo = other_repo
         new.other_ref = other_ref
         new.revisions = revisions
         new.title = title
         new.description = description
         new.owner = created_by_user
         Session().add(new)
         Session().flush()
         #reset state to under-review
         from kallithea.model.comment import ChangesetCommentsModel
         comment = ChangesetCommentsModel().create(
             text=u'',
             repo=org_repo,
             author=new.owner,
             pull_request=new,
             send_email=False,
             status_change=ChangesetStatus.STATUS_UNDER_REVIEW,
+        )
         ChangesetStatusModel().set_status(
             org_repo,
             ChangesetStatus.STATUS_UNDER_REVIEW,
             new.owner,
             comment,
             pull_request=new
+        )
         reviewers = set(self._get_valid_reviewers(reviewers))
         mention_recipients = extract_mentioned_users(new.description)
-        self.__add_reviewers(created_by_user, new, reviewers, mention_recipients)
+        self.add_reviewers(created_by_user, new, reviewers, mention_recipients)
         return new
     def __add_reviewers(self, user, pr, reviewers, mention_recipients):
         # reviewers and mention_recipients should be sets of User objects.
     def add_reviewers(self, user, pr, reviewers, mention_recipients=None):
         """Add reviewer and send notification to them.
         """
         reviewer_users = set(self._get_valid_reviewers(reviewers))
         #members
         for reviewer in reviewers:
             reviewer = PullRequestReviewers(reviewer, pr)
             Session().add(reviewer)
         revision_data = [(x.raw_id, x.message)
                          for x in map(pr.org_repo.get_changeset, pr.revisions)]
         for reviewer in reviewer_users:
             prr = PullRequestReviewers(reviewer, pr)
             Session().add(prr)
         #notification to reviewers
         pr_url = pr.url(canonical=True)
         threading = ['%s-pr-%s@%s' % (pr.other_repo.repo_name,
                                       pr.pull_request_id,
                                       h.canonical_hostname())]
         subject = safe_unicode(
             h.link_to(
               _('%(user)s wants you to review pull request %(pr_nice_id)s: %(pr_title)s') % \
                 {'user': user.username,
                  'pr_title': pr.title,
                  'pr_nice_id': pr.nice_id()},
                 pr_url)
+            )
         body = pr.description
         _org_ref_type, org_ref_name, _org_rev = pr.org_ref.split(':')
         _other_ref_type, other_ref_name, _other_rev = pr.other_ref.split(':')
         revision_data = [(x.raw_id, x.message)
                          for x in map(pr.org_repo.get_changeset, pr.revisions)]
         email_kwargs = {
             'pr_title': pr.title,
             'pr_title_short': h.shorter(pr.title, 50),
             'pr_user_created': user.full_name_and_username,
             'pr_repo_url': h.canonical_url('summary_home', repo_name=pr.other_repo.repo_name),
             'pr_url': pr_url,
             'pr_revisions': revision_data,
             'repo_name': pr.other_repo.repo_name,
             'org_repo_name': pr.org_repo.repo_name,
             'pr_nice_id': pr.nice_id(),
             'pr_target_repo': h.canonical_url('summary_home',
                                repo_name=pr.other_repo.repo_name),
             'pr_target_branch': other_ref_name,
             'pr_source_repo': h.canonical_url('summary_home',
                                repo_name=pr.org_repo.repo_name),
             'pr_source_branch': org_ref_name,
             'pr_owner': pr.owner,
             'pr_owner_username': pr.owner.username,
             'pr_username': user.username,
             'threading': threading,
             'is_mention': False,
+            }
         if reviewers:
             NotificationModel().create(created_by=user, subject=subject, body=body,
                                        recipients=reviewers,
                                        type_=Notification.TYPE_PULL_REQUEST,
                                        email_kwargs=email_kwargs)
         mention_recipient_users = set()
         if mention_recipients:
             mention_recipients.difference_update(reviewers)
         if mention_recipients:
             mention_recipient_users = set(self._get_valid_reviewers(mention_recipients))
             mention_recipient_users.difference_update(reviewers)
         if mention_recipient_users:
             email_kwargs['is_mention'] = True
             subject = _('[Mention]') + ' ' + subject
             # FIXME: this subject is wrong and unused!
             NotificationModel().create(created_by=user, subject=subject, body=body,
-                                       recipients=mention_recipients,
+                                       recipients=mention_recipient_users,
                                        type_=Notification.TYPE_PULL_REQUEST,
                                        email_kwargs=email_kwargs)
     def mention_from_description(self, user, pr, old_description=''):
         mention_recipients = (extract_mentioned_users(pr.description) -
                               extract_mentioned_users(old_description))
         log.debug("Mentioning %s", mention_recipients)
-        self.__add_reviewers(user, pr, set(), mention_recipients)
+        self.add_reviewers(user, pr, set(), mention_recipients)
     def update_reviewers(self, user, pull_request, reviewers):
         """Set PR reviewers to exactly the given list of users"""
         pull_request = PullRequest.guess_instance(pull_request)
         current_reviewers = PullRequestReviewers.query() \
             .options(joinedload('user')) \
             .filter_by(pull_request=pull_request) \
             .all()
         current_reviewer_users = set(x.user for x in current_reviewers)
         new_reviewer_users = set(self._get_valid_reviewers(reviewers))
     def remove_reviewers(self, user, pull_request, reviewer_ids):
         """Remove users in the given user_id list from being reviewers of the PR."""
         to_add = new_reviewer_users - current_reviewer_users
         to_remove = current_reviewer_users - new_reviewer_users
         if not to_add and not to_remove:
             return # all done
         log.debug("Adding %s reviewers", to_add)
         self.__add_reviewers(user, pull_request, to_add, set())
         log.debug("Removing %s reviewers", to_remove)
         for prr in current_reviewers:
             if prr.user in to_remove:
                 Session().delete(prr)
         PullRequestReviewers.query() \
             .filter_by(pull_request=pull_request) \
             .filter(PullRequestReviewers.user_id.in_(reviewer_ids)) \
             .delete(synchronize_session='fetch') # the default of 'evaluate' is not available
     def delete(self, pull_request):
         pull_request = PullRequest.guess_instance(pull_request)
         Session().delete(pull_request)
     def close_pull_request(self, pull_request):
         pull_request = PullRequest.guess_instance(pull_request)
         pull_request.status = PullRequest.STATUS_CLOSED
         pull_request.updated_on = datetime.datetime.now()

kallithea/tests/functional/test_pullrequests.py

➞

Show inline comments

@@ @@ -45,99 +45,99 @@ class TestPullrequestsController(TestCon @@
                                  {'org_repo': HG_REPO,
                                   'org_ref': 'branch:default:default',
                                   'other_repo': HG_REPO,
                                   'other_ref': 'branch:default:default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
                                  },
                                  status=302)
         pull_request1_id = re.search('/pull-request/(\d+)/', response.location).group(1)
         assert response.location == 'http://localhost/%s/pull-request/%s/_/title' % (HG_REPO, pull_request1_id)
         # create new iteration
         response = self.app.post(url(controller='pullrequests', action='post',
                                      repo_name=HG_REPO, pull_request_id=pull_request1_id),
+                                 {
                                   'updaterev': 'default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   'owner': TEST_USER_ADMIN_LOGIN,
                                   '_authentication_token': self.authentication_token(),
                                   'review_members': [regular_user.user_id],
                                  },
                                  status=302)
         pull_request2_id = re.search('/pull-request/(\d+)/', response.location).group(1)
         assert pull_request2_id != pull_request1_id
         assert response.location == 'http://localhost/%s/pull-request/%s/_/title_v2' % (HG_REPO, pull_request2_id)
         response = response.follow()
         # verify reviewer was added
         response.mustcontain('<input type="hidden" value="%s" name="review_members" />' % regular_user.user_id)
         # update without creating new iteration
         response = self.app.post(url(controller='pullrequests', action='post',
                                      repo_name=HG_REPO, pull_request_id=pull_request2_id),
+                                 {
                                   'pullrequest_title': 'Title',
                                   'pullrequest_desc': 'description',
                                   'owner': TEST_USER_ADMIN_LOGIN,
                                   '_authentication_token': self.authentication_token(),
                                   'org_review_members': [admin_user.user_id], # fake - just to get some 'meanwhile' warning ... but it is also added ...
                                   'review_members': [regular_user2.user_id, admin_user.user_id],
                                  },
                                  status=302)
         assert response.location == 'http://localhost/%s/pull-request/%s/_/Title' % (HG_REPO, pull_request2_id)
         response = response.follow()
         # verify reviewers were added / removed
         response.mustcontain('Meanwhile, the following reviewers have been added: test_regular')
         response.mustcontain('Meanwhile, the following reviewers have been removed: test_admin')
-        response.mustcontain(no='<input type="hidden" value="%s" name="review_members" />' % regular_user.user_id)
         response.mustcontain('<input type="hidden" value="%s" name="review_members" />' % regular_user.user_id)
         response.mustcontain('<input type="hidden" value="%s" name="review_members" />' % regular_user2.user_id)
         response.mustcontain('<input type="hidden" value="%s" name="review_members" />' % admin_user.user_id)
+        response.mustcontain(no='<input type="hidden" value="%s" name="review_members" />' % admin_user.user_id)
     def test_update_with_invalid_reviewer(self):
         invalid_user_id = 99999
         self.log_user()
         # create a valid pull request
         response = self.app.post(url(controller='pullrequests', action='create',
                                      repo_name=HG_REPO),
+                                 {
                                   'org_repo': HG_REPO,
                                   'org_ref': 'branch:default:default',
                                   'other_repo': HG_REPO,
                                   'other_ref': 'branch:default:default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
                                  },
                                 status=302)
         # location is of the form:
         # http://localhost/vcs_test_hg/pull-request/54/_/title
         m = re.search('/pull-request/(\d+)/', response.location)
         assert m != None
         pull_request_id = m.group(1)
         # update it
         response = self.app.post(url(controller='pullrequests', action='post',
                                      repo_name=HG_REPO, pull_request_id=pull_request_id),
+                                 {
                                   'updaterev': 'default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   'owner': TEST_USER_ADMIN_LOGIN,
                                   '_authentication_token': self.authentication_token(),
                                   'review_members': [str(invalid_user_id)],
                                  },
                                  status=400)
         response.mustcontain('Invalid reviewer &#34;%s&#34; specified' % invalid_user_id)
     def test_edit_with_invalid_reviewer(self):
         invalid_user_id = 99999
         self.log_user()
         # create a valid pull request
         response = self.app.post(url(controller='pullrequests', action='create',
                                      repo_name=HG_REPO),
+                                 {
                                   'org_repo': HG_REPO,
                                   'org_ref': 'branch:default:default',
                                   'other_repo': HG_REPO,
                                   'other_ref': 'branch:default:default',

0 comments (0 inline, 0 general)