search

reconquest / atlassian-external-hooks

External Hooks plugin for Atlassian Bitbucket
https://external-hooks.reconquest.io
Other
44 stars 37 forks source link

Add two new options for merge checks #76

Closed mmonsenhexagon closed 5 years ago

mmonsenhexagon commented 5 years ago

Add two new options for merge checks: Toggle result caching, and Include Details in summary (merge button tooltip)

I coded up these new options because we have need for them. We have been using an older version of the External Hooks plugin that had this behavior, but after upgrading we discovered that the new version had changed behavior.

First, we need our merge check results to be run each time the page is refreshed, rather than cached. This is because one of our hooks checks to see if the branch being merged has already been merged into a different branch. If it hasn't been merged yet, we return information about where the branch needs to be merged before this pull request can proceed. If this result is cached, then even if the user goes and merges the branch where it needs to go, the pull request will continue to show the error (since the merge doesn't change the current branch or the target branch.)

Second, we prefer to see the details of the check failure displayed in the tooltip of the Merge button, rather than as a comment to the pull request. This is especially important given the issue described in the paragraph above. If we didn't cache the results, we would get multiple comments logged to the pull request.

In summary, our normal workflow is to always have the merge check run, and to display the details of a failure in the tooltip of the Merge button (i.e. return the details in the summary portion of the RepositoryHookResult.)

If you would like any changes to be made, or code cleanup, I'll be happy to work on them. Thanks for considering my pull request.

seletskiy commented 5 years ago

Hey @mmonsenhexagon!

Thanks for valuable contribution.

There are little nitpicks that I would like to get fixed before merge.

seletskiy commented 5 years ago

Also, can you please split commit message into short summary and more detailed body, since long summary (>69 chars) doesn't fit in GitHub interface and is generally considered not good practice.

mmonsenhexagon commented 5 years ago

Hello,

I have made the changes you requested. I amended the commit to clean up the commit message and did a force-push to replace my previous commit. I hope that the wording I used on the descriptions is OK for you. Thanks again for considering my pull request.

I also changed the default value for the "Include Details In Tooltip" option from true to false. I think that using false as the default will cause the least disruption to current users of the plugin.

mmonsenhexagon commented 5 years ago

Also did another update to fix some formatting issues.

seletskiy commented 5 years ago

@mmonsenhexagon: thank you very much!

I will do thorough testing of new changes on my test setup soon and then will release new version on marketplace.