fix: detect approvals when approval_state reports no rules - #641
fix: detect approvals when approval_state reports no rules#641sergey-kalin wants to merge 1 commit into
Conversation
get_merge_request_approval_state derives approved_by solely from approval_state's rules array, and only falls back to /approvals on a 404. Approval rules are a paid-tier feature. A project without them answers GET /approval_state with HTTP 200 and `rules: []`, so the flatMap over rules yields no approvers even when someone has approved the MR. The fallback never fires because the request succeeded. Callers are told nobody approved. The same response also leaves `approved`, `user_has_approved` and `user_can_approve` null, since approval_state does not carry them -- only /approvals does. Two changes: - fall back on 402/403 as well as 404, for instances or namespaces that reject the endpoint rather than 404 it; - when approval_state returns an empty rules array, read /approvals and prefer its result if it lists approvers. Wrapped in try/catch so an unavailable /approvals keeps the previous behaviour. Keyed on the rules array rather than on the approver count, so a project using approval rules makes no extra request even while an MR is still waiting for its first approval -- a non-empty rules array means rules are in use and approval_state is authoritative. The added test covers the empty-rules case; the existing rules test asserts that path still resolves from approval_state alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change updates merge request approval lookup fallback behavior. It now falls back from ChangesMerge request approval fallback flow
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
README says:
That fallback doesn't work in certain setup.
get_merge_request_approval_statederives approved_by solely from approval_state's rules array, and only falls back to /approvals on a 404.Approval rules are a paid-tier feature. A project on Premium Gitlab account but without rules configured answers GET
/approval_statewith HTTP 200 andrules: [], so the flatMap over rules yields no approvers even when someone has approved the MR. The fallback never fires because the request succeeded. Callers are told nobody approved with Approval rules despite of real approvers status.The same response also leaves
approved,user_has_approvedanduser_can_approvenull, since approval_state does not carry them -- only /approvals does.Two changes:
approval_statereturns an empty rules array, read /approvals and prefer its result if it lists approvers. Wrapped in try/catch, so an unavailable /approvals preserves existing behaviour.Keyed on the rules array rather than on the resulting approver count. A non-empty rules array means rules are in use and
approval_stateis authoritative, so a project using approval rules makes no extra request.