Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BugFix : Fixed Improper Handling for Non-Numeric Values in Allotted hours section of Create Action item modal #3379

Closed
wants to merge 13 commits into from
Closed

BugFix : Fixed Improper Handling for Non-Numeric Values in Allotted hours section of Create Action item modal #3379

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6e67a76
Proper Handling for non numeric values in allotted hours section of a…
yugal07 Jan 21, 2025
13d4ef6
Added test cases and used Number.isNaN instead of isNaN
yugal07 Jan 22, 2025
6374d21
Removed code coverage disable lines that past contributors added and …
yugal07 Jan 23, 2025
fef4754
Added more test cases to improve code coverage
yugal07 Jan 23, 2025
4d575ac
Bugfix: fixed duplication of chats (#3360)
Aad1tya27 Jan 21, 2025
4fe4264
Update pull-request.yml
palisadoes Jan 21, 2025
e0b3ca7
Fixes: The Login and Organization Screens (#3309)
prathmesh703 Jan 21, 2025
ddfa246
Fixed ESLint bugs that required ts-specific rules for js functions an…
yugal07 Jan 22, 2025
d874f00
Fixes[MemberDetail] : Refactor CSS files in src/screens/MemberDetail …
Bittukr7479 Jan 22, 2025
190722a
refactor CSS src/components/EventRegistrantsModal (#3383)
prathmesh703 Jan 22, 2025
1423156
Updated incorrect color names (#3395)
palisadoes Jan 23, 2025
54de360
Tests: Added tests for ```src/components/MemberRequestCard/MemberRequ…
Aad1tya27 Jan 23, 2025
14615bb
Implemented coderabbit suggestion and added more test cases to improv…
yugal07 Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/screens/OrganizationActionItems/ItemModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,16 @@ const ItemModal: FC<InterfaceItemModalProps> = ({
field: keyof InterfaceFormStateType,
value: string | number | boolean | Date | undefined | null,
): void => {
// Special handling for allottedHours
if (field === 'allottedHours') {
// If the value is not a valid number or is negative, set to null
const numValue = typeof value === 'string' ? Number(value) : value;
if (typeof numValue !== 'number' || isNaN(numValue) || numValue < 0) {
setFormState((prevState) => ({ ...prevState, [field]: null }));
return;
}
}

Copy link
Contributor

@coderabbitai coderabbitai bot Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance the validation logic for allotted hours.

The current validation can be improved for better type safety and explicit handling:

  1. Replace unsafe isNaN with Number.isNaN to avoid type coercion.
  2. Make the validation more explicit and robust.
 if (field === 'allottedHours') {
-  // If the value is not a valid number or is negative, set to null
-  const numValue = typeof value === 'string' ? Number(value) : value;
-  if (typeof numValue !== 'number' || isNaN(numValue) || numValue < 0) {
+  // Convert to number and validate
+  const numValue = value === null ? null : Number(value);
+  if (numValue === null || Number.isNaN(numValue) || numValue < 0) {
     setFormState((prevState) => ({ ...prevState, [field]: null }));
     return;
   }
+  // Set the validated number
+  setFormState((prevState) => ({ ...prevState, [field]: numValue }));
+  return;
 }
📝 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.

Suggested change
// Special handling for allottedHours
if (field === 'allottedHours') {
// If the value is not a valid number or is negative, set to null
const numValue = typeof value === 'string' ? Number(value) : value;
if (typeof numValue !== 'number' || isNaN(numValue) || numValue < 0) {
setFormState((prevState) => ({ ...prevState, [field]: null }));
return;
}
}
// Special handling for allottedHours
if (field === 'allottedHours') {
// Convert to number and validate
const numValue = value === null ? null : Number(value);
if (numValue === null || Number.isNaN(numValue) || numValue < 0) {
setFormState((prevState) => ({ ...prevState, [field]: null }));
return;
}
// Set the validated number
setFormState((prevState) => ({ ...prevState, [field]: numValue }));
return;
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 233-233: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yugal07 I can still see the old code in the file. The changes we discussed (using Number.isNaN and making the validation more explicit) don't appear to be implemented yet. Could you please verify if you've committed the changes? Here's what I'm seeing in the file:

// Special handling for allottedHours
if (field === 'allottedHours') {
  // If the value is not a valid number or is negative, set to null
  const numValue = typeof value === 'string' ? Number(value) : value;
  if (
    typeof numValue !== 'number' ||

Let me know if you need any help with implementing the suggested changes!

🔗 Analysis chain
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the changes have been implemented in the file
rg -A 5 "Special handling for allottedHours" "src/screens/OrganizationActionItems/ItemModal.tsx"

Length of output: 372

Script:

#!/bin/bash
# Check recent git activity
git log -1 --pretty=format:"%h %s" --
git diff HEAD src/screens/OrganizationActionItems/ItemModal.tsx

Length of output: 65

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It loses functionality if i add all your code. I have replaced isNaN with Number.isNaN

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yugal07 You're absolutely right! The typeof check in the original code is important for proper form state management. Thank you for implementing the Number.isNaN change while preserving the necessary type checking functionality.

✏️ Learnings added 
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-admin#3379
File: src/screens/OrganizationActionItems/ItemModal.tsx:0-0
Timestamp: 2025-01-23T14:09:29.347Z
Learning: In form state management for the `allottedHours` field in `ItemModal.tsx`, it's crucial to maintain type checking using `typeof value !== 'number'` along with `Number.isNaN` to ensure proper validation of numeric values while preserving the form state integrity.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

setFormState((prevState) => ({ ...prevState, [field]: value }));
};

Expand Down
Loading