DangerJS incorrectly shows analyzed commit ID for Bitbucket Server

[platan]

Currently DangerJS always shows ID of the first commit from pull request in "Generated by ⚠️ dangerJS against ..." message. An example: for a pull request with commits: As you can see it shows a9fa68cc4af91bc783d4dda78e245ee2e0e550e8 instead of 2b7f54b0588.

DangerJS uses this resource to get list of commits of a pull request from Bitbucket Server https://docs.atlassian.com/bitbucket-server/rest/5.16.0/bitbucket-rest.html#idm8287054176, https://github.com/danger/danger-js/blob/caf593cbbfeb26215c3b740d6c1a23a1361cb84f/source/platforms/bitbucket_server/BitBucketServerAPI.ts#L112-L117

An example Bitbucket Server REST API response:

```bash curl http://localhost:7990/bitbucket/rest/api/1.0/projects/PROJECT_1/repos/rep_1/pull-requests/2/commits -s | jq . ``` ```json { "values": [ { "id": "2b7f54b0588ee30151b992c70f9c39255b2681dd", "displayId": "2b7f54b0588", "author": { "name": "admin", "emailAddress": "admin@example.com", "id": 1, "displayName": "Administrator", "active": true, "slug": "admin", "type": "NORMAL" }, "authorTimestamp": 1570031121000, "committer": { "name": "admin", "emailAddress": "admin@example.com", "id": 1, "displayName": "Administrator", "active": true, "slug": "admin", "type": "NORMAL" }, "committerTimestamp": 1570031121000, "message": "commit 2", "parents": [ { "id": "a9fa68cc4af91bc783d4dda78e245ee2e0e550e8", "displayId": "a9fa68cc4af" } ] }, { "id": "a9fa68cc4af91bc783d4dda78e245ee2e0e550e8", "displayId": "a9fa68cc4af", "author": { "name": "admin", "emailAddress": "admin@example.com", "id": 1, "displayName": "Administrator", "active": true, "slug": "admin", "type": "NORMAL" }, "authorTimestamp": 1570031111000, "committer": { "name": "admin", "emailAddress": "admin@example.com", "id": 1, "displayName": "Administrator", "active": true, "slug": "admin", "type": "NORMAL" }, "committerTimestamp": 1570031111000, "message": "commit 1", "parents": [ { "id": "0a943a29376f2336b78312d99e65da17048951db", "displayId": "0a943a29376" } ] } ], "size": 2, "isLastPage": true, "start": 0, "limit": 25, "nextPageStart": null } ```

The newest commit is at the beginning of the list.

DangerJS uses this resource to get list of commits of a pull request from GitHub https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request, https://github.com/danger/danger-js/blob/caf593cbbfeb26215c3b740d6c1a23a1361cb84f/source/platforms/github/GitHubAPI.ts#L211-L214

An example GitHub REST API response:

```bash curl https://api.github.com/repos/danger/danger-js/pulls/924/commits -H "Authorization: token xxx" -s | jq . ``` ```json [ { "sha": "6087602f560ed6da05de57cef161249f2ad92fb6", "node_id": "MDY6Q29tbWl0MTkwNTc4ODQwOjYwODc2MDJmNTYwZWQ2ZGEwNWRlNTdjZWYxNjEyNDlmMmFkOTJmYjY=", "commit": { "author": { "name": "Øyvind Smestad", "email": "oyvind.smestad@tidal.com", "date": "2019-09-26T15:58:14Z" }, "committer": { "name": "Øyvind Smestad", "email": "oyvind.smestad@tidal.com", "date": "2019-09-26T15:58:14Z" }, "message": "When getting diffs for binary files with BitBucket Server, Danger fails when trying to map over chunks\n\nThis fix avoids setting the `chunks` property in `structuredDiffForFile` if it is `undefined`.", "tree": { "sha": "fd50543b08f513597af623e611c7aefb48e77aab", "url": "https://api.github.com/repos/danger/danger-js/git/trees/fd50543b08f513597af623e611c7aefb48e77aab" }, "url": "https://api.github.com/repos/danger/danger-js/git/commits/6087602f560ed6da05de57cef161249f2ad92fb6", "comment_count": 0, "verification": { "verified": false, "reason": "unsigned", "signature": null, "payload": null } }, "url": "https://api.github.com/repos/danger/danger-js/commits/6087602f560ed6da05de57cef161249f2ad92fb6", "html_url": "https://github.com/danger/danger-js/commit/6087602f560ed6da05de57cef161249f2ad92fb6", "comments_url": "https://api.github.com/repos/danger/danger-js/commits/6087602f560ed6da05de57cef161249f2ad92fb6/comments", "author": null, "committer": null, "parents": [ { "sha": "35391126f11b25d8e268174109c91baef885130b", "url": "https://api.github.com/repos/danger/danger-js/commits/35391126f11b25d8e268174109c91baef885130b", "html_url": "https://github.com/danger/danger-js/commit/35391126f11b25d8e268174109c91baef885130b" } ] }, { "sha": "5194b63aed170881f52b8127876fea2ec053b64b", "node_id": "MDY6Q29tbWl0MTkwNTc4ODQwOjUxOTRiNjNhZWQxNzA4ODFmNTJiODEyNzg3NmZlYTJlYzA1M2I2NGI=", "commit": { "author": { "name": "Øyvind Smestad", "email": "oyvind.smestad@tidal.com", "date": "2019-09-26T16:03:38Z" }, "committer": { "name": "Øyvind Smestad", "email": "oyvind.smestad@tidal.com", "date": "2019-09-26T16:03:38Z" }, "message": "Update Changelog", "tree": { "sha": "e2e10f9cc569affb330de74f28c23ead3e79f88f", "url": "https://api.github.com/repos/danger/danger-js/git/trees/e2e10f9cc569affb330de74f28c23ead3e79f88f" }, "url": "https://api.github.com/repos/danger/danger-js/git/commits/5194b63aed170881f52b8127876fea2ec053b64b", "comment_count": 0, "verification": { "verified": false, "reason": "unsigned", "signature": null, "payload": null } }, "url": "https://api.github.com/repos/danger/danger-js/commits/5194b63aed170881f52b8127876fea2ec053b64b", "html_url": "https://github.com/danger/danger-js/commit/5194b63aed170881f52b8127876fea2ec053b64b", "comments_url": "https://api.github.com/repos/danger/danger-js/commits/5194b63aed170881f52b8127876fea2ec053b64b/comments", "author": null, "committer": null, "parents": [ { "sha": "6087602f560ed6da05de57cef161249f2ad92fb6", "url": "https://api.github.com/repos/danger/danger-js/commits/6087602f560ed6da05de57cef161249f2ad92fb6", "html_url": "https://github.com/danger/danger-js/commit/6087602f560ed6da05de57cef161249f2ad92fb6" } ] } ] ```

The newest commit is on the end of the list.

Commit ID for the " "Generated by ⚠️ dangerJS against ..." message is set here: https://github.com/danger/danger-js/blob/caf593cbbfeb26215c3b740d6c1a23a1361cb84f/source/runner/Executor.ts#L290 So the last commit in array should be the newest one.

We can reverse the list of commits from Bitbucket Server to fix this problem. I guess this should be a good place to do this https://github.com/danger/danger-js/blob/caf593cbbfeb26215c3b740d6c1a23a1361cb84f/source/platforms/bitbucket_server/BitBucketServerGit.ts#L127 (but maybe you have a better one). What do you think about this problem and the proposed solution? My only concert is that users may already use commits in their dangerjsfiles and such change can possibly break their configurations.

[orta]

This might just be a bug in the part which shows the status number, maybe that should use the opposite instead?

[platan]

As I wrote earlier GitHubAPI and BitbucketServerAPI returns commits sorted in different ways. And there is one place (Executors.ts) which selects commit - the one from the end of the list. I think both APIs should return commits sorted in the same order.

[nick28]

@platan Really Appreciate your analysis on this. I was seeing the same in Bitbucket Cloud. As already mentioned by you changing the commits can break the users' configuration. I feel the fix should be only in (Executors.ts). IMO Instead of the sort, we can just always compare the first and last commitIDs based on the timestamp to select the commitID. This fix would also be future proof as if any other PR review tools come around; possibility of the order of the commits can only be in one way or the other(reverse). @platan @orta Your thoughts over this ?

[orta]

seems like a good idea to me

[jayveersolanki]

Any update on above issue?