💊Fix unnecessary API Calls on updating an existing asset (#9189) #9192
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for care-ohc failed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/AssetCreate.tsx (3)
113-147: Consider using a more maintainable type definition.The type definition could be extracted and reused to improve maintainability and reduce duplication.
+type AssetFormValues = { + name: string; + description: string; + location: string; + asset_class: AssetClass | undefined; + is_working: string | undefined; + not_working_reason: string; + serial_number: string; + vendor_name: string; + support_name: string; + support_email: string; + support_phone: string; + qr_code_id: string; + manufacturer: string; + warranty_amc_end_of_validity: any; + last_serviced_on: any; + notes: string; +}; -const [initialValues, setInitialValues] = useState<{...}>({ +const [initialValues, setInitialValues] = useState<AssetFormValues>({
267-313: Optimize form value comparison logic.The current implementation recreates the
currentValuesobject on every render. Consider memoizing it withuseMemoto prevent unnecessary object creation.+const currentValues = useMemo(() => ({ + name, + description, + location, + asset_class, + is_working, + not_working_reason, + serial_number, + vendor_name, + support_name, + support_email, + support_phone, + qr_code_id: qrCodeId, + manufacturer, + warranty_amc_end_of_validity, + last_serviced_on, + notes, +}), [ + name, + description, + location, + asset_class, + is_working, + not_working_reason, + serial_number, + vendor_name, + support_name, + support_email, + support_phone, + qrCodeId, + manufacturer, + warranty_amc_end_of_validity, + last_serviced_on, + notes, +]); -const currentValues = { - name, - description, - // ... other fields -};
288-288: Remove debug console.log statement.Debug logging should be removed before merging to production.
- console.log("Initial Values " + currentValues);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(4 hunks)
🔇 Additional comments (1)
src/components/Facility/AssetCreate.tsx (1)
1005-1005: LGTM! Good implementation of form submission optimization.
The disabled state on submit buttons effectively prevents unnecessary API calls when no changes are made to the form, which directly addresses the PR objectives.
Also applies to: 1012-1012
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/components/Facility/AssetCreate.tsx (4)
64-88: Consider improving the AssetT interface.The interface has some potential improvements:
- The
notesfield is marked as required (no?), but it's optional in the form.- The
last_servicetype is set toany, which should be avoided for type safety.interface AssetT { - last_service?: any; + last_service?: { + serviced_on?: string; + note?: string; + }; - notes: string; + notes?: string; }
225-263: Enhance form validation for better user experience.The validation logic has some areas for improvement:
- Phone number validation is complex and could be extracted into a separate function
- Email validation could be moved closer to the email field validation
const AssetFormValidator = (form: AssetT): FormErrors<AssetT> => { const errors: Partial<Record<keyof AssetT, FieldError>> = {}; + const validateSupportPhone = (phone?: string): string | undefined => { + if (!phone) return t("field_required"); + const checkTollFree = phone.startsWith("1800"); + const supportPhoneSimple = phone.replace(/[^0-9]/g, "").slice(2); + if (supportPhoneSimple.length !== 10 && !checkTollFree) { + return "Please enter valid phone number"; + } + if ((phone.length < 10 || phone.length > 11) && checkTollFree) { + return "Please enter valid phone number"; + } + return undefined; + }; errors.name = RequiredFieldValidator()(form.name); errors.is_working = form.is_working === undefined ? t("field_required") : undefined; errors.location = !form.location || form.location === "0" || form.location === "" ? "Select a location" : undefined; - if (!form.support_phone) { - errors.support_phone = t("field_required"); - } else { - const checkTollFree = form.support_phone.startsWith("1800"); - // ... rest of phone validation - } + errors.support_phone = validateSupportPhone(form.support_phone); errors.support_email = form.support_email && !validateEmailAddress(form.support_email) ? "Please enter valid email id" : undefined; errors.serviced_on = form.notes && !form.serviced_on ? "Last serviced on date is required with notes" : undefined; return errors; };
710-734: Improve date validation logic.The warranty expiry date validation could be improved:
- The date comparison logic could be simplified using dayjs
- The validation message could be more user-friendly
<TextFormField {...field("warranty_amc_end_of_validity")} label={t("warranty_amc_expiry")} onChange={(event) => { const { value } = event; const selectedDate = dayjs(value); - const formattedDate = selectedDate.format("YYYY-MM-DD"); - const today = dayjs().format("YYYY-MM-DD"); - if (selectedDate.isBefore(today)) { + if (selectedDate.isBefore(dayjs().startOf('day'))) { Notification.Error({ - msg: "Warranty / AMC Expiry date can't be in the past", + msg: "Please select a future date for warranty/AMC expiry", }); } else { field("warranty_amc_end_of_validity").onChange({ name: "warranty_amc_end_of_validity", - value: formattedDate, + value: selectedDate.format("YYYY-MM-DD"), }); } }} type="date" - min={dayjs().format("YYYY-MM-DD")} + min={dayjs().startOf('day').format("YYYY-MM-DD")} />
813-847: Simplify last serviced date validation.Similar to the warranty date validation, the last serviced date validation could be simplified:
- Use dayjs consistently for date comparisons
- Improve error message clarity
<DateFormField {...field("serviced_on")} label={t("last_serviced_on")} className="mt-2" disableFuture value={field("serviced_on").value ? new Date(field("serviced_on").value) : undefined} onChange={(date) => { - const selectedDate = dayjs(date.value).format("YYYY-MM-DD"); - const today = new Date().toLocaleDateString("en-ca"); + const selectedDate = dayjs(date.value); + const today = dayjs(); - if (selectedDate > today) { + if (selectedDate.isAfter(today)) { Notification.Error({ - msg: "Last Serviced date can't be in the future", + msg: "Service date cannot be in the future", }); } else { field("serviced_on").onChange({ name: "serviced_on", - value: selectedDate, + value: selectedDate.format("YYYY-MM-DD"), }); } }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(9 hunks)
🔇 Additional comments (1)
src/components/Facility/AssetCreate.tsx (1)
139-141: Form state initialization aligns with PR objectives.
The implementation correctly addresses the PR objective by initializing the form state with fetched values, which will help prevent unnecessary API calls when no changes are made.
Also applies to: 197-221
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/AssetCreate.tsx (3)
225-263: Simplify phone number validation and ensure consistent translationsThe phone number validation logic could be simplified, and error messages should consistently use translations.
Consider this refactor:
if (!form.support_phone) { errors.support_phone = t("field_required"); } else { - const checkTollFree = form.support_phone.startsWith("1800"); - const supportPhoneSimple = form.support_phone - .replace(/[^0-9]/g, "") - .slice(2); - if (supportPhoneSimple.length !== 10 && !checkTollFree) { - errors.support_phone = "Please enter valid phone number"; - } else if ( - (form.support_phone.length < 10 || form.support_phone.length > 11) && - checkTollFree - ) { - errors.support_phone = "Please enter valid phone number"; + const isTollFree = form.support_phone.startsWith("1800"); + const phoneLength = form.support_phone.replace(/[^0-9]/g, "").length; + const isValidLength = isTollFree ? phoneLength >= 10 && phoneLength <= 11 : phoneLength === 10; + if (!isValidLength) { + errors.support_phone = t("please_enter_valid_phone_number"); } } if (form.support_email && !validateEmailAddress(form.support_email)) { - errors.support_email = "Please enter valid email id"; + errors.support_email = t("please_enter_valid_email"); }
Line range hint
294-356: Improve error handling specificityThe error handling in the submission function could be more specific and informative.
Consider this improvement:
- } catch (error) { - // Handle error (optional) - Notification.Error({ - msg: "An error occurred while processing the asset", - }); + } catch (error: unknown) { + const operation = form.id ? "updating" : "creating"; + console.error(`Error ${operation} asset:`, error); + Notification.Error({ + msg: t("error_processing_asset", { operation }), + });
748-769: Consolidate date validation logicThe date validation logic is duplicated between warranty and service date handling. Consider extracting this into a reusable function.
Consider creating a utility function:
const validateDate = (date: string, options: { disableFuture?: boolean, disablePast?: boolean, errorMessage: string }) => { const selectedDate = dayjs(date).format("YYYY-MM-DD"); const today = dayjs().format("YYYY-MM-DD"); if (options.disableFuture && selectedDate > today) { Notification.Error({ msg: options.errorMessage }); return false; } if (options.disablePast && selectedDate < today) { Notification.Error({ msg: options.errorMessage }); return false; } return true; };Then use it in both places:
onChange={(event) => { const { value } = event; - const selectedDate = dayjs(value); - const formattedDate = selectedDate.format("YYYY-MM-DD"); - const today = dayjs().format("YYYY-MM-DD"); - - if (selectedDate.isBefore(today)) { - Notification.Error({ - msg: "Warranty / AMC Expiry date can't be in the past", - }); - } else { + if (validateDate(value, { + disablePast: true, + errorMessage: t("warranty_date_past_error") + })) { field("warranty_amc_end_of_validity").onChange({ name: "warranty_amc_end_of_validity", - value: formattedDate, + value: dayjs(value).format("YYYY-MM-DD"), }); } }}Also applies to: 858-876
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(10 hunks)
🔇 Additional comments (1)
src/components/Facility/AssetCreate.tsx (1)
64-88: LGTM! Well-structured data management
The consolidated state management using the AssetData interface and proper initialization of form state aligns well with the PR objective of preventing unnecessary API calls.
Also applies to: 113-137, 139-140
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/AssetCreate.tsx (3)
225-263: Refactor validation logic for better maintainabilityConsider these improvements to the validation logic:
- Move phone validation to a separate validator function
- Use the existing
RequiredFieldValidatorfor support phone- Reuse
validateEmailAddressas a validator functionconst AssetFormValidator = (form: AssetData): FormErrors<AssetData> => { const errors: Partial<Record<keyof AssetData, FieldError>> = {}; errors.name = RequiredFieldValidator()(form.name); if (form.is_working === undefined) { errors.is_working = t("field_required"); } if (!form.location || form.location === "0" || form.location === "") { errors.location = t("select_local_body"); } - if (!form.support_phone) { - errors.support_phone = t("field_required"); - } else { - const checkTollFree = form.support_phone.startsWith("1800"); - const supportPhoneSimple = form.support_phone - .replace(/[^0-9]/g, "") - .slice(2); - if (supportPhoneSimple.length !== 10 && !checkTollFree) { - errors.support_phone = "Please enter valid phone number"; - } else if ( - (form.support_phone.length < 10 || form.support_phone.length > 11) && - checkTollFree - ) { - errors.support_phone = "Please enter valid phone number"; - } - } + errors.support_phone = RequiredFieldValidator()(form.support_phone); + if (form.support_phone && !validatePhoneNumber(form.support_phone)) { + errors.support_phone = t("invalid_phone_number"); + } - if (form.support_email && !validateEmailAddress(form.support_email)) { - errors.support_email = "Please enter valid email id"; + if (form.support_email) { + errors.support_email = validateEmailAddress(form.support_email) + ? undefined + : t("invalid_email"); } if (form.notes && !form.serviced_on) { errors.serviced_on = "Last serviced on date is required with notes"; } return errors; };
Line range hint
294-354: Improve error handling in form submissionThe error handling in the submission function could be more specific:
- The catch block should type the error properly
- The error message could be more descriptive based on the operation (create/update)
} catch (error) { - // Handle error (optional) + const operation = form.id ? "updating" : "creating"; + console.error(`Error ${operation} asset:`, error); Notification.Error({ - msg: "An error occurred while processing the asset", + msg: `An error occurred while ${operation} the asset. Please try again.`, }); }
745-769: Refactor warranty date validation for consistencyThe warranty date validation could be improved:
- Move validation logic to a common function
- Use consistent date formatting
+const validateFutureDate = (date: string, fieldName: string) => { + const selectedDate = dayjs(date).format("YYYY-MM-DD"); + const today = dayjs().format("YYYY-MM-DD"); + + if (dayjs(selectedDate).isBefore(today)) { + Notification.Error({ + msg: `${fieldName} date can't be in the past`, + }); + return false; + } + return true; +}; <TextFormField {...field("warranty_amc_end_of_validity")} label={t("warranty_amc_expiry")} onChange={(event) => { const { value } = event; - const selectedDate = dayjs(value); - const formattedDate = selectedDate.format("YYYY-MM-DD"); - const today = dayjs().format("YYYY-MM-DD"); - - if (selectedDate.isBefore(today)) { - Notification.Error({ - msg: "Warranty / AMC Expiry date can't be in the past", - }); - } else { + if (validateFutureDate(value, "Warranty / AMC Expiry")) { field("warranty_amc_end_of_validity").onChange({ name: "warranty_amc_end_of_validity", - value: formattedDate, + value: dayjs(value).format("YYYY-MM-DD"), }); } }} type="date" min={dayjs().format("YYYY-MM-DD")} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(10 hunks)
🔇 Additional comments (1)
src/components/Facility/AssetCreate.tsx (1)
113-137: LGTM! Form initialization implements the PR objectives
The implementation correctly initializes form state and updates it with fetched values, which enables comparison with current values to prevent unnecessary API calls.
Also applies to: 197-221
| interface AssetData { | ||
| id?: string; | ||
| name?: string; | ||
| location?: string; | ||
| description?: string; | ||
| is_working?: boolean | null; | ||
| not_working_reason?: string; | ||
| created_date?: string; | ||
| modified_date?: string; | ||
| serial_number?: string; | ||
| asset_type?: AssetType; | ||
| asset_class?: AssetClass; | ||
| status?: "ACTIVE" | "TRANSFER_IN_PROGRESS"; | ||
| vendor_name?: string; | ||
| support_name?: string; | ||
| support_email?: string; | ||
| support_phone?: string; | ||
| qr_code_id?: string; | ||
| manufacturer?: string; | ||
| warranty_amc_end_of_validity?: string; | ||
| serviced_on?: string; | ||
| latest_status?: string; | ||
| last_service?: any; | ||
| notes: string; | ||
| } |
There was a problem hiding this comment.
Fix required field annotation in interface
The notes field in the AssetData interface is marked as required (no ? optional modifier) but it's actually optional in the form.
- notes: string;
+ notes?: string;📝 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.
| interface AssetData { | |
| id?: string; | |
| name?: string; | |
| location?: string; | |
| description?: string; | |
| is_working?: boolean | null; | |
| not_working_reason?: string; | |
| created_date?: string; | |
| modified_date?: string; | |
| serial_number?: string; | |
| asset_type?: AssetType; | |
| asset_class?: AssetClass; | |
| status?: "ACTIVE" | "TRANSFER_IN_PROGRESS"; | |
| vendor_name?: string; | |
| support_name?: string; | |
| support_email?: string; | |
| support_phone?: string; | |
| qr_code_id?: string; | |
| manufacturer?: string; | |
| warranty_amc_end_of_validity?: string; | |
| serviced_on?: string; | |
| latest_status?: string; | |
| last_service?: any; | |
| notes: string; | |
| } | |
| interface AssetData { | |
| id?: string; | |
| name?: string; | |
| location?: string; | |
| description?: string; | |
| is_working?: boolean | null; | |
| not_working_reason?: string; | |
| created_date?: string; | |
| modified_date?: string; | |
| serial_number?: string; | |
| asset_type?: AssetType; | |
| asset_class?: AssetClass; | |
| status?: "ACTIVE" | "TRANSFER_IN_PROGRESS"; | |
| vendor_name?: string; | |
| support_name?: string; | |
| support_email?: string; | |
| support_phone?: string; | |
| qr_code_id?: string; | |
| manufacturer?: string; | |
| warranty_amc_end_of_validity?: string; | |
| serviced_on?: string; | |
| latest_status?: string; | |
| last_service?: any; | |
| notes?: string; | |
| } |
|
Depends on #8956. Merge only after that PR is merged. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/AssetCreate.tsx (1)
350-354: Improve error handling specificityThe error handling could be more specific to help users understand what went wrong.
- } catch (error) { - // Handle error (optional) - Notification.Error({ - msg: "An error occurred while processing the asset", - }); + } catch (error) { + const operation = form.id ? "updating" : "creating"; + console.error(`Error ${operation} asset:`, error); + Notification.Error({ + msg: `An error occurred while ${operation} the asset. Please try again.`, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/AssetCreate.tsx(9 hunks)
🔇 Additional comments (2)
src/components/Facility/AssetCreate.tsx (2)
518-539: LGTM! Form implementation meets requirements
The Form component implementation successfully addresses the PR objectives:
- Initializes form state with fetched values
- Properly handles the "Create & Add More" functionality
- Implements validation and error display
64-88:
Fix incorrect type definition for notes field
The notes field in the AssetData interface is marked as required but it's actually optional in the implementation. This is evident from:
- The validation logic only requiring notes when
serviced_onis present - The form treating it as an optional field
Apply this fix:
- notes: string;
+ notes?: string;Likely invalid or redundant comment.
| if (form.notes && !form.serviced_on) { | ||
| errors.serviced_on = "Last serviced on date is required with notes"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance service details validation
The current validation only requires serviced_on when notes are present. Consider validating both fields together to maintain data consistency.
- if (form.notes && !form.serviced_on) {
- errors.serviced_on = "Last serviced on date is required with notes";
+ if ((form.notes && !form.serviced_on) || (form.serviced_on && !form.notes)) {
+ const field = form.notes ? "serviced_on" : "notes";
+ errors[field] = "Both service date and notes are required together";📝 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.
| if (form.notes && !form.serviced_on) { | |
| errors.serviced_on = "Last serviced on date is required with notes"; | |
| } | |
| if ((form.notes && !form.serviced_on) || (form.serviced_on && !form.notes)) { | |
| const field = form.notes ? "serviced_on" : "notes"; | |
| errors[field] = "Both service date and notes are required together"; | |
| } |
@Jacobjeevan sir, can you please review the final changes? |
| .replace(/[^0-9]/g, "") | ||
| .slice(2); | ||
| if (supportPhoneSimple.length !== 10 && !checkTollFree) { | ||
| errors.support_phone = "Please enter valid phone number"; |
There was a problem hiding this comment.
Add translations (here and elsewhere in this file). @rajku-dev
Check en.json if there's already a value, if not add 👍
Nothing else apart from translations for now 👍 Also, don't need to refer to anyone here as sir/madam. Just refer to us by our names. |
| interface AssetData { | ||
| id?: string; | ||
| name?: string; | ||
| location?: string; | ||
| description?: string; | ||
| is_working?: boolean | null; | ||
| not_working_reason?: string; | ||
| created_date?: string; | ||
| modified_date?: string; | ||
| serial_number?: string; | ||
| asset_type?: AssetType; | ||
| asset_class?: AssetClass; | ||
| status?: "ACTIVE" | "TRANSFER_IN_PROGRESS"; | ||
| vendor_name?: string; | ||
| support_name?: string; | ||
| support_email?: string; | ||
| support_phone?: string; | ||
| qr_code_id?: string; | ||
| manufacturer?: string; | ||
| warranty_amc_end_of_validity?: string; | ||
| serviced_on?: string; | ||
| latest_status?: string; | ||
| last_service?: any; | ||
| notes: string; | ||
| } |
There was a problem hiding this comment.
This type is already defined in our code base. Let's not redefine it. Import the existing type instead.
There was a problem hiding this comment.
@rithviknishad that type contains last_service object which has notes and on_service date encapsulated , but we have individual fields for those in form that uses Provider
| onSubmit={(e) => handleSubmit(e, false)} | ||
| <Form<AssetData> | ||
| disabled={isLoading} | ||
| defaults={initAssetData} |
There was a problem hiding this comment.
Switch to using asyncGetDefaults instead. This allows asynchronously fetching initial data and simplifies lot of the state management.
There was a problem hiding this comment.
current code uses
const assetQuery = useQuery(routes.getAsset, {
pathParams: { external_id: assetId! },
prefetch: !!assetId,
which is not async?
There was a problem hiding this comment.
you can disable prefetch and call refetch in asyncGetDefaults
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/AssetCreate.tsx (2)
760-764: Use translations for date validation messages.The date validation error messages are hardcoded. Consider using translations for consistency.
Apply these changes:
- msg: "Warranty / AMC Expiry date can't be in the past", + msg: t("warranty_expiry_past_date_error"), - msg: "Last Serviced date can't be in the future", + msg: t("service_date_future_error"),Also applies to: 871-875
897-899: Improve placeholder text for notes field.The placeholder text could be more descriptive and consistent with other fields.
Consider this change:
- "Eg. Details on functionality, service, etc.", + "eg_service_details_placeholder"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/components/Facility/AssetCreate.tsx(10 hunks)
🔇 Additional comments (3)
src/components/Facility/AssetCreate.tsx (2)
67-91:
Fix required field annotation in interface.
The notes field in the AssetData interface is marked as required (no ? optional modifier) but it's actually optional in the form and initialization.
Apply this diff to fix the type:
- notes: string;
+ notes?: string;Also, consider using a union type for the status field:
- status?: "ACTIVE" | "TRANSFER_IN_PROGRESS";
+ type AssetStatus = "ACTIVE" | "TRANSFER_IN_PROGRESS";
+ status?: AssetStatus;Likely invalid or redundant comment.
294-356: 🛠️ Refactor suggestion
Enhance error handling and type safety.
- The error handling could be more specific and informative.
- The
dataobject usesanytype which reduces type safety.
Consider these improvements:
- const data: any = {
+ type AssetSubmitData = Omit<AssetData, 'serviced_on' | 'notes'> & {
+ last_serviced_on?: string;
+ note?: string;
+ };
+ const data: AssetSubmitData = {
- } catch (error) {
- // Handle error (optional)
- Notification.Error({
- msg: "An error occurred while processing the asset",
- });
+ } catch (error) {
+ const operation = form.id ? "updating" : "creating";
+ console.error(`Error ${operation} asset:`, error);
+ Notification.Error({
+ msg: `An error occurred while ${operation} the asset. Please try again.`,
+ });Likely invalid or redundant comment.
public/locale/en.json (1)
814-814: LGTM! The new translation entry follows the established conventions.
The added key-value pair maintains consistency with:
- Naming convention (snake_case)
- Message clarity and formatting
- Alphabetical ordering
| const AssetFormValidator = (form: AssetData): FormErrors<AssetData> => { | ||
| const errors: Partial<Record<keyof AssetData, FieldError>> = {}; // Initialize error object | ||
|
|
||
| errors.name = RequiredFieldValidator()(form.name); | ||
|
|
||
| if (form.is_working === undefined) { | ||
| errors.is_working = t("field_required"); | ||
| } | ||
|
|
||
| if (!form.location || form.location === "0" || form.location === "") { | ||
| errors.location = t("select_local_body"); | ||
| } | ||
|
|
||
| if (!form.support_phone) { | ||
| errors.support_phone = t("field_required"); | ||
| } else { | ||
| const validatePhoneNumber = PhoneNumberValidator( | ||
| ["mobile", "landline", "support"], | ||
| t("invalid_phone_number"), | ||
| ); | ||
| const isValid = validatePhoneNumber(form.support_phone); | ||
| // console.log("Is Valid" ,isValid) | ||
| if (isValid == "Invalid Phone Number") { | ||
| errors.support_phone = t("invalid_phone_number"); | ||
| } | ||
| return false; | ||
| } | ||
| dispatch({ type: "set_error", errors }); | ||
| return true; | ||
|
|
||
| if (form.support_email && !validateEmailAddress(form.support_email)) { | ||
| errors.support_email = t("invalid_email"); | ||
| } | ||
|
|
||
| if (form.notes && !form.serviced_on) { | ||
| errors.serviced_on = t("last_serviced_on_required"); | ||
| } | ||
|
|
||
| return errors; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve validation logic.
- The service details validation is one-sided. Consider validating both fields together to maintain data consistency.
- The phone number validation uses string comparison which is not type-safe.
Apply these improvements:
- if (form.notes && !form.serviced_on) {
- errors.serviced_on = t("last_serviced_on_required");
+ // Validate that both service date and notes are provided together
+ if ((form.notes && !form.serviced_on) || (!form.notes && form.serviced_on)) {
+ const field = form.notes ? "serviced_on" : "notes";
+ errors[field] = t("service_details_required_together");
}
- if (isValid == "Invalid Phone Number") {
+ if (isValid === t("invalid_phone_number")) {
errors.support_phone = t("invalid_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.
| const AssetFormValidator = (form: AssetData): FormErrors<AssetData> => { | |
| const errors: Partial<Record<keyof AssetData, FieldError>> = {}; // Initialize error object | |
| errors.name = RequiredFieldValidator()(form.name); | |
| if (form.is_working === undefined) { | |
| errors.is_working = t("field_required"); | |
| } | |
| if (!form.location || form.location === "0" || form.location === "") { | |
| errors.location = t("select_local_body"); | |
| } | |
| if (!form.support_phone) { | |
| errors.support_phone = t("field_required"); | |
| } else { | |
| const validatePhoneNumber = PhoneNumberValidator( | |
| ["mobile", "landline", "support"], | |
| t("invalid_phone_number"), | |
| ); | |
| const isValid = validatePhoneNumber(form.support_phone); | |
| // console.log("Is Valid" ,isValid) | |
| if (isValid == "Invalid Phone Number") { | |
| errors.support_phone = t("invalid_phone_number"); | |
| } | |
| return false; | |
| } | |
| dispatch({ type: "set_error", errors }); | |
| return true; | |
| if (form.support_email && !validateEmailAddress(form.support_email)) { | |
| errors.support_email = t("invalid_email"); | |
| } | |
| if (form.notes && !form.serviced_on) { | |
| errors.serviced_on = t("last_serviced_on_required"); | |
| } | |
| return errors; | |
| const AssetFormValidator = (form: AssetData): FormErrors<AssetData> => { | |
| const errors: Partial<Record<keyof AssetData, FieldError>> = {}; // Initialize error object | |
| errors.name = RequiredFieldValidator()(form.name); | |
| if (form.is_working === undefined) { | |
| errors.is_working = t("field_required"); | |
| } | |
| if (!form.location || form.location === "0" || form.location === "") { | |
| errors.location = t("select_local_body"); | |
| } | |
| if (!form.support_phone) { | |
| errors.support_phone = t("field_required"); | |
| } else { | |
| const validatePhoneNumber = PhoneNumberValidator( | |
| ["mobile", "landline", "support"], | |
| t("invalid_phone_number"), | |
| ); | |
| const isValid = validatePhoneNumber(form.support_phone); | |
| // console.log("Is Valid" ,isValid) | |
| if (isValid === t("invalid_phone_number")) { | |
| errors.support_phone = t("invalid_phone_number"); | |
| } | |
| } | |
| if (form.support_email && !validateEmailAddress(form.support_email)) { | |
| errors.support_email = t("invalid_email"); | |
| } | |
| // Validate that both service date and notes are provided together | |
| if ((form.notes && !form.serviced_on) || (!form.notes && form.serviced_on)) { | |
| const field = form.notes ? "serviced_on" : "notes"; | |
| errors[field] = t("service_details_required_together"); | |
| } | |
| return errors; |
|
👋 Hi, @rajku-dev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@rajku-dev can you share the status on this PR in the next 48 status, otherwise will close the PR and unassign you from the issue due to inactivity |
|
closing this PR due to inactivity |
Proposed Changes
issue_9198_fix.mp4
-Initialized form state using 'fetchedValues' to pre-fill the form with asset data.
-Introduced a mechanism to compare current form values with initial values and enable/disable the submit button based on changes made by the user in form.
-Used a useRef (isInitialLoad.current) to skip comparisons during the initial load and avoid unnecessary state updates.
-Once the initial values are set, this flag is updated to ensure further state checks and updates are triggered properly to enable or disable the 'Update' button accordingly.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements