Manual approval should remove rejected

[rajivnathan]

Summary by CodeRabbit

• Bug Fixes
  • Manual approval now correctly clears any previously rejected status, ensuring the final signup state reflects approval.
  • Updated behavior is covered by an expanded test flow that verifies rejected states are removed when approval is applied.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 5d047eae-9ee7-4718-a9a3-d598f2ff6e44

📥 Commits

Reviewing files that changed from the base of the PR and between 2be93a7 and a902e11.

📒 Files selected for processing (2)

• pkg/states/state_manager.go
• pkg/states/state_manager_test.go

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• codeready-toolchain/api (manual)
• codeready-toolchain/toolchain-common (manual)
• codeready-toolchain/host-operator (manual)
• codeready-toolchain/toolchain-e2e (manual)

📜 Recent review details

⏰ Context from checks skipped due to timeout. (2)

• GitHub Check: Verify Dependencies
• GitHub Check: GolangCI Lint

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/states/state_manager_test.go
• pkg/states/state_manager.go

🔀 Multi-repo context codeready-toolchain/host-operator, codeready-toolchain/toolchain-e2e, codeready-toolchain/api

Linked repositories findings

codeready-toolchain/host-operator

• [::codeready-toolchain/host-operator::] controllers/usersignup/usersignup_controller.go:185-192 explicitly branches on states.Rejected(userSignup) before deactivation/approval handling. If manual approval now clears rejected, this path will no longer trigger for a previously rejected signup once approved.
• [::codeready-toolchain/host-operator::] controllers/usersignupcleanup/usersignup_cleanup_controller_test.go:241 and [::codeready-toolchain/host-operator::] controllers/deactivation/deactivation_controller_test.go:838 both use states.SetApprovedManually(...), so these tests exercise the same state helper affected by the PR.
• [::codeready-toolchain/host-operator::] controllers/usersignup/usersignup_controller_test.go:4977 has a “with manual approval” test case; worth confirming it doesn’t assert the old lingering rejected label/state.

codeready-toolchain/toolchain-e2e

• [::codeready-toolchain/toolchain-e2e::] test/e2e/user_management_test.go:641-655 creates a signup with states.SetRejected(userSignup, true) and asserts it stays rejected and does not provision a MUR/Space. This remains consistent for direct rejection, but it’s the main e2e coverage around the rejected state.
• [::codeready-toolchain/toolchain-e2e::] test/e2e/parallel/phone_lookup_test.go:94-118 checks that a previously rejected signup remains rejected after a failed verification attempt; the PR should not affect this path unless manual approval is involved.
• [::codeready-toolchain/toolchain-e2e::] testsupport/signup_request.go:275, [::codeready-toolchain/toolchain-e2e::] test/e2e/usersignup_test.go:704, and other setup helpers call states.SetApprovedManually(...), so manual-approval state behavior is used broadly in e2e scenarios.

codeready-toolchain/api

• [::codeready-toolchain/api::] api/v1alpha1/usersignup_types.go:178-180 defines UserSignupStateRejected as a shared contract (“will not be provisioned”), which is the state being removed by manual approval in this PR.

🔇 Additional comments (2)

pkg/states/state_manager.go (1)

11-18: 🎯 Functional Correctness

Confirm downstream consumers handle rejected→approved transition correctly.

The usersignup_controller.go in host-operator branches on states.Rejected(userSignup) before deactivation/approval handling. Since this change now clears Rejected whenever SetApprovedManually(userSignup, true) is called, previously-rejected signups will silently transition out of that branch once approved. This matches the PR's stated intent, but confirm the host-operator reconciliation logic and any status/condition updates (e.g., removing a stale "Rejected" condition) are consistent with this new state transition.

Source: Linked repositories

pkg/states/state_manager_test.go (1)

44-51: LGTM!

---

Walkthrough

The SetApprovedManually function in the state manager now clears the UserSignupStateRejected flag when approving a user, in addition to existing verification/deactivation state clearing. A corresponding test case was extended to verify that setting approval removes a prior rejected state.

Changes

Rejected State Clearing on Manual Approval

Layer / File(s)	Summary
Clear rejected flag on approval pkg/states/state_manager.go, pkg/states/state_manager_test.go	SetApprovedManually clears UserSignupStateRejected when approving, and the test asserts the rejected state is removed after approval.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Suggested labels: bug, test

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: manual approval now clears the rejected state.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}