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

Jump to bottom

fix: [M3-8391] - Account maintenance CSV download in the first attempt #11025

Merged
merged 3 commits into from
Oct 3, 2024
Merged

fix: [M3-8391] - Account maintenance CSV download in the first attempt #11025

merged 3 commits into from
Oct 3, 2024

Conversation

harsh-akamai
Copy link
Contributor

Description 📝

When downloading CSVs on the Account Maintenance page at /account/maintenance, the generated CSV always contains empty data on the first attempt to download (per page load). The user must download the CSV a second time in order for the data to be populated.

Changes 🔄

  • Fixed the account maintenance CSV download on the first attempt

Target release date 🗓️

N/A

How to test 🧪

Verification steps

  • Enable MSW
  • Navigate to http://localhost:3000/account/maintenance
  • Click on the 'Download CSV' button in the Pending/Complete section
  • You must be able to view a populated csv file in the first attempt of downloading

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@harsh-akamai harsh-akamai requested a review from a team as a code owner October 1, 2024 13:40
@harsh-akamai harsh-akamai requested review from dwiley-akamai and abailly-akamai and removed request for a team October 1, 2024 13:40
@harsh-akamai harsh-akamai self-assigned this Oct 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 87.19%
Current Coverage: 87.19%

abailly-akamai
abailly-akamai approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thx! while not particularly elegant, setTimeOut may be the best way to make this click async without adding a lot of logic. Works for me.

Confirmed fix works 100% of the time (with MSW) ✅

packages/manager/src/features/Account/Maintenance/MaintenanceTable.tsx
@@ -136,7 +136,9 @@ const MaintenanceTable = ({ type }: Props) => {

const downloadCSV = async () => {
await getCSVData();
csvRef.current.link.click();
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a bit of a hack, please add a comment as to why we are doing this

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on the necessity of adding an explanatory comment here

dwiley-akamai
dwiley-akamai approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed fix works as expected ✅

packages/manager/src/features/Account/Maintenance/MaintenanceTable.tsx
@@ -136,7 +136,9 @@ const MaintenanceTable = ({ type }: Props) => {

const downloadCSV = async () => {
await getCSVData();
csvRef.current.link.click();
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on the necessity of adding an explanatory comment here

@bnussman-akamai
Copy link
Member

bnussman-akamai commented Oct 1, 2024 •
edited
Loading

react-csv has some async examples here: https://www.npmjs.com/package/react-csv#--onclick-props

Not sure if the examples are really relevant but maybe they have some more elegant ways of handling this?

@harsh-akamai
Copy link
Contributor Author

@bnussman-akamai Thanks for sharing this approach! Ideally, this async method should work, but after some digging, I discovered that the implementation doesn't behave as expected (the async operation isn't truly asynchronous). There are several open issues in react-csv regarding the same but none are addressed yet.

@harsh-akamai harsh-akamai added the Approved Multiple approvals and ready to merge! label Oct 3, 2024
@bnussman-akamai
Copy link
Member

Understood @harsh-akamai Thanks for taking a look!

I see react-csv was last updated 3 years ago 😳 Maybe at some point in the future we can find a more modern alternative that is maintained

@harsh-akamai
Copy link
Contributor Author

@bnussman-akamai Yes. react-csv would require a well-maintained substitute in the future.

@harsh-akamai harsh-akamai reopened this Oct 3, 2024
@harsh-akamai harsh-akamai merged commit e8c0ed4 into linode:develop Oct 3, 2024
37 checks passed
@harsh-akamai harsh-akamai deleted the feature/M3-8392-account-maint-csv-download-fix branch October 3, 2024 15:37
@cypress Cypress
Copy link

cypress bot commented Oct 3, 2024

Cloud Manager E2E    Run #6616

Run Properties:  status check passed Passed #6616  •  git commit e8c0ed4401: fix: [M3-8391] - Account maintenance CSV download in the first attempt (#11025)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6616
Run duration 25m 37s
Commit git commit e8c0ed4401: fix: [M3-8391] - Account maintenance CSV download in the first attempt (#11025)
Committer Harsh Shankar Rao
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 414
View all changes introduced in this branch ↗︎

This was referenced Oct 10, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abailly-akamai abailly-akamai abailly-akamai approved these changes

@bnussman-akamai bnussman-akamai bnussman-akamai approved these changes

@dwiley-akamai dwiley-akamai dwiley-akamai approved these changes

Assignees

@harsh-akamai harsh-akamai

Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@harsh-akamai @bnussman-akamai @dwiley-akamai @abailly-akamai