diff --git a/rhodecode/__init__.py b/rhodecode/__init__.py --- a/rhodecode/__init__.py +++ b/rhodecode/__init__.py @@ -51,7 +51,7 @@ PYRAMID_SETTINGS = {} EXTENSIONS = {} __version__ = ('.'.join((str(each) for each in VERSION[:3]))) -__dbversion__ = 73 # defines current db version for migrations +__dbversion__ = 77 # defines current db version for migrations __platform__ = platform.system() __license__ = 'AGPLv3, and Commercial License' __author__ = 'RhodeCode GmbH' diff --git a/rhodecode/api/tests/test_create_pull_request.py b/rhodecode/api/tests/test_create_pull_request.py --- a/rhodecode/api/tests/test_create_pull_request.py +++ b/rhodecode/api/tests/test_create_pull_request.py @@ -98,7 +98,12 @@ class TestCreatePullRequestApi(object): def test_create_with_reviewers_specified_by_names( self, backend, no_notifications): data = self._prepare_data(backend) - reviewers = [TEST_USER_REGULAR_LOGIN, TEST_USER_ADMIN_LOGIN] + reviewers = [ + {'username': TEST_USER_REGULAR_LOGIN, + 'reasons': ['added manually']}, + {'username': TEST_USER_ADMIN_LOGIN, + 'reasons': ['added manually']}, + ] data['reviewers'] = reviewers id_, params = build_data( self.apikey_regular, 'create_pull_request', **data) @@ -110,16 +115,26 @@ class TestCreatePullRequestApi(object): assert result['result']['msg'] == expected_message pull_request_id = result['result']['pull_request_id'] pull_request = PullRequestModel().get(pull_request_id) - actual_reviewers = [r.user.username for r in pull_request.reviewers] + actual_reviewers = [ + {'username': r.user.username, + 'reasons': ['added manually'], + } for r in pull_request.reviewers + ] assert sorted(actual_reviewers) == sorted(reviewers) @pytest.mark.backends("git", "hg") def test_create_with_reviewers_specified_by_ids( self, backend, no_notifications): data = self._prepare_data(backend) - reviewer_names = [TEST_USER_REGULAR_LOGIN, TEST_USER_ADMIN_LOGIN] reviewers = [ - UserModel().get_by_username(n).user_id for n in reviewer_names] + {'username': UserModel().get_by_username( + TEST_USER_REGULAR_LOGIN).user_id, + 'reasons': ['added manually']}, + {'username': UserModel().get_by_username( + TEST_USER_ADMIN_LOGIN).user_id, + 'reasons': ['added manually']}, + ] + data['reviewers'] = reviewers id_, params = build_data( self.apikey_regular, 'create_pull_request', **data) @@ -131,14 +146,17 @@ class TestCreatePullRequestApi(object): assert result['result']['msg'] == expected_message pull_request_id = result['result']['pull_request_id'] pull_request = PullRequestModel().get(pull_request_id) - actual_reviewers = [r.user.username for r in pull_request.reviewers] - assert sorted(actual_reviewers) == sorted(reviewer_names) + actual_reviewers = [ + {'username': r.user.user_id, + 'reasons': ['added manually'], + } for r in pull_request.reviewers + ] + assert sorted(actual_reviewers) == sorted(reviewers) @pytest.mark.backends("git", "hg") def test_create_fails_when_the_reviewer_is_not_found(self, backend): data = self._prepare_data(backend) - reviewers = ['somebody'] - data['reviewers'] = reviewers + data['reviewers'] = [{'username': 'somebody'}] id_, params = build_data( self.apikey_regular, 'create_pull_request', **data) response = api_call(self.app, params) @@ -153,7 +171,7 @@ class TestCreatePullRequestApi(object): id_, params = build_data( self.apikey_regular, 'create_pull_request', **data) response = api_call(self.app, params) - expected_message = 'reviewers should be specified as a list' + expected_message = {u'': '"test_regular,test_admin" is not iterable'} assert_error(id_, expected_message, given=response.body) @pytest.mark.backends("git", "hg") 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 @@ -109,7 +109,8 @@ class TestGetPullRequest(object): 'reasons': reasons, 'review_status': st[0][1].status if st else 'not_reviewed', } - for reviewer, reasons, st in pull_request.reviewers_statuses() + for reviewer, reasons, mandatory, st in + pull_request.reviewers_statuses() ] } assert_ok(id_, expected, response.body) diff --git a/rhodecode/api/tests/test_update_pull_request.py b/rhodecode/api/tests/test_update_pull_request.py --- a/rhodecode/api/tests/test_update_pull_request.py +++ b/rhodecode/api/tests/test_update_pull_request.py @@ -119,20 +119,28 @@ class TestUpdatePullRequest(object): @pytest.mark.backends("git", "hg") def test_api_update_change_reviewers( - self, pr_util, silence_action_logger, no_notifications): + self, user_util, pr_util, silence_action_logger, no_notifications): + a = user_util.create_user() + b = user_util.create_user() + c = user_util.create_user() + new_reviewers = [ + {'username': b.username,'reasons': ['updated via API'], + 'mandatory':False}, + {'username': c.username, 'reasons': ['updated via API'], + 'mandatory':False}, + ] - users = [x.username for x in User.get_all()] - new = [users.pop(0)] - removed = sorted(new) - added = sorted(users) + added = [b.username, c.username] + removed = [a.username] - pull_request = pr_util.create_pull_request(reviewers=new) + pull_request = pr_util.create_pull_request( + reviewers=[(a.username, ['added via API'], False)]) id_, params = build_data( self.apikey, 'update_pull_request', repoid=pull_request.target_repo.repo_name, pullrequestid=pull_request.pull_request_id, - reviewers=added) + reviewers=new_reviewers) response = api_call(self.app, params) expected = { "msg": "Updated pull request `{}`".format( @@ -152,7 +160,7 @@ class TestUpdatePullRequest(object): self.apikey, 'update_pull_request', repoid=pull_request.target_repo.repo_name, pullrequestid=pull_request.pull_request_id, - reviewers=['bad_name']) + reviewers=[{'username': 'bad_name'}]) response = api_call(self.app, params) expected = 'user `bad_name` does not exist' @@ -165,7 +173,7 @@ class TestUpdatePullRequest(object): self.apikey, 'update_pull_request', repoid='fake', pullrequestid='fake', - reviewers=['bad_name']) + reviewers=[{'username': 'bad_name'}]) response = api_call(self.app, params) expected = 'repository `fake` does not exist' @@ -181,7 +189,7 @@ class TestUpdatePullRequest(object): self.apikey, 'update_pull_request', repoid=pull_request.target_repo.repo_name, pullrequestid=999999, - reviewers=['bad_name']) + reviewers=[{'username': 'bad_name'}]) response = api_call(self.app, params) expected = 'pull request `999999` does not exist' 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 @@ -21,7 +21,7 @@ import logging -from rhodecode.api import jsonrpc_method, JSONRPCError +from rhodecode.api import jsonrpc_method, JSONRPCError, JSONRPCValidationError from rhodecode.api.utils import ( has_superadmin_permission, Optional, OAttr, get_repo_or_error, get_pull_request_or_error, get_commit_or_error, get_user_or_error, @@ -34,6 +34,9 @@ from rhodecode.model.comment import Comm from rhodecode.model.db import Session, ChangesetStatus, ChangesetComment from rhodecode.model.pull_request import PullRequestModel, MergeCheck from rhodecode.model.settings import SettingsModel +from rhodecode.model.validation_schema import Invalid +from rhodecode.model.validation_schema.schemas.reviewer_schema import \ + ReviewerListSchema log = logging.getLogger(__name__) @@ -537,7 +540,7 @@ def create_pull_request( :type reviewers: Optional(list) Accepts username strings or objects of the format: - {'username': 'nick', 'reasons': ['original author']} + {'username': 'nick', 'reasons': ['original author'], 'mandatory': } """ source = get_repo_or_error(source_repo) @@ -567,20 +570,19 @@ def create_pull_request( raise JSONRPCError('no common ancestor found') reviewer_objects = Optional.extract(reviewers) or [] - if not isinstance(reviewer_objects, list): - raise JSONRPCError('reviewers should be specified as a list') + if reviewer_objects: + schema = ReviewerListSchema() + try: + reviewer_objects = schema.deserialize(reviewer_objects) + except Invalid as err: + raise JSONRPCValidationError(colander_exc=err) - reviewers_reasons = [] + reviewers = [] 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)) + user = get_user_or_error(reviewer_object['username']) + reasons = reviewer_object['reasons'] + mandatory = reviewer_object['mandatory'] + reviewers.append((user.user_id, reasons, mandatory)) pull_request_model = PullRequestModel() pull_request = pull_request_model.create( @@ -591,7 +593,7 @@ def create_pull_request( target_ref=full_target_ref, revisions=reversed( [commit.raw_id for commit in reversed(commit_ranges)]), - reviewers=reviewers_reasons, + reviewers=reviewers, title=title, description=Optional.extract(description) ) @@ -624,6 +626,10 @@ def update_pull_request( :type description: Optional(str) :param reviewers: Update pull request reviewers list with new value. :type reviewers: Optional(list) + Accepts username strings or objects of the format: + + {'username': 'nick', 'reasons': ['original author'], 'mandatory': } + :param update_commits: Trigger update of commits for this pull request :type: update_commits: Optional(bool) :param close_pull_request: Close this pull request with rejected state @@ -669,23 +675,21 @@ def update_pull_request( 'pull request `%s` update failed, pull request is closed' % ( pullrequestid,)) + reviewer_objects = Optional.extract(reviewers) or [] - if not isinstance(reviewer_objects, list): - raise JSONRPCError('reviewers should be specified as a list') + if reviewer_objects: + schema = ReviewerListSchema() + try: + reviewer_objects = schema.deserialize(reviewer_objects) + except Invalid as err: + raise JSONRPCValidationError(colander_exc=err) - reviewers_reasons = [] - reviewer_ids = set() + reviewers = [] 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)) + user = get_user_or_error(reviewer_object['username']) + reasons = reviewer_object['reasons'] + mandatory = reviewer_object['mandatory'] + reviewers.append((user.user_id, reasons, mandatory)) title = Optional.extract(title) description = Optional.extract(description) @@ -704,9 +708,9 @@ def update_pull_request( Session().commit() reviewers_changes = {"added": [], "removed": []} - if reviewer_ids: + if reviewers: added_reviewers, removed_reviewers = \ - PullRequestModel().update_reviewers(pull_request, reviewers_reasons) + PullRequestModel().update_reviewers(pull_request, reviewers) reviewers_changes['added'] = sorted( [get_user_or_error(n).username for n in added_reviewers]) diff --git a/rhodecode/apps/_base/__init__.py b/rhodecode/apps/_base/__init__.py --- a/rhodecode/apps/_base/__init__.py +++ b/rhodecode/apps/_base/__init__.py @@ -290,6 +290,13 @@ class RepoTypeRoutePredicate(object): else: log.warning('Current view is not supported for repo type:%s', rhodecode_db_repo.repo_type) + # + # h.flash(h.literal( + # _('Action not supported for %s.' % rhodecode_repo.alias)), + # category='warning') + # return redirect( + # url('summary_home', repo_name=cls.rhodecode_db_repo.repo_name)) + return False diff --git a/rhodecode/apps/repository/tests/test_pull_requests_list.py b/rhodecode/apps/repository/tests/test_pull_requests_list.py new file mode 100644 --- /dev/null +++ b/rhodecode/apps/repository/tests/test_pull_requests_list.py @@ -0,0 +1,83 @@ +# -*- coding: utf-8 -*- + +# Copyright (C) 2010-2017 RhodeCode GmbH +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License, version 3 +# (only), as published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +# +# This program is dual-licensed. If you wish to learn more about the +# RhodeCode Enterprise Edition, including its added features, Support services, +# and proprietary license terms, please see https://rhodecode.com/licenses/ + +import pytest +from rhodecode.model.db import Repository + + +def route_path(name, params=None, **kwargs): + import urllib + + base_url = { + 'pullrequest_show_all': '/{repo_name}/pull-request', + 'pullrequest_show_all_data': '/{repo_name}/pull-request-data', + }[name].format(**kwargs) + + if params: + base_url = '{}?{}'.format(base_url, urllib.urlencode(params)) + return base_url + + +@pytest.mark.backends("git", "hg") +@pytest.mark.usefixtures('autologin_user', 'app') +class TestPullRequestList(object): + + @pytest.mark.parametrize('params, expected_title', [ + ({'source': 0, 'closed': 1}, 'Closed Pull Requests'), + ({'source': 0, 'my': 1}, 'opened by me'), + ({'source': 0, 'awaiting_review': 1}, 'awaiting review'), + ({'source': 0, 'awaiting_my_review': 1}, 'awaiting my review'), + ({'source': 1}, 'Pull Requests from'), + ]) + def test_showing_list_page(self, backend, pr_util, params, expected_title): + pull_request = pr_util.create_pull_request() + + response = self.app.get( + route_path('pullrequest_show_all', + repo_name=pull_request.target_repo.repo_name, + params=params)) + + assert_response = response.assert_response() + assert_response.element_equals_to('.panel-title', expected_title) + element = assert_response.get_element('.panel-title') + element_text = assert_response._element_to_string(element) + + def test_showing_list_page_data(self, backend, pr_util, xhr_header): + pull_request = pr_util.create_pull_request() + response = self.app.get( + route_path('pullrequest_show_all_data', + repo_name=pull_request.target_repo.repo_name), + extra_environ=xhr_header) + + assert response.json['recordsTotal'] == 1 + assert response.json['data'][0]['description'] == 'Description' + + def test_description_is_escaped_on_index_page(self, backend, pr_util, xhr_header): + xss_description = "" + pull_request = pr_util.create_pull_request(description=xss_description) + + response = self.app.get( + route_path('pullrequest_show_all_data', + repo_name=pull_request.target_repo.repo_name), + extra_environ=xhr_header) + + assert response.json['recordsTotal'] == 1 + assert response.json['data'][0]['description'] == \ + "<script>alert('Hi!')</script>" diff --git a/rhodecode/apps/repository/utils.py b/rhodecode/apps/repository/utils.py new file mode 100644 --- /dev/null +++ b/rhodecode/apps/repository/utils.py @@ -0,0 +1,76 @@ +# -*- coding: utf-8 -*- + +# Copyright (C) 2016-2017 RhodeCode GmbH +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License, version 3 +# (only), as published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +# +# This program is dual-licensed. If you wish to learn more about the +# RhodeCode Enterprise Edition, including its added features, Support services, +# and proprietary license terms, please see https://rhodecode.com/licenses/ + +from rhodecode.lib import helpers as h +from rhodecode.lib.utils2 import safe_int + + +def reviewer_as_json(user, reasons, mandatory): + """ + Returns json struct of a reviewer for frontend + + :param user: the reviewer + :param reasons: list of strings of why they are reviewers + :param mandatory: bool, to set user as mandatory + """ + + return { + 'user_id': user.user_id, + 'reasons': reasons, + 'mandatory': mandatory, + 'username': user.username, + 'firstname': user.firstname, + 'lastname': user.lastname, + 'gravatar_link': h.gravatar_url(user.email, 14), + } + + +def get_default_reviewers_data( + current_user, source_repo, source_commit, target_repo, target_commit): + + """ Return json for default reviewers of a repository """ + + reasons = ['Default reviewer', 'Repository owner'] + default = reviewer_as_json( + user=current_user, reasons=reasons, mandatory=False) + + return { + 'api_ver': 'v1', # define version for later possible schema upgrade + 'reviewers': [default], + 'rules': {}, + 'rules_data': {}, + } + + +def validate_default_reviewers(review_members, reviewer_rules): + """ + Function to validate submitted reviewers against the saved rules + + """ + reviewers = [] + reviewer_by_id = {} + for r in review_members: + reviewer_user_id = safe_int(r['user_id']) + entry = (reviewer_user_id, r['reasons'], r['mandatory']) + + reviewer_by_id[reviewer_user_id] = entry + reviewers.append(entry) + + return reviewers diff --git a/rhodecode/apps/repository/views/repo_review_rules.py b/rhodecode/apps/repository/views/repo_review_rules.py --- a/rhodecode/apps/repository/views/repo_review_rules.py +++ b/rhodecode/apps/repository/views/repo_review_rules.py @@ -23,7 +23,7 @@ import logging from pyramid.view import view_config from rhodecode.apps._base import RepoAppView -from rhodecode.controllers import utils +from rhodecode.apps.repository.utils import get_default_reviewers_data from rhodecode.lib.auth import LoginRequired, HasRepoPermissionAnyDecorator log = logging.getLogger(__name__) @@ -57,13 +57,8 @@ class RepoReviewRulesView(RepoAppView): route_name='repo_default_reviewers_data', request_method='GET', renderer='json_ext') def repo_default_reviewers_data(self): - reasons = ['Default reviewer', 'Repository owner'] - default = utils.reviewer_as_json( - user=self.db_repo.user, reasons=reasons, mandatory=False) + review_data = get_default_reviewers_data( + self.db_repo.user, None, None, None, None) + return review_data - return { - 'api_ver': 'v1', # define version for later possible schema upgrade - 'reviewers': [default], - 'rules': {}, - 'rules_data': {}, - } + diff --git a/rhodecode/controllers/pullrequests.py b/rhodecode/controllers/pullrequests.py --- a/rhodecode/controllers/pullrequests.py +++ b/rhodecode/controllers/pullrequests.py @@ -228,8 +228,6 @@ class PullrequestsController(BaseRepoCon target_repo = _form['target_repo'] target_ref = _form['target_ref'] commit_ids = _form['revisions'][::-1] - 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']) @@ -257,17 +255,29 @@ class PullrequestsController(BaseRepoCon ) description = _form['pullrequest_desc'] + + get_default_reviewers_data, validate_default_reviewers = \ + PullRequestModel().get_reviewer_functions() + + # recalculate reviewers logic, to make sure we can validate this + reviewer_rules = get_default_reviewers_data( + c.rhodecode_user, source_db_repo, source_commit, target_db_repo, + target_commit) + + reviewers = validate_default_reviewers( + _form['review_members'], reviewer_rules) + try: pull_request = PullRequestModel().create( c.rhodecode_user.user_id, source_repo, source_ref, target_repo, target_ref, commit_ids, reviewers, pullrequest_title, - description + description, reviewer_rules ) Session().commit() h.flash(_('Successfully opened new pull request'), category='success') except Exception as e: - msg = _('Error occurred during sending pull request') + msg = _('Error occurred during creation of this pull request.') log.exception(msg) h.flash(msg, category='error') return redirect(url('pullrequest_home', repo_name=repo_name)) @@ -292,7 +302,8 @@ class PullrequestsController(BaseRepoCon if 'review_members' in controls: self._update_reviewers( - pull_request_id, controls['review_members']) + pull_request_id, controls['review_members'], + pull_request.reviewer_data) elif str2bool(request.POST.get('update_commits', 'false')): self._update_commits(pull_request) elif str2bool(request.POST.get('close_pull_request', 'false')): @@ -435,10 +446,20 @@ class PullrequestsController(BaseRepoCon merge_resp.failure_reason) h.flash(msg, category='error') - def _update_reviewers(self, pull_request_id, review_members): - reviewers = [ - (int(r['user_id']), r['reasons']) for r in review_members] + def _update_reviewers(self, pull_request_id, review_members, reviewer_rules): + + get_default_reviewers_data, validate_default_reviewers = \ + PullRequestModel().get_reviewer_functions() + + try: + reviewers = validate_default_reviewers(review_members, reviewer_rules) + except ValueError as e: + log.error('Reviewers Validation:{}'.format(e)) + h.flash(e, category='error') + return + PullRequestModel().update_reviewers(pull_request_id, reviewers) + h.flash(_('Pull request reviewers updated.'), category='success') Session().commit() def _reject_close(self, pull_request): @@ -490,7 +511,8 @@ class PullrequestsController(BaseRepoCon _org_pull_request_obj = pull_request_ver.pull_request at_version = pull_request_ver.pull_request_version_id else: - _org_pull_request_obj = pull_request_obj = PullRequest.get_or_404(pull_request_id) + _org_pull_request_obj = pull_request_obj = PullRequest.get_or_404( + pull_request_id) pull_request_display_obj = PullRequest.get_pr_display_object( pull_request_obj, _org_pull_request_obj) @@ -597,9 +619,9 @@ class PullrequestsController(BaseRepoCon c.allowed_to_comment = False c.allowed_to_close = False else: - c.allowed_to_change_status = PullRequestModel(). \ - check_user_change_status(pull_request_at_ver, c.rhodecode_user) \ - and not pr_closed + can_change_status = PullRequestModel().check_user_change_status( + pull_request_at_ver, c.rhodecode_user) + c.allowed_to_change_status = can_change_status and not pr_closed c.allowed_to_update = PullRequestModel().check_user_update( pull_request_latest, c.rhodecode_user) and not pr_closed @@ -610,6 +632,18 @@ class PullrequestsController(BaseRepoCon c.allowed_to_comment = not pr_closed c.allowed_to_close = c.allowed_to_merge and not pr_closed + c.forbid_adding_reviewers = False + c.forbid_author_to_review = False + + if pull_request_latest.reviewer_data and \ + 'rules' in pull_request_latest.reviewer_data: + rules = pull_request_latest.reviewer_data['rules'] or {} + try: + c.forbid_adding_reviewers = rules.get('forbid_adding_reviewers') + c.forbid_author_to_review = rules.get('forbid_author_to_review') + except Exception: + pass + # check merge capabilities _merge_check = MergeCheck.validate( pull_request_latest, user=c.rhodecode_user) @@ -724,12 +758,18 @@ class PullrequestsController(BaseRepoCon c.commit_ranges.append(comm) commit_cache[comm.raw_id] = comm + # Order here matters, we first need to get target, and then + # the source target_commit = commits_source_repo.get_commit( commit_id=safe_str(target_ref_id)) + source_commit = commits_source_repo.get_commit( commit_id=safe_str(source_ref_id)) + except CommitDoesNotExistError: - pass + log.warning( + 'Failed to get commit from `{}` repo'.format( + commits_source_repo), exc_info=True) except RepositoryRequirementError: log.warning( 'Failed to get all required data from repo', exc_info=True) diff --git a/rhodecode/controllers/utils.py b/rhodecode/controllers/utils.py --- a/rhodecode/controllers/utils.py +++ b/rhodecode/controllers/utils.py @@ -87,23 +87,3 @@ def get_commit_from_ref_name(repo, ref_n '%s "%s" does not exist' % (ref_type, ref_name)) return repo_scm.get_commit(commit_id) - - -def reviewer_as_json(user, reasons, mandatory): - """ - Returns json struct of a reviewer for frontend - - :param user: the reviewer - :param reasons: list of strings of why they are reviewers - :param mandatory: bool, to set user as mandatory - """ - - return { - 'user_id': user.user_id, - 'reasons': reasons, - 'mandatory': mandatory, - 'username': user.username, - 'firstname': user.firstname, - 'lastname': user.lastname, - 'gravatar_link': h.gravatar_url(user.email, 14), - } diff --git a/rhodecode/lib/dbmigrate/versions/074_version_4_8_0.py b/rhodecode/lib/dbmigrate/versions/074_version_4_8_0.py new file mode 100644 --- /dev/null +++ b/rhodecode/lib/dbmigrate/versions/074_version_4_8_0.py @@ -0,0 +1,37 @@ +import logging + +from sqlalchemy import * +from rhodecode.model import meta +from rhodecode.lib.dbmigrate.versions import _reset_base, notify + +log = logging.getLogger(__name__) + + +def upgrade(migrate_engine): + """ + Upgrade operations go here. + Don't create your own engine; bind migrate_engine to your metadata + """ + _reset_base(migrate_engine) + from rhodecode.lib.dbmigrate.schema import db_4_7_0_1 as db + + repo_review_rule_table = db.RepoReviewRule.__table__ + + forbid_author_to_review = Column( + "forbid_author_to_review", Boolean(), nullable=True, default=False) + forbid_author_to_review.create(table=repo_review_rule_table) + + forbid_adding_reviewers = Column( + "forbid_adding_reviewers", Boolean(), nullable=True, default=False) + forbid_adding_reviewers.create(table=repo_review_rule_table) + + fixups(db, meta.Session) + + +def downgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + +def fixups(models, _SESSION): + pass diff --git a/rhodecode/lib/dbmigrate/versions/075_version_4_8_0.py b/rhodecode/lib/dbmigrate/versions/075_version_4_8_0.py new file mode 100644 --- /dev/null +++ b/rhodecode/lib/dbmigrate/versions/075_version_4_8_0.py @@ -0,0 +1,38 @@ +import logging + +from sqlalchemy import * +from rhodecode.model import meta +from rhodecode.lib.dbmigrate.versions import _reset_base, notify + +log = logging.getLogger(__name__) + + +def upgrade(migrate_engine): + """ + Upgrade operations go here. + Don't create your own engine; bind migrate_engine to your metadata + """ + _reset_base(migrate_engine) + from rhodecode.lib.dbmigrate.schema import db_4_7_0_1 as db + + repo_review_rule_user_table = db.RepoReviewRuleUser.__table__ + repo_review_rule_user_group_table = db.RepoReviewRuleUserGroup.__table__ + + mandatory_user = Column( + "mandatory", Boolean(), nullable=True, default=False) + mandatory_user.create(table=repo_review_rule_user_table) + + mandatory_user_group = Column( + "mandatory", Boolean(), nullable=True, default=False) + mandatory_user_group.create(table=repo_review_rule_user_group_table) + + fixups(db, meta.Session) + + +def downgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + +def fixups(models, _SESSION): + pass diff --git a/rhodecode/lib/dbmigrate/versions/076_version_4_8_0.py b/rhodecode/lib/dbmigrate/versions/076_version_4_8_0.py new file mode 100644 --- /dev/null +++ b/rhodecode/lib/dbmigrate/versions/076_version_4_8_0.py @@ -0,0 +1,33 @@ +import logging + +from sqlalchemy import * +from rhodecode.model import meta +from rhodecode.lib.dbmigrate.versions import _reset_base, notify + +log = logging.getLogger(__name__) + + +def upgrade(migrate_engine): + """ + Upgrade operations go here. + Don't create your own engine; bind migrate_engine to your metadata + """ + _reset_base(migrate_engine) + from rhodecode.lib.dbmigrate.schema import db_4_7_0_1 as db + + pull_request_reviewers = db.PullRequestReviewers.__table__ + + mandatory = Column( + "mandatory", Boolean(), nullable=True, default=False) + mandatory.create(table=pull_request_reviewers) + + fixups(db, meta.Session) + + +def downgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + +def fixups(models, _SESSION): + pass diff --git a/rhodecode/lib/dbmigrate/versions/077_version_4_8_0.py b/rhodecode/lib/dbmigrate/versions/077_version_4_8_0.py new file mode 100644 --- /dev/null +++ b/rhodecode/lib/dbmigrate/versions/077_version_4_8_0.py @@ -0,0 +1,40 @@ +import logging + +from sqlalchemy import * +from rhodecode.model import meta +from rhodecode.lib.dbmigrate.versions import _reset_base, notify + +log = logging.getLogger(__name__) + + +def upgrade(migrate_engine): + """ + Upgrade operations go here. + Don't create your own engine; bind migrate_engine to your metadata + """ + _reset_base(migrate_engine) + from rhodecode.lib.dbmigrate.schema import db_4_7_0_1 as db + + pull_request = db.PullRequest.__table__ + pull_request_version = db.PullRequestVersion.__table__ + + reviewer_data_1 = Column( + 'reviewer_data_json', + db.JsonType(dialect_map=dict(mysql=UnicodeText(16384)))) + reviewer_data_1.create(table=pull_request) + + reviewer_data_2 = Column( + 'reviewer_data_json', + db.JsonType(dialect_map=dict(mysql=UnicodeText(16384)))) + reviewer_data_2.create(table=pull_request_version) + + fixups(db, meta.Session) + + +def downgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + + +def fixups(models, _SESSION): + pass 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, reasons, statuses in statuses_by_reviewers: + for user, reasons, mandatory, statuses in statuses_by_reviewers: if statuses: ver, latest = statuses[0] votes[latest.status] += 1 @@ -248,13 +248,14 @@ class ChangesetStatusModel(BaseModel): for o in pull_request.reviewers: if not o.user: continue - st = commit_statuses.get(o.user.username, None) - if st: - st = [(x, list(y)[0]) - for x, y in (itertools.groupby(sorted(st, key=version), - version))] + statuses = commit_statuses.get(o.user.username, None) + if statuses: + statuses = [(x, list(y)[0]) + for x, y in (itertools.groupby( + sorted(statuses, key=version),version))] - pull_request_reviewers.append((o.user, o.reasons, st)) + pull_request_reviewers.append( + (o.user, o.reasons, o.mandatory, statuses)) 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 @@ -3229,6 +3229,14 @@ class _PullRequestBase(BaseModel): _last_merge_status = Column('merge_status', Integer(), nullable=True) merge_rev = Column('merge_rev', String(40), nullable=True) + reviewer_data = Column( + 'reviewer_data_json', MutationObj.as_mutable( + JsonType(dialect_map=dict(mysql=UnicodeText(16384))))) + + @property + def reviewer_data_json(self): + return json.dumps(self.reviewer_data) + @hybrid_property def revisions(self): return self._revisions.split(':') if self._revisions else [] @@ -3348,7 +3356,8 @@ class _PullRequestBase(BaseModel): 'reasons': reasons, 'review_status': st[0][1].status if st else 'not_reviewed', } - for reviewer, reasons, st in pull_request.reviewers_statuses() + for reviewer, reasons, mandatory, st in + pull_request.reviewers_statuses() ] } @@ -3438,6 +3447,8 @@ class PullRequest(Base, _PullRequestBase attrs.revisions = pull_request_obj.revisions attrs.shadow_merge_ref = org_pull_request_obj.shadow_merge_ref + attrs.reviewer_data = org_pull_request_obj.reviewer_data + attrs.reviewer_data_json = org_pull_request_obj.reviewer_data_json return PullRequestDisplay(attrs, internal=internal_methods) @@ -3516,11 +3527,6 @@ class PullRequestReviewers(Base, BaseMod 'mysql_charset': 'utf8', 'sqlite_autoincrement': True}, ) - 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: @@ -3545,7 +3551,7 @@ class PullRequestReviewers(Base, BaseMod _reasons = Column( 'reason', MutationList.as_mutable( JsonType('list', dialect_map=dict(mysql=UnicodeText(16384))))) - + mandatory = Column("mandatory", Boolean(), nullable=False, default=False) user = relationship('User') pull_request = relationship('PullRequest') @@ -3849,14 +3855,17 @@ class RepoReviewRuleUser(Base, BaseModel {'extend_existing': True, 'mysql_engine': 'InnoDB', 'mysql_charset': 'utf8', 'sqlite_autoincrement': True,} ) - repo_review_rule_user_id = Column( - 'repo_review_rule_user_id', Integer(), primary_key=True) - repo_review_rule_id = Column("repo_review_rule_id", - Integer(), ForeignKey('repo_review_rules.repo_review_rule_id')) - user_id = Column("user_id", Integer(), ForeignKey('users.user_id'), - nullable=False) + repo_review_rule_user_id = Column('repo_review_rule_user_id', Integer(), primary_key=True) + repo_review_rule_id = Column("repo_review_rule_id", Integer(), ForeignKey('repo_review_rules.repo_review_rule_id')) + user_id = Column("user_id", Integer(), ForeignKey('users.user_id'), nullable=False) + mandatory = Column("mandatory", Boolean(), nullable=False, default=False) user = relationship('User') + def rule_data(self): + return { + 'mandatory': self.mandatory + } + class RepoReviewRuleUserGroup(Base, BaseModel): __tablename__ = 'repo_review_rules_users_groups' @@ -3864,14 +3873,17 @@ class RepoReviewRuleUserGroup(Base, Base {'extend_existing': True, 'mysql_engine': 'InnoDB', 'mysql_charset': 'utf8', 'sqlite_autoincrement': True,} ) - repo_review_rule_users_group_id = Column( - 'repo_review_rule_users_group_id', Integer(), primary_key=True) - repo_review_rule_id = Column("repo_review_rule_id", - Integer(), ForeignKey('repo_review_rules.repo_review_rule_id')) - users_group_id = Column("users_group_id", Integer(), - ForeignKey('users_groups.users_group_id'), nullable=False) + repo_review_rule_users_group_id = Column('repo_review_rule_users_group_id', Integer(), primary_key=True) + repo_review_rule_id = Column("repo_review_rule_id", Integer(), ForeignKey('repo_review_rules.repo_review_rule_id')) + users_group_id = Column("users_group_id", Integer(),ForeignKey('users_groups.users_group_id'), nullable=False) + mandatory = Column("mandatory", Boolean(), nullable=False, default=False) users_group = relationship('UserGroup') + def rule_data(self): + return { + 'mandatory': self.mandatory + } + class RepoReviewRule(Base, BaseModel): __tablename__ = 'repo_review_rules' @@ -3886,13 +3898,13 @@ class RepoReviewRule(Base, BaseModel): "repo_id", Integer(), ForeignKey('repositories.repo_id')) repo = relationship('Repository', backref='review_rules') - _branch_pattern = Column("branch_pattern", UnicodeText().with_variant(UnicodeText(255), 'mysql'), - default=u'*') # glob - _file_pattern = Column("file_pattern", UnicodeText().with_variant(UnicodeText(255), 'mysql'), - default=u'*') # glob - - use_authors_for_review = Column("use_authors_for_review", Boolean(), - nullable=False, default=False) + _branch_pattern = Column("branch_pattern", UnicodeText().with_variant(UnicodeText(255), 'mysql'), default=u'*') # glob + _file_pattern = Column("file_pattern", UnicodeText().with_variant(UnicodeText(255), 'mysql'), default=u'*') # glob + + use_authors_for_review = Column("use_authors_for_review", Boolean(), nullable=False, default=False) + forbid_author_to_review = Column("forbid_author_to_review", Boolean(), nullable=False, default=False) + forbid_adding_reviewers = Column("forbid_adding_reviewers", Boolean(), nullable=False, default=False) + rule_users = relationship('RepoReviewRuleUser') rule_user_groups = relationship('RepoReviewRuleUserGroup') @@ -3948,16 +3960,26 @@ class RepoReviewRule(Base, BaseModel): def review_users(self): """ Returns the users which this rule applies to """ - users = set() - users |= set([ - rule_user.user for rule_user in self.rule_users - if rule_user.user.active]) - users |= set( - member.user - for rule_user_group in self.rule_user_groups - for member in rule_user_group.users_group.members - if member.user.active - ) + users = collections.OrderedDict() + + for rule_user in self.rule_users: + if rule_user.user.active: + if rule_user.user not in users: + users[rule_user.user.username] = { + 'user': rule_user.user, + 'source': 'user', + 'data': rule_user.rule_data() + } + + for rule_user_group in self.rule_user_groups: + for member in rule_user_group.users_group.members: + if member.user.active: + users[member.user.username] = { + 'user': member.user, + 'source': 'user_group', + 'data': rule_user_group.rule_data() + } + return users def __repr__(self): diff --git a/rhodecode/model/forms.py b/rhodecode/model/forms.py --- a/rhodecode/model/forms.py +++ b/rhodecode/model/forms.py @@ -535,12 +535,13 @@ def PullRequestForm(repo_id): class ReviewerForm(formencode.Schema): user_id = v.Int(not_empty=True) reasons = All() + mandatory = v.StringBoolean() class _PullRequestForm(formencode.Schema): allow_extra_fields = True filter_extra_fields = True - user = v.UnicodeString(strip=True, required=True) + common_ancestor = v.UnicodeString(strip=True, required=True) source_repo = v.UnicodeString(strip=True, required=True) source_ref = v.UnicodeString(strip=True, required=True) target_repo = v.UnicodeString(strip=True, required=True) 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 @@ -415,7 +415,9 @@ class PullRequestModel(BaseModel): .all() def create(self, created_by, source_repo, source_ref, target_repo, - target_ref, revisions, reviewers, title, description=None): + target_ref, revisions, reviewers, title, description=None, + reviewer_data=None): + created_by_user = self._get_user(created_by) source_repo = self._get_repo(source_repo) target_repo = self._get_repo(target_repo) @@ -429,6 +431,7 @@ class PullRequestModel(BaseModel): pull_request.title = title pull_request.description = description pull_request.author = created_by_user + pull_request.reviewer_data = reviewer_data Session().add(pull_request) Session().flush() @@ -436,15 +439,16 @@ class PullRequestModel(BaseModel): reviewer_ids = set() # members / reviewers for reviewer_object in reviewers: - if isinstance(reviewer_object, tuple): - user_id, reasons = reviewer_object - else: - user_id, reasons = reviewer_object, [] + user_id, reasons, mandatory = reviewer_object user = self._get_user(user_id) reviewer_ids.add(user.user_id) - reviewer = PullRequestReviewers(user, pull_request, reasons) + reviewer = PullRequestReviewers() + reviewer.user = user + reviewer.pull_request = pull_request + reviewer.reasons = reasons + reviewer.mandatory = mandatory Session().add(reviewer) # Set approval status to "Under Review" for all commits which are @@ -763,6 +767,7 @@ class PullRequestModel(BaseModel): version._last_merge_status = pull_request._last_merge_status version.shadow_merge_ref = pull_request.shadow_merge_ref version.merge_rev = pull_request.merge_rev + version.reviewer_data = pull_request.reviewer_data version.revisions = pull_request.revisions version.pull_request = pull_request @@ -903,16 +908,18 @@ class PullRequestModel(BaseModel): Update the reviewers in the pull request :param pull_request: the pr to update - :param reviewer_data: list of tuples [(user, ['reason1', 'reason2'])] + :param reviewer_data: list of tuples + [(user, ['reason1', 'reason2'], mandatory_flag)] """ - reviewers_reasons = {} - for user_id, reasons in reviewer_data: + reviewers = {} + for user_id, reasons, mandatory in reviewer_data: if isinstance(user_id, (int, basestring)): user_id = self._get_user(user_id).user_id - reviewers_reasons[user_id] = reasons + reviewers[user_id] = { + 'reasons': reasons, 'mandatory': mandatory} - reviewers_ids = set(reviewers_reasons.keys()) + reviewers_ids = set(reviewers.keys()) pull_request = self.__get_pull_request(pull_request) current_reviewers = PullRequestReviewers.query()\ .filter(PullRequestReviewers.pull_request == @@ -928,8 +935,12 @@ class PullRequestModel(BaseModel): for uid in ids_to_add: changed = True _usr = self._get_user(uid) - reasons = reviewers_reasons[uid] - reviewer = PullRequestReviewers(_usr, pull_request, reasons) + reviewer = PullRequestReviewers() + reviewer.user = _usr + reviewer.pull_request = pull_request + reviewer.reasons = reviewers[uid]['reasons'] + # NOTE(marcink): mandatory shouldn't be changed now + #reviewer.mandatory = reviewers[uid]['reasons'] Session().add(reviewer) for uid in ids_to_remove: @@ -1369,6 +1380,23 @@ class PullRequestModel(BaseModel): action=action, pr_id=pull_request.pull_request_id), pull_request.target_repo) + def get_reviewer_functions(self): + """ + Fetches functions for validation and fetching default reviewers. + If available we use the EE package, else we fallback to CE + package functions + """ + try: + from rc_reviewers.utils import get_default_reviewers_data + from rc_reviewers.utils import validate_default_reviewers + except ImportError: + from rhodecode.apps.repository.utils import \ + get_default_reviewers_data + from rhodecode.apps.repository.utils import \ + validate_default_reviewers + + return get_default_reviewers_data, validate_default_reviewers + class MergeCheck(object): """ diff --git a/rhodecode/model/validation_schema/schemas/reviewer_schema.py b/rhodecode/model/validation_schema/schemas/reviewer_schema.py new file mode 100644 --- /dev/null +++ b/rhodecode/model/validation_schema/schemas/reviewer_schema.py @@ -0,0 +1,34 @@ +# -*- coding: utf-8 -*- + +# Copyright (C) 2016-2017 RhodeCode GmbH +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License, version 3 +# (only), as published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +# +# This program is dual-licensed. If you wish to learn more about the +# RhodeCode Enterprise Edition, including its added features, Support services, +# and proprietary license terms, please see https://rhodecode.com/licenses/ + +import colander +from rhodecode.model.validation_schema import validators, preparers, types + + +class ReviewerSchema(colander.MappingSchema): + username = colander.SchemaNode(types.StrOrIntType()) + reasons = colander.SchemaNode(colander.List(), missing=[]) + mandatory = colander.SchemaNode(colander.Boolean(), missing=False) + + +class ReviewerListSchema(colander.SequenceSchema): + reviewers = ReviewerSchema() + + diff --git a/rhodecode/model/validation_schema/types.py b/rhodecode/model/validation_schema/types.py --- a/rhodecode/model/validation_schema/types.py +++ b/rhodecode/model/validation_schema/types.py @@ -190,7 +190,7 @@ class UserGroupType(UserOrUserGroupType) class StrOrIntType(colander.String): def deserialize(self, node, cstruct): - if isinstance(node, basestring): + if isinstance(cstruct, basestring): return super(StrOrIntType, self).deserialize(node, cstruct) else: return colander.Integer().deserialize(node, cstruct) diff --git a/rhodecode/public/css/buttons.less b/rhodecode/public/css/buttons.less --- a/rhodecode/public/css/buttons.less +++ b/rhodecode/public/css/buttons.less @@ -62,6 +62,10 @@ input[type="button"] { text-shadow: none; } + &.no-margin { + margin: 0 0 0 0; + } + } @@ -270,11 +274,17 @@ input[type="button"] { font-size: inherit; color: @rcblue; border: none; - .border-radius (0); + border-radius: 0; background-color: transparent; + &.last-item { + border: none; + padding: 0 0 0 0; + } + &:last-child { border: none; + padding: 0 0 0 0; } &:hover { diff --git a/rhodecode/public/css/deform.less b/rhodecode/public/css/deform.less --- a/rhodecode/public/css/deform.less +++ b/rhodecode/public/css/deform.less @@ -12,7 +12,7 @@ .control-label { width: 200px; - padding: 10px; + padding: 10px 0px; float: left; } .control-inputs { 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 @@ -359,9 +359,26 @@ ul.auth_plugins { } } +.pr-mergeinfo { + clear: both; + margin: .5em 0; + + input { + min-width: 100% !important; + padding: 0 !important; + border: 0; + } +} + .pr-pullinfo { clear: both; margin: .5em 0; + + input { + min-width: 100% !important; + padding: 0 !important; + border: 0; + } } #pr-title-input { @@ -1298,6 +1315,11 @@ table.integrations { width: 100%; margin-bottom: 8px; } + +.reviewer_entry { + min-height: 55px; +} + .reviewers_member { width: 100%; overflow: auto; @@ -1332,6 +1354,8 @@ table.integrations { } } +.reviewer_member_mandatory, +.reviewer_member_mandatory_remove, .reviewer_member_remove { position: absolute; right: 0; @@ -1341,6 +1365,15 @@ table.integrations { padding: 0; color: black; } + +.reviewer_member_mandatory_remove { + color: @grey4; +} + +.reviewer_member_mandatory { + padding-top:20px; +} + .reviewer_member_status { margin-top: 5px; } @@ -1370,6 +1403,11 @@ table.integrations { .pr-description { white-space:pre-wrap; } + +.pr-reviewer-rules { + padding: 10px 0px 20px 0px; +} + .group_members { margin-top: 0; padding: 0; 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 @@ -16,77 +16,327 @@ // # RhodeCode Enterprise Edition, including its added features, Support services, // # and proprietary license terms, please see https://rhodecode.com/licenses/ + +var prButtonLockChecks = { + 'compare': false, + 'reviewers': false +}; + /** - * Pull request reviewers + * lock button until all checks and loads are made. E.g reviewer calculation + * should prevent from submitting a PR + * @param lockEnabled + * @param msg + * @param scope */ -var removeReviewMember = function(reviewer_id, mark_delete){ - var reviewer = $('#reviewer_{0}'.format(reviewer_id)); +var prButtonLock = function(lockEnabled, msg, scope) { + scope = scope || 'all'; + if (scope == 'all'){ + prButtonLockChecks['compare'] = !lockEnabled; + prButtonLockChecks['reviewers'] = !lockEnabled; + } else if (scope == 'compare') { + prButtonLockChecks['compare'] = !lockEnabled; + } else if (scope == 'reviewers'){ + prButtonLockChecks['reviewers'] = !lockEnabled; + } + var checksMeet = prButtonLockChecks.compare && prButtonLockChecks.reviewers; + if (lockEnabled) { + $('#save').attr('disabled', 'disabled'); + } + else if (checksMeet) { + $('#save').removeAttr('disabled'); + } - if(typeof(mark_delete) === undefined){ - mark_delete = false; - } + if (msg) { + $('#pr_open_message').html(msg); + } +}; - if(mark_delete === true){ - if (reviewer){ - // now delete the input - $('#reviewer_{0} input'.format(reviewer_id)).remove(); - // mark as to-delete - var obj = $('#reviewer_{0}_name'.format(reviewer_id)); - obj.addClass('to-delete'); - obj.css({"text-decoration":"line-through", "opacity": 0.5}); - } - } - else{ - $('#reviewer_{0}'.format(reviewer_id)).remove(); - } + +/** +Generate Title and Description for a PullRequest. +In case of 1 commits, the title and description is that one commit +in case of multiple commits, we iterate on them with max N number of commits, +and build description in a form +- commitN +- commitN+1 +... + +Title is then constructed from branch names, or other references, +replacing '-' and '_' into spaces + +* @param sourceRef +* @param elements +* @param limit +* @returns {*[]} +*/ +var getTitleAndDescription = function(sourceRef, elements, limit) { + var title = ''; + var desc = ''; + + $.each($(elements).get().reverse().slice(0, limit), function(idx, value) { + var rawMessage = $(value).find('td.td-description .message').data('messageRaw'); + desc += '- ' + rawMessage.split('\n')[0].replace(/\n+$/, "") + '\n'; + }); + // only 1 commit, use commit message as title + if (elements.length === 1) { + title = $(elements[0]).find('td.td-description .message').data('messageRaw').split('\n')[0]; + } + else { + // use reference name + title = sourceRef.replace(/-/g, ' ').replace(/_/g, ' ').capitalizeFirstLetter(); + } + + return [title, desc] }; -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_inputs += ''; + + +ReviewersController = function () { + var self = this; + this.$reviewRulesContainer = $('#review_rules'); + this.$rulesList = this.$reviewRulesContainer.find('.pr-reviewer-rules'); + this.forbidReviewUsers = undefined; + this.$reviewMembers = $('#review_members'); + this.currentRequest = null; + + this.defaultForbidReviewUsers = function() { + return [ + {'username': 'default', + 'user_id': templateContext.default_user.user_id} + ]; + }; + + this.hideReviewRules = function() { + self.$reviewRulesContainer.hide(); + }; + + this.showReviewRules = function() { + self.$reviewRulesContainer.show(); + }; + + this.addRule = function(ruleText) { + self.showReviewRules(); + return '
- {0}
'.format(ruleText) + }; + + this.loadReviewRules = function(data) { + // reset forbidden Users + this.forbidReviewUsers = self.defaultForbidReviewUsers(); + + // reset state of review rules + self.$rulesList.html(''); + + if (!data || data.rules === undefined || $.isEmptyObject(data.rules)) { + // default rule, case for older repo that don't have any rules stored + self.$rulesList.append( + self.addRule( + _gettext('All reviewers must vote.')) + ); + return self.forbidReviewUsers + } + + if (data.rules.voting !== undefined) { + if (data.rules.voting < 0){ + self.$rulesList.append( + self.addRule( + _gettext('All reviewers must vote.')) + ) + } else if (data.rules.voting === 1) { + self.$rulesList.append( + self.addRule( + _gettext('At least {0} reviewer must vote.').format(data.rules.voting)) + ) + + } else { + self.$rulesList.append( + self.addRule( + _gettext('At least {0} reviewers must vote.').format(data.rules.voting)) + ) + } + } + if (data.rules.use_code_authors_for_review) { + self.$rulesList.append( + self.addRule( + _gettext('Reviewers picked from source code changes.')) + ) + } + if (data.rules.forbid_adding_reviewers) { + $('#add_reviewer_input').remove(); + self.$rulesList.append( + self.addRule( + _gettext('Adding new reviewers is forbidden.')) + ) + } + if (data.rules.forbid_author_to_review) { + self.forbidReviewUsers.push(data.rules_data.pr_author); + self.$rulesList.append( + self.addRule( + _gettext('Author is not allowed to be a reviewer.')) + ) + } + return self.forbidReviewUsers + }; + + this.loadDefaultReviewers = function(sourceRepo, sourceRef, targetRepo, targetRef) { + + if (self.currentRequest) { + // make sure we cleanup old running requests before triggering this + // again + self.currentRequest.abort(); } - } - var tmpl = '
  • '+ - ''+ - '
    '+ - '
    '+ - '
    '+ - 'gravatar'+ - '{1}'+ - reasons_html + - ''+ - ''+ - '{3}'+ - ''+ - '
    ' + - ''+ - '
    '+ - ''+ - ''+ - '
    • ' ; + + $('.calculate-reviewers').show(); + // reset reviewer members + self.$reviewMembers.empty(); + + prButtonLock(true, null, 'reviewers'); + $('#user').hide(); // hide user autocomplete before load + + var url = pyroutes.url('repo_default_reviewers_data', + { + 'repo_name': templateContext.repo_name, + 'source_repo': sourceRepo, + 'source_ref': sourceRef[2], + 'target_repo': targetRepo, + 'target_ref': targetRef[2] + }); + + self.currentRequest = $.get(url) + .done(function(data) { + self.currentRequest = null; + + // review rules + self.loadReviewRules(data); + + for (var i = 0; i < data.reviewers.length; i++) { + var reviewer = data.reviewers[i]; + self.addReviewMember( + reviewer.user_id, reviewer.firstname, + reviewer.lastname, reviewer.username, + reviewer.gravatar_link, reviewer.reasons, + reviewer.mandatory); + } + $('.calculate-reviewers').hide(); + prButtonLock(false, null, 'reviewers'); + $('#user').show(); // show user autocomplete after load + }); + }; + + // check those, refactor + this.removeReviewMember = function(reviewer_id, mark_delete) { + var reviewer = $('#reviewer_{0}'.format(reviewer_id)); + + if(typeof(mark_delete) === undefined){ + mark_delete = false; + } + + if(mark_delete === true){ + if (reviewer){ + // now delete the input + $('#reviewer_{0} input'.format(reviewer_id)).remove(); + // mark as to-delete + var obj = $('#reviewer_{0}_name'.format(reviewer_id)); + obj.addClass('to-delete'); + obj.css({"text-decoration":"line-through", "opacity": 0.5}); + } + } + else{ + $('#reviewer_{0}'.format(reviewer_id)).remove(); + } + }; + + this.addReviewMember = function(id, fname, lname, nname, gravatar_link, reasons, mandatory) { + var members = self.$reviewMembers.get(0); + var reasons_html = ''; + var reasons_inputs = ''; + var reasons = reasons || []; + var mandatory = mandatory || false; - var displayname = "{0} ({1} {2})".format( - nname, escapeHtml(fname), escapeHtml(lname)); - 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(); - for (el in _els){ - ids.push(_els[el].id) - } - if(ids.indexOf('reviewer_'+id) == -1){ - // only add if it's not there - members.innerHTML += element; - } + if (reasons) { + for (var i = 0; i < reasons.length; i++) { + reasons_html += '
    - {0}
    '.format(reasons[i]); + reasons_inputs += ''; + } + } + var tmpl = '' + + '
  • '+ + ''+ + '
    '+ + '
    '+ + '
    '+ + 'gravatar'+ + '{1}'+ + reasons_html + + ''+ + ''+ + '{3}'+ + ''; + + if (mandatory) { + tmpl += ''+ + '
    ' + + ''+ + '
    ' + + ''+ + '
    ' + + ''+ + '
    '; + + } else { + tmpl += ''+ + ''+ + '
    ' + + ''+ + '
    '; + } + // continue template + tmpl += ''+ + ''+ + '
    • ' ; + + var displayname = "{0} ({1} {2})".format( + nname, escapeHtml(fname), escapeHtml(lname)); + var element = tmpl.format(gravatar_link,displayname,id,reasons_inputs); + // check if we don't have this ID already in + var ids = []; + var _els = self.$reviewMembers.find('li').toArray(); + for (el in _els){ + ids.push(_els[el].id) + } + + var userAllowedReview = function(userId) { + var allowed = true; + $.each(self.forbidReviewUsers, function(index, member_data) { + if (parseInt(userId) === member_data['user_id']) { + allowed = false; + return false // breaks the loop + } + }); + return allowed + }; + + var userAllowed = userAllowedReview(id); + if (!userAllowed){ + alert(_gettext('User `{0}` not allowed to be a reviewer').format(nname)); + } + var shouldAdd = userAllowed && ids.indexOf('reviewer_'+id) == -1; + + if(shouldAdd) { + // only add if it's not there + members.innerHTML += element; + } + + }; + + this.updateReviewers = function(repo_name, pull_request_id){ + var postData = '_method=put&' + $('#reviewers input').serialize(); + _updatePullRequest(repo_name, pull_request_id, postData); + }; }; + var _updatePullRequest = function(repo_name, pull_request_id, postData) { var url = pyroutes.url( 'pullrequest_update', @@ -102,13 +352,6 @@ var _updatePullRequest = function(repo_n ajaxPOST(url, postData, success); }; -var updateReviewers = function(reviewers_ids, repo_name, pull_request_id){ - if (reviewers_ids === undefined){ - var postData = '_method=put&' + $('#reviewers input').serialize(); - _updatePullRequest(repo_name, pull_request_id, postData); - } -}; - /** * PULL REQUEST reject & close */ @@ -207,7 +450,7 @@ var ReviewerAutoComplete = function(inpu showNoSuggestionNotice: true, tabDisabled: true, autoSelectFirst: true, - params: { user_id: templateContext.rhodecode_user.user_id, user_groups:true, user_groups_expand:true }, + params: { user_id: templateContext.rhodecode_user.user_id, user_groups:true, user_groups_expand:true, skip_default_user:true }, formatResult: autocompleteFormatResult, lookupFilter: autocompleteFilterResult, onSelect: function(element, data) { @@ -217,13 +460,15 @@ var ReviewerAutoComplete = function(inpu reasons.push(_gettext('member of "{0}"').format(data.value_display)); $.each(data.members, function(index, member_data) { - addReviewMember(member_data.id, member_data.first_name, member_data.last_name, - member_data.username, member_data.icon_link, reasons); + reviewersController.addReviewMember( + member_data.id, member_data.first_name, member_data.last_name, + member_data.username, member_data.icon_link, reasons); }) } else { - addReviewMember(data.id, data.first_name, data.last_name, - data.username, data.icon_link, reasons); + reviewersController.addReviewMember( + data.id, data.first_name, data.last_name, + data.username, data.icon_link, reasons); } $(inputId).val(''); diff --git a/rhodecode/templates/admin/repos/repo_edit.mako b/rhodecode/templates/admin/repos/repo_edit.mako --- a/rhodecode/templates/admin/repos/repo_edit.mako +++ b/rhodecode/templates/admin/repos/repo_edit.mako @@ -24,7 +24,11 @@ <%def name="main_content()"> - <%include file="/admin/repos/repo_edit_${c.active}.mako"/> + % if hasattr(c, 'repo_edit_template'): + <%include file="${c.repo_edit_template}"/> + % else: + <%include file="/admin/repos/repo_edit_${c.active}.mako"/> + % endif diff --git a/rhodecode/templates/compare/compare_commits.mako b/rhodecode/templates/compare/compare_commits.mako --- a/rhodecode/templates/compare/compare_commits.mako +++ b/rhodecode/templates/compare/compare_commits.mako @@ -6,6 +6,7 @@ ${h.short_id(c.ancestor)} . ${_('Compare was calculated based on this shared commit.')} + %endif diff --git a/rhodecode/templates/forms/sequence.pt b/rhodecode/templates/forms/sequence.pt --- a/rhodecode/templates/forms/sequence.pt +++ b/rhodecode/templates/forms/sequence.pt @@ -32,21 +32,21 @@ tal:attributes="prototype prototype"/>
    -
    ${title}
    + tal:replace="structure subfield.render_template(item_tmpl,parent=field)" /> + orderable orderable;"> + +
    @@ -78,12 +78,12 @@ placeholder: '', onDragStart: function ($item, container, _super) { var offset = $item.offset(), - pointer = container.rootGroup.pointer + pointer = container.rootGroup.pointer; adjustment = { left: pointer.left - offset.left, top: pointer.top - offset.top - } + }; _super($item, container) }, diff --git a/rhodecode/templates/forms/sequence_item.pt b/rhodecode/templates/forms/sequence_item.pt --- a/rhodecode/templates/forms/sequence_item.pt +++ b/rhodecode/templates/forms/sequence_item.pt @@ -6,6 +6,7 @@ oid oid|field.oid" class="form-group row deform-seq-item ${field.error and error_class or ''} ${field.widget.item_css_class or ''}" i18n:domain="deform"> +
    ${msg}
    +
    - - ×
    - +
    diff --git a/rhodecode/templates/pullrequests/pullrequest.mako b/rhodecode/templates/pullrequests/pullrequest.mako --- a/rhodecode/templates/pullrequests/pullrequest.mako +++ b/rhodecode/templates/pullrequests/pullrequest.mako @@ -62,7 +62,7 @@ ##ORG
    - ${_('Origin repository')}: + ${_('Source repository')}: ${c.rhodecode_db_repo.description}
    @@ -102,6 +102,17 @@
    + ## REIEW RULES + + + ## REVIEWERS
    ${_('Pull request reviewers')} @@ -137,7 +148,6 @@ var defaultSourceRepoData = ${c.default_repo_data['source_refs_json']|n}; var defaultTargetRepo = '${c.default_repo_data['target_repo_name']}'; var defaultTargetRepoData = ${c.default_repo_data['target_refs_json']|n}; - var targetRepoName = '${c.repo_name}'; var $pullRequestForm = $('#pull_request_form'); var $sourceRepo = $('#source_repo', $pullRequestForm); @@ -145,6 +155,12 @@ var $sourceRef = $('#source_ref', $pullRequestForm); var $targetRef = $('#target_ref', $pullRequestForm); + var sourceRepo = function() { return $sourceRepo.eq(0).val() }; + var sourceRef = function() { return $sourceRef.eq(0).val().split(':') }; + + var targetRepo = function() { return $targetRepo.eq(0).val() }; + var targetRef = function() { return $targetRef.eq(0).val().split(':') }; + var calculateContainerWidth = function() { var maxWidth = 0; var repoSelect2Containers = ['#source_repo', '#target_repo']; @@ -204,6 +220,8 @@ // custom code mirror var codeMirrorInstance = initPullRequestsCodeMirror('#pullrequest_desc'); + reviewersController = new ReviewersController(); + var queryTargetRepo = function(self, query) { // cache ALL results if query is empty var cacheKey = query.term || '__'; @@ -213,7 +231,7 @@ query.callback({results: cachedData.results}); } else { $.ajax({ - url: pyroutes.url('pullrequest_repo_destinations', {'repo_name': targetRepoName}), + url: pyroutes.url('pullrequest_repo_destinations', {'repo_name': templateContext.repo_name}), data: {query: query.term}, dataType: 'json', type: 'GET', @@ -246,55 +264,21 @@ query.callback({results: data.results}); }; - - var prButtonLockChecks = { - 'compare': false, - 'reviewers': false - }; - - var prButtonLock = function(lockEnabled, msg, scope) { - scope = scope || 'all'; - if (scope == 'all'){ - prButtonLockChecks['compare'] = !lockEnabled; - prButtonLockChecks['reviewers'] = !lockEnabled; - } else if (scope == 'compare') { - prButtonLockChecks['compare'] = !lockEnabled; - } else if (scope == 'reviewers'){ - prButtonLockChecks['reviewers'] = !lockEnabled; - } - var checksMeet = prButtonLockChecks.compare && prButtonLockChecks.reviewers; - if (lockEnabled) { - $('#save').attr('disabled', 'disabled'); - } - else if (checksMeet) { - $('#save').removeAttr('disabled'); - } - - if (msg) { - $('#pr_open_message').html(msg); - } - }; - var loadRepoRefDiffPreview = function() { - var sourceRepo = $sourceRepo.eq(0).val(); - var sourceRef = $sourceRef.eq(0).val().split(':'); - - var targetRepo = $targetRepo.eq(0).val(); - var targetRef = $targetRef.eq(0).val().split(':'); var url_data = { - 'repo_name': targetRepo, - 'target_repo': sourceRepo, - 'source_ref': targetRef[2], + 'repo_name': targetRepo(), + 'target_repo': sourceRepo(), + 'source_ref': targetRef()[2], 'source_ref_type': 'rev', - 'target_ref': sourceRef[2], + 'target_ref': sourceRef()[2], 'target_ref_type': 'rev', 'merge': true, '_': Date.now() // bypass browser caching }; // gather the source/target ref and repo here - if (sourceRef.length !== 3 || targetRef.length !== 3) { - prButtonLock(true, "${_('Please select origin and destination')}"); + if (sourceRef().length !== 3 || targetRef().length !== 3) { + prButtonLock(true, "${_('Please select source and target')}"); return; } var url = pyroutes.url('compare_url', url_data); @@ -315,10 +299,11 @@ .done(function(data) { loadRepoRefDiffPreview._currentRequest = null; $('#pull_request_overview').html(data); + var commitElements = $(data).find('tr[commit_id]'); - var prTitleAndDesc = getTitleAndDescription(sourceRef[1], - commitElements, 5); + var prTitleAndDesc = getTitleAndDescription( + sourceRef()[1], commitElements, 5); var title = prTitleAndDesc[0]; var proposedDescription = prTitleAndDesc[1]; @@ -366,43 +351,6 @@ }); }; - /** - Generate Title and Description for a PullRequest. - In case of 1 commits, the title and description is that one commit - in case of multiple commits, we iterate on them with max N number of commits, - and build description in a form - - commitN - - commitN+1 - ... - - Title is then constructed from branch names, or other references, - replacing '-' and '_' into spaces - - * @param sourceRef - * @param elements - * @param limit - * @returns {*[]} - */ - var getTitleAndDescription = function(sourceRef, elements, limit) { - var title = ''; - var desc = ''; - - $.each($(elements).get().reverse().slice(0, limit), function(idx, value) { - var rawMessage = $(value).find('td.td-description .message').data('messageRaw'); - desc += '- ' + rawMessage.split('\n')[0].replace(/\n+$/, "") + '\n'; - }); - // only 1 commit, use commit message as title - if (elements.length == 1) { - title = $(elements[0]).find('td.td-description .message').data('messageRaw').split('\n')[0]; - } - else { - // use reference name - title = sourceRef.replace(/-/g, ' ').replace(/_/g, ' ').capitalizeFirstLetter(); - } - - return [title, desc] - }; - var Select2Box = function(element, overrides) { var globalDefaults = { dropdownAutoWidth: true, @@ -459,7 +407,7 @@ var targetRepoChanged = function(repoData) { // generate new DESC of target repo displayed next to select $('#target_repo_desc').html( - "${_('Destination repository')}: {0}".format(repoData['description']) + "${_('Target repository')}: {0}".format(repoData['description']) ); // generate dynamic select2 for refs. @@ -468,8 +416,7 @@ }; - var sourceRefSelect2 = Select2Box( - $sourceRef, { + var sourceRefSelect2 = Select2Box($sourceRef, { placeholder: "${_('Select commit reference')}", query: function(query) { var initialData = defaultSourceRepoData['refs']['select2_refs']; @@ -499,12 +446,14 @@ $sourceRef.on('change', function(e){ loadRepoRefDiffPreview(); - loadDefaultReviewers(); + reviewersController.loadDefaultReviewers( + sourceRepo(), sourceRef(), targetRepo(), targetRef()); }); $targetRef.on('change', function(e){ loadRepoRefDiffPreview(); - loadDefaultReviewers(); + reviewersController.loadDefaultReviewers( + sourceRepo(), sourceRef(), targetRepo(), targetRef()); }); $targetRepo.on('change', function(e){ @@ -515,7 +464,7 @@ $.ajax({ url: pyroutes.url('pullrequest_repo_refs', - {'repo_name': targetRepoName, 'target_repo_name':repoName}), + {'repo_name': templateContext.repo_name, 'target_repo_name':repoName}), data: {}, dataType: 'json', type: 'GET', @@ -531,43 +480,7 @@ }); - var loadDefaultReviewers = function() { - if (loadDefaultReviewers._currentRequest) { - loadDefaultReviewers._currentRequest.abort(); - } - $('.calculate-reviewers').show(); - prButtonLock(true, null, 'reviewers'); - - var url = pyroutes.url('repo_default_reviewers_data', {'repo_name': targetRepoName}); - - var sourceRepo = $sourceRepo.eq(0).val(); - var sourceRef = $sourceRef.eq(0).val().split(':'); - var targetRepo = $targetRepo.eq(0).val(); - var targetRef = $targetRef.eq(0).val().split(':'); - url += '?source_repo=' + sourceRepo; - url += '&source_ref=' + sourceRef[2]; - url += '&target_repo=' + targetRepo; - url += '&target_ref=' + targetRef[2]; - - loadDefaultReviewers._currentRequest = $.get(url) - .done(function(data) { - loadDefaultReviewers._currentRequest = null; - - // reset && add the reviewer based on selected repo - $('#review_members').html(''); - for (var i = 0; i < data.reviewers.length; i++) { - var reviewer = data.reviewers[i]; - addReviewMember( - reviewer.user_id, reviewer.firstname, - reviewer.lastname, reviewer.username, - reviewer.gravatar_link, reviewer.reasons); - } - $('.calculate-reviewers').hide(); - prButtonLock(false, null, 'reviewers'); - }); - }; - - prButtonLock(true, "${_('Please select origin and destination')}", 'all'); + prButtonLock(true, "${_('Please select source and target')}", 'all'); // auto-load on init, the target refs select2 calculateContainerWidth(); @@ -578,10 +491,11 @@ }); % if c.default_source_ref: - // in case we have a pre-selected value, use it now - $sourceRef.select2('val', '${c.default_source_ref}'); - loadRepoRefDiffPreview(); - loadDefaultReviewers(); + // in case we have a pre-selected value, use it now + $sourceRef.select2('val', '${c.default_source_ref}'); + loadRepoRefDiffPreview(); + reviewersController.loadDefaultReviewers( + sourceRepo(), sourceRef(), targetRepo(), targetRef()); % endif ReviewerAutoComplete('#user'); diff --git a/rhodecode/templates/pullrequests/pullrequest_show.mako b/rhodecode/templates/pullrequests/pullrequest_show.mako --- a/rhodecode/templates/pullrequests/pullrequest_show.mako +++ b/rhodecode/templates/pullrequests/pullrequest_show.mako @@ -48,13 +48,13 @@
    <% summary = lambda n:{False:'summary-short'}.get(n) %>
    - ${_('Pull request #%s') % c.pull_request.pull_request_id} ${_('From')} ${h.format_date(c.pull_request.created_on)} + ${_('Pull request #%s') % c.pull_request.pull_request_id} ${_('From')} ${h.format_date(c.pull_request.created_on)} %if c.allowed_to_update:
    % if c.allowed_to_delete: ${h.secure_form(url('pullrequest_delete', repo_name=c.pull_request.target_repo.repo_name, pull_request_id=c.pull_request.pull_request_id),method='delete')} ${h.submit('remove_%s' % c.pull_request.pull_request_id, _('Delete'), - class_="btn btn-link btn-danger",onclick="return confirm('"+_('Confirm to delete this pull request')+"');")} + class_="btn btn-link btn-danger no-margin",onclick="return confirm('"+_('Confirm to delete this pull request')+"');")} ${h.end_form()} % else: ${_('Delete')} @@ -68,7 +68,7 @@
    - +
    @@ -297,7 +297,7 @@
    @@ -316,13 +316,27 @@
    + + ## REVIEW RULES + + ## REVIEWERS
    ${_('Pull request reviewers')} %if c.allowed_to_update: - ${_('Edit')} - + ${_('Edit')} %endif
    @@ -330,8 +344,8 @@ ## members goes here !
      - %for member,reasons,status in c.pull_request_reviewers: -
    • + %for member,reasons,mandatory,status in c.pull_request_reviewers: +
    • @@ -348,30 +362,43 @@ %endfor + - %if c.allowed_to_update: - - %endif + % if mandatory: +
      + +
      +
      + +
      + % else: + %if c.allowed_to_update: + + %endif + % endif
      • %endfor
    - %if not c.pull_request.is_closed(): - - %endif
    @@ -429,7 +456,7 @@
    % if c.allowed_to_update and not c.pull_request.is_closed(): - ${_('Update commits')} + ${_('Update commits')} % else: ${_('Update commits')} % endif @@ -615,9 +642,10 @@ versionController = new VersionController(); versionController.init(); + reviewersController = new ReviewersController(); $(function(){ - ReviewerAutoComplete('#user'); + // custom code mirror var codeMirrorInstance = initPullRequestsCodeMirror('#pr-description-input'); @@ -655,13 +683,13 @@ var ReviewersPanel = { editButton: $('#open_edit_reviewers'), closeButton: $('#close_edit_reviewers'), - addButton: $('#add_reviewer_input'), - removeButtons: $('.reviewer_member_remove'), + addButton: $('#add_reviewer'), + removeButtons: $('.reviewer_member_remove,.reviewer_member_mandatory_remove,.reviewer_member_mandatory'), init: function() { - var that = this; - this.editButton.on('click', function(e) { that.edit(); }); - this.closeButton.on('click', function(e) { that.close(); }); + var self = this; + this.editButton.on('click', function(e) { self.edit(); }); + this.closeButton.on('click', function(e) { self.close(); }); }, edit: function(event) { @@ -669,6 +697,9 @@ this.closeButton.show(); this.addButton.show(); this.removeButtons.css('visibility', 'visible'); + // review rules + reviewersController.loadReviewRules( + ${c.pull_request.reviewer_data_json | n}); }, close: function(event) { @@ -676,6 +707,8 @@ this.closeButton.hide(); this.addButton.hide(); this.removeButtons.css('visibility', 'hidden'); + // hide review rules + reviewersController.hideReviewRules() } }; @@ -774,7 +807,8 @@ $(this).attr('disabled', 'disabled'); $(this).addClass('disabled'); $(this).html(_gettext('Saving...')); - updateReviewers(undefined, "${c.repo_name}", "${c.pull_request.pull_request_id}"); + reviewersController.updateReviewers( + "${c.repo_name}", "${c.pull_request.pull_request_id}"); }); $('#update_commits').on('click', function(e){ @@ -784,14 +818,16 @@ $(e.currentTarget).removeClass('btn-primary'); $(e.currentTarget).text(_gettext('Updating...')); if(isDisabled){ - updateCommits("${c.repo_name}", "${c.pull_request.pull_request_id}"); + updateCommits( + "${c.repo_name}", "${c.pull_request.pull_request_id}"); } }); // fixing issue with caches on firefox $('#update_commits').removeAttr("disabled"); $('#close_pull_request').on('click', function(e){ - closePullRequest("${c.repo_name}", "${c.pull_request.pull_request_id}"); + closePullRequest( + "${c.repo_name}", "${c.pull_request.pull_request_id}"); }); $('.show-inline-comments').on('click', function(e){ @@ -818,6 +854,8 @@ // initial injection injectCloseAction(); + ReviewerAutoComplete('#user'); + }) diff --git a/rhodecode/tests/functional/test_login.py b/rhodecode/tests/functional/test_login.py --- a/rhodecode/tests/functional/test_login.py +++ b/rhodecode/tests/functional/test_login.py @@ -25,7 +25,8 @@ import pytest from rhodecode.config.routing import ADMIN_PREFIX from rhodecode.tests import ( - assert_session_flash, url, HG_REPO, TEST_USER_ADMIN_LOGIN) + assert_session_flash, url, HG_REPO, TEST_USER_ADMIN_LOGIN, + no_newline_id_generator) from rhodecode.tests.fixture import Fixture from rhodecode.tests.utils import AssertResponse, get_session_from_response from rhodecode.lib.auth import check_password @@ -122,7 +123,7 @@ class TestLoginController(object): 'ftp://some.ftp.server', 'http://other.domain', '/\r\nX-Forwarded-Host: http://example.org', - ]) + ], ids=no_newline_id_generator) def test_login_bad_came_froms(self, url_came_from): _url = '{}?came_from={}'.format(login_url, url_came_from) response = self.app.post( diff --git a/rhodecode/tests/functional/test_pullrequests.py b/rhodecode/tests/functional/test_pullrequests.py --- a/rhodecode/tests/functional/test_pullrequests.py +++ b/rhodecode/tests/functional/test_pullrequests.py @@ -295,8 +295,8 @@ class TestPullrequestsController: def test_comment_force_close_pull_request(self, pr_util, csrf_token): pull_request = pr_util.create_pull_request() pull_request_id = pull_request.pull_request_id - reviewers_data = [(1, ['reason']), (2, ['reason2'])] - PullRequestModel().update_reviewers(pull_request_id, reviewers_data) + PullRequestModel().update_reviewers( + pull_request_id, [(1, ['reason'], False), (2, ['reason2'], False)]) author = pull_request.user_id repo = pull_request.target_repo.repo_id self.app.post( @@ -345,6 +345,7 @@ class TestPullrequestsController: ('source_ref', 'branch:default:' + commit_ids['change2']), ('target_repo', target.repo_name), ('target_ref', 'branch:default:' + commit_ids['ancestor']), + ('common_ancestor', commit_ids['ancestor']), ('pullrequest_desc', 'Description'), ('pullrequest_title', 'Title'), ('__start__', 'review_members:sequence'), @@ -353,6 +354,7 @@ class TestPullrequestsController: ('__start__', 'reasons:sequence'), ('reason', 'Some reason'), ('__end__', 'reasons:sequence'), + ('mandatory', 'False'), ('__end__', 'reviewer:mapping'), ('__end__', 'review_members:sequence'), ('__start__', 'revisions:sequence'), @@ -365,8 +367,9 @@ class TestPullrequestsController: status=302) location = response.headers['Location'] - pull_request_id = int(location.rsplit('/', 1)[1]) - pull_request = PullRequest.get(pull_request_id) + pull_request_id = location.rsplit('/', 1)[1] + assert pull_request_id != 'new' + pull_request = PullRequest.get(int(pull_request_id)) # check that we have now both revisions assert pull_request.revisions == [commit_ids['change2'], commit_ids['change']] @@ -403,6 +406,7 @@ class TestPullrequestsController: ('source_ref', 'branch:default:' + commit_ids['change']), ('target_repo', target.repo_name), ('target_ref', 'branch:default:' + commit_ids['ancestor-child']), + ('common_ancestor', commit_ids['ancestor']), ('pullrequest_desc', 'Description'), ('pullrequest_title', 'Title'), ('__start__', 'review_members:sequence'), @@ -411,6 +415,7 @@ class TestPullrequestsController: ('__start__', 'reasons:sequence'), ('reason', 'Some reason'), ('__end__', 'reasons:sequence'), + ('mandatory', 'False'), ('__end__', 'reviewer:mapping'), ('__end__', 'review_members:sequence'), ('__start__', 'revisions:sequence'), @@ -422,21 +427,22 @@ class TestPullrequestsController: status=302) location = response.headers['Location'] - pull_request_id = int(location.rsplit('/', 1)[1]) - pull_request = PullRequest.get(pull_request_id) + + pull_request_id = location.rsplit('/', 1)[1] + assert pull_request_id != 'new' + pull_request = PullRequest.get(int(pull_request_id)) # Check that a notification was made notifications = Notification.query()\ .filter(Notification.created_by == pull_request.author.user_id, Notification.type_ == Notification.TYPE_PULL_REQUEST, - Notification.subject.contains("wants you to review " - "pull request #%d" - % pull_request_id)) + Notification.subject.contains( + "wants you to review pull request #%s" % pull_request_id)) assert len(notifications.all()) == 1 # Change reviewers and check that a notification was made PullRequestModel().update_reviewers( - pull_request.pull_request_id, [(1, [])]) + pull_request.pull_request_id, [(1, [], False)]) assert len(notifications.all()) == 2 def test_create_pull_request_stores_ancestor_commit_id(self, backend, @@ -467,6 +473,7 @@ class TestPullrequestsController: ('source_ref', 'branch:default:' + commit_ids['change']), ('target_repo', target.repo_name), ('target_ref', 'branch:default:' + commit_ids['ancestor-child']), + ('common_ancestor', commit_ids['ancestor']), ('pullrequest_desc', 'Description'), ('pullrequest_title', 'Title'), ('__start__', 'review_members:sequence'), @@ -475,6 +482,7 @@ class TestPullrequestsController: ('__start__', 'reasons:sequence'), ('reason', 'Some reason'), ('__end__', 'reasons:sequence'), + ('mandatory', 'False'), ('__end__', 'reviewer:mapping'), ('__end__', 'review_members:sequence'), ('__start__', 'revisions:sequence'), @@ -486,8 +494,10 @@ class TestPullrequestsController: status=302) location = response.headers['Location'] - pull_request_id = int(location.rsplit('/', 1)[1]) - pull_request = PullRequest.get(pull_request_id) + + pull_request_id = location.rsplit('/', 1)[1] + assert pull_request_id != 'new' + pull_request = PullRequest.get(int(pull_request_id)) # target_ref has to point to the ancestor's commit_id in order to # show the correct diff @@ -954,14 +964,7 @@ class TestPullrequestsController: assert target.text.strip() == 'tag: target' assert target.getchildren() == [] - def test_description_is_escaped_on_index_page(self, backend, pr_util): - xss_description = "" - pull_request = pr_util.create_pull_request(description=xss_description) - response = self.app.get(url( - controller='pullrequests', action='show_all', - repo_name=pull_request.target_repo.repo_name)) - response.mustcontain( - "<script>alert('Hi!')</script>") + @pytest.mark.parametrize('mergeable', [True, False]) def test_shadow_repository_link( @@ -1061,23 +1064,20 @@ def assert_pull_request_status(pull_requ assert status == expected_status -@pytest.mark.parametrize('action', ['show_all', 'index', 'create']) +@pytest.mark.parametrize('action', ['index', 'create']) @pytest.mark.usefixtures("autologin_user") -def test_redirects_to_repo_summary_for_svn_repositories( - backend_svn, app, action): - denied_actions = ['show_all', 'index', 'create'] - for action in denied_actions: - response = app.get(url( - controller='pullrequests', action=action, - repo_name=backend_svn.repo_name)) - assert response.status_int == 302 +def test_redirects_to_repo_summary_for_svn_repositories(backend_svn, app, action): + response = app.get(url( + controller='pullrequests', action=action, + repo_name=backend_svn.repo_name)) + assert response.status_int == 302 - # Not allowed, redirect to the summary - redirected = response.follow() - summary_url = url('summary_home', repo_name=backend_svn.repo_name) + # Not allowed, redirect to the summary + redirected = response.follow() + summary_url = url('summary_home', repo_name=backend_svn.repo_name) - # URL adds leading slash and path doesn't have it - assert redirected.req.path == summary_url + # URL adds leading slash and path doesn't have it + assert redirected.req.path == summary_url def test_delete_comment_returns_404_if_comment_does_not_exist(pylonsapp): diff --git a/rhodecode/tests/models/test_pullrequest.py b/rhodecode/tests/models/test_pullrequest.py --- a/rhodecode/tests/models/test_pullrequest.py +++ b/rhodecode/tests/models/test_pullrequest.py @@ -116,7 +116,7 @@ class TestPullRequestModel: def test_get_awaiting_my_review(self, pull_request): PullRequestModel().update_reviewers( - pull_request, [(pull_request.author, ['author'])]) + pull_request, [(pull_request.author, ['author'], False)]) prs = PullRequestModel().get_awaiting_my_review( pull_request.target_repo, user_id=pull_request.author.user_id) assert isinstance(prs, list) @@ -124,7 +124,7 @@ class TestPullRequestModel: def test_count_awaiting_my_review(self, pull_request): PullRequestModel().update_reviewers( - pull_request, [(pull_request.author, ['author'])]) + pull_request, [(pull_request.author, ['author'], False)]) pr_count = PullRequestModel().count_awaiting_my_review( pull_request.target_repo, user_id=pull_request.author.user_id) assert pr_count == 1 diff --git a/rhodecode/tests/plugin.py b/rhodecode/tests/plugin.py --- a/rhodecode/tests/plugin.py +++ b/rhodecode/tests/plugin.py @@ -986,10 +986,9 @@ class PRTestUtility(object): return reference def _get_reviewers(self): - model = UserModel() return [ - model.get_by_username(TEST_USER_REGULAR_LOGIN), - model.get_by_username(TEST_USER_REGULAR2_LOGIN), + (TEST_USER_REGULAR_LOGIN, ['default1'], False), + (TEST_USER_REGULAR2_LOGIN, ['default2'], False), ] def update_source_repository(self, head=None):