pull-request: introduced new merge-checks.

- merge checks show nicer UI for status of merge approval inside PR - merge checks now also forbid a merge if TODO notes are present - use global refresh merge check when submitting comments. It no longer required page reload when submitting status change.
2017-01-16 22:58:44 +01:00 · 2017-01-16 22:58:44 +01:00 · e5f1efc7f7
commit e5f1efc7f7
parent 986141fb9f
8 changed files with 170 additions and 51 deletions
--- a/rhodecode/controllers/pullrequests.py
+++ b/rhodecode/controllers/pullrequests.py
@ -633,6 +633,18 @@ class PullrequestsController(BaseRepoController):
            msg = _('Pull request reviewer approval is pending.')
            h.flash(msg, category='error')
            return False
+
+        todos = CommentsModel().get_unresolved_todos(pull_request)
+        if todos:
+            log.debug("Cannot merge, unresolved todos left.")
+            if len(todos) == 1:
+                msg = _('Cannot merge, {} todo still not resolved.').format(
+                    len(todos))
+            else:
+                msg = _('Cannot merge, {} todos still not resolved.').format(
+                    len(todos))
+            h.flash(msg, category='error')
+            return False
        return True

    def _merge_pull_request(self, pull_request, user, extras):
@ -761,6 +773,7 @@ class PullrequestsController(BaseRepoController):
    def show(self, repo_name, pull_request_id):
        pull_request_id = safe_int(pull_request_id)
        version = request.GET.get('version')
+        merge_checks = request.GET.get('merge_checks')

        (pull_request_latest,
         pull_request_at_ver,
@ -778,6 +791,10 @@ class PullrequestsController(BaseRepoController):
        c.shadow_clone_url = PullRequestModel().get_shadow_clone_url(
            pull_request_at_ver)

+        c.ancestor = None  # TODO: add ancestor here
+        c.pull_request = pull_request_display_obj
+        c.pull_request_latest = pull_request_latest
+
        pr_closed = pull_request_latest.is_closed()
        if at_version and not at_version == 'latest':
            c.allowed_to_change_status = False
@ -800,12 +817,6 @@ class PullrequestsController(BaseRepoController):

        c.pull_request_reviewers = pull_request_at_ver.reviewers_statuses()
        c.pull_request_review_status = pull_request_at_ver.calculated_review_status()
-        c.pr_merge_status, c.pr_merge_msg = PullRequestModel().merge_status(
-            pull_request_at_ver)
-        c.approval_msg = None
-        if c.pull_request_review_status != ChangesetStatus.STATUS_APPROVED:
-            c.approval_msg = _('Reviewer approval is pending.')
-            c.pr_merge_status = False

        c.versions = pull_request_display_obj.versions()
        c.at_version = at_version
@ -830,7 +841,7 @@ class PullrequestsController(BaseRepoController):

        # if we use version, then do not show later comments
        # than current version
-        paths = collections.defaultdict(lambda: collections.defaultdict(list))
+        display_inline_comments = collections.defaultdict(lambda: collections.defaultdict(list))
        for co in inline_comments:
            if c.at_version_num:
                # pick comments that are at least UPTO given version, so we
@ -842,11 +853,32 @@ class PullrequestsController(BaseRepoController):
                should_render = True

            if should_render:
-                paths[co.f_path][co.line_no].append(co)
-        inline_comments = paths
+                display_inline_comments[co.f_path][co.line_no].append(co)
+
+        c.pr_merge_checks = []
+        c.pr_merge_status, c.pr_merge_msg = PullRequestModel().merge_status(
+            pull_request_at_ver)
+        c.pr_merge_checks.append(['warning' if not c.pr_merge_status else 'success', c.pr_merge_msg])
+
+        if c.pull_request_review_status != ChangesetStatus.STATUS_APPROVED:
+            approval_msg = _('Reviewer approval is pending.')
+            c.pr_merge_status = False
+            c.pr_merge_checks.append(['warning', approval_msg])
+
+        todos = cc_model.get_unresolved_todos(pull_request_latest)
+        if todos:
+            c.pr_merge_status = False
+            if len(todos) == 1:
+                msg = _('{} todo still not resolved.').format(len(todos))
+            else:
+                msg = _('{} todos still not resolved.').format(len(todos))
+            c.pr_merge_checks.append(['warning', msg])
+
+        if merge_checks:
+            return render('/pullrequests/pullrequest_merge_checks.mako')

        # load compare data into template context
-        self._load_compare_data(pull_request_at_ver, inline_comments)
+        self._load_compare_data(pull_request_at_ver, display_inline_comments)

        # this is a hack to properly display links, when creating PR, the
        # compare view and others uses different notation, and
@ -861,10 +893,6 @@ class PullrequestsController(BaseRepoController):
            statuses = ChangesetStatus.STATUSES
        c.commit_statuses = statuses

-        c.ancestor = None  # TODO: add ancestor here
-        c.pull_request = pull_request_display_obj
-        c.pull_request_latest = pull_request_latest
-
        c.changes = None
        c.file_changes = None

--- a/rhodecode/model/comment.py
+++ b/rhodecode/model/comment.py
@ -129,6 +129,18 @@ class CommentsModel(BaseModel):

        return comment_versions

+    def get_unresolved_todos(self, pull_request):
+
+        todos = Session().query(ChangesetComment) \
+            .filter(ChangesetComment.pull_request == pull_request) \
+            .filter(ChangesetComment.resolved_by == None) \
+            .filter(ChangesetComment.comment_type
+                    == ChangesetComment.COMMENT_TYPE_TODO) \
+            .filter(coalesce(ChangesetComment.display_state, '') !=
+                    ChangesetComment.COMMENT_OUTDATED).all()
+
+        return todos
+
    def create(self, text, repo, user, commit_id=None, pull_request=None,
               f_path=None, line_no=None, status_change=None,
               status_change_type=None, comment_type=None,
--- a/rhodecode/model/db.py
+++ b/rhodecode/model/db.py
@ -2972,6 +2972,10 @@ class ChangesetComment(Base, BaseModel):
    def resolved(self):
        return self.resolved_by[0] if self.resolved_by else None

+    @property
+    def is_todo(self):
+        return self.comment_type == self.COMMENT_TYPE_TODO
+
    def get_index_version(self, versions):
        return self.get_index_from_version(
            self.pull_request_version_id, versions)
--- a/rhodecode/public/css/main.less
+++ b/rhodecode/public/css/main.less
@ -1610,20 +1610,54 @@ BIN_FILENODE = 7
 }

 .pull-request-merge {
-    padding: 10px 0;
+    border: 1px solid @grey5;
+    padding: 10px 0px 20px;
    margin-top: 10px;
    margin-bottom: 20px;
 }

+.pull-request-merge ul {
+    padding: 0px 0px;
+}
+
+.pull-request-merge li:before{
+    content:none;
+}
+
 .pull-request-merge .pull-request-wrap {
-    height: 25px;
-    padding: 5px 0;
+    height: auto;
+    padding: 0px 0px;
+    text-align: right;
 }

 .pull-request-merge span {
-    margin-right: 10px;
+    margin-right: 5px;
 }

+.pull-request-merge-actions {
+    height: 30px;
+    padding: 0px 0px;
+}
+
+.merge-message {
+    font-size: 1.2em
+}
+.merge-message li{
+    text-decoration: none;
+}
+
+.merge-message.success i {
+    color:@alert1;
+}
+.merge-message.warning i {
+  color: @alert3;
+}
+.merge-message.error i {
+    color:@alert2;
+}
+
+
+
 .pr-versions {
    position: relative;
    top: 6px;
--- a/rhodecode/public/js/src/rhodecode/comments.js
+++ b/rhodecode/public/js/src/rhodecode/comments.js
@ -240,7 +240,12 @@ var bindToggleButtons = function() {
            $(this.statusChange).select2('readonly', false);
        };

-        this.globalSubmitSuccessCallback = function(){};
+        this.globalSubmitSuccessCallback = function(){
+            // default behaviour is to call GLOBAL hook, if it's registered.
+            if (window.commentFormGlobalSubmitSuccessCallback !== undefined){
+                commentFormGlobalSubmitSuccessCallback()
+            }
+        };

        this.submitAjaxPOST = function(url, postData, successHandler, failHandler) {
            failHandler = failHandler || function() {};
@ -300,7 +305,8 @@ var bindToggleButtons = function() {
            }

            var submitSuccessCallback = function(o) {
-                if (status) {
+                // reload page if we change status for single commit.
+                if (status && self.commitId) {
                    location.reload(true);
                } else {
                    $('#injected_page_comments').append(o.rendered_text);
--- a/rhodecode/templates/changeset/changeset_file_comment.mako
+++ b/rhodecode/templates/changeset/changeset_file_comment.mako
@ -170,29 +170,6 @@

 <%def name="comments(post_url, cur_status, is_pull_request=False, is_compare=False, change_status=True, form_extras=None)">

-## merge status, and merge action
-%if is_pull_request:
-<div class="pull-request-merge">
-    %if c.allowed_to_merge:
-    <div class="pull-request-wrap">
-        <div class="pull-right">
-          ${h.secure_form(url('pullrequest_merge', repo_name=c.repo_name, pull_request_id=c.pull_request.pull_request_id), id='merge_pull_request_form')}
-          <span data-role="merge-message">${c.pr_merge_msg} ${c.approval_msg if c.approval_msg else ''}</span>
-          <% merge_disabled = ' disabled' if c.pr_merge_status is False else '' %>
-          <input type="submit" id="merge_pull_request" value="${_('Merge Pull Request')}" class="btn${merge_disabled}"${merge_disabled}>
-          ${h.end_form()}
-        </div>
-    </div>
-    %else:
-    <div class="pull-request-wrap">
-        <div class="pull-right">
-          <span>${c.pr_merge_msg} ${c.approval_msg if c.approval_msg else ''}</span>
-        </div>
-    </div>
-    %endif
-</div>
-%endif
-
 <div class="comments">
    <%
      if is_pull_request:
--- a/rhodecode/templates/pullrequests/pullrequest_merge_checks.mako
+++ b/rhodecode/templates/pullrequests/pullrequest_merge_checks.mako
@ -0,0 +1,37 @@
+
+<div class="pull-request-wrap">
+
+    <ul>
+        % for pr_check_type, pr_check_msg in c.pr_merge_checks:
+            <li>
+                <span class="merge-message ${pr_check_type}" data-role="merge-message">
+                    % if pr_check_type in ['success']:
+                        <i class="icon-true"></i>
+                    % else:
+                        <i class="icon-false"></i>
+                    % endif
+                    ${pr_check_msg}
+                </span>
+            </li>
+        % endfor
+    </ul>
+
+    <div class="pull-request-merge-actions">
+        % if c.allowed_to_merge:
+        <div class="pull-right">
+          ${h.secure_form(url('pullrequest_merge', repo_name=c.repo_name, pull_request_id=c.pull_request.pull_request_id), id='merge_pull_request_form')}
+          <% merge_disabled = ' disabled' if c.pr_merge_status is False else '' %>
+          <a class="btn" href="#" onclick="refreshMergeChecks(); return false;">${_('refresh checks')}</a>
+          <input type="submit" id="merge_pull_request" value="${_('Merge Pull Request')}" class="btn${merge_disabled}"${merge_disabled}>
+          ${h.end_form()}
+        </div>
+        % elif c.rhodecode_user.username != h.DEFAULT_USER:
+            <a class="btn" href="#" onclick="refreshMergeChecks(); return false;">${_('refresh checks')}</a>
+            <input type="submit" value="${_('Merge Pull Request')}" class="btn disabled" disabled="disabled" title="${_('You are not allowed to merge this pull request.')}">
+        % else:
+          <input type="submit" value="${_('Login to Merge this Pull Request')}" class="btn disabled" disabled="disabled">
+        % endif
+    </div>
+
+</div>
+
--- a/rhodecode/templates/pullrequests/pullrequest_show.mako
+++ b/rhodecode/templates/pullrequests/pullrequest_show.mako
@ -482,23 +482,28 @@ Changed files:

      <div id="comment-tr-show">
          <div class="comment">
+            % if general_outdated_comm_count_ver:
            <div class="meta">
-                % if general_outdated_comm_count_ver:
-                    % if general_outdated_comm_count_ver == 1:
-                        ${_('there is {num} general comment from older versions').format(num=general_outdated_comm_count_ver)},
-                        <a href="#" onclick="$('.comment-general.comment-outdated').show(); $(this).parent().hide(); return false;">${_('show it')}</a>
-                    % else:
-                        ${_('there are {num} general comments from older versions').format(num=general_outdated_comm_count_ver)},
-                        <a href="#" onclick="$('.comment-general.comment-outdated').show(); $(this).parent().hide(); return false;">${_('show them')}</a>
-                    % endif
+                % if general_outdated_comm_count_ver == 1:
+                    ${_('there is {num} general comment from older versions').format(num=general_outdated_comm_count_ver)},
+                    <a href="#" onclick="$('.comment-general.comment-outdated').show(); $(this).parent().hide(); return false;">${_('show it')}</a>
+                % else:
+                    ${_('there are {num} general comments from older versions').format(num=general_outdated_comm_count_ver)},
+                    <a href="#" onclick="$('.comment-general.comment-outdated').show(); $(this).parent().hide(); return false;">${_('show them')}</a>
                % endif
            </div>
+            % endif
          </div>
      </div>

      ${comment.generate_comments(c.comments, include_pull_request=True, is_pull_request=True)}

      % if not c.pull_request.is_closed():
+        ## merge status, and merge action
+        <div class="pull-request-merge">
+            <%include file="/pullrequests/pullrequest_merge_checks.mako"/>
+        </div>
+
        ## main comment form and it status
        ${comment.comments(h.url('pullrequest_comment', repo_name=c.repo_name,
                                  pull_request_id=c.pull_request.pull_request_id),
@ -603,6 +608,16 @@ Changed files:
                $('.showOutdatedComments').show();
            };

+            refreshMergeChecks = function(){
+                var loadUrl = "${h.url.current(merge_checks=1)}";
+                $('.pull-request-merge').css('opacity', 0.3);
+                $('.pull-request-merge').load(
+                    loadUrl,function() {
+                        $('.pull-request-merge').css('opacity', 1);
+                    }
+                );
+            };
+
            $('#show-outdated-comments').on('click', function(e){
                var button = $(this);
                var outdated = $('.comment-outdated');
@ -683,6 +698,12 @@ Changed files:
                  button.addClass("comments-visible");
                }
            });
+
+            // register submit callback on commentForm form to track TODOs
+            window.commentFormGlobalSubmitSuccessCallback = function(){
+                refreshMergeChecks();
+            };
+
        })
      </script>