feat/ update new-signup flow for RDS users #1103
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe PR refactors the new signup flow from supporting multiple role selections with username input to a single role selection via radio buttons. It updates the controller, constants, components, and tests to reflect this behavioral change from multi-select to single-select role selection. Changes
Sequence DiagramsequenceDiagram
participant User
participant Form as Signup Form
participant Controller as NewSignupController
participant API as API
rect rgb(240, 248, 255)
Note over User,API: Old Flow: Multi-Select Roles + Username
User->>Form: Select multiple roles (checkboxes)
User->>Form: Enter username
Form->>Controller: handleCheckboxInputChange(key, value)
Controller->>Controller: Build signupDetails with roles object
User->>Form: Click Register
Form->>Controller: newRegisterUser(signupDetails, roles)
end
rect rgb(240, 255, 240)
Note over User,API: New Flow: Single Role Selection (Radio)
User->>Form: Select one role (radio button)
Form->>Controller: checkboxFieldChanged(selectedRole, event)
Controller->>Controller: handleCheckboxInputChange(selectedRole)
Controller->>Controller: Set signupDetails.role = selectedRole
User->>Form: Click Register
Form->>Controller: signup() - computes firstName, lastName, role
Controller->>Controller: Compute or generate username
Controller->>API: registerUser(signupDetails, devFlag)
API-->>Controller: res.status === 204
Controller->>Controller: Advance to LAST_STEP
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
app/components/profile/image-cropper.js (1)
6-11: Move cropper cleanup from getter to willDestroy lifecycle hook.The
imagegetter is dead code—the template uses@image(the argument) directly, and tests confirm the getter is never accessed. Additionally, having destructive side effects likethis.cropper.destroy()in a getter is an anti-pattern. The cleanup logic must be preserved but relocated.Required changes:
- Remove the unused getter (lines 6–11)
- Add a
willDestroy()hook to handle cropper cleanupApply these changes:
- get image() { - if (this.cropper) { - this.cropper.destroy(); - } - return this.args.image; - } + @action + willDestroy() { + super.willDestroy(...arguments); + if (this.cropper) { + this.cropper.destroy(); + } + }tests/unit/services/onboarding-test.js (1)
69-107: Align non‑Developer signup test stub with store.createRecord signatureIn the non‑Developer test, the
createRecordstub takes onlyproperties:store.createRecord = (properties) => { assert.step('store.createRecord called'); … Object.assign(mockRecord, properties); return mockRecord; };Whereas the Developer test uses
(modelName, properties). If the real service callsstore.createRecord('onboarding', dataToUpdate), this stub will receive the model name asproperties, andObject.assignwill copy indices from the string instead of the payload.To keep the test aligned with production behavior:
- store.createRecord = (properties) => { + store.createRecord = (modelName, properties) => { assert.step('store.createRecord called'); let mockRecord = { setProperties() { assert.step('setProperties called'); }, save() { assert.step('save called'); return Promise.resolve(); }, }; Object.assign(mockRecord, properties); return mockRecord; };tests/integration/components/profile/upload-image-test.js (1)
76-81: Keep relaxed assertion, but fix Prettier formattingThe new presence‑based assertion for the error state is good and less brittle. Prettier is complaining about the long chained call; reflowing it will keep lint green:
- await waitFor('p.message-text__failure'); - assert.dom('p.message-text__failure').exists('Error message is shown for invalid file type'); + await waitFor('p.message-text__failure'); + assert + .dom('p.message-text__failure') + .exists('Error message is shown for invalid file type');tests/integration/components/new-signup/checkbox-test.js (2)
52-88: Fix out‑of‑range NEW_SIGNUP_STEPS index and Prettier nitsThis test correctly asserts 7 role options and 16 expectations, but:
NEW_SIGNUP_STEPSnow has indices 0–4; using index 5 makesonClicksetcurrentSteptoundefined:- onClick: function () { - this.currentStep = NEW_SIGNUP_STEPS[5]; - }, + onClick: function () { + this.currentStep = NEW_SIGNUP_STEPS[3]; + },Even though this test doesn’t currently invoke
onClick, updating it keeps it consistent with the actual step list.
- Prettier is flagging long chained
.dom(...).hasText(...)lines. Reflow to satisfy lint, for example:- assert.dom('[data-test-checkbox-label="product_manager"]').hasText('Product Manager'); - assert.dom('[data-test-checkbox-label="project_manager"]').hasText('Project Manager'); - assert.dom('[data-test-checkbox-label="social_media"]').hasText('Social Media'); + assert + .dom('[data-test-checkbox-label="product_manager"]') + .hasText('Product Manager'); + assert + .dom('[data-test-checkbox-label="project_manager"]') + .hasText('Project Manager'); + assert + .dom('[data-test-checkbox-label="social_media"]') + .hasText('Social Media');
90-122: New onChange assertion is correct; adjust spacing for PrettierThe updated
onChange(selectedRole, value)expectation nicely validates the new API of the checkbox component. To clear the Prettier warning, just add a space after the comma in the parameter list:- this.set('onChange', function (selectedRole,value) { + this.set('onChange', function (selectedRole, value) {tests/unit/controllers/new-signup-test.js (1)
212-223: Confirm whether signupDetails should still use roles or the new role field under dev flagIn the dev‑flag test you’re initializing:
controller.signupDetails = { firstName: fakeUserData.first_name, lastName: fakeUserData.last_name, roles: 'developer', };Everywhere else in this PR the structure has moved to
signupDetails.role(singular). If the productionsignup/registerUserpath undersignupDev = 'true'also expectsrole, this test may no longer reflect the real payload shape.Please confirm whether the dev path still intentionally uses
roles, and either:
- Align this test to
role: 'developer', or- Add a short comment in the controller explaining why dev mode still depends on
roles.app/controllers/new-signup.js (1)
83-92: Add error handling for user profile retrieval.The method doesn't handle failures when fetching the user profile or when
userData.idis unavailable, which could result in invalid API calls or unclear error messages.Apply this diff to add proper error handling:
async registerUser(signupDetails, devFlag) { const getResponse = await apiRequest(SELF_USER_PROFILE_URL); + + if (!getResponse.ok) { + throw new Error(SIGNUP_ERROR_MESSAGES.others); + } + const userData = await getResponse.json(); + + if (!userData?.id) { + throw new Error(SIGNUP_ERROR_MESSAGES.others); + } + const url = SELF_USERS_URL(userData.id, devFlag); const res = await apiRequest(url, 'PATCH', signupDetails); if (!res) { throw new Error(SIGNUP_ERROR_MESSAGES.others); } return res; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
app/components/new-signup/checkbox.hbs(1 hunks)app/components/new-signup/checkbox.js(1 hunks)app/components/profile/image-cropper.hbs(1 hunks)app/components/profile/image-cropper.js(1 hunks)app/constants/apis.js(1 hunks)app/constants/new-signup.js(1 hunks)app/controllers/new-signup.js(3 hunks)app/styles/new-signup.module.css(1 hunks)app/templates/new-signup.hbs(0 hunks)tests/integration/components/new-signup/checkbox-test.js(4 hunks)tests/integration/components/profile/upload-image-test.js(1 hunks)tests/unit/controllers/new-signup-test.js(2 hunks)tests/unit/services/onboarding-test.js(2 hunks)
💤 Files with no reviewable changes (1)
- app/templates/new-signup.hbs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: AnujChhikara
Repo: Real-Dev-Squad/website-www PR: 1090
File: app/templates/admin/applications.hbs:0-0
Timestamp: 2025-11-06T20:21:16.032Z
Learning: In the Real-Dev-Squad/website-www repository, user AnujChhikara prefers to submit incremental PRs where UI templates/infrastructure are added first, followed by backing implementation (routes, controllers, data fetching logic) in subsequent PRs.
Learnt from: MayankBansal12
Repo: Real-Dev-Squad/website-www PR: 1038
File: tests/integration/components/new-signup/info-test.js:12-14
Timestamp: 2025-05-06T21:51:26.893Z
Learning: In the website-www repository, step constants for the new signup flow (FIRST_STEP, SECOND_STEP, etc.) are defined in controllers based on the NEW_SIGNUP_STEPS array. Tests that simulate controller behavior need to define these constants in the test context.
Learnt from: MayankBansal12
Repo: Real-Dev-Squad/website-www PR: 1038
File: tests/integration/components/new-signup/info-test.js:12-14
Timestamp: 2025-05-06T21:51:26.893Z
Learning: In the website-www repository's new-signup implementation, step constants (FIRST_STEP, SECOND_STEP, etc.) are defined in controllers based on the NEW_SIGNUP_STEPS array from constants. When testing components that use these controller properties, these constants need to be defined in the test context before being referenced in test functions.
📚 Learning: 2025-05-06T21:51:26.893Z
Learnt from: MayankBansal12
Repo: Real-Dev-Squad/website-www PR: 1038
File: tests/integration/components/new-signup/info-test.js:12-14
Timestamp: 2025-05-06T21:51:26.893Z
Learning: In the website-www repository's new-signup implementation, step constants (FIRST_STEP, SECOND_STEP, etc.) are defined in controllers based on the NEW_SIGNUP_STEPS array from constants. When testing components that use these controller properties, these constants need to be defined in the test context before being referenced in test functions.
Applied to files:
tests/integration/components/new-signup/checkbox-test.jsapp/constants/new-signup.jsapp/controllers/new-signup.jstests/unit/controllers/new-signup-test.js
📚 Learning: 2025-05-06T21:51:26.893Z
Learnt from: MayankBansal12
Repo: Real-Dev-Squad/website-www PR: 1038
File: tests/integration/components/new-signup/info-test.js:12-14
Timestamp: 2025-05-06T21:51:26.893Z
Learning: In the website-www repository, step constants for the new signup flow (FIRST_STEP, SECOND_STEP, etc.) are defined in controllers based on the NEW_SIGNUP_STEPS array. Tests that simulate controller behavior need to define these constants in the test context.
Applied to files:
tests/integration/components/new-signup/checkbox-test.jsapp/constants/new-signup.jsapp/controllers/new-signup.js
📚 Learning: 2025-11-08T13:14:05.646Z
Learnt from: MayankBansal12
Repo: Real-Dev-Squad/website-www PR: 1083
File: app/components/new-join-steps/new-step-one.hbs:47-56
Timestamp: 2025-11-08T13:14:05.646Z
Learning: In the Real-Dev-Squad/website-www repository onboarding form (app/components/new-join-steps/new-step-one.hbs), the fullName field is intentionally disabled as a product requirement. The name is picked from logged-in user data and users cannot change it during onboarding.
Applied to files:
tests/integration/components/new-signup/checkbox-test.jsapp/constants/new-signup.jsapp/controllers/new-signup.js
🧬 Code graph analysis (4)
tests/integration/components/new-signup/checkbox-test.js (1)
app/constants/new-signup.js (2)
NEW_SIGNUP_STEPS(7-13)NEW_SIGNUP_STEPS(7-13)
app/constants/apis.js (1)
app/constants/urls.js (2)
APPS(65-65)APPS(65-65)
app/components/profile/image-cropper.js (1)
tests/integration/components/profile/image-cropper-test.js (1)
image(27-27)
app/controllers/new-signup.js (3)
app/constants/new-signup.js (4)
NEW_SIGNUP_STEPS(7-13)NEW_SIGNUP_STEPS(7-13)SIGNUP_ERROR_MESSAGES(22-30)SIGNUP_ERROR_MESSAGES(22-30)app/utils/api-request.js (1)
apiRequest(1-26)app/constants/apis.js (4)
SELF_USER_PROFILE_URL(42-42)SELF_USER_PROFILE_URL(42-42)SELF_USERS_URL(21-23)SELF_USERS_URL(21-23)
🪛 ESLint
tests/integration/components/profile/upload-image-test.js
[error] 80-80: Replace .dom('p.message-text__failure') with ⏎······.dom('p.message-text__failure')⏎······
(prettier/prettier)
app/components/new-signup/checkbox.js
[error] 16-16: Insert ·
(prettier/prettier)
[error] 18-18: Insert ·
(prettier/prettier)
tests/integration/components/new-signup/checkbox-test.js
[error] 84-84: Replace .dom('[data-test-checkbox-label="product_manager"]') with ⏎······.dom('[data-test-checkbox-label="product_manager"]')⏎······
(prettier/prettier)
[error] 85-85: Replace .dom('[data-test-checkbox-label="project_manager"]') with ⏎······.dom('[data-test-checkbox-label="project_manager"]')⏎······
(prettier/prettier)
[error] 87-87: Replace .dom('[data-test-checkbox-label="social_media"]') with ⏎······.dom('[data-test-checkbox-label="social_media"]')⏎······
(prettier/prettier)
[error] 101-101: Insert ·
(prettier/prettier)
app/components/profile/image-cropper.js
[error] 15-15: Replace (!image) with ·(!image)·
(prettier/prettier)
tests/unit/controllers/new-signup-test.js
[error] 93-93: Insert ··
(prettier/prettier)
[error] 94-94: Insert ··
(prettier/prettier)
[error] 95-95: Insert ··
(prettier/prettier)
[error] 95-95: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 96-96: Replace ·· with ····
(prettier/prettier)
[error] 97-97: Insert ··
(prettier/prettier)
[error] 98-98: Replace ···· with ······
(prettier/prettier)
[error] 99-99: Insert ··
(prettier/prettier)
[error] 101-101: Insert ··
(prettier/prettier)
[error] 102-102: Insert ··
(prettier/prettier)
[error] 102-106: Unexpected assert.equal. Use assert.strictEqual, assert.deepEqual, or assert.propEqual.
(qunit/no-assert-equal)
[error] 103-103: Insert ··
(prettier/prettier)
[error] 104-104: Replace ···· with ······
(prettier/prettier)
[error] 105-105: Insert ··
(prettier/prettier)
[error] 106-106: Insert ··
(prettier/prettier)
[error] 107-107: Insert ··
(prettier/prettier)
[error] 108-108: Insert ··
(prettier/prettier)
[error] 109-109: Insert ··
(prettier/prettier)
[error] 110-110: Insert ··
(prettier/prettier)
[error] 112-112: Replace ·· with ····
(prettier/prettier)
[error] 113-113: Insert ··
(prettier/prettier)
[error] 114-114: Replace ···· with ······
(prettier/prettier)
[error] 115-115: Insert ··
(prettier/prettier)
[error] 116-116: Insert ··
(prettier/prettier)
[error] 117-117: Insert ··
(prettier/prettier)
[error] 118-118: Insert ··
(prettier/prettier)
[error] 119-119: Replace ···· with ······
(prettier/prettier)
[error] 120-120: Insert ··
(prettier/prettier)
[error] 121-121: Insert ··
(prettier/prettier)
[error] 122-122: Insert ··
(prettier/prettier)
⏰ 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: build (18.x)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
app/styles/new-signup.module.css (1)
4-4: Clarify the purpose of the added padding-top.The PR objectives state "No UI updates; only API and functionality changes," but adding
padding-top: 1rem;is a CSS styling change. Please clarify:
- Is this styling change necessary due to the removal of the username step?
- Should the PR objectives be updated to reflect styling modifications?
Additionally, verify that this padding works correctly across all responsive breakpoints (the
@mediaqueries extend down to 480px).app/constants/new-signup.js (1)
45-60: Remove the review comment—the backend contract concern is unfounded.The original concern conflates two separate code paths:
- CHECK_BOX_DATA (new-signup.js lines 45–60): A UI constant with 6 roles, including the renamed
product_managerand new slugs likesocial_mediaandqa.- step-one.js signup() (lines 98–104): The actual backend submission, which sends a hardcoded payload:
{ maven: bool, designer: bool, product_manager: bool }.step-one.js does not use CHECK_BOX_DATA—it imports and uses only the
ROLEconstant (which lists['Developer']). The hardcoded payload object is independent of the checkbox data changes, so the new/renamed slugs are never sent to the backend. Theproduct_managerkey has always been used here; there is noproductmanager→product_managermigration in the actual submission logic.There is no breaking API contract.
Likely an incorrect or invalid review comment.
app/controllers/new-signup.js (3)
34-34: LGTM! Step constant updated correctly.The LAST_STEP now correctly points to the THANK_YOU step after removing the username step from the flow.
36-40: LGTM! Data structure correctly refactored.The signupDetails structure now reflects single role selection and removal of the username field, consistent with the PR objectives.
129-170: The suggested validation is unnecessary—the UI already constrains role selection to valid options.The NewSignup::Checkbox component renders radio buttons exclusively from the predefined
CHECK_BOX_DATAconstant, which contains seven valid role options (developer, designer, maven, social_media, product_manager, qa, project_manager). Users cannot select an invalid role through the UI, so client-side validation inhandleCheckboxInputChangeis redundant. The component's radio button design ensures that only valid values can be submitted.Likely an incorrect or invalid review comment.
Deploying www-rds with
|
| Latest commit: |
12f737b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ea3c10d4.www-rds.pages.dev |
| Branch Preview URL: | https://feat-role-based-signup-flow.www-rds.pages.dev |
Suvidh-kaushik
force-pushed
the
feat/role_based_signup_flow
branch
from
981506f to
50fd119
Compare
Date: 20/11/2025
Developer Name: Suvidh Kaushik
Issue Ticket Number:-
Closes - #1104
Description:
Is Under Feature Flag
Database changes
Breaking changes (If your feature is breaking/missing something please mention pending tickets)
Is Development Tested?
Tested in staging?
Screenshots
Screen Shot and Video Recording
screen-capture.8.mp4
Test Coverage
Screenshot
Additional Notes