Fixed the registration of email of different domains#1034
Conversation
📝 WalkthroughWalkthroughThis pull request modifies the registration component to improve username generation and form handling. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/components/registration/registration.js (2)
150-165: 🛠️ Refactor suggestion | 🟠 Major
generateUsernamedefined insidehandleSubmit— consider hoisting.This utility function is pure and stateless. Defining it inside
handleSubmitrecreates it on every submit and leads to the ESLintno-shadowviolation (parameter♻️ Suggested refactor
At module scope (e.g. before the class declaration):
+const generateUsername = (addr) => + addr.replace("@", "_at_").replace(/\./g, "_"); + export default class Registration extends React.Component {Then in
handleSubmit:- const generateUsername = (email) => { - return email.replace(/[@.]/g, "_"); // user@gmail.com → user_gmail_com - }; - const postData = { email, username: generateUsername(email),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/registration/registration.js` around lines 150 - 165, The generateUsername utility is currently declared inside handleSubmit, causing it to be recreated on every submit and triggering a no-shadow ESLint issue for the email parameter; hoist generateUsername to module scope (or a static method on the component) so it is defined once (e.g., declare const generateUsername = (email) => ... before the component/function) and remove the inner definition from handleSubmit, then call that hoisted generateUsername(email) when building postData to avoid shadowing and repeated recreation.
183-196:⚠️ Potential issue | 🔴 Critical
postData.usernameoverwrite defeatsgenerateUsernamefor plan-selected paths.When
selectedPlan !== null, line 184 unconditionally overwritespostData.usernamewith the rawusernamestate value. The username input field (lines 498–516) is only rendered whenisPlanIdentityVerifier()is true (i.e.requires_payment === true). For plans whererequires_payment === false, no username field is shown, sousernameremains""— sending an empty string to the backend.Consider guarding this overwrite:
🛠️ Suggested guard
if (selectedPlan !== null && plan_pricing) { - postData.username = username; + if (this.isPlanIdentityVerifier()) { + postData.username = username; + } if (plan_pricing.requires_invoice === true) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/registration/registration.js` around lines 183 - 196, The code unconditionally sets postData.username = username when selectedPlan !== null, which overwrites the generated username with an empty string for plans that don't render the username input; change the assignment to only set postData.username when a real username was provided (e.g., username.trim() is non-empty) or when the plan actually requires identity verification (use isPlanIdentityVerifier() or plan_pricing.requires_payment) so that generateUsername() result remains intact otherwise; update the block around selectedPlan/plan_pricing (the postData.username assignment) to guard the overwrite accordingly.
🤖 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/registration/registration.js`:
- Around line 518-519: The component accesses input_fields.first_name.setting
(and similarly input_fields.last_name.setting, input_fields.birth_date.setting,
input_fields.location.setting) without guarding for missing keys which can throw
TypeError; update those accesses in the Registration component render logic to
use optional chaining or defaults (e.g., input_fields.first_name?.setting or
(input_fields.first_name || {}).setting) and ensure the conditional that uses
doesPlanRequireInvoice() (method doesPlanRequireInvoice) still behaves correctly
when fields are undefined; apply the same fix to the occurrences at the other
indicated checks so missing input_fields entries no longer crash the component.
- Around line 271-272: The phone_number field in registration.js is being spread
raw while other fields are converted to strings, causing inconsistent types for
getError; update the object construction where phone_number is spread (the lines
building the payload in registration.js) to stringify phone_number like the
others (use data.phone_number.toString() when present and provide a consistent
fallback such as an empty string) so phone_number always has the same string
type as email/username/first_name.
- Around line 151-158: The generateUsername function (used to set
postData.username) currently shadows the outer email variable, uses an
unnecessary block body, and can produce collisions by flattening both dots and @
into underscores; fix by renaming the parameter (e.g., srcEmail) to avoid
shadowing, convert the arrow to a concise body (single expression), and change
the transformation to preserve uniqueness — for example derive a base from the
local part and append a short deterministic suffix (timestamp, short hash of the
full email, or domain-based segment) so usernames for different addresses like
a.b@c.d and a@b.c.d won't collide; update references to postData.username to use
the new generateUsername signature (and ensure any randomness is safe for
retries or collisions).
- Around line 407-431: The Suspense fallback input’s onChange currently treats
its argument as a raw string (matching react-phone-input-2) which causes
this.handleChange to receive an object-wrapped non-string; update the fallback
input’s onChange so it reads the actual DOM value from the event (use
event.target.value) before calling this.handleChange with target.name
"phone_number" and the formatted value (e.g., prepend '+' if desired), keeping
the existing submitOnEnter handler and registration-form id intact.
- Around line 246-248: The catch block destructures error.response without
guarding for network errors; modify the .catch((error) => { ... }) handler to
first check if error.response is present (e.g., if (!error.response) { /* handle
network/timeout/CORS error: show generic network error toast/log and return */
}) and only then destructure const { data, status } = error.response; also
ensure subsequent checks that inspect data (like '"billing_info" in data') only
run when data is non-null to avoid TypeError.
- Around line 498-516: The JSX unconditionally accesses
input_fields.username.pattern inside the username input while
input_fields.username can be undefined; update accesses in the username field
render (inside isPlanIdentityVerifier() block in registration.js) to use
optional chaining when reading configuration (e.g., replace
input_fields.username.pattern with input_fields.username?.pattern and similarly
guard any other direct reads like title/placeholder if present) so the component
won't throw when username config is omitted; keep the rest of the handlers
(handleChange, errors, username) unchanged.
---
Outside diff comments:
In `@client/components/registration/registration.js`:
- Around line 150-165: The generateUsername utility is currently declared inside
handleSubmit, causing it to be recreated on every submit and triggering a
no-shadow ESLint issue for the email parameter; hoist generateUsername to module
scope (or a static method on the component) so it is defined once (e.g., declare
const generateUsername = (email) => ... before the component/function) and
remove the inner definition from handleSubmit, then call that hoisted
generateUsername(email) when building postData to avoid shadowing and repeated
recreation.
- Around line 183-196: The code unconditionally sets postData.username =
username when selectedPlan !== null, which overwrites the generated username
with an empty string for plans that don't render the username input; change the
assignment to only set postData.username when a real username was provided
(e.g., username.trim() is non-empty) or when the plan actually requires identity
verification (use isPlanIdentityVerifier() or plan_pricing.requires_payment) so
that generateUsername() result remains intact otherwise; update the block around
selectedPlan/plan_pricing (the postData.username assignment) to guard the
overwrite accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
client/components/registration/registration.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). (1)
- GitHub Check: Tests and Coverage
🧰 Additional context used
🪛 ESLint
client/components/registration/registration.js
[error] 151-151: 'email' is already declared in the upper scope on line 120 column 7.
(no-shadow)
[error] 151-153: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.
(arrow-body-style)
🔇 Additional comments (2)
client/components/registration/registration.js (2)
480-496: LGTM!Email input field is well-structured with proper validation pattern, autoComplete, and error styling.
619-671: LGTM!Password and confirm-password fields are well-structured with toggle icons, proper patterns, and autoComplete attributes.
| const generateUsername = (email) => { | ||
| return email.replace(/[@.]/g, "_"); // user@gmail.com → user_gmail_com | ||
| }; | ||
|
|
||
|
|
||
| const postData = { | ||
| email, | ||
| username: email, | ||
| username: generateUsername(email), // NEW |
There was a problem hiding this comment.
generateUsername shadows outer email and uses unnecessary block body (ESLint errors).
The parameter email shadows the identically-named variable from line 120. Also, the block body wrapping a single return violates arrow-body-style. Both are flagged by ESLint.
Beyond the lint issues, this function can produce username collisions — e.g. a.b@c.d and a@b.c.d both map to a_b_c_d. If the backend enforces unique usernames, some registrations will fail unexpectedly.
🛠️ Proposed fix (lint + clarity)
- const generateUsername = (email) => {
- return email.replace(/[@.]/g, "_"); // user@gmail.com → user_gmail_com
- };
-
+ const generateUsername = (addr) =>
+ addr.replace("@", "_at_").replace(/\./g, "_"); // user@gmail.com → user_at_gmail_com
const postData = {
email,
- username: generateUsername(email), // NEW
+ username: generateUsername(email),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const generateUsername = (email) => { | |
| return email.replace(/[@.]/g, "_"); // user@gmail.com → user_gmail_com | |
| }; | |
| const postData = { | |
| email, | |
| username: email, | |
| username: generateUsername(email), // NEW | |
| const generateUsername = (addr) => | |
| addr.replace("@", "_at_").replace(/\./g, "_"); // user@gmail.com → user_at_gmail_com | |
| const postData = { | |
| email, | |
| username: generateUsername(email), |
🧰 Tools
🪛 ESLint
[error] 151-151: 'email' is already declared in the upper scope on line 120 column 7.
(no-shadow)
[error] 151-153: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.
(arrow-body-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/components/registration/registration.js` around lines 151 - 158, The
generateUsername function (used to set postData.username) currently shadows the
outer email variable, uses an unnecessary block body, and can produce collisions
by flattening both dots and @ into underscores; fix by renaming the parameter
(e.g., srcEmail) to avoid shadowing, convert the arrow to a concise body (single
expression), and change the transformation to preserve uniqueness — for example
derive a base from the local part and append a short deterministic suffix
(timestamp, short hash of the full email, or domain-based segment) so usernames
for different addresses like a.b@c.d and a@b.c.d won't collide; update
references to postData.username to use the new generateUsername signature (and
ensure any randomness is safe for retries or collisions).
| .catch((error) => { | ||
| const {data, status} = error.response; | ||
| const { data, status } = error.response; | ||
| if (status === 404) { |
There was a problem hiding this comment.
Missing null-check on error.response — network errors will crash.
If the request fails without a server response (e.g. timeout, DNS failure, CORS), error.response is undefined. Destructuring it on line 247 produces data = undefined, which causes a TypeError at line 259 ("billing_info" in data).
🛠️ Proposed guard
.catch((error) => {
+ if (!error.response) {
+ setLoading(false);
+ const errorText = getErrorText(error, t`REGISTER_ERR`);
+ logError(error, errorText);
+ toast.error(errorText);
+ return;
+ }
const { data, status } = error.response;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .catch((error) => { | |
| const {data, status} = error.response; | |
| const { data, status } = error.response; | |
| if (status === 404) { | |
| .catch((error) => { | |
| if (!error.response) { | |
| setLoading(false); | |
| const errorText = getErrorText(error, t`REGISTER_ERR`); | |
| logError(error, errorText); | |
| toast.error(errorText); | |
| return; | |
| } | |
| const { data, status } = error.response; | |
| if (status === 404) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/components/registration/registration.js` around lines 246 - 248, The
catch block destructures error.response without guarding for network errors;
modify the .catch((error) => { ... }) handler to first check if error.response
is present (e.g., if (!error.response) { /* handle network/timeout/CORS error:
show generic network error toast/log and return */ }) and only then destructure
const { data, status } = error.response; also ensure subsequent checks that
inspect data (like '"billing_info" in data') only run when data is non-null to
avoid TypeError.
| ...(data.phone_number ? { phone_number: data.phone_number } : null), | ||
| ...(data.email ? { email: data.email.toString() } : { email: "" }), |
There was a problem hiding this comment.
phone_number error is not stringified, unlike every other field.
All other fields (email, username, first_name, …) call .toString(), but phone_number on line 271 is spread raw. If the backend returns an array (e.g. ["Invalid phone number."]), this field will have a different type than the rest, which may cause inconsistent rendering in getError.
- ...(data.phone_number ? { phone_number: data.phone_number } : null),
+ ...(data.phone_number ? { phone_number: data.phone_number.toString() } : { phone_number: "" }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ...(data.phone_number ? { phone_number: data.phone_number } : null), | |
| ...(data.email ? { email: data.email.toString() } : { email: "" }), | |
| ...(data.phone_number ? { phone_number: data.phone_number.toString() } : { phone_number: "" }), | |
| ...(data.email ? { email: data.email.toString() } : { email: "" }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/components/registration/registration.js` around lines 271 - 272, The
phone_number field in registration.js is being spread raw while other fields are
converted to strings, causing inconsistent types for getError; update the object
construction where phone_number is spread (the lines building the payload in
registration.js) to stringify phone_number like the others (use
data.phone_number.toString() when present and provide a consistent fallback such
as an empty string) so phone_number always has the same string type as
email/username/first_name.
| <Suspense | ||
| fallback={ | ||
| <input | ||
| type="tel" | ||
| className="input" | ||
| name="phone_number" | ||
| value={phone_number} | ||
| onChange={(value) => | ||
| this.handleChange({ | ||
| target: { | ||
| name: "phone_number", | ||
| value: `+${value}`, | ||
| }, | ||
| }) | ||
| } | ||
| onKeyDown={(event) => { | ||
| submitOnEnter( | ||
| event, | ||
| this, | ||
| "registration-form", | ||
| ); | ||
| }} | ||
| placeholder={t`PHONE_PHOLD`} | ||
| /> | ||
| } |
There was a problem hiding this comment.
Suspense fallback <input> onChange handler is incompatible with standard DOM events.
The fallback <input type="tel"> receives a standard SyntheticEvent in its onChange, but the handler at line 414 treats the argument as a raw value string (matching react-phone-input-2's callback signature). This will store "+[object Object]" (or similar) in state instead of the actual typed value.
🛠️ Fix: extract value from the event
onChange={(value) =>
+ onChange={(e) =>
this.handleChange({
target: {
name: "phone_number",
- value: `+${value}`,
+ value: `+${e.target.value}`,
},
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/components/registration/registration.js` around lines 407 - 431, The
Suspense fallback input’s onChange currently treats its argument as a raw string
(matching react-phone-input-2) which causes this.handleChange to receive an
object-wrapped non-string; update the fallback input’s onChange so it reads the
actual DOM value from the event (use event.target.value) before calling
this.handleChange with target.name "phone_number" and the formatted value (e.g.,
prepend '+' if desired), keeping the existing submitOnEnter handler and
registration-form id intact.
| {this.isPlanIdentityVerifier() && ( | ||
| <div className="row username"> | ||
| <label htmlFor="username">{t`USERNAME_REG_LBL`}</label> | ||
| {getError(errors, "username")} | ||
| <input | ||
| className={`input ${errors.street ? "error" : ""}`} | ||
| className={`input ${errors.username ? "error" : ""}`} | ||
| type="text" | ||
| id="street" | ||
| id="username" | ||
| required | ||
| name="street" | ||
| value={street} | ||
| name="username" | ||
| value={username} | ||
| onChange={this.handleChange} | ||
| autoComplete="address" | ||
| placeholder={t`STREET_PHOLD`} | ||
| placeholder={t`USERNAME_REG_PHOLD`} | ||
| pattern={input_fields.username.pattern} | ||
| autoComplete="username" | ||
| title={t`USERNAME_PTRN_DESC`} | ||
| /> | ||
| </div> | ||
| <div className="row zipcode"> | ||
| <label htmlFor="zipcode">{t`ZIP_CODE_LBL`}</label> | ||
| {getError(errors, "zipcode")} | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the file structure and find the PropTypes definition
wc -l client/components/registration/registration.jsRepository: openwisp/openwisp-wifi-login-pages
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Read the PropTypes definition around line 858-862
sed -n '850,880p' client/components/registration/registration.jsRepository: openwisp/openwisp-wifi-login-pages
Length of output: 1131
🏁 Script executed:
#!/bin/bash
# Check the broader context of input_fields PropTypes definition
sed -n '840,920p' client/components/registration/registration.jsRepository: openwisp/openwisp-wifi-login-pages
Length of output: 2328
🏁 Script executed:
#!/bin/bash
# Search for where input_fields is used in the file
rg -n "input_fields\." client/components/registration/registration.js | head -30Repository: openwisp/openwisp-wifi-login-pages
Length of output: 1975
🏁 Script executed:
#!/bin/bash
# Check the context before line 498 to see if username is guarded
sed -n '490,520p' client/components/registration/registration.jsRepository: openwisp/openwisp-wifi-login-pages
Length of output: 1618
🏁 Script executed:
#!/bin/bash
# Check if there's any guard or check for input_fields.username before usage
rg -B5 "input_fields\.username" client/components/registration/registration.jsRepository: openwisp/openwisp-wifi-login-pages
Length of output: 404
🏁 Script executed:
#!/bin/bash
# Check the isPlanIdentityVerifier method to understand what it guarantees
rg -n "isPlanIdentityVerifier" client/components/registration/registration.js -A10 | head -40Repository: openwisp/openwisp-wifi-login-pages
Length of output: 1811
Add optional chaining to guard against missing username field configuration.
input_fields.username is optional per PropTypes, yet line 511 accesses input_fields.username.pattern unconditionally. The rendering is gated by isPlanIdentityVerifier(), which only checks if a payment plan is selected—it does not guarantee that the username field is configured. If an organization has payment plans but omits the username field from config, this will throw a TypeError.
Use optional chaining:
- pattern={input_fields.username.pattern}
+ pattern={input_fields.username?.pattern}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {this.isPlanIdentityVerifier() && ( | |
| <div className="row username"> | |
| <label htmlFor="username">{t`USERNAME_REG_LBL`}</label> | |
| {getError(errors, "username")} | |
| <input | |
| className={`input ${errors.street ? "error" : ""}`} | |
| className={`input ${errors.username ? "error" : ""}`} | |
| type="text" | |
| id="street" | |
| id="username" | |
| required | |
| name="street" | |
| value={street} | |
| name="username" | |
| value={username} | |
| onChange={this.handleChange} | |
| autoComplete="address" | |
| placeholder={t`STREET_PHOLD`} | |
| placeholder={t`USERNAME_REG_PHOLD`} | |
| pattern={input_fields.username.pattern} | |
| autoComplete="username" | |
| title={t`USERNAME_PTRN_DESC`} | |
| /> | |
| </div> | |
| <div className="row zipcode"> | |
| <label htmlFor="zipcode">{t`ZIP_CODE_LBL`}</label> | |
| {getError(errors, "zipcode")} | |
| )} | |
| {this.isPlanIdentityVerifier() && ( | |
| <div className="row username"> | |
| <label htmlFor="username">{t`USERNAME_REG_LBL`}</label> | |
| {getError(errors, "username")} | |
| <input | |
| className={`input ${errors.username ? "error" : ""}`} | |
| type="text" | |
| id="username" | |
| required | |
| name="username" | |
| value={username} | |
| onChange={this.handleChange} | |
| placeholder={t`USERNAME_REG_PHOLD`} | |
| pattern={input_fields.username?.pattern} | |
| autoComplete="username" | |
| title={t`USERNAME_PTRN_DESC`} | |
| /> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/components/registration/registration.js` around lines 498 - 516, The
JSX unconditionally accesses input_fields.username.pattern inside the username
input while input_fields.username can be undefined; update accesses in the
username field render (inside isPlanIdentityVerifier() block in registration.js)
to use optional chaining when reading configuration (e.g., replace
input_fields.username.pattern with input_fields.username?.pattern and similarly
guard any other direct reads like title/placeholder if present) so the component
won't throw when username config is omitted; keep the rest of the handlers
(handleChange, errors, username) unchanged.
| {(input_fields.first_name.setting !== "disabled" || | ||
| this.doesPlanRequireInvoice()) && ( |
There was a problem hiding this comment.
Accessing .setting on potentially undefined input_fields entries.
first_name, last_name, birth_date, and location are all optional in PropTypes, but lines 518, 544, 570, and 593 unconditionally access .setting on them. If any of these fields are missing from the configuration, the component will crash with a TypeError.
Consider adding optional chaining (e.g. input_fields.first_name?.setting) or providing defaults.
Also applies to: 544-545, 570-570, 593-593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/components/registration/registration.js` around lines 518 - 519, The
component accesses input_fields.first_name.setting (and similarly
input_fields.last_name.setting, input_fields.birth_date.setting,
input_fields.location.setting) without guarding for missing keys which can throw
TypeError; update those accesses in the Registration component render logic to
use optional chaining or defaults (e.g., input_fields.first_name?.setting or
(input_fields.first_name || {}).setting) and ensure the conditional that uses
doesPlanRequireInvoice() (method doesPlanRequireInvoice) still behaves correctly
when fields are undefined; apply the same fix to the occurrences at the other
indicated checks so missing input_fields entries no longer crash the component.
1 participant
Checklist
Reference to Existing Issue
Closes #.
Please open a new issue if there isn't an existing issue yet.
Description of Changes
Please describe these changes.
Screenshot
Please include any relevant screenshots.