Uploaded image for project: 'Bitbucket Cloud'
  1. Bitbucket Cloud
  2. BCLOUD-11208

Comment with Approval (BB-12666)

    XMLWordPrintable

Details

    • Our product teams collect and evaluate feedback from a number of different sources. To learn more about how we use customer feedback in the planning process, check out our new feature policy.

    Description

      Hey, thanks for responding on twitter.

      At work we use bitbucket as our repo for everything, (I'd be happy to provide the name or our account, but I'm not sure if these tickets are public or not. I'm logged in as I submit this with my work account that's associated with out companies team).

      At work we try to follow Clojure programming conventions quite strictly, but we've also been hiring a lot. We get many PRs from people who may not know the in's and out's of the style guide, they are technically sound, but have a lot of trivial mistakes (Extra spaces, extra newlines, camelcase function) names and the like. Of course, even experienced developers miss a typo or punctuation error once in a while.

      The thought is to have an "Approved with Comments" or an "Pending/Conditional Approval" option as to say:

      "I've noticed and commented on some issues, and once they are fixed you will have my approval. I trust you are capable to address these without me double checking, and once you do you will have my approval."

      I see it going like this:

      Submitter:
      Submits their branch as a PR message.

      Reviewer:
      Runs through the changes, and sees there is no technical issues. It looks good, but they have a typo in a log message. It's all good, but I don't want to approve it with that potentially embarrassing and possibly client visible problem.

      So I make a comment, but instead of pressing the comment button, I press the "Submit and mark pending". The comment shows up like any other comment.

      I don't see any other issues so I approve it. My avatar at the top shows up with a "?" in a yellow circle, instead of the normal green checkmark.

      I notice my comment, which has yet to be addressed shows up a slightly more prominent to indicate that I selected the comment to hold up my approval.

      Submitter:
      Gets the alert that there is a comment and a pending approval on their PR, they address the issue, push their typo correcting commit to bitbucket and goes to reply to the comment to indicate that it's fixed. They type their message and instead of selecting "comment" as usual, they select the "Comment and mark resolved" option that shows up when replying to to a pending comment.

      As soon as they reply they notice the yellow "?" over the commenters avatar turn to that satisfying green check.

      Reviewer:
      Notices a message that the submitter has resolved my comment. No action needed.

      (As an alternative it could be marked as resolved when the line changes, but that seems to detract from the implied culpability that pressing "I acknowledge and believe I have resolve this as per your comment" that clicking "Mark Resolved" has)

      For our case the other stakeholders may be.
      Team Lead (w/Merge permissions):

      • Can see that reviewing is close. (They saw it and approve except for these 2 things)
      • If all have approved, except for one guy waiting on one trivial comment, they can either override and merge anyway (Jira raised as tech debt for un-addressed comment?), or fix it for the submitter.
      • Has a good idea of the team consensus on how close a PR is to being ready to merge.

      Alternatives:
      Instead of a "?" for pending, put the number of comments marked as pending and unresolved by the user.

      Perhaps, this is too specific to our particular situation, but for us we work around this by approving anyways and then adding a comment to the PR that states "Approved with comments". Which works, but leads to a lot of time checking every comment, to make sure they addressed them all. Not to mention a team lead who can miss the "with comments" comment, and merge it without it being fixed.

      If you know of any way there is any way to do this or something similar with an API I'd be interested as well.

      Attachments

        Activity

          People

            Unassigned Unassigned
            706223b15a93 AndrewTarzwell
            Votes:
            2 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: