Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Fix ping stale reviewers on subsequent reviews #293

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Dec 28, 2020

#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 jablko force-pushed the patch-49 branch 3 times, most recently from 4346ceb to f7c6bb3 Compare January 6, 2021 14:47
Copy link
Contributor

@elibarzilay elibarzilay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused here. The current code in pr-info chooses the most recent review for each reviewer, and then the code that your changing here takes the earliest of these which I think makes sense (but not sure). With this change, it would potentially make more nagging notifications when there are new reviews that become stale--??

One thing that is definitely missing (and would help clarify the above) is if you have a specific PR in mind where this change can be demonstrated (and used as a new test).

@@ -59,7 +57,7 @@ ${names.map(n => `${n}`).join(" ")}
});

export const PingStaleReviewer = (reviewedAbbrOid: string, reviewers: string[]) => ({
tag: `stale-ping-${sha256(reviewers.join("-")).substr(0, 6)}-${reviewedAbbrOid}`,
tag: `stale-ping-${reviewedAbbrOid}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you dropping the reviewers' hash too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A change in reviewers implies a change in reviewedAbbrOid, which the tag already contains, because the only way to change the stale reviewers is for a new review to become stale.

@jablko
Copy link
Contributor Author

jablko commented Jan 18, 2021

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

{
"type": "stale",
"reviewer": "ExE-Boss",
"date": "2021-01-15T07:34:48.000Z",
"abbrOid": "9ba9fdd"
},
{
"type": "stale",
"reviewer": "rbuckton",
"date": "2020-12-08T01:35:34.000Z",
"abbrOid": "54b22fd"
}

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.

@elibarzilay elibarzilay force-pushed the master branch 3 times, most recently from 1d39a68 to 9e931d1 Compare July 7, 2021 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants