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

Upload api update #1449

Merged
merged 11 commits into from
Oct 3, 2024
Merged

Upload api update #1449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a32e539
upload error message bug
mithunhegde-egov Sep 30, 2024
9c91a66
update error message
mithunhegde-egov Oct 1, 2024
b83d8e5
logo mobile css and upload success message
mithunhegde-egov Oct 1, 2024
4d26203
upload labels changed
mithunhegde-egov Oct 1, 2024
644fd02
error message when no file is added
mithunhegde-egov Oct 1, 2024
951db0f
toast message time out and error checks
mithunhegde-egov Oct 1, 2024
b8c6264
css version upgrade
mithunhegde-egov Oct 1, 2024
c583c48
Merge branch 'develop' into upload-api-update
nabeelmd-eGov Oct 1, 2024
21d59c6
review changes
mithunhegde-egov Oct 3, 2024
9844f55
Merge branch 'upload-api-update' of https://github.com/egovernments/D…
mithunhegde-egov Oct 3, 2024
8a0f745
remov unused state variables
mithunhegde-egov Oct 3, 2024
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
2 changes: 1 addition & 1 deletion micro-ui/web/micro-ui-internals/packages/css/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@egovernments/digit-ui-css",
"version": "1.8.2-beta.33",
"version": "1.8.2-beta.34",
"license": "MIT",
"main": "dist/index.css",
"author": "Jagankumar <jagan.kumar@egov.org.in>",
Expand Down
8 changes: 7 additions & 1 deletion micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,11 +379,17 @@

.digit-topbar-ulb {
.state {
width: 80px;
width: 5rem;
height: 19px;
}
}

.digit-topbar .digit-header-img-ulb-wrapper-mobileview .digit-topbar-ulb-mobileview {
.state {
width: 5rem;
height: 19px;
}
}
.digit-popup-footer.masterHandlerPopUpFooter {
.digit-popup-footer-buttons {
margin-left: unset;
Expand Down
4 changes: 4 additions & 0 deletions ...ternals/packages/modules/sandbox/src/pages/employee/tenantMgmt/ConfigUploaderComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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("");
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Improved error handling implementation.

The overall implementation of error handling for file uploads is well done:

  1. Introduction of uploadErrorMessage state (line 14).
  2. Proper error message management in handleUploadFile (lines 24 and 28).
  3. Integration with the Uploader component (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

const { t } = useTranslation();


Expand All @@ -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("");
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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

} catch (error) {
setToastMessage(t("BANNER_UPLOAD_FAILED"));
setIsError(true);
setUploadErrorMessage(t("BANNER_UPLOAD_FAILED"));
setShowToast(true);
setTimeout(() => {
setShowToast(false);
Expand Down Expand Up @@ -56,6 +59,7 @@ const ConfigUploaderComponent = ({ onSelect, ...props }) => {
uploadedFiles={[]}
variant="uploadFile"
onUpload={(files) => selectFile(files)}
iserror={uploadErrorMessage}
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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.

accept="image/*, .jpg, .png, .jpeg"
// if (files && files.length > 0) {
// handleUploadFile(files);
Expand Down
11 changes: 8 additions & 3 deletions ...internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/LogoUploaderComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,29 @@ import { useTranslation } from "react-i18next";
import { Fragment } from "react";

const LogoUploaderComponent = ({ onSelect, ...props }) => {
const [file, setFile] = useState(null);
const [fileStoreId, setFileStoreId] = useState(null);
const [showToast, setShowToast] = useState(null);
const [toastMessage, setToastMessage] = useState("");
const [isError, setIsError] = useState(false);
const [file, setFile] = useState(null);
const [fileStoreId, setFileStoreId] = useState(null);
const tenantId = Digit.ULBService.getCurrentTenantId();
const [uploadErrorMessage, setUploadErrorMessage] = useState("");
const { t } = useTranslation();

const handleUploadFile = async () => {
// Upload the file first
try {
const response = await Digit.UploadServices.Filestorage("Sandbox", file, tenantId);
const fileStoreId = response?.data?.files?.[0]?.fileStoreId;
setFileStoreId(fileStoreId)
setUploadErrorMessage("");
} catch (error) {
setToastMessage(t("LOGO_UPLOAD_FAILED"));
setIsError(true);
setUploadErrorMessage(t("LOGO_UPLOAD_FAILED"));
setShowToast(true);
setTimeout(() => {
closeToast();
setShowToast(false);
Comment on lines +23 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Approve error handling improvements with a suggestion.

The changes in the handleUploadFile function significantly improve error handling and user feedback:

  1. Resetting the error message on successful upload (line 23) is good practice.
  2. Setting a specific error message on upload failure (line 27) enhances user feedback.
  3. Using setShowToast(false) instead of closeToast() (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.

}, 2000);
}
};
Expand Down Expand Up @@ -52,6 +56,7 @@ const LogoUploaderComponent = ({ onSelect, ...props }) => {
uploadedFiles={[]}
variant="uploadFile"
onUpload={(files) => selectFile(files)}
iserror={uploadErrorMessage}
accept="image/*, .jpg, .png, .jpeg"
// if (files && files.length > 0) {
// handleUploadFile(files);
Expand Down
22 changes: 20 additions & 2 deletions ...ui-internals/packages/modules/sandbox/src/pages/employee/tenantMgmt/TenantConfigUpload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const TenantConfigUpload = () => {
const [isError, setIsError] = useState(false);
const [uploadData, setUploadData] = useState([]); // State to store the uploaded data
const [tenantDocument, setDocuments] = useState([]);



const mutation = Digit.Hooks.useCustomAPIMutationHook({
Expand Down Expand Up @@ -74,6 +75,20 @@ const TenantConfigUpload = () => {
type: data[key]?.type,
};
});

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;
Comment on lines +86 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
setTimeout(() => {
setShowToast(false);
}, 2000);
return;
closeToast();
return;

}

Comment on lines +78 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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;
}

setUploadData(documents);
};

Expand Down Expand Up @@ -127,12 +142,12 @@ const TenantConfigUpload = () => {
},
{
onError: (error) => {
setToastMessage(error.message || t("ERROR_MESSAGE"));
setToastMessage(error.message || t("CONFIG_UPLOAD_ERROR_MESSAGE"));
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 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 debugging

This 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.

Suggested change
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

setIsError(true);
setShowToast(true);
Comment on lines +145 to 147
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
setToastMessage(error.message || t("CONFIG_UPLOAD_ERROR_MESSAGE"));
setIsError(true);
setShowToast(true);
setToastMessage(t("CONFIG_UPLOAD_ERROR_MESSAGE"));
setIsError(true);
setShowToast(true);

},
onSuccess: () => {
setToastMessage(t("SANDBOX_TENANT_CREATE_SUCCESS_TOAST"));
setToastMessage(t("CONFIG_UPLOAD_SUCCESSFUL_TOAST_MESSAGE"));
setIsError(false);
setShowToast(true);
setTimeout(() => {
Expand All @@ -146,6 +161,9 @@ const TenantConfigUpload = () => {
setToastMessage(error.message);
setIsError(true);
setShowToast(true);
setTimeout(() => {
setShowToast(false);
}, 2000);
Comment on lines +164 to +166
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
setTimeout(() => {
setShowToast(false);
}, 2000);
closeToast();

}
};

Expand Down
2 changes: 1 addition & 1 deletion micro-ui/web/public/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<link
href="https://fonts.googleapis.com/css2?family=Roboto+Condensed:wght@400;500;700&family=Roboto:wght@400;500;700&display=swap"
rel="stylesheet" type="text/css" />
<link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.8.2-beta.33/dist/index.css" />
<link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.8.2-beta.34/dist/index.css" />
<link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-components-css@0.0.2-beta.34/dist/index.css"/>
<meta name="viewport" content="width=device-width, initial-scale=1" />
<meta name="theme-color" content="#00bcd1" />
Expand Down