dt-mergebot icon indicating copy to clipboard operation
dt-mergebot copied to clipboard

Fix ping stale reviewers on subsequent reviews

Open jablko opened this issue 5 years ago • 1 comments

#79 put mostRecentReview.reviewedAbbrOid in the ping stale reviewers comment tag however .sort((l, r) => l.date.localeCompare(r.date))[0] in fact returns the least-recent review https://github.com/DefinitelyTyped/dt-mergebot/pull/79/files#diff-0dfef229af227cfaffa9c063923ac5b13c53e14422a6147d11bb95c28d98f37eR204

Since there are never any new least-recent reviews this makes that part of the tag impotent: subsequent reviews get the same tag.

This PR replaces min() with max() to get the behavior that presumably was intended.

jablko avatar Dec 28 '20 16:12 jablko

I added 49699 as a test (I had to edit it slightly because the CI isn't passing today).

I ran create-fixture 49699, then changed the checkSuite.conclusion from "FAILURE" to "SUCCESS".

  1. ExE-Boss and rbuckton both reviewed 54b22fd
  2. The author pushed new commits on Jan 1
  3. The bot pinged ExE-Boss and rbuckton (stale-ping-54b22fd)
  4. ExE-Boss reviewed 9ba9fd on Jan 15
  5. The author pushed new commits
  6. If the tests pass, the bot re-pings ExE-Boss and rbuckton (stale-ping-9ba9fdd)

The most recent reviews are https://github.com/DefinitelyTyped/dt-mergebot/blob/b8021e774f55d6bba2b49119953e118aadc89d89/src/_tests/fixtures/49699/derived.json#L126-L137

If we use the min() of these (current behavior) then the bot won't re-ping stale reviewers because all pings get the same tag, stale-ping-54b22fd from the min() review.

I confirmed that switching from max() back to min() does cause the test fail, because the bot doesn't re-ping after the latest commits.

jablko avatar Jan 18 '21 23:01 jablko