# HG changeset patch # User Daniel Dourvaris # Date 2016-09-26 23:34:15 # Node ID 930d1a1fb9e73ca8dc5e1a0cb831f54d57d00e08 # Parent 2df101bec030f83dd521886138a25986b047e10e reviewers: store reviewer reasons to database, fixes #4238 diff --git a/rhodecode/api/tests/test_get_pull_request.py b/rhodecode/api/tests/test_get_pull_request.py --- a/rhodecode/api/tests/test_get_pull_request.py +++ b/rhodecode/api/tests/test_get_pull_request.py @@ -95,9 +95,10 @@ class TestGetPullRequest(object): { 'user': reviewer.get_api_data(include_secrets=False, details='basic'), + 'reasons': reasons, 'review_status': st[0][1].status if st else 'not_reviewed', } - for reviewer, st in pull_request.reviewers_statuses() + for reviewer, reasons, st in pull_request.reviewers_statuses() ] } assert_ok(id_, expected, response.body) diff --git a/rhodecode/api/views/pull_request_api.py b/rhodecode/api/views/pull_request_api.py --- a/rhodecode/api/views/pull_request_api.py +++ b/rhodecode/api/views/pull_request_api.py @@ -463,7 +463,12 @@ def create_pull_request( :type description: Optional(str) :param reviewers: Set the new pull request reviewers list. :type reviewers: Optional(list) + Accepts username strings or objects of the format: + { + 'username': 'nick', 'reasons': ['original author'] + } """ + source = get_repo_or_error(source_repo) target = get_repo_or_error(target_repo) if not has_superadmin_permission(apiuser): @@ -490,12 +495,21 @@ def create_pull_request( if not ancestor: raise JSONRPCError('no common ancestor found') - reviewer_names = Optional.extract(reviewers) or [] - if not isinstance(reviewer_names, list): + reviewer_objects = Optional.extract(reviewers) or [] + if not isinstance(reviewer_objects, list): raise JSONRPCError('reviewers should be specified as a list') - reviewer_users = [get_user_or_error(n) for n in reviewer_names] - reviewer_ids = [u.user_id for u in reviewer_users] + reviewers_reasons = [] + for reviewer_object in reviewer_objects: + reviewer_reasons = [] + if isinstance(reviewer_object, (basestring, int)): + reviewer_username = reviewer_object + else: + reviewer_username = reviewer_object['username'] + reviewer_reasons = reviewer_object.get('reasons', []) + + user = get_user_or_error(reviewer_username) + reviewers_reasons.append((user.user_id, reviewer_reasons)) pull_request_model = PullRequestModel() pull_request = pull_request_model.create( @@ -506,7 +520,7 @@ def create_pull_request( target_ref=full_target_ref, revisions=reversed( [commit.raw_id for commit in reversed(commit_ranges)]), - reviewers=reviewer_ids, + reviewers=reviewers_reasons, title=title, description=Optional.extract(description) ) @@ -585,12 +599,23 @@ def update_pull_request( 'pull request `%s` update failed, pull request is closed' % ( pullrequestid,)) - reviewer_names = Optional.extract(reviewers) or [] - if not isinstance(reviewer_names, list): + reviewer_objects = Optional.extract(reviewers) or [] + if not isinstance(reviewer_objects, list): raise JSONRPCError('reviewers should be specified as a list') - reviewer_users = [get_user_or_error(n) for n in reviewer_names] - reviewer_ids = [u.user_id for u in reviewer_users] + reviewers_reasons = [] + reviewer_ids = set() + for reviewer_object in reviewer_objects: + reviewer_reasons = [] + if isinstance(reviewer_object, (int, basestring)): + reviewer_username = reviewer_object + else: + reviewer_username = reviewer_object['username'] + reviewer_reasons = reviewer_object.get('reasons', []) + + user = get_user_or_error(reviewer_username) + reviewer_ids.add(user.user_id) + reviewers_reasons.append((user.user_id, reviewer_reasons)) title = Optional.extract(title) description = Optional.extract(description) @@ -611,7 +636,7 @@ def update_pull_request( reviewers_changes = {"added": [], "removed": []} if reviewer_ids: added_reviewers, removed_reviewers = \ - PullRequestModel().update_reviewers(pull_request, reviewer_ids) + PullRequestModel().update_reviewers(pull_request, reviewers_reasons) reviewers_changes['added'] = sorted( [get_user_or_error(n).username for n in added_reviewers]) diff --git a/rhodecode/controllers/pullrequests.py b/rhodecode/controllers/pullrequests.py --- a/rhodecode/controllers/pullrequests.py +++ b/rhodecode/controllers/pullrequests.py @@ -22,6 +22,7 @@ pull requests controller for rhodecode for initializing pull requests """ +import peppercorn import formencode import logging @@ -399,8 +400,10 @@ class PullrequestsController(BaseRepoCon if not repo: raise HTTPNotFound + controls = peppercorn.parse(request.POST.items()) + try: - _form = PullRequestForm(repo.repo_id)().to_python(request.POST) + _form = PullRequestForm(repo.repo_id)().to_python(controls) except formencode.Invalid as errors: if errors.error_dict.get('revisions'): msg = 'Revisions: %s' % errors.error_dict['revisions'] @@ -419,7 +422,8 @@ class PullrequestsController(BaseRepoCon target_repo = _form['target_repo'] target_ref = _form['target_ref'] commit_ids = _form['revisions'][::-1] - reviewers = _form['review_members'] + reviewers = [ + (r['user_id'], r['reasons']) for r in _form['review_members']] # find the ancestor for this pr source_db_repo = Repository.get_by_repo_name(_form['source_repo']) @@ -478,8 +482,11 @@ class PullrequestsController(BaseRepoCon allowed_to_update = PullRequestModel().check_user_update( pull_request, c.rhodecode_user) if allowed_to_update: - if 'reviewers_ids' in request.POST: - self._update_reviewers(pull_request_id) + controls = peppercorn.parse(request.POST.items()) + + if 'review_members' in controls: + self._update_reviewers( + pull_request_id, controls['review_members']) elif str2bool(request.POST.get('update_commits', 'false')): self._update_commits(pull_request) elif str2bool(request.POST.get('close_pull_request', 'false')): @@ -631,11 +638,10 @@ class PullrequestsController(BaseRepoCon merge_resp.failure_reason) h.flash(msg, category='error') - def _update_reviewers(self, pull_request_id): - reviewers_ids = map(int, filter( - lambda v: v not in [None, ''], - request.POST.get('reviewers_ids', '').split(','))) - PullRequestModel().update_reviewers(pull_request_id, reviewers_ids) + def _update_reviewers(self, pull_request_id, review_members): + reviewers = [ + (int(r['user_id']), r['reasons']) for r in review_members] + PullRequestModel().update_reviewers(pull_request_id, reviewers) Session().commit() def _reject_close(self, pull_request): diff --git a/rhodecode/lib/dbmigrate/schema/db_4_5_0_0.py b/rhodecode/lib/dbmigrate/schema/db_4_5_0_0.py --- a/rhodecode/lib/dbmigrate/schema/db_4_5_0_0.py +++ b/rhodecode/lib/dbmigrate/schema/db_4_5_0_0.py @@ -59,7 +59,7 @@ from rhodecode.lib.utils2 import ( str2bool, safe_str, get_commit_safe, safe_unicode, remove_prefix, md5_safe, time_to_datetime, aslist, Optional, safe_int, get_clone_url, AttributeDict, glob2re) -from rhodecode.lib.jsonalchemy import MutationObj, JsonType, JSONDict +from rhodecode.lib.jsonalchemy import MutationObj, MutationList, JsonType, JSONDict from rhodecode.lib.ext_json import json from rhodecode.lib.caching_query import FromCache from rhodecode.lib.encrypt import AESCipher @@ -3149,9 +3149,10 @@ class PullRequest(Base, _PullRequestBase { 'user': reviewer.get_api_data(include_secrets=False, details='basic'), + 'reasons': reasons, 'review_status': st[0][1].status if st else 'not_reviewed', } - for reviewer, st in pull_request.reviewers_statuses() + for reviewer, reasons, st in pull_request.reviewers_statuses() ] } @@ -3203,9 +3204,23 @@ class PullRequestReviewers(Base, BaseMod 'mysql_charset': 'utf8', 'sqlite_autoincrement': True}, ) - def __init__(self, user=None, pull_request=None): + def __init__(self, user=None, pull_request=None, reasons=None): self.user = user self.pull_request = pull_request + self.reasons = reasons or [] + + @hybrid_property + def reasons(self): + if not self._reasons: + return [] + return self._reasons + + @reasons.setter + def reasons(self, val): + val = val or [] + if any(not isinstance(x, basestring) for x in val): + raise Exception('invalid reasons type, must be list of strings') + self._reasons = val pull_requests_reviewers_id = Column( 'pull_requests_reviewers_id', Integer(), nullable=False, @@ -3215,8 +3230,9 @@ class PullRequestReviewers(Base, BaseMod ForeignKey('pull_requests.pull_request_id'), nullable=False) user_id = Column( "user_id", Integer(), ForeignKey('users.user_id'), nullable=True) - reason = Column('reason', - UnicodeText().with_variant(UnicodeText(255), 'mysql'), nullable=True) + _reasons = Column( + 'reason', MutationList.as_mutable( + JsonType('list', dialect_map=dict(mysql=UnicodeText(16384))))) user = relationship('User') pull_request = relationship('PullRequest') diff --git a/rhodecode/lib/dbmigrate/versions/060_version_4_5_0.py b/rhodecode/lib/dbmigrate/versions/060_version_4_5_0.py --- a/rhodecode/lib/dbmigrate/versions/060_version_4_5_0.py +++ b/rhodecode/lib/dbmigrate/versions/060_version_4_5_0.py @@ -26,7 +26,7 @@ def upgrade(migrate_engine): _reset_base(migrate_engine) from rhodecode.lib.dbmigrate.schema import db_4_5_0_0 - db_4_5_0_0.PullRequestReviewers.reason.create( + db_4_5_0_0.PullRequestReviewers.reasons.create( table=db_4_5_0_0.PullRequestReviewers.__table__) def downgrade(migrate_engine): diff --git a/rhodecode/model/changeset_status.py b/rhodecode/model/changeset_status.py --- a/rhodecode/model/changeset_status.py +++ b/rhodecode/model/changeset_status.py @@ -80,7 +80,7 @@ class ChangesetStatusModel(BaseModel): """ votes = defaultdict(int) reviewers_number = len(statuses_by_reviewers) - for user, statuses in statuses_by_reviewers: + for user, reasons, statuses in statuses_by_reviewers: if statuses: ver, latest = statuses[0] votes[latest.status] += 1 @@ -254,7 +254,7 @@ class ChangesetStatusModel(BaseModel): for x, y in (itertools.groupby(sorted(st, key=version), version))] - pull_request_reviewers.append([o.user, st]) + pull_request_reviewers.append((o.user, o.reasons, st)) return pull_request_reviewers def calculated_review_status(self, pull_request, reviewers_statuses=None): diff --git a/rhodecode/model/db.py b/rhodecode/model/db.py --- a/rhodecode/model/db.py +++ b/rhodecode/model/db.py @@ -59,7 +59,7 @@ from rhodecode.lib.utils2 import ( str2bool, safe_str, get_commit_safe, safe_unicode, remove_prefix, md5_safe, time_to_datetime, aslist, Optional, safe_int, get_clone_url, AttributeDict, glob2re) -from rhodecode.lib.jsonalchemy import MutationObj, JsonType, JSONDict +from rhodecode.lib.jsonalchemy import MutationObj, MutationList, JsonType, JSONDict from rhodecode.lib.ext_json import json from rhodecode.lib.caching_query import FromCache from rhodecode.lib.encrypt import AESCipher @@ -3149,9 +3149,10 @@ class PullRequest(Base, _PullRequestBase { 'user': reviewer.get_api_data(include_secrets=False, details='basic'), + 'reasons': reasons, 'review_status': st[0][1].status if st else 'not_reviewed', } - for reviewer, st in pull_request.reviewers_statuses() + for reviewer, reasons, st in pull_request.reviewers_statuses() ] } @@ -3203,9 +3204,23 @@ class PullRequestReviewers(Base, BaseMod 'mysql_charset': 'utf8', 'sqlite_autoincrement': True}, ) - def __init__(self, user=None, pull_request=None): + def __init__(self, user=None, pull_request=None, reasons=None): self.user = user self.pull_request = pull_request + self.reasons = reasons or [] + + @hybrid_property + def reasons(self): + if not self._reasons: + return [] + return self._reasons + + @reasons.setter + def reasons(self, val): + val = val or [] + if any(not isinstance(x, basestring) for x in val): + raise Exception('invalid reasons type, must be list of strings') + self._reasons = val pull_requests_reviewers_id = Column( 'pull_requests_reviewers_id', Integer(), nullable=False, @@ -3215,8 +3230,9 @@ class PullRequestReviewers(Base, BaseMod ForeignKey('pull_requests.pull_request_id'), nullable=False) user_id = Column( "user_id", Integer(), ForeignKey('users.user_id'), nullable=True) - reason = Column('reason', - UnicodeText().with_variant(UnicodeText(255), 'mysql'), nullable=True) + _reasons = Column( + 'reason', MutationList.as_mutable( + JsonType('list', dialect_map=dict(mysql=UnicodeText(16384))))) user = relationship('User') pull_request = relationship('PullRequest') diff --git a/rhodecode/model/forms.py b/rhodecode/model/forms.py --- a/rhodecode/model/forms.py +++ b/rhodecode/model/forms.py @@ -519,7 +519,12 @@ def UserExtraIpForm(): return _UserExtraIpForm + def PullRequestForm(repo_id): + class ReviewerForm(formencode.Schema): + user_id = v.Int(not_empty=True) + reasons = All() + class _PullRequestForm(formencode.Schema): allow_extra_fields = True filter_extra_fields = True @@ -531,8 +536,7 @@ def PullRequestForm(repo_id): target_ref = v.UnicodeString(strip=True, required=True) revisions = All(#v.NotReviewedRevisions(repo_id)(), v.UniqueList()(not_empty=True)) - review_members = v.UniqueList(convert=int)(not_empty=True) - + review_members = formencode.ForEach(ReviewerForm()) pullrequest_title = v.UnicodeString(strip=True, required=True) pullrequest_desc = v.UnicodeString(strip=True, required=False) diff --git a/rhodecode/model/pull_request.py b/rhodecode/model/pull_request.py --- a/rhodecode/model/pull_request.py +++ b/rhodecode/model/pull_request.py @@ -333,10 +333,18 @@ class PullRequestModel(BaseModel): Session().add(pull_request) Session().flush() + reviewer_ids = set() # members / reviewers - for user_id in set(reviewers): + for reviewer_object in reviewers: + if isinstance(reviewer_object, tuple): + user_id, reasons = reviewer_object + else: + user_id, reasons = reviewer_object, [] + user = self._get_user(user_id) - reviewer = PullRequestReviewers(user, pull_request) + reviewer_ids.add(user.user_id) + + reviewer = PullRequestReviewers(user, pull_request, reasons) Session().add(reviewer) # Set approval status to "Under Review" for all commits which are @@ -348,7 +356,7 @@ class PullRequestModel(BaseModel): pull_request=pull_request ) - self.notify_reviewers(pull_request, reviewers) + self.notify_reviewers(pull_request, reviewer_ids) self._trigger_pull_request_hook( pull_request, created_by_user, 'create') @@ -570,6 +578,7 @@ class PullRequestModel(BaseModel): Session().commit() self._trigger_pull_request_hook(pull_request, pull_request.author, 'update') + return (pull_request_version, changes) def _create_version_from_snapshot(self, pull_request): @@ -711,8 +720,21 @@ class PullRequestModel(BaseModel): pull_request.updated_on = datetime.datetime.now() Session().add(pull_request) - def update_reviewers(self, pull_request, reviewers_ids): - reviewers_ids = set(reviewers_ids) + def update_reviewers(self, pull_request, reviewer_data): + """ + Update the reviewers in the pull request + + :param pull_request: the pr to update + :param reviewer_data: list of tuples [(user, ['reason1', 'reason2'])] + """ + + reviewers_reasons = {} + for user_id, reasons in reviewer_data: + if isinstance(user_id, (int, basestring)): + user_id = self._get_user(user_id).user_id + reviewers_reasons[user_id] = reasons + + reviewers_ids = set(reviewers_reasons.keys()) pull_request = self.__get_pull_request(pull_request) current_reviewers = PullRequestReviewers.query()\ .filter(PullRequestReviewers.pull_request == @@ -728,7 +750,8 @@ class PullRequestModel(BaseModel): for uid in ids_to_add: changed = True _usr = self._get_user(uid) - reviewer = PullRequestReviewers(_usr, pull_request) + reasons = reviewers_reasons[uid] + reviewer = PullRequestReviewers(_usr, pull_request, reasons) Session().add(reviewer) self.notify_reviewers(pull_request, ids_to_add) diff --git a/rhodecode/public/css/main.less b/rhodecode/public/css/main.less --- a/rhodecode/public/css/main.less +++ b/rhodecode/public/css/main.less @@ -145,7 +145,7 @@ input.inline[type="file"] { .alert { margin: @padding 0; } - + .error-branding { font-family: @text-semibold; color: @grey4; @@ -1282,6 +1282,9 @@ table.integrations { width: 100%; overflow: auto; } +.reviewer_reason { + padding-left: 20px; +} .reviewer_status { display: inline-block; vertical-align: top; diff --git a/rhodecode/public/js/src/rhodecode/pullrequests.js b/rhodecode/public/js/src/rhodecode/pullrequests.js --- a/rhodecode/public/js/src/rhodecode/pullrequests.js +++ b/rhodecode/public/js/src/rhodecode/pullrequests.js @@ -32,7 +32,7 @@ var removeReviewMember = function(review var obj = $('#reviewer_{0}_name'.format(reviewer_id)); obj.addClass('to-delete'); // now delete the input - $('#reviewer_{0}_input'.format(reviewer_id)).remove(); + $('#reviewer_{0} input'.format(reviewer_id)).remove(); } } else{ @@ -43,29 +43,36 @@ var removeReviewMember = function(review var addReviewMember = function(id, fname, lname, nname, gravatar_link, reasons) { var members = $('#review_members').get(0); var reasons_html = ''; + var reasons_inputs = ''; + var reasons = reasons || []; if (reasons) { for (var i = 0; i < reasons.length; i++) { - reasons_html += '
- {0}
'.format( - reasons[i] - ); + reasons_html += '
- {0}
'.format(reasons[i]); + reasons_inputs += ''; } } var tmpl = '
  • '+ + ''+ '
    '+ '
    '+ '
    '+ 'gravatar'+ '{1}'+ reasons_html + - ''+ + ''+ + ''+ + '{3}'+ + ''+ '
    ' + ''+ '
    '+ ''+ + ''+ '
    • ' ; + var displayname = "{0} ({1} {2})".format( nname, escapeHtml(fname), escapeHtml(lname)); - var element = tmpl.format(gravatar_link,displayname,id); + var element = tmpl.format(gravatar_link,displayname,id,reasons_inputs); // check if we don't have this ID already in var ids = []; var _els = $('#review_members li').toArray(); @@ -83,7 +90,11 @@ var _updatePullRequest = function(repo_n var url = pyroutes.url( 'pullrequest_update', {"repo_name": repo_name, "pull_request_id": pull_request_id}); - postData.csrf_token = CSRF_TOKEN; + if (typeof postData === 'string' ) { + postData += '&csrf_token=' + CSRF_TOKEN; + } else { + postData.csrf_token = CSRF_TOKEN; + } var success = function(o) { window.location.reload(); }; @@ -92,17 +103,9 @@ var _updatePullRequest = function(repo_n var updateReviewers = function(reviewers_ids, repo_name, pull_request_id){ if (reviewers_ids === undefined){ - var reviewers_ids = []; - var ids = $('#review_members input').toArray(); - for(var i=0; i
    ## members goes here, filled via JS based on initial selection ! +
      +
      ${h.text('user', class_='ac-input', placeholder=_('Add reviewer'))} diff --git a/rhodecode/templates/pullrequests/pullrequest_show.html b/rhodecode/templates/pullrequests/pullrequest_show.html --- a/rhodecode/templates/pullrequests/pullrequest_show.html +++ b/rhodecode/templates/pullrequests/pullrequest_show.html @@ -188,17 +188,27 @@
      ## members goes here ! +
        - %for member,status in c.pull_request_reviewers: + %for member,reasons,status in c.pull_request_reviewers:
      • - ${self.gravatar_with_user(member.email, 16)}
        (${_('owner') if c.pull_request.user_id == member.user_id else _('reviewer')})
        + ${self.gravatar_with_user(member.email, 16)}
        - + + + %for reason in reasons: +
        - ${reason}
        + + + %endfor + + + %if c.allowed_to_update:
        • %endfor
      + %if not c.pull_request.is_closed():