Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5806,7 +5806,13 @@ async function getMergeRequestApprovalState(
method: "GET",
});

if (approvalStateResponse.status === 404) {
// 404 when the endpoint is unavailable; 402/403 when the instance or namespace is
// not licensed for approval rules, which are a paid-tier feature.
if (
approvalStateResponse.status === 404 ||
approvalStateResponse.status === 402 ||
approvalStateResponse.status === 403
) {
return getMergeRequestApprovalsFallback(projectId, mergeRequestIid);
}

Expand All @@ -5820,6 +5826,22 @@ async function getMergeRequestApprovalState(
);
const approvedByUsernames = approvedByUsers.map(user => user.username);

// approval_state only reports approvals attributed to rules, so a project with no
// approval rules answers 200 with `rules: []` and yields no approvers even when
// someone has approved. Read /approvals in that case. A non-empty rules array means
// rules are in use and approval_state is authoritative, so no extra request is made
// there - including while an MR is still waiting for its first approval.
if ((parsedApprovalState.rules || []).length === 0) {
try {
const viaApprovals = await getMergeRequestApprovalsFallback(projectId, mergeRequestIid);
if ((viaApprovals.approved_by ?? []).length > 0) {
return viaApprovals;
}
} catch {
// /approvals unavailable as well - keep the approval_state answer below.
}
}

return {
...parsedApprovalState,
approved_by: approvedByUsers,
Expand Down
58 changes: 58 additions & 0 deletions test/test-merge-request-approval-state-tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const MOCK_TOKEN = "glpat-mock-token-approval";
const TEST_PROJECT_ID = "123";
const TEST_MR_IID_WITH_FALLBACK = "88";
const TEST_MR_IID_WITH_APPROVAL_STATE = "89";
const TEST_MR_IID_WITHOUT_RULES = "90";

async function callTool(
toolName: string,
Expand Down Expand Up @@ -160,6 +161,43 @@ describe("merge request approval state tools", () => {
}
);

// A project without approval rules answers 200 with an empty rules array, while
// the approval itself is only visible on /approvals.
mockGitLab.addMockHandler(
"get",
`/projects/${TEST_PROJECT_ID}/merge_requests/${TEST_MR_IID_WITHOUT_RULES}/approval_state`,
(_req, res) => {
res.json({
approval_rules_overwritten: false,
rules: [],
});
}
);

mockGitLab.addMockHandler(
"get",
`/projects/${TEST_PROJECT_ID}/merge_requests/${TEST_MR_IID_WITHOUT_RULES}/approvals`,
(_req, res) => {
res.json({
approved: true,
user_has_approved: false,
user_can_approve: true,
approved_by: [
{
user: {
id: "35",
username: "sergey.kravchenya",
name: "Sergey Kravchenya",
state: "active",
avatar_url: "https://gitlab.mock/uploads/avatar.png",
web_url: "https://gitlab.mock/sergey.kravchenya",
},
},
],
});
}
);

await mockGitLab.start();
mockGitLabUrl = mockGitLab.getUrl();
});
Expand Down Expand Up @@ -188,6 +226,26 @@ describe("merge request approval state tools", () => {
assert.strictEqual(result.approved_by[0].username, "sergey.kravchenya");
});

test("finds approvers via approvals when approval_state reports no rules", async () => {
const result = await callTool(
"get_merge_request_approval_state",
{
project_id: TEST_PROJECT_ID,
merge_request_iid: TEST_MR_IID_WITHOUT_RULES,
},
{
GITLAB_API_URL: `${mockGitLabUrl}/api/v4`,
GITLAB_PERSONAL_ACCESS_TOKEN: MOCK_TOKEN,
}
);

assert.strictEqual(result.source_endpoint, "approvals");
assert.strictEqual(result.approved, true);
assert.deepStrictEqual(result.approved_by_usernames, ["sergey.kravchenya"]);
assert.ok(Array.isArray(result.approved_by));
assert.strictEqual(result.approved_by[0].username, "sergey.kravchenya");
});

test("returns deduplicated approvers from approval_state rules", async () => {
const result = await callTool(
"get_merge_request_approval_state",
Expand Down