Upload api update #1449
Upload api update #1449
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes encompass enhancements to the styling and functionality of the employee sandbox UI. Key updates include modifications to the Changes
Possibly related PRs
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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (3 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (4 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (1)
Pattern
**/*.js: check
📓 Learnings (2)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (2)
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:468-481 Timestamp: 2024-03-13T07:33:45.211Z Learning: The error message in the `checkForErrorInUploadedFile` function within `Upload.js` is being localized and improved for better user experience, as clarified by the user.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:0-0 Timestamp: 2024-03-13T05:11:36.400Z Learning: A more detailed message for file parsing errors in the `Upload.js` file is displayed elsewhere in the code, as clarified by the user.micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (2)
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:468-481 Timestamp: 2024-03-13T07:33:45.211Z Learning: The error message in the `checkForErrorInUploadedFile` function within `Upload.js` is being localized and improved for better user experience, as clarified by the user.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:0-0 Timestamp: 2024-03-13T05:11:36.400Z Learning: A more detailed message for file parsing errors in the `Upload.js` file is displayed elsewhere in the code, as clarified by the user.
🔇 Additional comments (7)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (2)
14-14: LGTM: New state variable for error handling.The introduction of
uploadErrorMessagestate variable is a good addition for managing upload-related error messages. This aligns well with the PR objective of implementing proper error messages for the upload API.
Line range hint
1-78: Overall implementation looks good with minor suggestions.The changes effectively address the PR objectives by implementing proper error handling and user feedback for file uploads. The use of React hooks and localized error messages is commendable.
Key points:
- New state variable
uploadErrorMessagefor managing upload errors.- Proper error handling in
handleUploadFilefunction.- Integration of error feedback with the
Uploadercomponent.The only suggestion is to rename the
iserrorprop on theUploadercomponent for better clarity. Once this is addressed, the implementation will be fully aligned with best practices and the PR objectives.micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (1)
Line range hint
1-76: Approve overall changes and alignment with PR objectives.The changes made in this file align well with the PR objectives of implementing proper error messages and enhancing user experience for the upload API. Specifically:
- The introduction of
uploadErrorMessageand its usage in theUploadercomponent directly addresses the goal of implementing proper error messages.- The improvements in error handling in the
handleUploadFilefunction enhance the user experience by providing more specific feedback.These changes contribute to resolving the issues mentioned in the Jira tickets SN-278 and SN-277. The implementation of proper error messages and enhanced user feedback in the logo upload process significantly improves the functionality of the upload API.
Great job on addressing the PR objectives! Once the minor issues mentioned in previous comments are addressed, this implementation will greatly improve the user experience of the logo upload feature.
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (4)
130-130: Improved error message specificityThe change from a generic error message to a more specific one (
CONFIG_UPLOAD_ERROR_MESSAGE) enhances the user experience by providing clearer feedback. This aligns well with the PR objective of implementing proper error messages.
135-135: Enhanced success message clarityThe modification from a general success message to a more specific one (
CONFIG_UPLOAD_SUCCESSFUL_TOAST_MESSAGE) improves the clarity of user feedback. This change is in line with the PR's goal of enhancing the user experience of the upload API.
Line range hint
1-185: Summary of changesThe visible changes in this file improve error and success message handling, aligning well with the PR objectives of implementing proper error messages and enhancing the user experience. However, the changes to the
transformDatafunction mentioned in the AI-generated summary are not visible in the provided code snippet and require verification.Overall, the changes contribute to a better user experience with more specific feedback messages. Once the
transformDatafunction changes are verified, this update should significantly improve the upload API's functionality and user interaction.
Line range hint
1-185: Verify changes in thetransformDatafunctionThe AI-generated summary mentions updates to the
transformDatafunction to correctly assign thetenantConfigproperty. However, these changes are not visible in the provided code snippet. It's crucial to verify these changes to ensure proper data transformation before the API call.Could you please provide the updated
transformDatafunction code for review? Here's a script to help locate the function:
| @@ -20,9 +21,11 @@ const ConfigUploaderComponent = ({ onSelect, ...props }) => { | |||
| const response = await Digit.UploadServices.Filestorage("Sandbox", file, tenantId); | |||
| const fileStoreId = response?.data?.files?.[0]?.fileStoreId; | |||
| setFileStoreId(fileStoreId) | |||
| setUploadErrorMessage(""); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Rename iserror prop for clarity.
The implementation of error handling using uploadErrorMessage is good. However, the prop name iserror on the Uploader component is misleading as it's being passed a string value (the error message) rather than a boolean.
Consider renaming this prop to something like errorMessage or uploadError to better reflect its purpose and type.
Example:
<Uploader
// ...other props
errorMessage={uploadErrorMessage}
// ...
/>Make sure to update the Uploader component's implementation to handle this prop name change.
Also applies to: 28-28, 62-62
| setUploadErrorMessage(""); | ||
| } catch (error) { | ||
| setToastMessage(t("LOGO_UPLOAD_FAILED")); | ||
| setUploadErrorMessage(t("LOGO_UPLOAD_FAILED")); | ||
| setIsError(true); | ||
| setShowToast(true); | ||
| setTimeout(() => { | ||
| closeToast(); | ||
| setShowToast(false); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve error handling improvements with a minor suggestion.
The changes in the handleUploadFile function improve error handling and user feedback:
- Resetting the error message on successful upload (line 23) is good practice.
- Setting a specific error message on upload failure (line 26) enhances user feedback.
- Using
setShowToast(false)instead ofcloseToast()(line 30) is correct ifcloseToast()was not defined.
Consider adding a success message for a successful upload to provide positive feedback to the user.
setToastMessage(t("LOGO_UPLOAD_SUCCESS"));
setIsError(false);
setShowToast(true);| @@ -36,6 +40,7 @@ const LogoUploaderComponent = ({ onSelect, ...props }) => { | |||
|
|
|||
|
|
|||
| const selectFile = (file) => { | |||
| console.log("file uploaded is ", file); | |||
There was a problem hiding this comment.
Remove or conditionally render console.log statement.
The added console.log statement on line 43 may be useful for debugging but should not be present in production code. Consider one of the following options:
- Remove the console.log statement entirely:
- console.log("file uploaded is ", file);- Conditionally render it based on a development environment flag:
if (process.env.NODE_ENV === 'development') {
console.log("file uploaded is ", file);
}This ensures that potentially sensitive information is not logged in production environments.
| @@ -52,6 +57,7 @@ const LogoUploaderComponent = ({ onSelect, ...props }) => { | |||
| uploadedFiles={[]} | |||
| variant="uploadFile" | |||
| onUpload={(files) => selectFile(files)} | |||
| iserror={uploadErrorMEssage} | |||
There was a problem hiding this comment.
Approve iserror prop addition and fix typo.
The addition of the iserror prop to the Uploader component is a good practice for displaying error states. However, please ensure that the typo in the prop value is corrected to match the fix suggested earlier:
- iserror={uploadErrorMEssage}
+ iserror={uploadErrorMessage}This change will ensure that the error state is correctly reflected in the Uploader component.
📝 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.
| iserror={uploadErrorMEssage} | |
| iserror={uploadErrorMessage} |
| const [uploadErrorMEssage, setUploadErrorMessage] = useState(""); | ||
|
|
There was a problem hiding this comment.
Fix typo in variable name and approve new state variable.
There's a typo in the new state variable name. Please correct it as follows:
- const [uploadErrorMEssage, setUploadErrorMessage] = useState("");
+ const [uploadErrorMessage, setUploadErrorMessage] = useState("");The addition of this state variable for managing upload error messages is a good practice for improving user feedback.
📝 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 [uploadErrorMEssage, setUploadErrorMessage] = useState(""); | |
| const [uploadErrorMessage, setUploadErrorMessage] = useState(""); |
| .digit-topbar .digit-header-img-ulb-wrapper-mobileview .digit-topbar-ulb-mobileview { | ||
| .state { | ||
| width: 80px; | ||
| height: 19px; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating duplicate styles for the .state class
The styles for the .state class within .digit-topbar .digit-header-img-ulb-wrapper-mobileview .digit-topbar-ulb-mobileview duplicate those of .digit-topbar-ulb .state. To enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring by creating a shared class or extending the existing styles.
Apply this diff to refactor the styles:
- .digit-topbar .digit-header-img-ulb-wrapper-mobileview .digit-topbar-ulb-mobileview {
- .state {
- width: 80px;
- height: 19px;
- }
- }
+ /* Reuse the existing .digit-topbar-ulb .state styles */Alternatively, if the styles need to differ for mobile view, consider using a mixin or extending the existing class with modifications specific to mobile devices.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (3 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (1)
Pattern
**/*.js: check
📓 Learnings (2)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (2)
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:468-481 Timestamp: 2024-03-13T07:33:45.211Z Learning: The error message in the `checkForErrorInUploadedFile` function within `Upload.js` is being localized and improved for better user experience, as clarified by the user.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:0-0 Timestamp: 2024-03-13T05:11:36.400Z Learning: A more detailed message for file parsing errors in the `Upload.js` file is displayed elsewhere in the code, as clarified by the user.micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (2)
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:468-481 Timestamp: 2024-03-13T07:33:45.211Z Learning: The error message in the `checkForErrorInUploadedFile` function within `Upload.js` is being localized and improved for better user experience, as clarified by the user.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:0-0 Timestamp: 2024-03-13T05:11:36.400Z Learning: A more detailed message for file parsing errors in the `Upload.js` file is displayed elsewhere in the code, as clarified by the user.
🔇 Additional comments (3)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (3)
11-12: LGTM: Improved file state management.The addition of the
filestate variable enhances the component's ability to manage the selected file before upload. This change aligns well with the PR objectives of improving the upload API functionality.
Line range hint
1-75: Summary: Improved error handling and user feedback with minor corrections needed.Overall, the changes in this file significantly improve error handling and user feedback for the logo upload process, aligning well with the PR objectives. Key improvements include:
- Better file state management
- Introduction of specific error messaging
- Enhanced error state handling in the UI
To finalize these improvements:
- Correct the typo in the
uploadErrorMessagevariable name (lines 14 and 59).- Consider adding a success message for successful uploads, as suggested earlier.
These changes will result in a more robust and user-friendly logo upload component.
59-59:⚠️ Potential issueApprove
iserrorprop addition and fix typo.The addition of the
iserrorprop to theUploadercomponent is a good practice for displaying error states. However, please ensure that the typo in the prop value is corrected to match the fix suggested earlier:- iserror={uploadErrorMEssage} + iserror={uploadErrorMessage}This change will ensure that the error state is correctly reflected in the
Uploadercomponent.Likely invalid or redundant comment.
| const tenantId = Digit.ULBService.getCurrentTenantId(); | ||
| const [uploadErrorMEssage, setUploadErrorMessage] = useState(""); |
There was a problem hiding this comment.
Approve new error state with a minor correction.
The addition of the uploadErrorMEssage state variable is a good practice for managing and displaying upload errors, aligning with the PR objectives. However, there's a typo in the variable name.
Please correct the typo:
- const [uploadErrorMEssage, setUploadErrorMessage] = useState("");
+ const [uploadErrorMessage, setUploadErrorMessage] = useState("");📝 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 [uploadErrorMEssage, setUploadErrorMessage] = useState(""); | |
| const [uploadErrorMessage, setUploadErrorMessage] = useState(""); |
| @@ -11,6 +11,7 @@ const ConfigUploaderComponent = ({ onSelect, ...props }) => { | |||
| const [file, setFile] = useState(null); | |||
| const [fileStoreId, setFileStoreId] = useState(null); | |||
| const tenantId = Digit.ULBService.getCurrentTenantId(); | |||
| const [uploadErrorMEssage, setUploadErrorMessage] = useState(""); | |||
There was a problem hiding this comment.
Fix the typo in 'uploadErrorMessage' variable name.
The state variable uploadErrorMEssage has a typo in its name ('MEssage' should be 'Message'). This typo propagates to all instances where the variable is used. Correcting the spelling ensures consistency and prevents potential bugs.
Apply this diff to correct the typo:
- const [uploadErrorMEssage, setUploadErrorMessage] = useState("");
+ const [uploadErrorMessage, setUploadErrorMessage] = useState("");
...
- setUploadErrorMEssage("");
+ setUploadErrorMessage("");
...
- setUploadErrorMEssage(t("BANNER_UPLOAD_FAILED"));
+ setUploadErrorMessage(t("BANNER_UPLOAD_FAILED"));
...
- iserror={uploadErrorMEssage}
+ iserror={uploadErrorMessage}Also applies to: 24-24, 28-28, 62-62
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (1)
Line range hint
117-124: RefactortransformDatafunction for improved clarity and immutability.While the function correctly transforms the data, it directly modifies the input parameter, which can lead to unexpected side effects. Consider using a more immutable approach and improving the property access style.
Here's a suggested refactor:
const transformData = (inputData, requestData) => { const updatedDocuments = requestData[0].documents.map(doc => { const matchingInput = inputData.find(input => input.type === doc.type); return matchingInput ? { ...doc, fileStoreId: matchingInput.fileStoreId } : doc; }); return { ...requestData, tenantConfig: { ...requestData[0], documents: updatedDocuments } }; };This approach creates a new object instead of modifying the existing one, and uses more conventional object property access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (1)
Pattern
**/*.js: check
🔇 Additional comments (2)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (2)
143-143: Approve improved error handling.The updated error message now uses a translation key, which is excellent for internationalization. The fallback to a generic error message when
error.messageis undefined is a good practice for handling various error scenarios.
148-148: Approve updated success message.The success message now uses a translation key, which is excellent for internationalization. This change maintains consistency with the error handling approach and improves the overall user experience by providing localized feedback.
| const isBannerUndefined = documents.find(doc => doc.type === "bannerUrl")?.fileStoreId === undefined; | ||
| const isLogoUndefined = documents.find(doc => doc.type === "logoUrl")?.fileStoreId === undefined; | ||
|
|
||
| if (isBannerUndefined && isLogoUndefined) { | ||
| setToastMessage(t("BOTH_FILESTOREIDS_ARE_UNDEFINED")); | ||
| setIsError(true); | ||
| setShowToast(true); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve validation logic with suggestion for improvement.
The added validation logic improves user experience by preventing submission when both files are missing. However, consider if allowing submission when only one file is present is the desired behavior.
If both files should be required, consider modifying the condition:
if (isBannerUndefined || isLogoUndefined) {
setToastMessage(t("BOTH_FILES_REQUIRED"));
setIsError(true);
setShowToast(true);
return;
}| const [canSubmit, setCanSubmit] = useState(false); | ||
|
|
There was a problem hiding this comment.
Remove unused state variable canSubmit.
The canSubmit state variable is declared but not used anywhere in the component. To maintain clean and efficient code, it's recommended to remove unused variables.
Apply this diff to remove the unused state variable:
- const [canSubmit, setCanSubmit] = useState(false);📝 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 [canSubmit, setCanSubmit] = useState(false); |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (1)
Pattern
**/*.js: check
🔇 Additional comments (1)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (1)
20-21: Unused state variablecanSubmitremains unused.The state variable
canSubmitis declared but not used anywhere in the component. This was previously pointed out in past review comments and is still applicable.
| setToastMessage(error.message || t("CONFIG_UPLOAD_ERROR_MESSAGE")); | ||
| setIsError(true); | ||
| setShowToast(true); |
There was a problem hiding this comment.
Avoid displaying raw error messages to users.
Using error.message directly in the toast may expose internal error details or sensitive information to the user. It's safer to log the error internally and display a generic error message.
Apply this diff to display a user-friendly error message:
- setToastMessage(error.message || t("CONFIG_UPLOAD_ERROR_MESSAGE"));
+ setToastMessage(t("CONFIG_UPLOAD_ERROR_MESSAGE"));Consider logging the error details for debugging purposes.
📝 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.
| setToastMessage(error.message || t("CONFIG_UPLOAD_ERROR_MESSAGE")); | |
| setIsError(true); | |
| setShowToast(true); | |
| setToastMessage(t("CONFIG_UPLOAD_ERROR_MESSAGE")); | |
| setIsError(true); | |
| setShowToast(true); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
micro-ui/web/public/index.html (1)
Line range hint
1-37: Summary: Minor CSS update, main PR objectives not addressed in this fileThis file only contains a minor update to the CSS version. While this change is appropriate, it doesn't directly address the main objectives of the PR (adding file upload checks, implementing error messages, and updating the success toast message).
To fully review the implementation of the PR objectives:
- Please provide the other changed files in the PR, particularly those related to the upload API functionality.
- If no other files were changed, please clarify how the objectives were achieved through this CSS update alone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
micro-ui/web/micro-ui-internals/packages/css/package.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
- micro-ui/web/public/index.html (1 hunks)
🔇 Additional comments (1)
micro-ui/web/public/index.html (1)
10-10: Approved: CSS version updateThe update to the latest version (1.8.2-beta.34) of the @egovernments/digit-ui-css stylesheet is appropriate. This minor version update likely includes bug fixes or small improvements that can enhance the overall user experience.
While this change doesn't directly address the specific issues mentioned in the PR objectives (file upload checks, error messages, success toast), it might be a prerequisite or complementary change. To ensure this update aligns with the PR objectives, please verify:
- If this CSS update is necessary for the implementation of the file upload checks and error messages.
- If there are any visual changes in the new CSS version that affect the upload API UI.
You can use the following script to check for any related changes in the CSS file:
✅ Verification successful
Verified: CSS update aligns with PR objectives
The updated version (1.8.2-beta.34) of the @egovernments/digit-ui-css stylesheet includes styles related to file uploads, error messages, and toast notifications. This confirms that the CSS update supports the implementation of the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for changes related to file upload, error messages, or toast notifications in the CSS file # Test: Search for relevant CSS classes or IDs rg --type css -i '(upload|error|toast|notification)'Length of output: 17223
| const tenantId = Digit.ULBService.getCurrentTenantId(); | ||
| const [uploadErrorMEssage, setUploadErrorMessage] = useState(""); |
| @@ -384,6 +384,12 @@ | |||
| } | |||
| } | |||
|
|
|||
| .digit-topbar .digit-header-img-ulb-wrapper-mobileview .digit-topbar-ulb-mobileview { | |||
| .state { | |||
| width: 80px; | |||
…IGIT-Frontend into upload-api-update
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (1)
Line range hint
317-362: Consolidate repeated.homeWrapperdefinitions for maintainabilityThe
.homeWrapperclass is defined multiple times with different styles throughout the file. This can make the code harder to maintain and may introduce inconsistencies. Consider merging these definitions into a single.homeWrapperclass and nest the specific styles as needed.Apply this diff to consolidate the styles:
/* Combine all .homeWrapper styles into a single definition */ .homeWrapper { display: flex; justify-content: space-between; /* Ensure spacing between the two components */ align-items: flex-start; > :first-child { flex-grow: 1; /* Makes the first child take up remaining space */ margin-right: 1rem; /* Optional spacing between the components */ } > :last-child { width: 20.688rem; /* Fixed width for QuickSetupConfigComponent */ flex-shrink: 0; /* Prevents it from shrinking */ margin-left: auto; /* Aligns the component to the right end */ } .digit-employee-card, .digit-card-component { .digit-card-header { color: #0b4b66 !important; font-size: 1.87rem !important; text-align: center; font-weight: 700; } .digit-card-text { font-size: 1rem !important; font-weight: 700; } } }This refactoring combines all the
.homeWrapperstyles into one block, enhancing readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (3 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (2 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (1)
Pattern
**/*.js: check
📓 Learnings (2)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (2)
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:468-481 Timestamp: 2024-03-13T07:33:45.211Z Learning: The error message in the `checkForErrorInUploadedFile` function within `Upload.js` is being localized and improved for better user experience, as clarified by the user.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:0-0 Timestamp: 2024-03-13T05:11:36.400Z Learning: A more detailed message for file parsing errors in the `Upload.js` file is displayed elsewhere in the code, as clarified by the user.micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (2)
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:468-481 Timestamp: 2024-03-13T07:33:45.211Z Learning: The error message in the `checkForErrorInUploadedFile` function within `Upload.js` is being localized and improved for better user experience, as clarified by the user.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:0-0 Timestamp: 2024-03-13T05:11:36.400Z Learning: A more detailed message for file parsing errors in the `Upload.js` file is displayed elsewhere in the code, as clarified by the user.
🔇 Additional comments (10)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js (2)
14-14: LGTM: New state variable for error handling.The addition of
uploadErrorMessagestate variable is a good improvement for managing upload-related error messages. This aligns well with the PR objective of implementing proper error messages.
24-24: LGTM: Improved error handling in file upload.The changes in the
handleUploadFilefunction enhance error handling:
- Clearing the error message on successful upload (line 24).
- Setting a localized error message on upload failure (line 28).
These improvements align with the PR objective of implementing proper error messages and enhancing the user experience.
Also applies to: 28-28
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js (4)
11-12: LGTM: New state variables for file management.The addition of
fileandfileStoreIdstate variables is a good practice for managing the file upload process. The naming is clear and follows React conventions.
14-14: LGTM: New state variable for upload error messages.The addition of
uploadErrorMessagestate variable is a good practice for managing and displaying upload errors. This aligns well with the PR objectives to implement proper error messages. The naming is clear and correct, addressing the typo mentioned in a past review comment.
Line range hint
43-47: LGTM: Simplified file selection logic.The
selectFilefunction has been simplified, improving code readability. The removal of the console.log statement addresses a previous review comment about potential information leakage in production environments.
59-59: LGTM: Error state properly added to Uploader component.The addition of the
iserrorprop to theUploadercomponent is a good practice for displaying error states. The prop name uses the correct camelCase convention, and its value is correctly set touploadErrorMessage. This change aligns well with the PR objectives to implement proper error messages.micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js (3)
20-20: LGTM: New state variable for managing uploaded data.The addition of the
uploadDatastate variable is a good approach for managing the uploaded documents.
150-150: LGTM: Improved success message localization.The use of a translated success message (
t("CONFIG_UPLOAD_SUCCESSFUL_TOAST_MESSAGE")) enhances the user experience by providing localized feedback. This change aligns well with the PR objectives.
Line range hint
1-200: Overall approval with acknowledgment of addressed issues.The changes in this file successfully implement the PR objectives, including adding checks for file uploads and updating error and success messages. The unused
canSubmitstate variable mentioned in a past review has been removed, which improves code cleanliness.The overall structure and logic of the component are sound, with the suggested refactorings in previous comments being the main areas for potential improvement.
Great job on addressing the issues and enhancing the functionality of the tenant configuration upload process!
micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (1)
382-384: Also applies to: 387-392
| @@ -56,6 +59,7 @@ const ConfigUploaderComponent = ({ onSelect, ...props }) => { | |||
| uploadedFiles={[]} | |||
| variant="uploadFile" | |||
| onUpload={(files) => selectFile(files)} | |||
| iserror={uploadErrorMessage} | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Rename iserror prop for clarity.
While the implementation is correct, the prop name iserror is misleading as it's being passed a string value (the error message) rather than a boolean.
Consider renaming this prop to something like errorMessage or uploadError to better reflect its purpose and type. For example:
<Uploader
// ...other props
errorMessage={uploadErrorMessage}
// ...
/>Make sure to update the Uploader component's implementation to handle this prop name change.
| @@ -11,6 +11,7 @@ const ConfigUploaderComponent = ({ onSelect, ...props }) => { | |||
| const [file, setFile] = useState(null); | |||
| const [fileStoreId, setFileStoreId] = useState(null); | |||
| const tenantId = Digit.ULBService.getCurrentTenantId(); | |||
| const [uploadErrorMessage, setUploadErrorMessage] = useState(""); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Improved error handling implementation.
The overall implementation of error handling for file uploads is well done:
- Introduction of
uploadErrorMessagestate (line 14). - Proper error message management in
handleUploadFile(lines 24 and 28). - Integration with the
Uploadercomponent (line 62).
These changes effectively address the PR objectives of implementing proper error messages and enhancing the user experience. The use of localized error messages is a good practice.
For further improvement, consider addressing the iserror prop naming issue mentioned in the previous comment.
Also applies to: 24-24, 28-28, 62-62
| setUploadErrorMessage(""); | ||
| } catch (error) { | ||
| setToastMessage(t("LOGO_UPLOAD_FAILED")); | ||
| setIsError(true); | ||
| setUploadErrorMessage(t("LOGO_UPLOAD_FAILED")); | ||
| setShowToast(true); | ||
| setTimeout(() => { | ||
| closeToast(); | ||
| setShowToast(false); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve error handling improvements with a suggestion.
The changes in the handleUploadFile function significantly improve error handling and user feedback:
- Resetting the error message on successful upload (line 23) is good practice.
- Setting a specific error message on upload failure (line 27) enhances user feedback.
- Using
setShowToast(false)instead ofcloseToast()(line 30) is correct.
These improvements align well with the PR objectives. To further enhance user experience, consider adding a success message for successful uploads:
if (fileStoreId) {
setToastMessage(t("LOGO_UPLOAD_SUCCESS"));
setIsError(false);
setShowToast(true);
}This addition would provide positive feedback to users upon successful upload.
|
|
||
| const isBannerUndefined = documents.find(doc => doc.type === "bannerUrl")?.fileStoreId === undefined; | ||
| const isLogoUndefined = documents.find(doc => doc.type === "logoUrl")?.fileStoreId === undefined; | ||
|
|
||
| if (isBannerUndefined && isLogoUndefined) { | ||
| setToastMessage(t("BOTH_FILESTOREIDS_ARE_UNDEFINED")); | ||
| setIsError(true); | ||
| setShowToast(true); | ||
| setTimeout(() => { | ||
| setShowToast(false); | ||
| }, 2000); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve validation logic with suggestion for improvement.
The added validation logic improves user experience by preventing submission when both files are missing. However, consider if allowing submission when only one file is present is the desired behavior.
If both files should be required, consider modifying the condition:
if (isBannerUndefined || isLogoUndefined) {
setToastMessage(t("BOTH_FILES_REQUIRED"));
setIsError(true);
setShowToast(true);
return;
}| setTimeout(() => { | ||
| setShowToast(false); | ||
| }, 2000); | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor toast dismissal logic for consistency.
Multiple instances use setTimeout with different durations to hide the toast. Refactoring this logic into a single function or constant ensures consistency and maintainability.
Apply this diff to use the existing closeToast function:
- setTimeout(() => {
- setShowToast(false);
- }, 2000);
+ closeToast();If different durations are needed, modify closeToast to accept a duration parameter.
📝 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.
| setTimeout(() => { | |
| setShowToast(false); | |
| }, 2000); | |
| return; | |
| closeToast(); | |
| return; |
| setTimeout(() => { | ||
| setShowToast(false); | ||
| }, 2000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor toast dismissal logic for consistency.
As mentioned earlier, using setTimeout directly is inconsistent with other parts of the code. For better maintainability and consistency, use the closeToast function here as well.
Apply this diff:
- setTimeout(() => {
- setShowToast(false);
- }, 2000);
+ closeToast();Remember to modify the closeToast function if different durations are needed for different scenarios.
📝 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.
| setTimeout(() => { | |
| setShowToast(false); | |
| }, 2000); | |
| closeToast(); |
| @@ -127,12 +142,12 @@ const TenantConfigUpload = () => { | |||
| }, | |||
| { | |||
| onError: (error) => { | |||
| setToastMessage(error.message || t("ERROR_MESSAGE")); | |||
| setToastMessage(error.message || t("CONFIG_UPLOAD_ERROR_MESSAGE")); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve error message handling, but consider further enhancement.
The use of a fallback to a translated error message is an improvement. However, consider avoiding the display of raw error messages to users as they may contain sensitive information.
Apply this diff for a more user-friendly approach:
- setToastMessage(error.message || t("CONFIG_UPLOAD_ERROR_MESSAGE"));
+ setToastMessage(t("CONFIG_UPLOAD_ERROR_MESSAGE"));
+ console.error("API Error:", error.message); // Log the error for debuggingThis ensures a consistent user-friendly message while still logging the detailed error for debugging purposes.
📝 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.
| setToastMessage(error.message || t("CONFIG_UPLOAD_ERROR_MESSAGE")); | |
| setToastMessage(t("CONFIG_UPLOAD_ERROR_MESSAGE")); | |
| console.error("API Error:", error.message); // Log the error for debugging |
https://digit-discuss.atlassian.net/jira/software/c/projects/SN/issues/SN-278
https://digit-discuss.atlassian.net/jira/software/c/projects/SN/issues/SN-277
added checks when no file is uploaded. cannot submit until atleast one file is uploaded.
resolved these issues with proper error messages and success toast message updated.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation