test: add tests for new join steps four, five and review step#1155
Open
MayankBansal12 wants to merge 4 commits into
Open
test: add tests for new join steps four, five and review step#1155MayankBansal12 wants to merge 4 commits into
MayankBansal12 wants to merge 4 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a "changes requested" status for applications with feedback display and edit capability, refactors social field input handling to normalize usernames, implements role auto-detection and image URL validation in the onboarding flow, updates form submission and navigation logic, and adds comprehensive test attributes across multiple components. Changes
Sequence DiagramsequenceDiagram
participant User
participant StatusCard as StatusCard Component
participant Application as Application Data
participant Router
participant Join as Join Component
User->>Application: Submit initial application
activate Application
Application-->>Application: Status set to changes_requested
Application->>Application: Admin adds feedback
deactivate Application
Application->>StatusCard: Render with status
activate StatusCard
StatusCard->>StatusCard: Display changes_requested state
StatusCard->>StatusCard: Show feedback section if exists
StatusCard->>User: Render Edit Application button
deactivate StatusCard
User->>StatusCard: Click Edit Application
activate StatusCard
StatusCard->>Router: editApplication action triggered
deactivate StatusCard
activate Router
Router->>Router: transitionTo('join', {<br/>queryParams: { edit: true, dev: true, step: 1 }})
Router->>Join: Navigate with params
deactivate Router
activate Join
Join->>Join: Initialize in edit mode
Join->>User: Display form with pre-filled data
deactivate Join
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/components/new-signup/info-test.js (1)
51-54: 🧹 Nitpick | 🔵 TrivialConsider using constants for other heading assertions for consistency.
Lines 51 and 54 still use hardcoded strings ("Congratulations!" and "Let's get you started on your journey"). If these values are defined as constants elsewhere, consider importing and using them for consistency with line 28.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/components/new-signup/info-test.js` around lines 51 - 54, Replace the hardcoded strings in the assertions targeting '[data-test-mainHeading]' and '[data-test-subHeading]' with the shared heading constants used elsewhere in this test file (the same constants referenced on line 28) by importing them at the top of tests/integration/components/new-signup/info-test.js and using those constants in the assert.dom(...).hasText(...) calls so the assertions for the main and sub headings stay consistent with the rest of the file.app/components/new-stepper.js (1)
137-155:⚠️ Potential issue | 🟡 MinorGuard JSON parsing against responses without bodies (like 204 No Content).
response.json()is called before any status or content checks. The backend uses 204 No Content responses in other endpoints, and if create/PATCH returns 204,response.json()throws before checkingresponse.status === 409or!response.ok. While the catch block provides a fallback, this loses the specific error message handling—users see the generic error instead of the 409 message or context-specific error fromdata.message.Confirm whether the application endpoints ever return 204 or non-JSON responses. If so, parse conditionally after checking
response.statusor guard the parse with a content-type check.Suggested approach
const response = await apiRequest(url, method, applicationData); - const data = await response.json(); + let data = null; + const isJson = + response.status !== 204 && + response.headers.get('content-type')?.includes('application/json'); + if (isJson) { + try { + data = await response.json(); + } catch { + data = null; + } + }Also applies to: 171-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/new-stepper.js` around lines 137 - 155, The code calls response.json() unconditionally which throws on 204/no-body responses; update the submit flow around apiRequest so you first check response.status and/or response.headers.get('content-type') before parsing JSON (or safely attempt JSON parse in a try/catch and default data to null/object), then use that parsed data for the 409 branch and the response.ok branch; adjust logic in the block referencing response, data, this.isEditMode, this.toast and this.isSubmitting so 409 and non-OK handling run without throwing when there is no JSON body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/join-steps/status-card.js`:
- Around line 58-63: The feedback getter assumes an array always has elements
and will throw for an empty feedback array; update the getter in the component
(feedback) to check if lastMessage is an array and has length > 0 before
accessing lastMessage[lastMessage.length - 1].feedback, and return
null/undefined (or an empty string) when the array is empty; use the existing
locals this.args.feedback and this.fetchedFeedback to determine lastMessage and
only access the final element when lastMessage.length > 0.
In `@app/components/new-join-steps/base-step.js`:
- Around line 102-112: The inputHandler updates pre-validation immediately but
debounces syncFormValidity, which delays calling setIsValid and can cause a
brief mismatch with the parent UI; fix by invoking the parent's authoritative
validity setter immediately for critical actions—e.g., after
this.args.setIsPreValid(this.isDataValid()) call
this.args.setIsValid(this.isDataValid()) (or call a provided immediate setter)
and keep debounceTask(this, 'syncFormValidity', JOIN_DEBOUNCE_TIME) only for the
non-critical/deferred sync; modify inputHandler (and ensure syncFormValidity/any
caller of debounceTask remains) so isDataValid() is passed to both setIsPreValid
and setIsValid immediately to remove the transient inconsistency.
In `@app/components/new-join-steps/new-step-four.hbs`:
- Around line 21-39: The inline social inputs lost the per-field invalid-state
class from the previous Reusables::InputBox, so restore conditional error
styling on each .social-input-field (e.g., add the same invalid/error class
binding that was used before when the field has validation errors) and/or
extract a shared social-input block/component to apply it consistently; update
the twitter input (references: .social-input-field, .social-input-prefix,
getSocialPrefix, inputHandler, this.data.twitter) and replicate the same change
for the other six social fields so the invalid visual state appears when their
validations fail.
In `@app/components/new-join-steps/new-step-four.js`:
- Around line 71-83: In extractUsername, strip a leading '@' from the trimmed
input before building/normalizing the URL so handles like "@alice" become
"alice" for URL parsing; update the logic in extractUsername to compute
something like cleaned = trimmed.replace(/^@/, '') and use cleaned when
constructing the normalized URL and when returning fallback values (ensure the
catch branch also returns cleaned instead of trimmed), keeping the existing
socialFields check and pathname segment extraction.
In `@app/components/new-join-steps/new-step-one.js`:
- Around line 58-67: The prefill currently overwrites an existing step role from
the login profile; update the logic in the block that reads
this.login.userData?.role / USER_ROLE_MAP so it only runs when the step does not
already have a role (e.g., check this.data.role or the step's existing value is
null/empty). If this.data.role is present, skip calling
this.updateFieldValue('role', ...) and do not toggle this.isRoleAvailable;
otherwise perform the existing lookup (Object.keys(USER_ROLE_MAP).find(...)),
set the role via updateFieldValue('role', roleKey) and set isRoleAvailable =
true. Ensure you reference the same symbols: this.login.userData?.role,
USER_ROLE_MAP, updateFieldValue('role', ...), this.isRoleAvailable and
this.data.role.
In `@app/components/new-join-steps/new-step-six.js`:
- Line 46: The getter name showdribbble is incorrectly cased; rename the getter
in the new-step-six component from showdribbble to showDribbble (matching
showGitHub and showBehance), update the analogous getter in the new-step-four
component, and update all template usages that reference showdribbble in the
new-step-six.hbs and new-step-four.hbs to use showDribbble so casing is
consistent between JS and templates.
In `@app/components/new-join-steps/new-step-two.hbs`:
- Line 29: Update the `@field` attribute value in the new-step-two.hbs template to
use consistent sentence case by changing "Your institution/College" to "Your
institution/college" (i.e., lowercase the word "College") so the label matches
surrounding copy and casing conventions.
In `@app/controllers/goto.js`:
- Around line 31-36: redirectionHandler currently ignores the isDev flag so
dev-mode redirects break; update redirectionHandler(isDev, user) to honor isDev
when building the target URL: if user.incompleteUserDetails redirect to
this.SIGN_UP_URL (preserving dev if isDev), otherwise redirect to this.HOME_URL
(preserving dev if isDev); implement this by computing the target URL (eg. base
= this.SIGN_UP_URL or this.HOME_URL) and appending the dev query parameter when
isDev is true before calling redirectUserToPage, referencing redirectionHandler,
redirectUserToPage, SIGN_UP_URL and HOME_URL so callers that emit dev=true will
behave consistently.
In `@app/styles/application-detail.module.css`:
- Line 129: The background declaration in application-detail.module.css uses an
undefined CSS variable (--color-lightgray); replace that token with a defined
design token (or add a definition) so the rule resolves correctly—update the
background property to use an existing variable (e.g., the project’s gray/bg
token) or add a matching --color-lightgray definition to :root, and ensure you
change the reference wherever background: var(--color-lightgray) appears (search
for the literal --color-lightgray).
In `@tests/integration/components/new-join-steps/new-step-six-test.js`:
- Around line 54-60: The test "shows Not provided for empty fields" is brittle
because assert.dom('[data-test="field-value"]').hasText('Not provided') only
checks the first matching element; change the assertion to target a specific
field or explicitly check all empty fields: update the selector used in this
test (for example reference the NewJoinSteps::NewStepSix field by adding/using a
more specific attribute like data-test-field="FIELD_NAME" or target the nth
element via findAll and
assert.dom(findAll('[data-test="field-value"]')[INDEX]).hasText('Not
provided')), or iterate over findAll('[data-test="field-value"]') and assert
each item's text equals 'Not provided' to ensure the test validates the intended
field(s).
---
Outside diff comments:
In `@app/components/new-stepper.js`:
- Around line 137-155: The code calls response.json() unconditionally which
throws on 204/no-body responses; update the submit flow around apiRequest so you
first check response.status and/or response.headers.get('content-type') before
parsing JSON (or safely attempt JSON parse in a try/catch and default data to
null/object), then use that parsed data for the 409 branch and the response.ok
branch; adjust logic in the block referencing response, data, this.isEditMode,
this.toast and this.isSubmitting so 409 and non-OK handling run without throwing
when there is no JSON body.
In `@tests/integration/components/new-signup/info-test.js`:
- Around line 51-54: Replace the hardcoded strings in the assertions targeting
'[data-test-mainHeading]' and '[data-test-subHeading]' with the shared heading
constants used elsewhere in this test file (the same constants referenced on
line 28) by importing them at the top of
tests/integration/components/new-signup/info-test.js and using those constants
in the assert.dom(...).hasText(...) calls so the assertions for the main and sub
headings stay consistent with the rest of the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19b9a1f8-eef9-49f8-bb82-aa8ef20f356c
📒 Files selected for processing (33)
app/components/application/detail-header.jsapp/components/join-steps/status-card.hbsapp/components/join-steps/status-card.jsapp/components/new-join-steps/base-step.jsapp/components/new-join-steps/new-step-five.hbsapp/components/new-join-steps/new-step-four.hbsapp/components/new-join-steps/new-step-four.jsapp/components/new-join-steps/new-step-one.hbsapp/components/new-join-steps/new-step-one.jsapp/components/new-join-steps/new-step-six.hbsapp/components/new-join-steps/new-step-six.jsapp/components/new-join-steps/new-step-two.hbsapp/components/new-stepper.jsapp/components/signup-steps/step-zero.hbsapp/components/signup-steps/step-zero.jsapp/constants/applications.jsapp/constants/join.jsapp/constants/new-join-form.jsapp/constants/new-signup.jsapp/constants/urls.jsapp/controllers/applications/detail.jsapp/controllers/goto.jsapp/controllers/join.jsapp/styles/application-detail.module.cssapp/styles/input.module.cssapp/styles/new-stepper.module.cssapp/templates/applications/detail.hbstests/integration/components/new-join-steps/new-step-five-test.jstests/integration/components/new-join-steps/new-step-one-test.jstests/integration/components/new-join-steps/new-step-six-test.jstests/integration/components/new-signup/info-test.jstests/integration/components/new-stepper-test.jstests/integration/components/signup-steps/step-zero-test.js
MayankBansal12
force-pushed
the
test/new-join-steps-2
branch
from
c4ac21b to
e9b3f59
Compare
Deploying www-rds with
|
| Latest commit: |
2104ebb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ab9fa142.www-rds.pages.dev |
| Branch Preview URL: | https://test-new-join-steps-2.www-rds.pages.dev |
MayankBansal12
changed the title
MayankBansal12
force-pushed
the
test/new-join-steps-2
branch
from
cecb0d4 to
e9b3f59
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Date: 14-03-26
Developer Name: @MayankBansal12
Issue Ticket Number:-
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?
Add relevant Screenshot below ( e.g test coverage etc. )
tests run