[feature] Added support for cross-organization login #1055#1104
[feature] Added support for cross-organization login #1055#1104
Conversation
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a user registration method selection workflow enabling users to join different organizations with varying identity verification requirements. It introduces a new Sequence DiagramsequenceDiagram
participant User
participant CompleteSignup
participant Settings
participant Plans as Plans API
participant UpdateMethod as Update Method API
participant Navigation
User->>CompleteSignup: Navigate to /complete-signup
CompleteSignup->>Settings: Check subscriptions enabled
alt Subscriptions Enabled
CompleteSignup->>Plans: getPlans()
Plans-->>CompleteSignup: Return plans list
CompleteSignup->>CompleteSignup: Render plan selection
User->>CompleteSignup: Select plan
alt Paid Plan
CompleteSignup->>Plans: upgradePlan()
Plans-->>CompleteSignup: Return payment_url
CompleteSignup->>UpdateMethod: updateRegistrationMethod("bank_card")
UpdateMethod-->>CompleteSignup: Success
CompleteSignup->>Navigation: Redirect to /payment/draft
else Free Plan
CompleteSignup->>UpdateMethod: updateRegistrationMethod("mobile_phone")
UpdateMethod-->>CompleteSignup: Success
CompleteSignup->>Navigation: Redirect to /mobile-phone-verification
end
else Subscriptions Disabled
CompleteSignup->>Settings: Check mobile_phone_verification
alt Mobile Phone Enabled
CompleteSignup->>UpdateMethod: updateRegistrationMethod("mobile_phone")
UpdateMethod-->>CompleteSignup: Success
CompleteSignup->>Navigation: Redirect to /mobile-phone-verification
else Mobile Phone Disabled
CompleteSignup->>UpdateMethod: updateRegistrationMethod(null)
UpdateMethod-->>CompleteSignup: Success
CompleteSignup->>Navigation: Redirect to /status
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Test and Lint FailuresHello @pandafy,
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/components/complete-signup/complete-signup.js`:
- Around line 163-188: The code currently performs two separate backend writes
(upgradePendingVerificationPlan and updateRegistrationMethod) which can leave
state half-applied; replace the client-side sequential calls with a single
atomic API call (e.g., POST /api/organizations/:orgSlug/upgrade-and-set-method)
or add compensation logic: implement a server-side transactional handler in the
backend that exposes a new function/endpoint (name suggestion:
upgradeAndSetRegistrationMethod) that runs upgradePendingVerificationPlan and
updateRegistrationMethod in one DB/transactional operation and returns
success/failure, then call that from the client (instead of calling
upgradePendingVerificationPlan and updateRegistrationMethod separately). If you
cannot change the server, implement idempotent/compensating behavior on failure
in the client: after calling upgradePendingVerificationPlan, call
updateRegistrationMethod and if it fails call a rollback endpoint (or call
upgradePendingVerificationPlan with previous plan data) to revert the first
change, and ensure any navigation (navigate(getVerificationRoute(...))) and
setUserData updates only happen after the combined operation succeeds; reference
functions: upgradePendingVerificationPlan, updateRegistrationMethod,
setUserData, navigate, getVerificationRoute, and selectedPlan.
- Around line 75-80: handlePlansFailure and handlePlansSuccess allow an
empty/failed plans load to leave the UI stuck with no message, retry or
fallback; update handlePlansFailure and handlePlansSuccess (the handlers that
set {plans, plansFetched}) to set an error flag/message in state (e.g.,
plansError) when fetch fails or when plans is an empty array, and trigger a
retry mechanism or fallback behavior (e.g., show a retry button, load a
default/free plan, or redirect to an alternate onboarding path) so the user is
never left on a dead-end; make the same change for the duplicate handlers
referenced around the other occurrence (lines 273-283) so both code paths
present an error UI and retry/fallback options.
- Around line 118-145: The async flow calls setUserData, navigate and
setLoading(false) after awaits without checking unmount; update the block around
updateRegistrationMethod so all post-await side effects are guarded by the
component mount check (or moved into a single finalOperations helper that first
verifies isComponentMounted()). Specifically, after awaiting
updateRegistrationMethod in the try branch, verify isComponentMounted() (or call
finalOperations()) before calling setUserData(nextUserData) and
navigate(getVerificationRoute(...)) and likewise ensure the finally
setLoading(false) runs only when mounted; reference updateRegistrationMethod,
setUserData, navigate, setLoading, handleFlowError, and
isComponentMounted()/finalOperations to locate and fix the code.
In `@client/components/complete-signup/complete-signup.test.js`:
- Around line 76-122: Add tests that exercise the failed/empty plan-loading
branch: mock getPlans to invoke the failure path (e.g., make getPlans reject or
call wrapper.instance().handlePlansFailure with an error) and assert the
component does not render the .plans element and instead renders the expected
fallback UI (error message or no-plans copy) and does not navigate; also add a
test where getPlans returns an empty array (call
wrapper.instance().handlePlansSuccess([]) or mock resolved value) and assert the
same fallback behavior. Reference CompleteSignup, getPlans, handlePlansSuccess,
handlePlansFailure, wrapper, and props.navigate when adding these assertions.
In `@client/components/mobile-phone-change/mobile-phone-change.test.js`:
- Around line 426-434: The test mutates the shared userData object (used with
validateToken and mountComponent) causing order-dependent flaky tests; fix by
creating an isolated copy before modifying it (e.g., const localUser = {
...userData, is_active: true, method: 'pending_verification' }) and assign
props.userData = localUser so validateToken/mountComponent operate on the cloned
object, leaving the original userData unchanged and ensuring deterministic
behavior for the wrapper and StatusMock assertions.
In `@client/components/organization-wrapper/organization-wrapper.js`:
- Line 123: Call to userPendingVerification currently passes a second unused
argument (settings); remove the extra parameter so the call becomes
userPendingVerification(userData). Update the invocation at the location where
needsMethodSelection is assigned and ensure no other callers rely on a
two-argument signature; if any exist, either remove their second argument as
well or confirm the helper's signature (userPendingVerification) matches
single-argument usage.
In `@client/components/organization-wrapper/organization-wrapper.test.js`:
- Around line 620-626: The beforeEach in the OrganizationWrapper tests mocks
console.error but never restores it, leaking global test state; add an afterEach
in the same describe that restores console.error to its original value (capture
the original in the beforeEach or outer scope if needed) so tests using
beforeEach (which sets console.error = jest.fn()) are cleaned up; update the
describe containing beforeEach, needsVerify, userPendingVerification, and
wrapper initialization to include this afterEach that resets console.error back
to the captured original.
In `@client/utils/pending-verification.js`:
- Around line 1-10: The exported helper getEnabledVerificationMethods is
currently unused and causing the CI failure; either remove it or wire it into
the new verification flow. To fix: locate where verification methods are
computed (e.g., the new flow component or module that handles user settings) and
import getEnabledVerificationMethods to derive methods from settings, or if the
helper is redundant, delete the function and its export; alternatively, if it
should remain but has a different export pattern, change its export to the
module's primary export (or add a usage stub) so it is referenced. Update any
related export index files or imports (search for getEnabledVerificationMethods)
so the linter/Knip no longer reports it as an unused export.
In `@server/controllers/registration-method-controller.js`:
- Around line 15-30: The controller currently forwards req.body.method directly
in the axios call (see conf, host, url, data: {method: req.body.method}) which
allows disallowed methods to be chosen; add a validation step before the axios
request that computes allowedMethods from the merged config (conf) — e.g.
include bank_card only when conf.subscriptions is true and include mobile_phone
only when conf.phone_verification/enabled flag is true — then if req.body.method
is not in allowedMethods return a 400/validation error response and do not call
axios; keep the existing axios call unchanged except to use the validated method
value.
In `@server/controllers/registration-method-controller.test.js`:
- Around line 33-45: The tests call updateRegistrationMethod without awaiting it
and use await Promise.resolve()/setTimeout to sync, causing flaky timing; change
each async test to await the controller invocation directly (e.g., await
updateRegistrationMethod({...}, res)) and remove the subsequent await
Promise.resolve()/setTimeout calls—apply the same fix to the other async tests
flagged (the ones using await Promise.resolve() / setTimeout) so all assertions
run after the awaited handler resolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cff295e-b7a4-4533-ab1a-139482c55e66
⛔ Files ignored due to path filters (1)
client/components/organization-wrapper/__snapshots__/organization-wrapper.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
client/components/complete-signup/complete-signup.jsclient/components/complete-signup/complete-signup.test.jsclient/components/complete-signup/index.cssclient/components/complete-signup/index.jsclient/components/login/login.jsclient/components/login/login.test.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/organization-wrapper/lazy-import.jsclient/components/organization-wrapper/organization-wrapper.jsclient/components/organization-wrapper/organization-wrapper.test.jsclient/constants/index.jsclient/utils/get-plans.jsclient/utils/pending-verification.jsclient/utils/update-registration-method.jsclient/utils/upgrade-pending-verification-plan.jsserver/controllers/registration-method-controller.jsserver/controllers/registration-method-controller.test.jsserver/routes/account.jsserver/utils/openwisp-urls.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}
📄 CodeRabbit inference engine (Custom checks)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}: Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
General: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Files:
client/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/login/login.test.jsserver/controllers/registration-method-controller.test.jsclient/components/complete-signup/complete-signup.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/login/login.test.jsserver/controllers/registration-method-controller.test.jsclient/components/complete-signup/complete-signup.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-18T17:34:09.903Z
Learnt from: prathmeshkulkarni-coder
Repo: openwisp/openwisp-wifi-login-pages PR: 1063
File: client/components/status/status.test.js:2890-2916
Timestamp: 2026-03-18T17:34:09.903Z
Learning: Do not rely on mocking or altering window.location in Jest/JSDOM for this project. Since window.location is non-configurable in the test environment and attempts to redefine or delete it can cause state leaks across tests, avoid hacks like delete window.location or Object.defineProperty(window, 'location', ...) in tests. Instead, migrate detection-related tests to React Testing Library (RTL) and use RTL-friendly patterns (e.g., render components and inspect behavior via UI, or mock network/detection logic with explicit dependency injection or module mocks). This pattern should apply to all test files under client/components (any .test.js) where window.location might be interacted with.
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/login/login.test.jsclient/components/complete-signup/complete-signup.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), `finalOperations()` has a single early return before setting up intervals/listeners to prevent the scenario where the component unmounts during async awaits. `safeSetState` is applied to success paths in `getUserRadiusSessions()`, `getUserRadiusUsage()`, `getPlansSuccessCallback()`, and `handlePostMessage()`.
Applied to files:
client/components/login/login.jsclient/components/organization-wrapper/organization-wrapper.jsclient/components/complete-signup/complete-signup.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), the Status component uses `this.isComponentMounted` (set to `true` in the constructor, `false` in `componentWillUnmount`) together with a `safeSetState()` helper method that checks `isComponentMounted` before calling `setState()`. This is the preferred pattern to guard against "setState on unmounted component" warnings.
Applied to files:
client/components/organization-wrapper/organization-wrapper.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
client/components/login/login.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-25T00:22:16.841Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-25T00:22:16.841Z
Learning: In `openwisp/openwisp-wifi-login-pages` browser tests (e.g., `browser-test/mobile-verfication.test.js`), fixed delay waits (e.g., `driver.sleep()`) are intentionally used instead of `driver.wait()` for toast message animations. This is because the JS animation on toast messages does not expose a reliable DOM state to poll against, and testing confirmed that fixed waits are more robust in this context. Do not flag these as flaky-by-design without considering this justification.
Applied to files:
client/components/login/login.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
Applied to files:
client/components/complete-signup/complete-signup.test.js
🪛 GitHub Actions: OpenWisp WiFi Login Pages CI BUILD
client/components/mobile-phone-change/mobile-phone-change.test.js
[error] 436-441: Jest test failed: expected validateToken to be called with specific arguments, but it was not called (Number of calls: 0).
client/utils/pending-verification.js
[error] 1-1: Knip found unused export: getEnabledVerificationMethods
🔇 Additional comments (13)
client/components/complete-signup/index.css (1)
1-9: Scoped and clear styling changes.The selectors are appropriately scoped to
#complete-signup, and the layout/margin additions look good.client/constants/index.js (1)
23-24: API URL helper addition looks consistent.The new URL builder aligns with the existing constants pattern and endpoint naming.
server/utils/openwisp-urls.js (1)
16-16: Route-name mapping is correctly integrated.This path entry cleanly extends the existing reverse URL map.
client/utils/get-plans.js (1)
8-13: Failure callback wiring is a good addition.The optional callback is safely guarded and improves caller control over error-state handling.
Also applies to: 28-30
client/utils/update-registration-method.js (1)
1-18: Implementation is clean and aligned with the flow.Request shape and headers are consistent with the new registration-method API usage.
client/utils/upgrade-pending-verification-plan.js (1)
1-25: Plan-upgrade helper is implemented consistently.The payload and headers match the expected contract for pending-verification upgrades.
server/routes/account.js (1)
16-16:/registration-methodroute integration is consistent with existing account endpoints.Import and route wiring are clean, and wrapping with
errorHandlermatches the established pattern.Also applies to: 33-33
client/components/organization-wrapper/lazy-import.js (1)
37-39: Lazy import forCompleteSignupis correctly added.The new export matches existing chunk-loading conventions and keeps route-level code splitting intact.
client/components/complete-signup/index.js (1)
6-23: Redux connector wiring forCompleteSignuplooks correct.State mapping and dispatcher binding are coherent with the organization reducer and dispatcher helpers.
client/components/login/login.js (1)
303-323:handleAuthenticationrefactor is solid and the pending-verification branch is correctly prioritized.
nextUserDataconstruction is cleaner, and the early-return redirect to/${orgSlug}/complete-signupis correctly isolated.client/components/login/login.test.js (1)
40-40: Good coverage added for the pending-verification login flow.The new interaction tests validate both redirect and non-redirect paths and assert the updated
setUserDatacontract.Also applies to: 508-587
client/components/organization-wrapper/organization-wrapper.js (1)
343-368:complete-signuproute guard logic is correctly structured.The auth gate plus downstream redirects (
mobile-phone-verification,payment/draft,status) align well with the new flow.client/components/organization-wrapper/organization-wrapper.test.js (1)
606-658: Pending-verification route assertions are comprehensive and match the new guard behavior.The new cases cover redirect precedence and query-string preservation for the
login/*path.
| if (selectedPlan && selectedPlan.id) { | ||
| await upgradePendingVerificationPlan( | ||
| orgSlug, | ||
| selectedPlan.id, | ||
| userData.auth_token, | ||
| language, | ||
| ); | ||
| } | ||
| if (settings.mobile_phone_verification) { | ||
| await updateRegistrationMethod( | ||
| orgSlug, | ||
| "mobile_phone", | ||
| userData.auth_token, | ||
| language, | ||
| ); | ||
| nextUserData.method = "mobile_phone"; | ||
| setUserData(nextUserData); | ||
| navigate(getVerificationRoute(orgSlug, "mobile_phone")); | ||
| return; | ||
| } | ||
| await updateRegistrationMethod( | ||
| orgSlug, | ||
| "", | ||
| userData.auth_token, | ||
| language, | ||
| ); |
There was a problem hiding this comment.
The method/plan transition can commit only half of the flow.
Both submit handlers perform two backend writes in sequence. If the second call fails, the first one is already committed: e.g. bank_card is set without a payment draft, or the plan is changed while the user stays in pending_verification. Please make this transition atomic server-side, or add compensation/idempotency around the pair.
Also applies to: 217-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/components/complete-signup/complete-signup.js` around lines 163 - 188,
The code currently performs two separate backend writes
(upgradePendingVerificationPlan and updateRegistrationMethod) which can leave
state half-applied; replace the client-side sequential calls with a single
atomic API call (e.g., POST /api/organizations/:orgSlug/upgrade-and-set-method)
or add compensation logic: implement a server-side transactional handler in the
backend that exposes a new function/endpoint (name suggestion:
upgradeAndSetRegistrationMethod) that runs upgradePendingVerificationPlan and
updateRegistrationMethod in one DB/transactional operation and returns
success/failure, then call that from the client (instead of calling
upgradePendingVerificationPlan and updateRegistrationMethod separately). If you
cannot change the server, implement idempotent/compensating behavior on failure
in the client: after calling upgradePendingVerificationPlan, call
updateRegistrationMethod and if it fails call a rollback endpoint (or call
upgradePendingVerificationPlan with previous plan data) to revert the first
change, and ensure any navigation (navigate(getVerificationRoute(...))) and
setUserData updates only happen after the combined operation succeeds; reference
functions: upgradePendingVerificationPlan, updateRegistrationMethod,
setUserData, navigate, getVerificationRoute, and selectedPlan.
pandafy
force-pushed
the
issues/1055-join-multi-org
branch
from
9e4784d to
0d3113b
Compare
Knip Unused Exports and Django Admin Registration ErrorHello @pandafy, There are two main issues in the CI logs:
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
client/components/organization-wrapper/organization-wrapper.js (1)
122-123: 🧹 Nitpick | 🔵 TrivialRemove the unused second argument in
userPendingVerificationcall.The
userPendingVerificationhelper (fromclient/utils/pending-verification.js) only accepts a singleuserparameter. Passingsettingsas a second argument is unnecessary and misleading.Suggested fix
const needsVerifyBankCard = needsVerify("bank_card", userData, settings); - const needsMethodSelection = userPendingVerification(userData, settings); + const needsMethodSelection = userPendingVerification(userData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/organization-wrapper/organization-wrapper.js` around lines 122 - 123, The call to userPendingVerification is passing an extra, unused second argument; update the invocation in organization-wrapper.js by removing the second parameter so it calls userPendingVerification(userData) (keep the existing needsVerifyBankCard = needsVerify("bank_card", userData, settings) unchanged), ensuring only the single required user argument (userData) is passed to userPendingVerification to match the helper's signature.server/controllers/registration-method-controller.test.js (1)
33-44:⚠️ Potential issue | 🟠 MajorMake async assertions deterministic by awaiting the controller directly.
The test calls
updateRegistrationMethodwithout awaiting and usesawait Promise.resolve()for synchronization. This pattern is fragile and may cause flaky tests.Deterministic async test pattern
- updateRegistrationMethod( + await updateRegistrationMethod( { params: {organization: "default"}, body: {method: "mobile_phone"}, headers: { authorization: "Bearer test-token", "accept-language": "en", }, }, res, ); - await Promise.resolve(); expect(axios).toHaveBeenCalledWith({Apply the same fix to all tests using
await Promise.resolve()orsetTimeout(...)for synchronization (Lines 87-100, 114-127, 141-150, 172-180).As per coding guidelines: "Tests must be deterministic and not flaky, avoiding dependencies on timing … or unawaited async operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/controllers/registration-method-controller.test.js` around lines 33 - 44, The test currently calls updateRegistrationMethod(...) without awaiting and relies on await Promise.resolve() for synchronization, which is flaky; change the test to await the call to updateRegistrationMethod(...) directly and remove the await Promise.resolve() lines, and apply the same change to the other tests that use await Promise.resolve() or setTimeout(...) (the blocks around lines referenced: 87-100, 114-127, 141-150, 172-180) so each async controller invocation is awaited deterministically.client/components/mobile-phone-change/mobile-phone-change.test.js (1)
438-446:⚠️ Potential issue | 🟡 MinorAvoid shared-state mutation in test setup.
The test directly mutates the shared
userDataobject (Lines 440-442), which can cause test order dependency and flaky behavior.Proposed fix
it("should redirect if user registration method is pending_verification", async () => { validateToken.mockReturnValue(true); - props.userData = userData; - props.userData.is_active = true; - props.userData.method = "pending_verification"; - props.settings.mobile_phone_verification = true; + props.userData = { + ...userData, + is_active: true, + method: "pending_verification", + }; + props.settings = { + ...props.settings, + mobile_phone_verification: true, + }; wrapper = await mountComponent(props); expect(wrapper.find(StatusMock)).toHaveLength(1); });As per coding guidelines: "Tests must be deterministic and not flaky, avoiding ... shared global state."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/mobile-phone-change/mobile-phone-change.test.js` around lines 438 - 446, The test mutates the shared userData fixture (props.userData = userData; then setting is_active and method), which can cause flaky tests; update the setup in this spec ("should redirect if user registration method is pending_verification") to assign a fresh copy of userData to props.userData (e.g., via object spread or a clone) and then set is_active and method on that new object before calling mountComponent; keep validateToken.mockReturnValue(true), wrapper = await mountComponent(props), and the assertion expect(wrapper.find(StatusMock)).toHaveLength(1) unchanged.client/components/complete-signup/complete-signup.js (3)
118-145:⚠️ Potential issue | 🟠 MajorGuard post-await redirects and global updates after unmount.
isComponentMountedcurrently protects onlysetStateSafe(). These async flows still callsetUserData,navigate, andsetLoading(false)after awaited requests resolve, so leaving/complete-signupmid-flight can still trigger stale redirects or overwrite global user state from an abandoned flow.Based on learnings, this codebase uses a mounted check before async success-path side effects to avoid unmount-during-await issues.
Also applies to: 161-196, 216-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/complete-signup/complete-signup.js` around lines 118 - 145, The async success path performs post-await side effects (setUserData, navigate, setLoading) without checking component mount state; update the try block after each await in the complete-signup flow (the branches that call updateRegistrationMethod and then setUserData/navigate) to early-return if isComponentMounted() is false, and only call setUserData, navigate, and setLoading when mounted; also ensure the catch/finally use setStateSafe or guard setLoading(false) with isComponentMounted() to avoid updating global state or triggering redirects after unmount.
163-188:⚠️ Potential issue | 🟠 MajorThe method/plan transition can still commit only half of the flow.
Both submit handlers perform two backend writes in sequence. If the second request fails, the first one is already persisted, which can leave the user half-migrated—for example
bank_cardset without a payment draft, or the plan upgraded while the method remains pending. This needs an atomic backend operation or compensation/idempotency around the pair.Also applies to: 217-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/complete-signup/complete-signup.js` around lines 163 - 188, The current submit handlers call upgradePendingVerificationPlan(...) and updateRegistrationMethod(...) sequentially which can leave the user half-migrated if the second call fails; change the flow to use an atomic backend endpoint (e.g., create a single API like POST /orgs/:orgSlug/complete-registration that takes selectedPlan.id and method) that performs both operations in one transaction, or implement a compensating/idempotent server-side routine so either both succeed or any partial change is rolled back; update the client handlers to call that single endpoint (instead of calling upgradePendingVerificationPlan and updateRegistrationMethod separately) and apply the same fix to the other submit location around lines 217-229.
75-80:⚠️ Potential issue | 🟠 MajorFailed or empty plan loading still dead-ends this flow.
Both handlers mark plans as fetched even when the list is empty, and the render path then leaves the user on
/complete-signupwithout any selectable plan, retry, or fallback route. A transient plans API failure can therefore block signup completion entirely.Also applies to: 273-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/complete-signup/complete-signup.js` around lines 75 - 80, handlePlansSuccess and handlePlansFailure both set plansFetched: true even when plans is empty or the API failed, which leaves the user stuck on /complete-signup with no options; modify these handlers so that when plans is empty or an error occurs (in handlePlansFailure and the empty-case in handlePlansSuccess) you set an error/retry state (e.g., plansError or plansRetryCount) instead of treating it as success, and update the component render logic to show a retry button, fallback message, or navigate to an alternate route when plansError is set; locate and update handlePlansSuccess, handlePlansFailure and the render path that depends on plansFetched to implement the retry/fallback behavior.server/controllers/registration-method-controller.js (1)
15-30:⚠️ Potential issue | 🟠 MajorReject registration methods that the target organization does not allow.
This endpoint still forwards
req.body.methodverbatim, so a direct POST can choosebank_cardfor an org withsubscriptions: falseormobile_phonewhen phone verification is disabled. That bypasses the UI gating this feature is supposed to preserve.Suggested guard
const conf = merge(defaultConfig, org); const {host} = conf; const url = reverse("update_registration_method", getSlug(conf)); const timeout = conf.timeout * 1000; const requestHeaders = req.headers || {}; + const requestedMethod = req.body?.method ?? ""; + const allowedMethods = new Set([ + "", + ...(conf.mobile_phone_verification ? ["mobile_phone"] : []), + ...(conf.subscriptions ? ["bank_card"] : []), + ]); + if (!allowedMethods.has(requestedMethod)) { + res.status(400).type("application/json").send({ + response_code: "BAD_REQUEST", + }); + return true; + } // make AJAX request axios({ method: "post", @@ url: `${host}${url}/`, timeout, - data: {method: req.body.method}, + data: {method: requestedMethod}, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/controllers/registration-method-controller.js` around lines 15 - 30, Before forwarding the request in registration-method-controller.js, validate req.body.method against the target org config (conf) and reject if the org disallows that method: compute allowed methods from conf (e.g., skip "bank_card" when conf.subscriptions is false and skip "mobile_phone" when conf.phone_verification is false), check that req.body.method is included, and if not respond early with a 400/appropriate error (do not call axios). Ensure this check happens before the axios call that currently uses conf, host, url, timeout and data: {method: req.body.method}.client/components/complete-signup/complete-signup.test.js (1)
76-122:⚠️ Potential issue | 🟡 MinorAdd coverage for failed and empty plan loads.
This suite exercises only the successful fetch path. Please add tests for
handlePlansFailure()andhandlePlansSuccess([])and assert the component does not leave the user with silent no-op UI or an unexpected navigation.As per coding guidelines, tests must cover error paths, boundary values, and unusual inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/complete-signup/complete-signup.test.js` around lines 76 - 122, Add two tests to cover error and empty-plan boundaries: call wrapper.instance().handlePlansFailure(new Error("fetch failed")) and assert the component does not call props.navigate or updateRegistrationMethod and that the UI shows an error/fallback message (e.g., wrapper.text() contains an error or retry prompt) instead of silently doing nothing; and call wrapper.instance().handlePlansSuccess([]) and assert no ".plans" are rendered, no navigation occurs, and the UI shows the expected empty-state/fallback copy (organization name or retry link). Reference CompleteSignup, handlePlansFailure, handlePlansSuccess, getPlans, updateRegistrationMethod, and props.navigate when adding these assertions.client/components/organization-wrapper/organization-wrapper.test.js (1)
620-626:⚠️ Potential issue | 🟡 MinorRestore
console.errorand the pending-verification mock after this describe.
console.error = jest.fn()anduserPendingVerification.mockReturnValue(true)both leak beyond this block. The later route suite only clears mock call history, so its behavior becomes order-dependent unless you restore the global and reset the mock implementation here.As per coding guidelines, deterministic tests must avoid shared global state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/organization-wrapper/organization-wrapper.test.js` around lines 620 - 626, Save the original console.error in the describe scope (e.g., const originalConsoleError = console.error) and then add an afterEach that restores it (console.error = originalConsoleError) and also resets the mocked implementations so they don't leak: call userPendingVerification.mockReset() (and needsVerify.mockReset() if it was changed elsewhere) in that afterEach to clear any mockReturnValue calls; this ensures the beforeEach's console.error = jest.fn() and userPendingVerification.mockReturnValue(true) do not affect other suites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/components/login/login.js`:
- Around line 303-324: The bank_card verification check mixes sources; replace
the condition that reads data.is_verified with nextUserData.is_verified for
consistency with the earlier use of nextUserData.method and to ensure you’re
checking the same object you just set via setUserData; update the conditional in
the block that calls redirectToPayment(orgSlug, navigate) to use
nextUserData.is_verified while leaving userPendingVerification(data),
setUserData(nextUserData), authenticate(true), and the redirect logic unchanged.
In `@client/components/mobile-phone-change/mobile-phone-change.js`:
- Around line 41-45: The submit-success path still calls this.setState directly
and can trigger setState-on-unmounted warnings; locate the submit handler’s
success callback (the function that currently calls this.setState after an async
submit) and replace those calls with this.setStateSafe(...) so it honors the
isComponentMounted guard; ensure you pass the same state object and optional
callback into setStateSafe and update any other async success paths in this
component to use setStateSafe as well.
In `@client/utils/get-plans.js`:
- Around line 28-30: The code calls failureCallback() without ensuring it's a
function, which can throw if a truthy non-function is passed; update the
invocation in get-plans.js (the failureCallback reference) to first check typeof
failureCallback === "function" before calling it (e.g., if (typeof
failureCallback === "function") failureCallback();), preserving existing
behavior and any arguments you currently pass to the callback.
In `@client/utils/pending-verification.js`:
- Around line 15-18: getVerificationRoute currently treats any method !==
"bank_card" as mobile verification which silently accepts unknown/future
methods; change it to explicitly handle supported methods (e.g., "bank_card" and
"mobile_phone" or whatever valid values you expect) using an explicit
conditional or switch inside getVerificationRoute(orgSlug, method) and return
the correct route for each supported method, and have the default branch throw
an informative error or return null so unsupported methods are not silently
routed to mobile verification; reference function name getVerificationRoute and
parameters orgSlug and method when updating logic.
In `@server/controllers/registration-method-controller.js`:
- Around line 38-39: The catch currently calls logResponseError(error) which may
end up logging the upstream axios request config including the forwarded
Authorization header; before calling logResponseError in the controller's catch
block, create a sanitized error object that removes or redacts sensitive headers
(e.g., remove error.response.config.headers.Authorization or replace with
"[REDACTED]") and pass that sanitized object to logResponseError; locate the
catch where logResponseError is invoked and also ensure any other logging paths
in this file (or the upstream request builder that forwards the Authorization
header) do not directly log error.response.config.headers without redaction.
---
Duplicate comments:
In `@client/components/complete-signup/complete-signup.js`:
- Around line 118-145: The async success path performs post-await side effects
(setUserData, navigate, setLoading) without checking component mount state;
update the try block after each await in the complete-signup flow (the branches
that call updateRegistrationMethod and then setUserData/navigate) to
early-return if isComponentMounted() is false, and only call setUserData,
navigate, and setLoading when mounted; also ensure the catch/finally use
setStateSafe or guard setLoading(false) with isComponentMounted() to avoid
updating global state or triggering redirects after unmount.
- Around line 163-188: The current submit handlers call
upgradePendingVerificationPlan(...) and updateRegistrationMethod(...)
sequentially which can leave the user half-migrated if the second call fails;
change the flow to use an atomic backend endpoint (e.g., create a single API
like POST /orgs/:orgSlug/complete-registration that takes selectedPlan.id and
method) that performs both operations in one transaction, or implement a
compensating/idempotent server-side routine so either both succeed or any
partial change is rolled back; update the client handlers to call that single
endpoint (instead of calling upgradePendingVerificationPlan and
updateRegistrationMethod separately) and apply the same fix to the other submit
location around lines 217-229.
- Around line 75-80: handlePlansSuccess and handlePlansFailure both set
plansFetched: true even when plans is empty or the API failed, which leaves the
user stuck on /complete-signup with no options; modify these handlers so that
when plans is empty or an error occurs (in handlePlansFailure and the empty-case
in handlePlansSuccess) you set an error/retry state (e.g., plansError or
plansRetryCount) instead of treating it as success, and update the component
render logic to show a retry button, fallback message, or navigate to an
alternate route when plansError is set; locate and update handlePlansSuccess,
handlePlansFailure and the render path that depends on plansFetched to implement
the retry/fallback behavior.
In `@client/components/complete-signup/complete-signup.test.js`:
- Around line 76-122: Add two tests to cover error and empty-plan boundaries:
call wrapper.instance().handlePlansFailure(new Error("fetch failed")) and assert
the component does not call props.navigate or updateRegistrationMethod and that
the UI shows an error/fallback message (e.g., wrapper.text() contains an error
or retry prompt) instead of silently doing nothing; and call
wrapper.instance().handlePlansSuccess([]) and assert no ".plans" are rendered,
no navigation occurs, and the UI shows the expected empty-state/fallback copy
(organization name or retry link). Reference CompleteSignup, handlePlansFailure,
handlePlansSuccess, getPlans, updateRegistrationMethod, and props.navigate when
adding these assertions.
In `@client/components/mobile-phone-change/mobile-phone-change.test.js`:
- Around line 438-446: The test mutates the shared userData fixture
(props.userData = userData; then setting is_active and method), which can cause
flaky tests; update the setup in this spec ("should redirect if user
registration method is pending_verification") to assign a fresh copy of userData
to props.userData (e.g., via object spread or a clone) and then set is_active
and method on that new object before calling mountComponent; keep
validateToken.mockReturnValue(true), wrapper = await mountComponent(props), and
the assertion expect(wrapper.find(StatusMock)).toHaveLength(1) unchanged.
In `@client/components/organization-wrapper/organization-wrapper.js`:
- Around line 122-123: The call to userPendingVerification is passing an extra,
unused second argument; update the invocation in organization-wrapper.js by
removing the second parameter so it calls userPendingVerification(userData)
(keep the existing needsVerifyBankCard = needsVerify("bank_card", userData,
settings) unchanged), ensuring only the single required user argument (userData)
is passed to userPendingVerification to match the helper's signature.
In `@client/components/organization-wrapper/organization-wrapper.test.js`:
- Around line 620-626: Save the original console.error in the describe scope
(e.g., const originalConsoleError = console.error) and then add an afterEach
that restores it (console.error = originalConsoleError) and also resets the
mocked implementations so they don't leak: call
userPendingVerification.mockReset() (and needsVerify.mockReset() if it was
changed elsewhere) in that afterEach to clear any mockReturnValue calls; this
ensures the beforeEach's console.error = jest.fn() and
userPendingVerification.mockReturnValue(true) do not affect other suites.
In `@server/controllers/registration-method-controller.js`:
- Around line 15-30: Before forwarding the request in
registration-method-controller.js, validate req.body.method against the target
org config (conf) and reject if the org disallows that method: compute allowed
methods from conf (e.g., skip "bank_card" when conf.subscriptions is false and
skip "mobile_phone" when conf.phone_verification is false), check that
req.body.method is included, and if not respond early with a 400/appropriate
error (do not call axios). Ensure this check happens before the axios call that
currently uses conf, host, url, timeout and data: {method: req.body.method}.
In `@server/controllers/registration-method-controller.test.js`:
- Around line 33-44: The test currently calls updateRegistrationMethod(...)
without awaiting and relies on await Promise.resolve() for synchronization,
which is flaky; change the test to await the call to
updateRegistrationMethod(...) directly and remove the await Promise.resolve()
lines, and apply the same change to the other tests that use await
Promise.resolve() or setTimeout(...) (the blocks around lines referenced:
87-100, 114-127, 141-150, 172-180) so each async controller invocation is
awaited deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce9b23fd-19c1-4513-9a98-d87f77f8c17a
⛔ Files ignored due to path filters (1)
client/components/organization-wrapper/__snapshots__/organization-wrapper.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
client/components/complete-signup/complete-signup.jsclient/components/complete-signup/complete-signup.test.jsclient/components/complete-signup/index.cssclient/components/complete-signup/index.jsclient/components/login/login.jsclient/components/login/login.test.jsclient/components/mobile-phone-change/mobile-phone-change.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/organization-wrapper/lazy-import.jsclient/components/organization-wrapper/organization-wrapper.jsclient/components/organization-wrapper/organization-wrapper.test.jsclient/constants/index.jsclient/utils/get-plans.jsclient/utils/pending-verification.jsclient/utils/update-registration-method.jsclient/utils/upgrade-pending-verification-plan.jsserver/controllers/registration-method-controller.jsserver/controllers/registration-method-controller.test.jsserver/routes/account.jsserver/utils/openwisp-urls.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests and Coverage
- GitHub Check: QA-Checks
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}
📄 CodeRabbit inference engine (Custom checks)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}: Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
General: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Files:
client/components/mobile-phone-change/mobile-phone-change.test.jsserver/controllers/registration-method-controller.test.jsclient/components/complete-signup/complete-signup.test.jsclient/components/login/login.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.test.jsserver/controllers/registration-method-controller.test.jsclient/components/complete-signup/complete-signup.test.jsclient/components/login/login.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-18T17:34:09.903Z
Learnt from: prathmeshkulkarni-coder
Repo: openwisp/openwisp-wifi-login-pages PR: 1063
File: client/components/status/status.test.js:2890-2916
Timestamp: 2026-03-18T17:34:09.903Z
Learning: Do not rely on mocking or altering window.location in Jest/JSDOM for this project. Since window.location is non-configurable in the test environment and attempts to redefine or delete it can cause state leaks across tests, avoid hacks like delete window.location or Object.defineProperty(window, 'location', ...) in tests. Instead, migrate detection-related tests to React Testing Library (RTL) and use RTL-friendly patterns (e.g., render components and inspect behavior via UI, or mock network/detection logic with explicit dependency injection or module mocks). This pattern should apply to all test files under client/components (any .test.js) where window.location might be interacted with.
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/complete-signup/complete-signup.test.jsclient/components/login/login.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.test.jsserver/controllers/registration-method-controller.test.jsclient/components/complete-signup/complete-signup.test.jsclient/components/login/login.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-25T00:22:16.841Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-25T00:22:16.841Z
Learning: In `openwisp/openwisp-wifi-login-pages` browser tests (e.g., `browser-test/mobile-verfication.test.js`), fixed delay waits (e.g., `driver.sleep()`) are intentionally used instead of `driver.wait()` for toast message animations. This is because the JS animation on toast messages does not expose a reliable DOM state to poll against, and testing confirmed that fixed waits are more robust in this context. Do not flag these as flaky-by-design without considering this justification.
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.test.jsserver/controllers/registration-method-controller.test.jsclient/components/login/login.test.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), the Status component uses `this.isComponentMounted` (set to `true` in the constructor, `false` in `componentWillUnmount`) together with a `safeSetState()` helper method that checks `isComponentMounted` before calling `setState()`. This is the preferred pattern to guard against "setState on unmounted component" warnings.
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/mobile-phone-change/mobile-phone-change.jsclient/components/complete-signup/complete-signup.jsclient/components/organization-wrapper/organization-wrapper.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), `finalOperations()` has a single early return before setting up intervals/listeners to prevent the scenario where the component unmounts during async awaits. `safeSetState` is applied to success paths in `getUserRadiusSessions()`, `getUserRadiusUsage()`, `getPlansSuccessCallback()`, and `handlePostMessage()`.
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/mobile-phone-change/mobile-phone-change.jsclient/components/login/login.jsclient/components/complete-signup/complete-signup.jsclient/components/organization-wrapper/organization-wrapper.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), catch blocks deliberately do NOT check `isComponentMounted` — console warnings from catch paths are considered harmless. Only real memory leaks (timers, event listeners) and "setState on unmounted" in success paths are fixed. The maintainer (nemesifier) explicitly wants to avoid over-engineering with defensive checks in every catch block.
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.jsclient/components/complete-signup/complete-signup.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : General: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Applied to files:
client/components/complete-signup/complete-signup.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
Applied to files:
client/components/complete-signup/complete-signup.test.jsclient/components/login/login.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-23T21:28:04.132Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1072
File: config/__tests__/es5-compliance.test.js:22-29
Timestamp: 2026-03-23T21:28:04.132Z
Learning: In openwisp/openwisp-wifi-login-pages, `config/setup.js` and `config/build-translations.js` are project-level scripts that must be executed with `cwd` set to the project root (CURRENT_WORKING_DIR). They read from and write to project-relative directories (organizations/, public/, client/locales/, etc.) and will fail if run from any other directory. In the ES5 compliance test (`config/__tests__/es5-compliance.test.js`), the `beforeAll()` call to these scripts via `execSync` with `cwd: CURRENT_WORKING_DIR` is intentional and correct. There is no race risk because this is the only test file that calls setup. `yarn setup` is also already run before tests in CI, making the execSync just a local safety net.
Applied to files:
client/components/login/login.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), real memory leaks are fixed by calling `clearTimeout(this.usageRetryTimeoutId)` and `window.removeEventListener("message", ...)` in `componentWillUnmount()`. These were actual leaks — the retry timeout could fire and the message handler could run after unmount.
Applied to files:
client/components/complete-signup/complete-signup.js
🔇 Additional comments (12)
client/constants/index.js (1)
23-24: Endpoint helper aligns with the new account route.The new URL builder is consistent with the server-side
/registration-methodendpoint and existing API constant patterns.client/utils/upgrade-pending-verification-plan.js (1)
6-24: Utility wiring looks correct for pending-plan upgrade.Headers, payload shape, URL composition, and return value are consistent with this flow.
client/components/complete-signup/index.css (1)
1-9: Scoped layout styles are clean and consistent.The selectors are well-scoped and the spacing/flex changes fit the new complete-signup UI structure.
client/components/organization-wrapper/lazy-import.js (1)
37-39: Lazy route export is consistent with existing chunk-loading strategy.The new
CompleteSignuplazy import follows the same loading pattern used by neighboring route components.server/routes/account.js (1)
16-16: Route wiring is correct for registration-method updates.The new controller import and
POST /registration-methodregistration are consistent with the account router pattern.Also applies to: 33-33
server/utils/openwisp-urls.js (1)
16-16: LGTM!The new
update_registration_methodpath correctly follows the existing naming conventions and is properly excluded from the subscriptions prefix transformation, as expected for a radius-based account endpoint.client/components/login/login.test.js (2)
40-40: Good addition of default subscriptions setting.Adding
subscriptions: falseas a default ensures consistent baseline behavior across all tests.
508-587: Comprehensive test coverage for pending_verification redirect flow.Good coverage of:
- Phone-only enabled (508-529)
- Subscriptions-only enabled (530-551)
- Both methods enabled (552-573)
- Negative case when method is not
pending_verification(574-587)The tests properly validate
setUserDataarguments, navigation target, andauthenticate(true)call.client/components/complete-signup/index.js (1)
1-23: LGTM!Standard Redux connector implementation that correctly maps organization configuration and language state to props, and binds the required dispatcher functions. Follows the same pattern as other connected components in the codebase.
client/utils/update-registration-method.js (1)
6-16: LGTM!The utility correctly constructs the request with proper headers (
content-type,accept-language,Authorization) and delegates error handling to the caller. This is consistent with other API utilities in the codebase.client/components/organization-wrapper/organization-wrapper.js (1)
343-368: Well-structured complete-signup route with proper precedence checks.The route correctly:
- Requires authentication (redirects to login if not)
- Handles completion states by routing to the appropriate next step (phone verification, payment draft, or status)
- Only renders
CompleteSignupwhen method selection is still pendingserver/controllers/registration-method-controller.test.js (1)
61-77: Good synchronous test case for 404 handling.This test correctly validates the synchronous early-return path for unknown organizations without needing async handling.
| .catch((error) => { | ||
| logResponseError(error); |
There was a problem hiding this comment.
Do not log upstream axios errors with the forwarded Authorization header intact.
logResponseError logs error.response for 5xx responses, and that object includes the axios request config. Because this controller forwards the caller's Authorization header on Lines 23-26, a failing upstream call can write bearer tokens into server logs. Redact sensitive headers or log only sanitized fields here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/controllers/registration-method-controller.js` around lines 38 - 39,
The catch currently calls logResponseError(error) which may end up logging the
upstream axios request config including the forwarded Authorization header;
before calling logResponseError in the controller's catch block, create a
sanitized error object that removes or redacts sensitive headers (e.g., remove
error.response.config.headers.Authorization or replace with "[REDACTED]") and
pass that sanitized object to logResponseError; locate the catch where
logResponseError is invoked and also ensure any other logging paths in this file
(or the upstream request builder that forwards the Authorization header) do not
directly log error.response.config.headers without redaction.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/utils/pending-verification.js (1)
4-7:⚠️ Potential issue | 🟡 MinorHandle unsupported verification methods explicitly.
Any non-
"bank_card"value currently routes to mobile verification, which silently misroutes unknown/future methods.Suggested patch
export const getVerificationRoute = (orgSlug, method) => - method === "bank_card" - ? `/${orgSlug}/payment/draft` - : `/${orgSlug}/mobile-phone-verification`; + method === "bank_card" + ? `/${orgSlug}/payment/draft` + : method === "mobile_phone" + ? `/${orgSlug}/mobile-phone-verification` + : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/utils/pending-verification.js` around lines 4 - 7, getVerificationRoute currently treats every non-"bank_card" value as mobile verification which misroutes unknown methods; update the getVerificationRoute function to explicitly validate supported methods (e.g., "bank_card" and the explicit mobile method name used in your app such as "mobile_phone") and handle unsupported values by either throwing a clear error or returning null/undefined so callers can handle it; reference and change the getVerificationRoute(orgSlug, method) implementation to use an explicit conditional or switch that maps "bank_card" -> `/${orgSlug}/payment/draft`, "mobile_phone" (or your app's exact mobile method) -> `/${orgSlug}/mobile-phone-verification`, and otherwise produce an explicit failure path instead of defaulting to mobile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 110-111: Replace the mutable branch checkout in the CI git clone
command with an immutable pinned commit: instead of cloning --branch
issues/692-different-identity-verification, pin to a specific commit SHA
(replace with the chosen <COMMIT_SHA>) and update the command to fetch that
exact commit (either by using the commit SHA as the clone ref or by cloning then
checking out the SHA into the openwisp-radius clone). Update the git clone
invocation in the workflow so CI uses the fixed commit SHA rather than a branch
name.
---
Duplicate comments:
In `@client/utils/pending-verification.js`:
- Around line 4-7: getVerificationRoute currently treats every non-"bank_card"
value as mobile verification which misroutes unknown methods; update the
getVerificationRoute function to explicitly validate supported methods (e.g.,
"bank_card" and the explicit mobile method name used in your app such as
"mobile_phone") and handle unsupported values by either throwing a clear error
or returning null/undefined so callers can handle it; reference and change the
getVerificationRoute(orgSlug, method) implementation to use an explicit
conditional or switch that maps "bank_card" -> `/${orgSlug}/payment/draft`,
"mobile_phone" (or your app's exact mobile method) ->
`/${orgSlug}/mobile-phone-verification`, and otherwise produce an explicit
failure path instead of defaulting to mobile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79526f33-6dc8-4bd2-9984-5fdd292acfc7
📒 Files selected for processing (2)
.github/workflows/ci.ymlclient/utils/pending-verification.js
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it
🔇 Additional comments (1)
client/utils/pending-verification.js (1)
1-2:userPendingVerificationlogic is solid.The predicate is explicit and correctly guards the pending-verification state without unsafe property access.
| # TODO: Remove branch before merging | ||
| git clone --depth=1 --branch issues/692-different-identity-verification https://github.com/openwisp/openwisp-radius/ openwisp-radius |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mutable branch pinning pattern in CI workflow
rg -n --type=yaml -C2 'git clone.*openwisp-radius' .github/workflows/ci.yml
rg -n --type=yaml 'TODO: Remove branch before merging|--branch issues/692-different-identity-verification' .github/workflows/ci.ymlRepository: openwisp/openwisp-wifi-login-pages
Length of output: 592
Pin openwisp-radius to an immutable commit before merge.
At Line 111, cloning a mutable branch head makes CI non-deterministic—upstream force-push or updates can change results without changes in this PR. Use a fixed commit SHA instead.
Suggested change
- # TODO: Remove branch before merging
- git clone --depth=1 --branch issues/692-different-identity-verification https://github.com/openwisp/openwisp-radius/ openwisp-radius
+ OW_RADIUS_REF=<commit-sha-from-openwisp-radius-related-work>
+ git clone --depth=1 https://github.com/openwisp/openwisp-radius/ openwisp-radius
cd openwisp-radius
+ git fetch --depth=1 origin "$OW_RADIUS_REF"
+ git checkout --detach "$OW_RADIUS_REF"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 110 - 111, Replace the mutable branch
checkout in the CI git clone command with an immutable pinned commit: instead of
cloning --branch issues/692-different-identity-verification, pin to a specific
commit SHA (replace with the chosen <COMMIT_SHA>) and update the command to
fetch that exact commit (either by using the commit SHA as the clone ref or by
cloning then checking out the SHA into the openwisp-radius clone). Update the
git clone invocation in the workflow so CI uses the fixed commit SHA rather than
a branch name.
There was a problem hiding this comment.
@coderabbitai we will remove this before merging!
There was a problem hiding this comment.
@pandafy, understood! The # TODO comment itself serves as a good reminder. Just make sure to also remove the pinned openwisp-users force-reinstall step (Line 128) and revert the openwisp-radius branch dependency back to the default clone once openwisp/openwisp-radius#692 is merged and released.
🧠 Learnings used
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it
coderabbitai
Bot
added
enhancement
Code Style and Test FailuresHello @pandafy,
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 135: Replace the mutable branch tarball URL that installs openwisp-users
from "issues/497-export-users" with an immutable tarball URL pinned to a
specific commit SHA (i.e., change the pip install source to the same GitHub
tarball but using the full commit hash instead of the branch name), and
remove/update the associated TODO about removing branch references so CI uses
the pinned commit; locate the line containing the pip install command that
references
"https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" and
replace it with the tarball URL that includes the chosen commit SHA.
In `@browser-test/browser_test_utils.py`:
- Around line 11-19: The cleanup is deleting the cross-org user by
cross_org_data["email"] but the test creates/authenticates that user with
username=cross_org_data["phoneNumber"]; update the User.objects.filter and
RadiusAccounting.objects.filter calls that reference cross_org_data["email"] to
use cross_org_data["phoneNumber"] instead (keep the same delete() calls and
other filters unchanged) so the cross-org user and accounting rows are correctly
removed.
In `@browser-test/cross-org-phone-verification.test.js`:
- Around line 46-47: The CSS selector "div > p:nth-child(5) > span" used in the
final assertion is brittle; update the test to query a stable attribute (e.g.,
an id/class or data-testid) instead. Replace the call to getElementByCss(driver,
"div > p:nth-child(5) > span") so it selects the email element by a durable
selector you add in the app (for example data-testid="user-email") and keep the
assertion using emailElement.getText() compared to data.email; ensure
getElementByCss and the variable emailElement are updated to use that stable
selector.
- Line 41: The test currently calls getPhoneToken() which resolves the token
using a shared mobileVerificationTestUser.phoneNumber; change the test to call
getPhoneToken(phoneNumber) with the specific fixture's phoneNumber so the token
is fetched for the correct user, and update browser-test/utils.js and
browser-test/get_phone_token.py to accept a phoneNumber argument and use it
instead of reading mobileVerificationTestUser.phoneNumber (update function
signatures and any callers accordingly); ensure the test passes its fixture
phoneNumber through the utils layer into get_phone_token.py so the verification
token is deterministically fetched for that user.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3d921a6-daad-45e6-a79e-750ad40c4f8a
📒 Files selected for processing (5)
.github/workflows/ci.ymlbrowser-test/browser_test_utils.pybrowser-test/cross-org-phone-verification.test.jsbrowser-test/initialize_data.pybrowser-test/testData.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests and Coverage
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
browser-test/browser_test_utils.pybrowser-test/initialize_data.py
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}
📄 CodeRabbit inference engine (Custom checks)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}: Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
General: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Files:
browser-test/cross-org-phone-verification.test.js
🧠 Learnings (7)
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-25T00:22:16.841Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-25T00:22:16.841Z
Learning: In `openwisp/openwisp-wifi-login-pages` browser tests (e.g., `browser-test/mobile-verfication.test.js`), fixed delay waits (e.g., `driver.sleep()`) are intentionally used instead of `driver.wait()` for toast message animations. This is because the JS animation on toast messages does not expose a reliable DOM state to poll against, and testing confirmed that fixed waits are more robust in this context. Do not flag these as flaky-by-design without considering this justification.
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-23T21:28:04.132Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1072
File: config/__tests__/es5-compliance.test.js:22-29
Timestamp: 2026-03-23T21:28:04.132Z
Learning: In openwisp/openwisp-wifi-login-pages, `config/setup.js` and `config/build-translations.js` are project-level scripts that must be executed with `cwd` set to the project root (CURRENT_WORKING_DIR). They read from and write to project-relative directories (organizations/, public/, client/locales/, etc.) and will fail if run from any other directory. In the ES5 compliance test (`config/__tests__/es5-compliance.test.js`), the `beforeAll()` call to these scripts via `execSync` with `cwd: CURRENT_WORKING_DIR` is intentional and correct. There is no race risk because this is the only test file that calls setup. `yarn setup` is also already run before tests in CI, making the execSync just a local safety net.
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Bug Fixes: If the bug affects the user interface, a Selenium browser test should be included
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-04-14T18:44:55.656Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1101
File: browser-test/js-file-inject.test.js:0-0
Timestamp: 2026-04-14T18:44:55.656Z
Learning: In this repository’s browser tests under `browser-test/`, it is intentional and acceptable for tests to hardcode the local test base URL/port as `http://127.0.0.1:8080`. Do not flag occurrences like `browser-test/js-file-inject.test.js` for “environment coupling” or “hardcoded port/URL” issues, since the browser test environment is expected to run on that address.
Applied to files:
browser-test/cross-org-phone-verification.test.js
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
110-111: Use an immutable ref foropenwisp-radiuscheckout (already reported).At Line 111, CI still tracks a mutable branch head, so runs are non-deterministic if that branch moves.
| pip install -U pip setuptools wheel | ||
| pip install -U -e ".[saml]" | ||
| pip install -U -r requirements-test.txt | ||
| pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mutable dependency refs in workflow
rg -n --type=yaml -C2 \
'git clone.*--branch.*openwisp-radius|openwisp-users/tarball/issues/' \
.github/workflows/ci.ymlRepository: openwisp/openwisp-wifi-login-pages
Length of output: 724
🏁 Script executed:
cat -n .github/workflows/ci.yml | sed -n '105,140p'Repository: openwisp/openwisp-wifi-login-pages
Length of output: 1778
🏁 Script executed:
# Check the git history/blame for context on when line 135 was added relative to the TODO
git log -p --all -S "issues/497-export-users" -- .github/workflows/ci.yml | head -50Repository: openwisp/openwisp-wifi-login-pages
Length of output: 60
Pin openwisp-users to a commit SHA before merge.
Line 135 installs from a mutable branch tarball (issues/497-export-users), which breaks CI reproducibility. The TODO at line 110 notes that branch references must be removed before merging; ensure line 135 is also resolved by pinning to an immutable commit hash.
Suggested fix
- pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"
+ OW_USERS_REF=<commit-sha>
+ pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/${OW_USERS_REF}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 135, Replace the mutable branch tarball URL
that installs openwisp-users from "issues/497-export-users" with an immutable
tarball URL pinned to a specific commit SHA (i.e., change the pip install source
to the same GitHub tarball but using the full commit hash instead of the branch
name), and remove/update the associated TODO about removing branch references so
CI uses the pinned commit; locate the line containing the pip install command
that references
"https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" and
replace it with the tarball URL that includes the chosen commit SHA.
There was a problem hiding this comment.
@coderabbitai we will remove this before merging!
There was a problem hiding this comment.
@pandafy, sounds good! Just make sure to also clean up the --branch issues/692-different-identity-verification reference for openwisp-radius on line 110 (which already has a TODO: Remove branch before merging comment), along with the openwisp-users tarball on line 135. Both should be resolved before the PR is ready to merge.
🧠 Learnings used
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it
| const emailElement = await getElementByCss(driver, "div > p:nth-child(5) > span"); | ||
| expect(await emailElement.getText()).toEqual(data.email); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a stable selector for the final email assertion.
div > p:nth-child(5) > span is tied to the current markup order, so unrelated copy/layout changes will break this test. Prefer an id/class owned by the email field or a dedicated data-testid.
As per coding guidelines, tests must be deterministic and not flaky.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@browser-test/cross-org-phone-verification.test.js` around lines 46 - 47, The
CSS selector "div > p:nth-child(5) > span" used in the final assertion is
brittle; update the test to query a stable attribute (e.g., an id/class or
data-testid) instead. Replace the call to getElementByCss(driver, "div >
p:nth-child(5) > span") so it selects the email element by a durable selector
you add in the app (for example data-testid="user-email") and keep the assertion
using emailElement.getText() compared to data.email; ensure getElementByCss and
the variable emailElement are updated to use that stable selector.
2b8c2f5 to
6a7cad3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
browser-test/browser_test_utils.py (1)
14-14:⚠️ Potential issue | 🟠 MajorDelete cross-org records by
phoneNumber, notLine 14 and Line 19 use
cross_org_data["email"], but the created username is the phone number. These deletes can miss target rows and leave stale test data.Suggested fix
- User.objects.filter(username=cross_org_data["email"]).delete() + User.objects.filter(username=cross_org_data["phoneNumber"]).delete() @@ - RadiusAccounting.objects.filter(username=cross_org_data["email"]).delete() + RadiusAccounting.objects.filter(username=cross_org_data["phoneNumber"]).delete()Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser-test/browser_test_utils.py` at line 14, The delete calls are filtering by the wrong key: User.objects.filter(username=cross_org_data["email"]).delete() will miss rows because the username was set to the phone number; update both occurrences (the User.objects.filter(...).delete() calls) to use cross_org_data["phoneNumber"] instead of cross_org_data["email"] so the cleanup targets the created user records correctly..github/workflows/ci.yml (1)
110-111:⚠️ Potential issue | 🟠 MajorPin both external dependency refs to immutable commits before merge.
Line 111 and Line 135 still depend on mutable branch heads. That keeps CI non-deterministic and weakens supply-chain reproducibility.
Suggested fix
- # TODO: Remove branch before merging - git clone --depth=1 --branch issues/692-different-identity-verification https://github.com/openwisp/openwisp-radius/ openwisp-radius + OW_RADIUS_REF=<openwisp-radius-commit-sha> + git clone --depth=1 https://github.com/openwisp/openwisp-radius/ openwisp-radius cd openwisp-radius + git fetch --depth=1 origin "$OW_RADIUS_REF" + git checkout --detach "$OW_RADIUS_REF" echo "OpenWISP RADIUS commit: $(git rev-parse HEAD)" echo "OW_RADIUS_VERSION=$(git rev-parse HEAD)" >> $GITHUB_ENV python --version @@ - pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" + OW_USERS_REF=<openwisp-users-commit-sha> + pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/${OW_USERS_REF}"Also applies to: 135-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 110 - 111, The CI workflow is cloning external repositories using mutable branch names (the git clone command referencing branch issues/692-different-identity-verification and the similar clone at line 135); replace those mutable branch refs with immutable commit SHAs (or annotated tags) to pin dependencies for reproducible CI. Update the git clone lines to fetch the same repo but checkout the specific commit SHA (or use --branch with a tag), or perform git clone then git checkout <commit-sha> for the repository referenced by the git clone command for openwisp-radius so both occurrences (the clone at the current line and the other at line 135) reference an exact immutable commit instead of a branch name.browser-test/cross-org-phone-verification.test.js (2)
51-54:⚠️ Potential issue | 🟡 MinorUse a stable selector for the final email assertion.
At Lines 51-54, the positional selector is brittle (
nth-child). Prefer a dedicated id/class ordata-testidowned by the email field.As per coding guidelines, tests must be deterministic and not flaky.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser-test/cross-org-phone-verification.test.js` around lines 51 - 54, The positional CSS selector used to find the final email (the call to getElementByCss that assigns emailElement) is brittle; update the test to target a stable selector (e.g., a dedicated id, class, or data-testid) instead of "div > p:nth-child(5) > span". Modify the page under test to add a predictable attribute like data-testid="final-email" (or use an existing stable id/class) and change the getElementByCss call in cross-org-phone-verification.test.js to query that selector so the final email assertion uses a deterministic element reference.
43-43:⚠️ Potential issue | 🟠 MajorFetch the phone token for this test user explicitly.
At Line 43,
getPhoneToken()relies on implicit shared lookup. Pass the current fixture phone number so the token source is deterministic for this scenario.Suggested change in this file
- const token = getPhoneToken(); + const token = getPhoneToken(data.phoneNumber);As per coding guidelines, tests must be deterministic and avoid shared global state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser-test/cross-org-phone-verification.test.js` at line 43, Replace the implicit call to getPhoneToken() with a deterministic call that passes the test fixture's phone number: locate the getPhoneToken invocation in the test and change it to getPhoneToken(<the phone number variable used by this test fixture>) so the token source is explicit and deterministic (use the test's phoneNumber or fixture.phone variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 110-111: The CI workflow is cloning external repositories using
mutable branch names (the git clone command referencing branch
issues/692-different-identity-verification and the similar clone at line 135);
replace those mutable branch refs with immutable commit SHAs (or annotated tags)
to pin dependencies for reproducible CI. Update the git clone lines to fetch the
same repo but checkout the specific commit SHA (or use --branch with a tag), or
perform git clone then git checkout <commit-sha> for the repository referenced
by the git clone command for openwisp-radius so both occurrences (the clone at
the current line and the other at line 135) reference an exact immutable commit
instead of a branch name.
In `@browser-test/browser_test_utils.py`:
- Line 14: The delete calls are filtering by the wrong key:
User.objects.filter(username=cross_org_data["email"]).delete() will miss rows
because the username was set to the phone number; update both occurrences (the
User.objects.filter(...).delete() calls) to use cross_org_data["phoneNumber"]
instead of cross_org_data["email"] so the cleanup targets the created user
records correctly.
In `@browser-test/cross-org-phone-verification.test.js`:
- Around line 51-54: The positional CSS selector used to find the final email
(the call to getElementByCss that assigns emailElement) is brittle; update the
test to target a stable selector (e.g., a dedicated id, class, or data-testid)
instead of "div > p:nth-child(5) > span". Modify the page under test to add a
predictable attribute like data-testid="final-email" (or use an existing stable
id/class) and change the getElementByCss call in
cross-org-phone-verification.test.js to query that selector so the final email
assertion uses a deterministic element reference.
- Line 43: Replace the implicit call to getPhoneToken() with a deterministic
call that passes the test fixture's phone number: locate the getPhoneToken
invocation in the test and change it to getPhoneToken(<the phone number variable
used by this test fixture>) so the token source is explicit and deterministic
(use the test's phoneNumber or fixture.phone variable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3ac4acf-6f2b-4d42-a595-24caa765e43c
📒 Files selected for processing (5)
.github/workflows/ci.ymlbrowser-test/browser_test_utils.pybrowser-test/cross-org-phone-verification.test.jsbrowser-test/initialize_data.pybrowser-test/testData.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: QA-Checks
- GitHub Check: Tests and Coverage
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
browser-test/browser_test_utils.pybrowser-test/initialize_data.py
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}
📄 CodeRabbit inference engine (Custom checks)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}: Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
General: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Files:
browser-test/cross-org-phone-verification.test.js
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-25T00:22:16.841Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-25T00:22:16.841Z
Learning: In `openwisp/openwisp-wifi-login-pages` browser tests (e.g., `browser-test/mobile-verfication.test.js`), fixed delay waits (e.g., `driver.sleep()`) are intentionally used instead of `driver.wait()` for toast message animations. This is because the JS animation on toast messages does not expose a reliable DOM state to poll against, and testing confirmed that fixed waits are more robust in this context. Do not flag these as flaky-by-design without considering this justification.
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-23T21:28:04.132Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1072
File: config/__tests__/es5-compliance.test.js:22-29
Timestamp: 2026-03-23T21:28:04.132Z
Learning: In openwisp/openwisp-wifi-login-pages, `config/setup.js` and `config/build-translations.js` are project-level scripts that must be executed with `cwd` set to the project root (CURRENT_WORKING_DIR). They read from and write to project-relative directories (organizations/, public/, client/locales/, etc.) and will fail if run from any other directory. In the ES5 compliance test (`config/__tests__/es5-compliance.test.js`), the `beforeAll()` call to these scripts via `execSync` with `cwd: CURRENT_WORKING_DIR` is intentional and correct. There is no race risk because this is the only test file that calls setup. `yarn setup` is also already run before tests in CI, making the execSync just a local safety net.
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Bug Fixes: If the bug affects the user interface, a Selenium browser test should be included
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-04-14T18:44:55.656Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1101
File: browser-test/js-file-inject.test.js:0-0
Timestamp: 2026-04-14T18:44:55.656Z
Learning: In this repository’s browser tests under `browser-test/`, it is intentional and acceptable for tests to hardcode the local test base URL/port as `http://127.0.0.1:8080`. Do not flag occurrences like `browser-test/js-file-inject.test.js` for “environment coupling” or “hardcoded port/URL” issues, since the browser test environment is expected to run on that address.
Applied to files:
browser-test/cross-org-phone-verification.test.js
🔇 Additional comments (3)
browser-test/testData.json (1)
22-29: Cross-org fixture payload looks complete for the new flow.The added object includes all fields required by initialization and Selenium flow orchestration.
browser-test/initialize_data.py (1)
83-109: Cross-organization test initialization flow is well-structured.
get_or_createusage plus explicit settings update makes reruns safer and keeps org verification prerequisites explicit.browser-test/cross-org-phone-verification.test.js (1)
13-41: Good addition of a Selenium E2E path for cross-org verification.This meaningfully exercises the user-facing flow from login to verification route transition.
Based on learnings: for UI features in this repository, Selenium browser tests are preferred and should cover non-trivial behavior changes.
Test Failures in Browser TestsHello @pandafy, The following browser tests failed:
Please investigate these test failures and address the issues in the code or the tests themselves. |
Test Failures in Mobile VerificationHello @pandafy, There are two test failures in the
To fix these, you should:
|
Code Review SummaryStatus: 1 Issue Found | Recommendation: Fix typo before merge OverviewIncremental review of commit Changes in This Commit ✅
Issues Found
Previous Issues Now Resolved (Cumulative) ✅
Remaining Issues Before Merge
Security Assessment
This PR is nearly complete. Please fix the typo Fix remaining issues in Kilo Cloud Reviewed by kimi-k2.5-0127 · 591,981 tokens |
Test and Lint FailuresHello @pandafy, Test Failure: Code Style/QA Failures:
To fix the code style issues, please run |
Test Failure in Browser TestsHello @pandafy, The test Fix: |
Commit Message and Test FailuresHello @pandafy,
For example:
|
pandafy
force-pushed
the
issues/1055-join-multi-org
branch
from
64f667c to
6ad9ec1
Compare
Multiple Test Failures and Import Order IssueHello @pandafy,
|
pandafy
force-pushed
the
issues/1055-join-multi-org
branch
from
6ad9ec1 to
b0e2467
Compare
|
@coderabbitai do a full review of the changes, focus on security, bugs, performance, unused code or outdated comments/docs. Ensure that the code follows OpenWISP Conventions. Flag if any user facing string is untranslated. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 70896 Let me gather a few more details before writing the review. 🏁 Script executed: Length of output: 2752
1. 🌐 Untranslated / Non-conventional i18n Strings —
|
| Line | Current | Should use |
|---|---|---|
| 77 | t\Registration is currently disabled for this organization`` |
e.g. t\ORG_REGISTRATION_DISABLED`` |
| 92, 95 | t\Failed to load plans`` |
e.g. t\PLANS_FETCH_ERR`` |
| 300 | t\Retry`` |
e.g. t\RETRY`` |
| 288 | t\Please complete your registration to ${orgName}.`` |
Named interpolation key convention |
The template-literal interpolation on line 288 (t\Please complete your registration to ${orgName}.`) is particularly problematic — ttagextracts the entire template string as the msgid, meaning every invocation of this component for a differentorgNamegenerates a *different* message ID, making translation impossible in practice. Use a static message ID with substitution (e.g., thesprintf`-style approach used elsewhere in the codebase).
2. 🐛 message Not Initialized in Component State
File: client/components/complete-signup/complete-signup.js
message is referenced in the render() method:
const {plans, plansFetched, plansError, message, selectedPlan, errors} = this.state;…and in handlePlansRetry:
this.setStateSafe({plansError: null, message: null}, ...)But the constructor's state never declares message:
this.state = {
errors: {},
plans: [],
plansFetched: false,
plansError: null,
selectedPlan: null,
// message is missing ← add: message: null
};While JS allows accessing undeclared state keys (returns undefined), it violates the OpenWISP convention of declaring all state up front and makes the code harder to reason about.
3. 🐛 No Double-Submit Guard in handlePlanChange
File: client/components/complete-signup/complete-signup.js
handlePlanChange immediately fires handleSubmitPlan, but there is no early-return guard if loading is already in progress. A rapid second click/change can trigger a second concurrent submission before setLoading(true) inside handleSubmitPlan takes effect:
handlePlanChange = (event) => {
const planIndex = event.target.value;
const {plans} = this.state;
const selectedPlan = plans[planIndex];
if (!selectedPlan) {
return;
}
this.setStateSafe({selectedPlan: planIndex});
this.handleSubmitPlan(planIndex); // ← no loading guard here
};Consider checking the loading context or a local submitting flag before proceeding.
4. 🐛 Redundant Null Check in handleSubmitPlan
File: client/components/complete-signup/complete-signup.js, around line 175
if (selectedPlan && selectedPlan.id) { // selectedPlan was already validated aboveSince there's already an early-return guard at the top of the function (if (!selectedPlan) { return; }), the selectedPlan && part of the inner condition is always truthy at that point. Simplify to:
if (selectedPlan.id) {5. 🐛 handleFlowError Does Not Update Form Error State
File: client/components/complete-signup/complete-signup.js
handleFlowError is a module-level function that only fires toast.error(). When a plan submission fails, the errors state (which feeds getError(errors) in the render) is never updated, so the inline error UI is never triggered. This is inconsistent with the pattern used in other components (e.g., login.js, mobile-phone-change.js), where setState({errors: ...}) is also called.
6. ⚠️ upgrade-pending-verification-plan.js Referenced in Summary but Missing from Codebase
The AI summary describes a new upgrade-pending-verification-plan.js utility. However, running fd "upgrade-pending-verification-plan" returns no results — the file does not exist in this branch. The complete-signup.js component imports the pre-existing upgrade-plan.js instead. If upgrade-pending-verification-plan.js was meant to be a new, purpose-specific utility (e.g. using a different endpoint or payload shape), it needs to be added. Otherwise, the AI summary is misleading and should be disregarded on this point.
7. 🔒 Server-Side: No Response for Async Axios Completion (Controller Inconsistency)
File: server/controllers/registration-method-controller.js
The validSlug check uses config.some(...). Inside the callback, the axios call is asynchronous, but Array.prototype.some is synchronous — the response/error from axios is handled via .then/.catch, but the outer if (!validSlug) block runs synchronously and immediately. If the org slug is found, validSlug is true and the 404 is not sent, which is correct. However, if the axios call inside the some callback throws synchronously (before the promise settles), the 404 response would still be suppressed. This is the same structural pattern used by other controllers in this codebase, so it's an inherited pattern — but worth noting.
8. ⚠️ CI: Feature Branch References Must Be Removed Before Merge
File: .github/workflows/ci.yml, Line 110
# TODO: Remove branch before merging
git clone --depth=1 --branch issues/692-different-identity-verification https://github.com/openwisp/openwisp-radius/ openwisp-radiusThe openwisp-users force-reinstall also pins to a feature branch:
pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"These must be replaced with stable release references before this PR is merged to master.
9. ⚠️ getVerificationRoute Returns Mobile Verification for Empty/Unknown Method
File: client/utils/pending-verification.js
export const getVerificationRoute = (orgSlug, method) =>
method === "bank_card"
? `/${orgSlug}/payment/draft`
: `/${orgSlug}/mobile-phone-verification`;The function is currently only called with "mobile_phone" or "bank_card", so there's no current bug. However, the fallback silently routes any unrecognized method (including "") to mobile phone verification, which could cause unexpected behavior if the call sites change. Consider an explicit check:
export const getVerificationRoute = (orgSlug, method) => {
if (method === "bank_card") return `/${orgSlug}/payment/draft`;
if (method === "mobile_phone") return `/${orgSlug}/mobile-phone-verification`;
throw new Error(`Unknown verification method: ${method}`);
};10. 🧹 errors State Never Reset on Retry
File: client/components/complete-signup/complete-signup.js
handlePlansRetry resets plansError and message, but does not reset errors:
this.setStateSafe({plansError: null, message: null}, () => { ... });For consistency with how other components handle retries, also reset errors: {}.
Summary Table
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 High | complete-signup.js |
Non-conventional / untranslatable i18n strings |
| 2 | 🟡 Medium | complete-signup.js |
message not in initial state |
| 3 | 🟡 Medium | complete-signup.js |
No double-submit guard in handlePlanChange |
| 4 | 🟢 Low | complete-signup.js |
Redundant null check in handleSubmitPlan |
| 5 | 🟡 Medium | complete-signup.js |
handleFlowError doesn't update form error state |
| 6 | 🟡 Medium | PR scope | upgrade-pending-verification-plan.js missing |
| 7 | 🟢 Low | registration-method-controller.js |
Async-in-some structural note |
| 8 | 🔴 High | ci.yml |
Feature branch pins must be removed before merge |
| 9 | 🟡 Medium | pending-verification.js |
Unsafe fallback in getVerificationRoute |
| 10 | 🟢 Low | complete-signup.js |
errors not reset on retry |
🧠 Learnings used
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{py,html} : General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: General: Flag potential security vulnerabilities in all code
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it
Test Failure in Complete Signup ComponentHello @pandafy, There is a test failure in Fix: |
pandafy
force-pushed
the
issues/1055-join-multi-org
branch
from
e66d274 to
ce8bd13
Compare
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
.github/workflows/ci.yml (1)
110-111:⚠️ Potential issue | 🟠 MajorPin external dependency refs to immutable SHAs before merge.
Line 111 and Line 135 still use mutable branch refs, so CI results can change without any change in this PR. Please replace both with fixed commit SHAs and remove the temporary TODO before merge.
Suggested fix
- name: Get openwisp-radius run: | - # TODO: Remove branch before merging - git clone --depth=1 --branch issues/692-different-identity-verification https://github.com/openwisp/openwisp-radius/ openwisp-radius + OW_RADIUS_REF=<openwisp-radius-commit-sha> + git clone --depth=1 https://github.com/openwisp/openwisp-radius/ openwisp-radius cd openwisp-radius + git fetch --depth=1 origin "$OW_RADIUS_REF" + git checkout --detach "$OW_RADIUS_REF" echo "OpenWISP RADIUS commit: $(git rev-parse HEAD)" echo "OW_RADIUS_VERSION=$(git rev-parse HEAD)" >> $GITHUB_ENV python --version @@ - pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" + OW_USERS_REF=<openwisp-users-commit-sha> + pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/${OW_USERS_REF}"Also applies to: 135-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 110 - 111, The CI workflow currently clones openwisp-radius using a mutable branch ref (the git clone command that uses --branch issues/692-different-identity-verification) and leaves a TODO to remove the branch; update both occurrences (the git clone lines at the two places) to use the repo commit SHA instead of the branch name, remove the "TODO: Remove branch before merging" comment, and ensure the clone commands use --branch <commit-sha> or the equivalent immutable ref so CI is reproducible.server/controllers/registration-method-controller.js (1)
42-43:⚠️ Potential issue | 🟠 MajorRedact authorization data before calling
logResponseError.Because the proxied request includes
Authorization(Line 42), logging raw axios errors at Line 56 can leak bearer tokens if logger prints request/response config headers.Proposed fix
.catch((error) => { - logResponseError(error); + const sanitizedError = { + ...error, + config: { + ...error.config, + headers: { + ...error.config?.headers, + Authorization: error.config?.headers?.Authorization + ? "[REDACTED]" + : error.config?.headers?.Authorization, + }, + }, + response: error.response + ? { + ...error.response, + config: { + ...error.response.config, + headers: { + ...error.response.config?.headers, + Authorization: error.response.config?.headers + ?.Authorization + ? "[REDACTED]" + : error.response.config?.headers?.Authorization, + }, + }, + } + : error.response, + }; + logResponseError(sanitizedError);#!/bin/bash # Verify whether logger prints unsanitized axios response/config headers. fd -i 'logger.js' server/utils rg -n "logResponseError|Authorization|headers|response|config" server/utils -C3Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/controllers/registration-method-controller.js` around lines 42 - 43, The proxied request's Authorization header is being passed through and may be leaked when calling logResponseError; before invoking logResponseError (in registration-method-controller.js where the axios error is handled), sanitize the axios error object by removing or replacing any Authorization header in error.config.headers and in error.response?.config?.headers (and any nested headers objects) so the logger never receives raw bearer tokens; target the error handling block that calls logResponseError and mutate or clone those header objects to set Authorization = '[REDACTED]' (or delete the key) prior to logging.server/controllers/registration-method-controller.test.js (1)
156-158:⚠️ Potential issue | 🟠 MajorRemove fixed-delay sleeps from async tests.
At Line 156 and Line 179, the tests add
setTimeout(10)after already awaitingupdateRegistrationMethod(...)(Line 145 and Line 168). This is timing-dependent and can make the tests flaky.✅ Proposed fix
@@ - await new Promise((resolve) => { - setTimeout(resolve, 10); - }); @@ - await new Promise((resolve) => { - setTimeout(resolve, 10); - });As per coding guidelines, “Tests must be deterministic and not flaky, avoiding dependencies on timing … or unawaited async operations.”
Also applies to: 179-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/controllers/registration-method-controller.test.js` around lines 156 - 158, Remove the fixed 10ms setTimeout sleeps in the test and make it deterministically wait for the actual async work triggered by updateRegistrationMethod; specifically, delete the Promise/setTimeout blocks around Lines after calling updateRegistrationMethod in registration-method-controller.test.js and instead await or return the real promise(s) that complete the side-effect (e.g., await updateRegistrationMethod(...)’s returned promise, await any related mocked service call or use testing utilities like waitFor/flushPromises or resolve spies on the function that performs the final work). Ensure you reference and await the same async function or mock (updateRegistrationMethod and any mockedNotification/mockedDb functions or spies) so the test doesn't rely on timing.browser-test/browser_test_utils.py (1)
14-14:⚠️ Potential issue | 🟠 MajorDelete the cross-org browser-test user by
username, not
browser-test/initialize_data.pycreates this fixture withusername=data["phoneNumber"](Lines 99-103 there). Cleaning it up viaProposed fix
- User.objects.filter(email=cross_org_data["email"]).delete() + User.objects.filter(username=cross_org_data["phoneNumber"]).delete()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser-test/browser_test_utils.py` at line 14, Replace the deletion that filters by email with a deletion by username: change the User.objects.filter(email=cross_org_data["email"]).delete() call to User.objects.filter(username=cross_org_data["username"]).delete() so the teardown matches creation/auth (created with username=data["phoneNumber"]) and avoids deleting the wrong user.browser-test/cross-org-phone-verification.test.js (1)
51-55: 🧹 Nitpick | 🔵 TrivialUse a stable selector for the email assertion.
The CSS selector
div > p:nth-child(5) > spanis brittle and tied to the current markup order. Unrelated layout changes will break this test. Consider adding adata-testidattribute to the email element in the Status component.♻️ Suggested approach
In the Status component, add a test ID:
<span data-testid="user-email">{email}</span>Then update the test:
- const emailElement = await getElementByCss( - driver, - "div > p:nth-child(5) > span", - ); + const emailElement = await getElementByCss( + driver, + "[data-testid='user-email']", + );As per coding guidelines, tests must be deterministic and not flaky.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser-test/cross-org-phone-verification.test.js` around lines 51 - 55, Replace the brittle CSS selector used in the test by adding and using a stable test id: in the Status component add a data-testid="user-email" on the element that renders the email (e.g., the <span> that outputs email), then update the test to locate the element via getElementByCss/getElementBySelector using a selector for [data-testid="user-email"] (replace the current usage of getElementByCss(driver, "div > p:nth-child(5) > span") and the emailElement variable) and assert its text equals data.email; this ensures the test uses the stable "user-email" test id rather than markup-dependent nth-child selectors.client/components/complete-signup/complete-signup.js (1)
191-210:⚠️ Potential issue | 🟠 MajorPaid-plan transition is still non-atomic and can leave partial backend state.
If
upgradePlansucceeds (Line 191) andupdateRegistrationMethodfails (Line 205), the user is left half-transitioned. This was previously flagged and still applies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/complete-signup/complete-signup.js` around lines 191 - 210, The current flow calls upgradePlan(...) then updateRegistrationMethod(...), which can leave partial state if the second call fails; modify this block so the transition is atomic by adding error handling and a compensating rollback: after calling upgradePlan(orgSlug, selectedPlan.id, userData.auth_token, language) and before returning success, wrap updateRegistrationMethod(orgSlug, "bank_card", userData.auth_token, language) in try/catch; if updateRegistrationMethod throws, call a rollback API (e.g., a new or existing cancel/downgrade endpoint) or call downgradePlan/ cancelUpgrade with the same selectedPlan.id and userData.auth_token to revert the upgrade, surface an error to the user, and ensure paymentUrl/ state is cleared—alternatively move registration-method update into a single backend transaction and call that endpoint instead of separate upgradePlan/updateRegistrationMethod calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@browser-test/initialize_data.py`:
- Around line 105-106: Wrap the lookup of the source organization used to create
the cross-org membership in a guarded try/except similar to how
test_user_organization is handled: catch Organization.DoesNotExist around the
call to Organization.objects.get(slug=data["sourceOrganization"]) and only call
OrganizationUser.objects.create(organization=source_org, user=cross_org_user) if
the lookup succeeded; on exception, log or handle the missing source_org case
(same pattern used for test_user_organization) so CI produces an actionable
error instead of an uncaught traceback.
In `@browser-test/testData.json`:
- Around line 22-29: The cross-org fixture object
"crossOrgPhoneVerificationUser" reuses the same phoneNumber as
"mobileVerificationTestUser", causing order-dependent uniqueness failures;
update the "phoneNumber" value inside "crossOrgPhoneVerificationUser" to a
distinct, unused number (keep the "phoneNumber" key and format like
"+<country><digits>"), so both fixtures can be created independently without
uniqueness conflicts.
In `@client/components/complete-signup/complete-signup.js`:
- Around line 173-185: The handleSubmitPlan flow currently races because
setState is async; add a synchronous guard (e.g. a class-level boolean like
this._submitting) at the start of handleSubmitPlan to return early if already
submitting, set this._submitting = true immediately before any async work, then
keep the existing this.setStateSafe({submitting: true}) and in the final finally
block reset both this._submitting = false and state submitting to false (and
clear on success/error paths); apply the same pattern to the other plan-change
handlers referenced (the blocks around lines 249-257 and 259-268) so all
submission paths use the same immediate guard and reliably reset it on
completion.
In `@client/utils/upgrade-plan.js`:
- Around line 6-18: Add a direct unit test for the upgradePlan helper: mock
axios to intercept the POST call made by upgradePlan(orgSlug, planPricing,
authToken, language) and assert the request URL equals
upgradePlanApiUrl.replace("{orgSlug}", orgSlug), the headers include
"content-type":"application/json", Authorization:`Bearer ${authToken}`, and
"accept-language" from getLanguageHeaders(language), and that the request body
contains { plan_pricing: planPricing }; also ensure the test returns a mocked
response object and verifies upgradePlan resolves to response.data (i.e., the
function unwraps response.data). Use the same function name upgradePlan and
reference getLanguageHeaders and upgradePlanApiUrl to locate the helper when
adding the test.
In `@client/utils/utils.test.js`:
- Around line 878-880: The afterEach currently only calls jest.clearAllMocks(),
which leaves mocked implementations (like the console.log and toast.error spies
created later) installed and can leak between tests; update the afterEach blocks
(the one shown and the similar block around lines 931-933) to restore mocks as
well by calling jest.restoreAllMocks() (or replace clearAllMocks() with
restoreAllMocks() or call both) so spies are fully cleaned up and deterministic
for subsequent tests.
In `@server/controllers/registration-method-controller.js`:
- Around line 31-33: Guard against missing or non-object request bodies before
accessing req.body.method: replace direct accesses like
allowedMethods.includes(req.body.method) with a safe check (e.g., if (!req.body
|| typeof req.body.method !== "string" ||
!allowedMethods.includes(req.body.method)) ) and return the existing
res.status(400).type("application/json").send({ response_code: "INVALID_METHOD",
... }) path. Apply the same fix to the second occurrence that checks
req.body.method so both checks use the safe existence/type guard or optional
chaining plus a type check to avoid throwing when body parsing fails.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 110-111: The CI workflow currently clones openwisp-radius using a
mutable branch ref (the git clone command that uses --branch
issues/692-different-identity-verification) and leaves a TODO to remove the
branch; update both occurrences (the git clone lines at the two places) to use
the repo commit SHA instead of the branch name, remove the "TODO: Remove branch
before merging" comment, and ensure the clone commands use --branch <commit-sha>
or the equivalent immutable ref so CI is reproducible.
In `@browser-test/browser_test_utils.py`:
- Line 14: Replace the deletion that filters by email with a deletion by
username: change the User.objects.filter(email=cross_org_data["email"]).delete()
call to User.objects.filter(username=cross_org_data["username"]).delete() so the
teardown matches creation/auth (created with username=data["phoneNumber"]) and
avoids deleting the wrong user.
In `@browser-test/cross-org-phone-verification.test.js`:
- Around line 51-55: Replace the brittle CSS selector used in the test by adding
and using a stable test id: in the Status component add a
data-testid="user-email" on the element that renders the email (e.g., the <span>
that outputs email), then update the test to locate the element via
getElementByCss/getElementBySelector using a selector for
[data-testid="user-email"] (replace the current usage of getElementByCss(driver,
"div > p:nth-child(5) > span") and the emailElement variable) and assert its
text equals data.email; this ensures the test uses the stable "user-email" test
id rather than markup-dependent nth-child selectors.
In `@client/components/complete-signup/complete-signup.js`:
- Around line 191-210: The current flow calls upgradePlan(...) then
updateRegistrationMethod(...), which can leave partial state if the second call
fails; modify this block so the transition is atomic by adding error handling
and a compensating rollback: after calling upgradePlan(orgSlug, selectedPlan.id,
userData.auth_token, language) and before returning success, wrap
updateRegistrationMethod(orgSlug, "bank_card", userData.auth_token, language) in
try/catch; if updateRegistrationMethod throws, call a rollback API (e.g., a new
or existing cancel/downgrade endpoint) or call downgradePlan/ cancelUpgrade with
the same selectedPlan.id and userData.auth_token to revert the upgrade, surface
an error to the user, and ensure paymentUrl/ state is cleared—alternatively move
registration-method update into a single backend transaction and call that
endpoint instead of separate upgradePlan/updateRegistrationMethod calls.
In `@server/controllers/registration-method-controller.js`:
- Around line 42-43: The proxied request's Authorization header is being passed
through and may be leaked when calling logResponseError; before invoking
logResponseError (in registration-method-controller.js where the axios error is
handled), sanitize the axios error object by removing or replacing any
Authorization header in error.config.headers and in
error.response?.config?.headers (and any nested headers objects) so the logger
never receives raw bearer tokens; target the error handling block that calls
logResponseError and mutate or clone those header objects to set Authorization =
'[REDACTED]' (or delete the key) prior to logging.
In `@server/controllers/registration-method-controller.test.js`:
- Around line 156-158: Remove the fixed 10ms setTimeout sleeps in the test and
make it deterministically wait for the actual async work triggered by
updateRegistrationMethod; specifically, delete the Promise/setTimeout blocks
around Lines after calling updateRegistrationMethod in
registration-method-controller.test.js and instead await or return the real
promise(s) that complete the side-effect (e.g., await
updateRegistrationMethod(...)’s returned promise, await any related mocked
service call or use testing utilities like waitFor/flushPromises or resolve
spies on the function that performs the final work). Ensure you reference and
await the same async function or mock (updateRegistrationMethod and any
mockedNotification/mockedDb functions or spies) so the test doesn't rely on
timing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9248abd5-7fb2-4d0a-a1ba-31dd8f8f6422
⛔ Files ignored due to path filters (1)
client/components/organization-wrapper/__snapshots__/organization-wrapper.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (35)
.eslintrc.json.github/workflows/ci.ymlbrowser-test/browser_test_utils.pybrowser-test/cross-org-phone-verification.test.jsbrowser-test/initialize_data.pybrowser-test/testData.jsonclient/components/complete-signup/complete-signup.jsclient/components/complete-signup/complete-signup.test.jsclient/components/complete-signup/index.cssclient/components/complete-signup/index.jsclient/components/login/login.jsclient/components/login/login.test.jsclient/components/mobile-phone-change/mobile-phone-change.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/organization-wrapper/lazy-import.jsclient/components/organization-wrapper/organization-wrapper.jsclient/components/organization-wrapper/organization-wrapper.test.jsclient/components/status/status.jsclient/constants/index.jsclient/utils/get-plans.jsclient/utils/pending-verification.jsclient/utils/update-registration-method.jsclient/utils/upgrade-plan.jsclient/utils/utils.test.jsi18n/de.poi18n/en.poi18n/es.poi18n/fur.poi18n/it.poi18n/ru.poi18n/sl.poserver/controllers/registration-method-controller.jsserver/controllers/registration-method-controller.test.jsserver/routes/account.jsserver/utils/openwisp-urls.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
browser-test/browser_test_utils.pybrowser-test/initialize_data.py
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}
📄 CodeRabbit inference engine (Custom checks)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}: Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
General: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Files:
browser-test/cross-org-phone-verification.test.jsclient/components/login/login.test.jsclient/utils/utils.test.jsclient/components/organization-wrapper/organization-wrapper.test.jsserver/controllers/registration-method-controller.test.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/complete-signup/complete-signup.test.js
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), `finalOperations()` has a single early return before setting up intervals/listeners to prevent the scenario where the component unmounts during async awaits. `safeSetState` is applied to success paths in `getUserRadiusSessions()`, `getUserRadiusUsage()`, `getPlansSuccessCallback()`, and `handlePostMessage()`.
Applied to files:
client/components/status/status.jsclient/components/mobile-phone-change/mobile-phone-change.jsclient/components/login/login.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/organization-wrapper/organization-wrapper.jsclient/components/complete-signup/complete-signup.jsclient/components/complete-signup/complete-signup.test.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), the Status component uses `this.isComponentMounted` (set to `true` in the constructor, `false` in `componentWillUnmount`) together with a `safeSetState()` helper method that checks `isComponentMounted` before calling `setState()`. This is the preferred pattern to guard against "setState on unmounted component" warnings.
Applied to files:
client/components/status/status.jsclient/components/mobile-phone-change/mobile-phone-change.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/organization-wrapper/organization-wrapper.jsclient/components/complete-signup/complete-signup.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
Applied to files:
browser-test/cross-org-phone-verification.test.jsclient/components/login/login.test.jsclient/utils/utils.test.jsclient/components/complete-signup/complete-signup.test.js
📚 Learning: 2026-03-25T00:22:16.841Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-25T00:22:16.841Z
Learning: In `openwisp/openwisp-wifi-login-pages` browser tests (e.g., `browser-test/mobile-verfication.test.js`), fixed delay waits (e.g., `driver.sleep()`) are intentionally used instead of `driver.wait()` for toast message animations. This is because the JS animation on toast messages does not expose a reliable DOM state to poll against, and testing confirmed that fixed waits are more robust in this context. Do not flag these as flaky-by-design without considering this justification.
Applied to files:
browser-test/cross-org-phone-verification.test.jsclient/components/login/login.test.jsserver/controllers/registration-method-controller.test.jsclient/components/mobile-phone-change/mobile-phone-change.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Bug Fixes: If the bug affects the user interface, a Selenium browser test should be included
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
browser-test/cross-org-phone-verification.test.jsclient/components/login/login.test.jsclient/utils/utils.test.jsclient/components/organization-wrapper/organization-wrapper.test.jsserver/controllers/registration-method-controller.test.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/complete-signup/complete-signup.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
browser-test/cross-org-phone-verification.test.jsclient/components/login/login.test.jsclient/utils/utils.test.jsclient/components/organization-wrapper/organization-wrapper.test.jsserver/controllers/registration-method-controller.test.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/complete-signup/complete-signup.test.js
📚 Learning: 2026-03-23T21:28:04.132Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1072
File: config/__tests__/es5-compliance.test.js:22-29
Timestamp: 2026-03-23T21:28:04.132Z
Learning: In openwisp/openwisp-wifi-login-pages, `config/setup.js` and `config/build-translations.js` are project-level scripts that must be executed with `cwd` set to the project root (CURRENT_WORKING_DIR). They read from and write to project-relative directories (organizations/, public/, client/locales/, etc.) and will fail if run from any other directory. In the ES5 compliance test (`config/__tests__/es5-compliance.test.js`), the `beforeAll()` call to these scripts via `execSync` with `cwd: CURRENT_WORKING_DIR` is intentional and correct. There is no race risk because this is the only test file that calls setup. `yarn setup` is also already run before tests in CI, making the execSync just a local safety net.
Applied to files:
browser-test/cross-org-phone-verification.test.jsclient/components/organization-wrapper/organization-wrapper.test.js
📚 Learning: 2026-04-14T18:44:55.656Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 1101
File: browser-test/js-file-inject.test.js:0-0
Timestamp: 2026-04-14T18:44:55.656Z
Learning: In this repository’s browser tests under `browser-test/`, it is intentional and acceptable for tests to hardcode the local test base URL/port as `http://127.0.0.1:8080`. Do not flag occurrences like `browser-test/js-file-inject.test.js` for “environment coupling” or “hardcoded port/URL” issues, since the browser test environment is expected to run on that address.
Applied to files:
browser-test/cross-org-phone-verification.test.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), catch blocks deliberately do NOT check `isComponentMounted` — console warnings from catch paths are considered harmless. Only real memory leaks (timers, event listeners) and "setState on unmounted" in success paths are fixed. The maintainer (nemesifier) explicitly wants to avoid over-engineering with defensive checks in every catch block.
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.jsclient/components/organization-wrapper/organization-wrapper.test.jsclient/components/complete-signup/complete-signup.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), real memory leaks are fixed by calling `clearTimeout(this.usageRetryTimeoutId)` and `window.removeEventListener("message", ...)` in `componentWillUnmount()`. These were actual leaks — the retry timeout could fire and the message handler could run after unmount.
Applied to files:
client/components/mobile-phone-change/mobile-phone-change.jsclient/components/complete-signup/complete-signup.js
📚 Learning: 2026-03-18T17:34:09.903Z
Learnt from: prathmeshkulkarni-coder
Repo: openwisp/openwisp-wifi-login-pages PR: 1063
File: client/components/status/status.test.js:2890-2916
Timestamp: 2026-03-18T17:34:09.903Z
Learning: Do not rely on mocking or altering window.location in Jest/JSDOM for this project. Since window.location is non-configurable in the test environment and attempts to redefine or delete it can cause state leaks across tests, avoid hacks like delete window.location or Object.defineProperty(window, 'location', ...) in tests. Instead, migrate detection-related tests to React Testing Library (RTL) and use RTL-friendly patterns (e.g., render components and inspect behavior via UI, or mock network/detection logic with explicit dependency injection or module mocks). This pattern should apply to all test files under client/components (any .test.js) where window.location might be interacted with.
Applied to files:
client/components/login/login.test.jsclient/components/organization-wrapper/organization-wrapper.test.jsclient/components/mobile-phone-change/mobile-phone-change.test.jsclient/components/complete-signup/complete-signup.test.js
📚 Learning: 2026-03-18T17:34:17.347Z
Learnt from: prathmeshkulkarni-coder
Repo: openwisp/openwisp-wifi-login-pages PR: 1063
File: client/components/status/status.test.js:2890-2916
Timestamp: 2026-03-18T17:34:17.347Z
Learning: In openwisp/openwisp-wifi-login-pages, JSDOM marks `window.location` as non-configurable (configurable: false) in the Jest/Enzyme test environment used by this project. Object.defineProperty on window.location throws a TypeError. Hacks like `delete window.location` cause state leaks across sequential tests. The maintainer (prathmeshkulkarni-coder) considers the window.location mocking approach infeasible and plans to migrate detection tests to RTL instead.
Applied to files:
client/utils/utils.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : General: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Applied to files:
client/components/complete-signup/complete-signup.test.js
🔇 Additional comments (23)
.eslintrc.json (1)
50-51: Scoped lint exclusion looks correct.Ignoring
openwisp-radius/at the ESLint config level is a clean way to avoid lint noise from external checked-out code while keeping project lint rules unchanged.i18n/sl.po (1)
668-683: Looks good: new complete-signup translation keys are consistently added.The added msgids and source mappings are consistent with the existing catalog structure.
i18n/es.po (1)
689-704: Looks good: Spanish catalog updates are complete for new signup flow strings.All four new keys are added with matching source references.
i18n/ru.po (1)
676-691: Looks good: Russian translations for the new complete-signup keys are wired correctly.server/utils/openwisp-urls.js (1)
16-16: Good addition: reverse URL mapping now includes registration-method endpoint.client/utils/get-plans.js (1)
8-13: Nice fix: optional failure callback is now safely supported.The
typeof failureCallback === "function"guard prevents secondary failures in the error path.Also applies to: 28-30
client/utils/pending-verification.js (1)
1-12: Good utility design: pending-verification checks and route selection are explicit.Unknown methods throwing is a safer default than silent fallback.
server/routes/account.js (1)
16-16: Route integration is clean and consistent with existing account endpoints.Also applies to: 33-33
client/components/complete-signup/index.css (1)
1-9: Looks good.The new layout spacing and vertical button stacking are clear and consistent with the component scope.
i18n/en.po (1)
677-692: Translation entries look correct.New keys are properly registered and aligned with complete-signup source references.
client/components/organization-wrapper/lazy-import.js (1)
37-39: Good addition.
CompleteSignuplazy loading follows the existing chunking and import pattern.client/constants/index.js (1)
23-24: Looks correct.The new URL builder is consistent with the constants module and endpoint intent.
i18n/it.po (1)
684-699: Italian catalog update is in good shape.New complete-signup entries are present and consistent with the added msgids.
client/components/mobile-phone-change/mobile-phone-change.js (1)
36-49: Good hardening for async lifecycle updates.The mount guard +
setStateSafeusage is correctly applied on updated async state paths.Based on learnings: In
client/components/status/status.js, the preferred pattern is to useisComponentMountedwith a safe setState helper on success paths.Also applies to: 69-73, 97-97, 113-119
client/components/login/login.test.js (1)
508-587: LGTM! Well-structured tests for pending verification redirect behavior.The new test cases thoroughly cover the
pending_verificationflow across multiple configuration scenarios (phone-only, subscriptions-only, both enabled) and include a negative case to ensure no redirect when the method differs. The tests are deterministic with controlled inputs and properly assert the expectedsetUserData,navigate, andauthenticatecalls.client/utils/update-registration-method.js (1)
1-18: LGTM! Clean utility following existing codebase patterns.The utility is properly structured with appropriate headers including Bearer authentication. Error handling is correctly delegated to callers, consistent with similar utilities like
upgradePlanandgetPlans.client/components/complete-signup/index.js (1)
1-23: LGTM! Properly structured Redux connector.The
mapStateToPropsandmapDispatchToPropscorrectly wire the required props to theCompleteSignupcomponent. Thenavigateprop is intentionally omitted here as it's passed directly from the parentOrganizationWrappercomponent (as seen in line 364 oforganization-wrapper.js), consistent with how other lazy-loaded components in this codebase receive navigation capabilities.client/components/login/login.js (1)
303-327: LGTM! Clean refactoring of authentication handling.The restructured code properly extracts
keyasauth_token, builds a unifiednextUserDataobject, and uses early returns for bothpending_verificationandbank_cardflows. The consistency improvement (usingnextUserData.is_verifiedinstead ofdata.is_verified) from previous review feedback has been addressed.client/components/organization-wrapper/organization-wrapper.test.js (1)
630-687: LGTM! Comprehensive test coverage for pending verification routing.The new test block properly covers the
complete-signupredirect behavior for pending verification users, including:
- Redirects from multiple entry routes (
registration/*,login/*,status,mobile-phone-verification)- Lazy rendering of
CompleteSignupcomponent withSuspense- Query parameter preservation when redirecting from login
The
console.errorsave/restore pattern inbeforeEach/afterEachaddresses the previous review feedback about preventing global test-state leakage.client/components/organization-wrapper/organization-wrapper.js (1)
122-123: LGTM! Well-structured routing logic for the complete-signup flow.The new
needsMethodSelectionflag correctly prioritizes method selection before other verification steps. Thecomplete-signuproute properly handles the cascading redirect logic:
- Unauthenticated → redirect to login
- Method selection complete → redirect based on remaining verification needs (phone → bank card → status)
- Method selection pending → render
CompleteSignupcomponentThe query string preservation in the login redirect (lines 235-237) ensures URL state is maintained during navigation.
Also applies to: 343-368
client/components/complete-signup/complete-signup.test.js (1)
60-391: LGTM! Comprehensive test coverage for CompleteSignup component.The test suite thoroughly covers:
- Happy paths: Plan fetching, free/paid plan selection, auto-transitions
- Error paths: Fetch failures, submission failures, network errors (with and without response)
- Boundary values: Empty plans array, null plans, invalid selection indexes
- Edge cases: Unmount safety via
setStateSafe, query param handlingThe tests are deterministic with properly reset mocks and no timing dependencies.
browser-test/cross-org-phone-verification.test.js (1)
42-44: > Likely an incorrect or invalid review comment.client/components/complete-signup/complete-signup.js (1)
46-54: Good use of mounted-safe success-path updates.
setStateSafeandfinalOperationscorrectly guard success-path state/navigation side effects.Based on learnings, this matches the repository’s preferred
isComponentMounted+ safe state-update pattern for success flows.Also applies to: 103-111
| handleSubmitPlan = async (planIndex) => { | ||
| const {setLoading} = this.context; | ||
| const {orgSlug, userData, setUserData, navigate, settings, language} = | ||
| this.props; | ||
| const {plans, submitting} = this.state; | ||
| const selectedPlan = plans[planIndex]; | ||
| if (!selectedPlan || submitting) { | ||
| return; | ||
| } | ||
|
|
||
| const requiresPayment = selectedPlan.requires_payment === true; | ||
| this.setStateSafe({errors: {}, submitting: true}); | ||
| setLoading(true); |
There was a problem hiding this comment.
Prevent concurrent submissions in the plan-change flow.
setState is asynchronous, so rapid interactions can trigger handleSubmitPlan multiple times before submitting is observed as true (Line 184 and Line 267). That can duplicate backend writes.
Suggested fix
constructor(props) {
super(props);
this.isComponentMounted = true;
+ this.planSubmitInFlight = false;
this.state = {
@@
handleSubmitPlan = async (planIndex) => {
@@
- const {plans, submitting} = this.state;
+ const {plans, submitting} = this.state;
const selectedPlan = plans[planIndex];
- if (!selectedPlan || submitting) {
+ if (!selectedPlan || submitting || this.planSubmitInFlight) {
return;
}
+ this.planSubmitInFlight = true;
@@
try {
@@
} catch (error) {
if (this.isComponentMounted) {
handleFlowError(error, (errors) =>
this.setStateSafe({errors, submitting: false}),
);
setLoading(false);
}
+ } finally {
+ this.planSubmitInFlight = false;
+ this.setStateSafe({submitting: false});
}
};Also applies to: 249-257, 259-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/components/complete-signup/complete-signup.js` around lines 173 - 185,
The handleSubmitPlan flow currently races because setState is async; add a
synchronous guard (e.g. a class-level boolean like this._submitting) at the
start of handleSubmitPlan to return early if already submitting, set
this._submitting = true immediately before any async work, then keep the
existing this.setStateSafe({submitting: true}) and in the final finally block
reset both this._submitting = false and state submitting to false (and clear on
success/error paths); apply the same pattern to the other plan-change handlers
referenced (the blocks around lines 249-257 and 259-268) so all submission paths
use the same immediate guard and reliably reset it on completion.
| const upgradePlan = (orgSlug, planPricing, authToken, language) => | ||
| axios({ | ||
| method: "post", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| "accept-language": getLanguageHeaders(language), | ||
| Authorization: `Bearer ${authToken}`, | ||
| }, | ||
| url: upgradePlanApiUrl.replace("{orgSlug}", orgSlug), | ||
| data: { | ||
| plan_pricing: planPricing, | ||
| }, | ||
| }).then((response) => response.data); |
There was a problem hiding this comment.
Add direct unit coverage for this helper.
This utility now drives the paid-plan path in both client/components/status/status.js and client/components/complete-signup/complete-signup.js, but the new test additions only cover updateRegistrationMethod and getPlans. A small axios-based test here should lock the URL, headers, payload, and response.data unwrapping before this lands.
As per coding guidelines "Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/utils/upgrade-plan.js` around lines 6 - 18, Add a direct unit test for
the upgradePlan helper: mock axios to intercept the POST call made by
upgradePlan(orgSlug, planPricing, authToken, language) and assert the request
URL equals upgradePlanApiUrl.replace("{orgSlug}", orgSlug), the headers include
"content-type":"application/json", Authorization:`Bearer ${authToken}`, and
"accept-language" from getLanguageHeaders(language), and that the request body
contains { plan_pricing: planPricing }; also ensure the test returns a mocked
response object and verifies upgradePlan resolves to response.data (i.e., the
function unwraps response.data). Use the same function name upgradePlan and
reference getLanguageHeaders and upgradePlanApiUrl to locate the helper when
adding the test.
Test Failure in Browser TestsHello @pandafy, The CI failed due to a test timeout in Fix: |
Test Timeout in Mobile Phone ChangeHello @pandafy,
|
Browser Test Timeout ErrorHello @pandafy,
To resolve this, you may need to:
|
pandafy
force-pushed
the
issues/1055-join-multi-org
branch
from
8b8db24 to
ca890cc
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Blockers
CI workflow still pinned to unmerged upstream branches. .github/workflows/ci.yml:111 clones openwisp-radius from issues/692-different-identity-verification and :135 force-reinstalls openwisp-users from issues/497-export-users. The PR description still lists openwisp/openwisp-radius#698 as a blocker. Once those land, the workflow needs to revert to the default clone and the released openwisp-users before this branch can merge — otherwise master CI is left pinned to mutable feature branches.
Untranslated user-facing string REGISTRATION_TITLE. client/components/complete-signup/complete-signup.js:60 and :297 call t`REGISTRATION_TITLE`, but no .po file (en/de/es/fur/it/ru/sl) defines this msgid. The existing convention is the suffix _TITL (see REGISTRATION_TITL in client/components/registration/registration.js:75 → "Sign up" in i18n/en.po:349). Result: the heading and <title> will render the literal "REGISTRATION_TITLE" in every locale. Either rename the key to REGISTRATION_TITL (reusing the existing translation) or add new entries to every catalog and re-extract.
Bugs
server/utils/logger.test.js does not exercise the new redaction logic. The top-level jest.mock("../utils/logger", () => ({logResponseError: jest.fn()})) resolves to the file under test (from server/utils/, ../utils/logger → server/utils/logger). After jest.resetModules() + require("./logger") the test still receives the auto-mocked jest.fn() — verified locally: jest.isMockFunction(logResponseError) returns true inside the test, and the test names themselves admit "sanitization happens internally" / "sanitization is internal". Every assertion is the trivial expect(jest.fn()).toHaveBeenCalledWith(error) and redactHeaders / sanitizeResponse / sanitizeRequest in server/utils/logger.js:122-151 are never run. Drop the top-level mock, spy on the underlying Logger.error / Logger.info, and assert the payload no longer contains the bearer token.
Duplicate toast on plans-fetch failure. client/utils/get-plans.js:26 calls toast.error(t\ERR_OCCUR`)and then invokesfailureCallback, and client/components/complete-signup/complete-signup.js:100 (handlePlansFailure) calls toast.error(t`PLANS_FETCH_ERR`)as well — so a single failure stacks two toasts ("Error occurred!" then "Failed to load plans"). Either gate the toast inget-plans.jsbehind the absence of a callback, or drop the toast inhandlePlansFailure` (the inline alert + retry button is already shown on the page).
Security (defense-in-depth)
Proxy unconditionally allows method: "". server/controllers/registration-method-controller.js:23 initializes allowedMethods = [""], so even when the org has mobile_phone_verification: true and/or subscriptions: true, a pending_verification user can POST {"method": ""} directly to /api/v1/{org}/account/registration-method and the proxy forwards it. The frontend only ever sends "" when both settings are disabled (complete-signup.js:139-145), so the empty string is the org-internal "no verification required" marker; the proxy validation should mirror that:
const allowedMethods = [];
if (!settings.subscriptions && !settings.mobile_phone_verification) allowedMethods.push("");
if (settings.subscriptions) allowedMethods.push("bank_card");
if (settings.mobile_phone_verification) allowedMethods.push("mobile_phone");The backend should also enforce this, but the issue explicitly says "without breaking security" so it is worth tightening here too.
An organization can have a registration option which does not require verification when when mobille_phone_verification and subscription is set to `true. |
2 participants
Checklist
Reference to Existing Issue
Closes #1055
Blockers