lgtm/blocked not being removed on review re-request

[silverwind]

See go-gitea/gitea#29191. The bot did not remove the label after re-request which lifted the block. Maybe GitHub has changed something about this API because I recall this did work before.

Correctly, it should have removed lgtm/blocked and added lgtm/need-2.

[yardenshoham]

I think it's because @denykson removed the self-review request. He should try to request a self-review

[denyskon]

@yardenshoham Re-requesting review didn't work, so I tried removing the review request

[yardenshoham]

Hmmm that's weird, you can review the approval logic here

gitea-backporter/src/github.ts

Lines 288 to 342 in c76b286

	export const getPrReviewers = async (
	pr: { number: number; requested_reviewers: { login: string }[] },
	): Promise<{ approvers: Set<string>; blockers: Set<string> }> => {
	// load all reviews
	const reviews: {
	state:
	| "APPROVED"
	| "CHANGES_REQUESTED"
	| "COMMENTED"
	| "DISMISSED"
	| "PENDING";
	user: { login: string };
	}[] = [];
	let page = 1;
	while (true) {
	const response = await fetch(
	`${GITHUB_API}/repos/go-gitea/gitea/pulls/${pr.number}/reviews?per_page=100&page=${page}`,
	{ headers: HEADERS },
	);
	if (!response.ok) throw new Error(await response.text());
	const results: [] = await response.json();
	if (results.length === 0) break;
	reviews.push(...results);
	page++;
	}
	// count approvers and blockers by replaying all reviews (they are already sorted)
	const approvers = new Set<string>();
	const blockers = new Set<string>();
	for (const review of reviews) {
	switch (review.state) {
	case "APPROVED":
	approvers.add(review.user.login);
	blockers.delete(review.user.login);
	break;
	case "DISMISSED":
	approvers.delete(review.user.login);
	blockers.delete(review.user.login);
	break;
	case "CHANGES_REQUESTED":
	approvers.delete(review.user.login);
	blockers.add(review.user.login);
	break;
	default:
	break;
	}
	}
	// any requested reviewers are not approvers
	for (const requestedReviewer of pr.requested_reviewers) {
	approvers.delete(requestedReviewer.login);
	}
	return { approvers, blockers };
	};

[yardenshoham]

Actually this is working as designed, we don't dismiss blockers on review request. Only on review dismiss. The problem is that we can't detect a self-review request, only that a review from the user is requested. So the algorithm has a flaw, you can't dismiss a blocker if you are a non-merger maintainer.

You could approve then request self-review, that'll do it.