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.

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

Fix: Advanced payments file upload errors #4171

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Jan 23, 2025

Description

  • This PR aims to differentiate between the structure and too-large-content (aka too many rows) errors when uploading a file for the Advanced Payment action, by adding a brand new error DropzoneErrors.CONTENT_TOO_LARGE. Too make the message more dynamical, the fileOptions values can now be used in the error message descriptors.
  • During working on this issue, noticed react-dropzone will not consider a file upload failed if the options provided will be fulfilled, but the file upload handler will set an error, so the filename will not be available for display in the error component. Thus, I have added a fallbackFileName property to the FileUploadProps.

Testing

TODO: Let's test the changes are working as expected.

  • Step 1. Open the Advanced Payment action

  • Step 2. Click the Upload CSV button underneath the table
    Screenshot 2025-01-27 at 09 55 15

  • Step 3. Check the modal description has been updated to Bulk upload up to 400 recipients using a CSV. Use the template to ensure the correct structure, be sure to remove the sample data.
    Screenshot 2025-01-27 at 10 19 23

  • Step 4. Now upload this file content-too-large.csv and make sure you get this error
    Screenshot 2025-01-27 at 09 55 29

  • Step 5. Upload this other file wrong-structure.csv and make sure you get this error
    Screenshot 2025-01-27 at 09 55 35

  • Step 6. Now try this file correct-structure.csv and you should not get any error in the modal, but should see the rows filled in the payment table

Screen.Recording.2025-01-27.at.09.56.54.mov

Important

Some user addresses will not be of known users, but you can change that before uploading the .csv, however it is not mandatory.

If you have any other testing scenarios coming to mind, please try them out 🎉

Diffs

New stuff

  • Added CONTENT_TOO_LARGE to DropzoneErrors
  • Added fallbackFileName to FileUploadProps

Changes 🏗

  • Renamed isAvatarUploaded to isFileUploaded in FileUploadProps

Resolves #3715

@mmioana mmioana self-assigned this Jan 23, 2025
@mmioana mmioana marked this pull request as ready for review January 27, 2025 09:26
@mmioana mmioana requested a review from a team as a code owner January 27, 2025 09:26
@mmioana mmioana changed the title [WIP] Fix: Update file upload modal description Fix: Advanced payments file upload errors Jan 27, 2025
iamsamgibbs
iamsamgibbs previously approved these changes Jan 27, 2025
Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Amazing! Nice job, appreciate the prop rename too!

Too large:
Screenshot 2025-01-27 at 14 40 30

Wrong structure:
Screenshot 2025-01-27 at 14 40 38

Correct structure:
Screenshot 2025-01-27 at 14 41 04

@mmioana mmioana marked this pull request as draft January 29, 2025 10:32
Fix: Update file upload modal description
Fix: Update file upload error message
@mmioana mmioana force-pushed the fix/3715-advanced-payments-csv-error branch from 38d38c4 to 8874caf Compare January 29, 2025 12:38
@@ -40,7 +44,7 @@ const MSG = defineMessages({
},
fileMinDimensionsError: {
id: `${displayName}.fileMinDimensionsError`,
defaultMessage: 'Image dimensions should be at least 120x120px',
defaultMessage: 'Image dimensions should be at least {fileDimension}',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took the change to refactor this message to be more dynamical based on the provisioned fileDimension prop

@mmioana mmioana marked this pull request as ready for review January 29, 2025 12:39
@mmioana
Copy link
Contributor Author

mmioana commented Jan 29, 2025

Hey @iamsamgibbs thanks for your review. Unfortunately had to rebase, so your review got dismissed :(

Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Really nice @mmioana, super clean changes and fixes.

CSV message and errors are correct:
Screenshot 2025-01-29 at 13 01 35
Screenshot 2025-01-29 at 13 01 44
Screenshot 2025-01-29 at 13 02 08

And it still accepts a correct input:
Screenshot 2025-01-29 at 13 03 22

I also checked the user avatar error just to be sure, and all good there:
Screenshot 2025-01-29 at 13 06 15

Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Still working after rebase!

Screenshot 2025-01-29 at 16 47 14

Screenshot 2025-01-29 at 16 47 21

Copy link
Contributor

@rumzledz rumzledz left a comment

Choose a reason for hiding this comment

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

Pleasant error handling, thanks @mmioana 🚀

Updated file upload prompt

Screenshot 2025-01-30 at 16 13 20

Error messages

Screenshot 2025-01-30 at 16 14 44 Screenshot 2025-01-30 at 16 13 48

@mmioana mmioana merged commit 64b5ee6 into master Jan 30, 2025
2 checks passed
@mmioana mmioana deleted the fix/3715-advanced-payments-csv-error branch January 30, 2025 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Advanced payment] Error displayed when uploading CSV file with more than 300 rows in advanced payment
4 participants