-
Notifications
You must be signed in to change notification settings - Fork 78
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
LF-3622 finances dashboard report #2942
LF-3622 finances dashboard report #2942
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good (and you can remove the original interim "Download report" button now!), and it seems like a solid organization for the code structure! Thank you for including the logic needed for the other report type!
I left a few inline comments from my read-through. I didn't spot a cause from reading, but in testing it seemed that that dateRange state handling is not quite correct; could you confirm these issues occur for you too?
- Export once with default date (Year to Date). If you then select a different time period than year to date in the dashboard filters and then open the report modal, the dropdown reflects the new selection but the data (both the
config
and thetransactions
are still set with year-to-date. You need to refresh the dashboard once picking a new selection to get the update in the modal. - If you load up the main dashboard and change the date filter directly (before opening the report modal), the dropdown on the report modal will not match what shown on the dashboard
- If you have a custom range set in the dashboard, it's possible to clear the custom range in the report filters and still export. However, you get a weird date object like this
- If there is no custom date range set in the dashboard and you set a custom range in the component, you get this:
Are those errors reproducible for you?
const filterRef = useRef({}); | ||
const transactions = useTransactions({ | ||
dateFilter, | ||
expenseTypeFilter: filterRef.current[EXPENSE_TYPE], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooooh this is the first ever bug I suspected before I loaded up the app to test!! 😁 I'm having fun with the reading first review method!
As a ref, I think this isn't going to trigger the useMemo
dependency array of useTransactions
. The transaction list will always be the same regardless of which filters are applied in the modal. Do these filters need to go into state like dateRange (or does useTransactions
have to be called in some other way/at a different point)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading first is the way! 😅 In testing this I do see the filters being applied correctly -- I think if the ref value changes, that new value will still get passed to the transactions hook and trigger the memo. Can you let me know though if you are seeing any problems with the filters?
That being said, we should be using state variables instead of refs in filters altogether. I've also seen the same with dropdowns, where we're passing a ref instead of a state variable. In general we shouldn't be using refs unless absolutely necessary for something that needs to happen outside of the normal React flow, but since it's a broad issue we'll have to tackle it as a refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! No I definitely double-checked the behaviour (I don't trust my reading skills that much yet 😂) and saw the update issue in testing as well. Is this different than what you see? I am logging in the saga:
filters-persistence-bug.mov
For me, the data sent 1) doesn't update based on edits made in the modal after loading it, 2) but are applied after the modal is reloaded (minor bug is that the visual filter state appears to be reset to what the dashboard shows but the data is using the filter setting from the last modal edit!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antsgar sorry I realized I didn't tag you on the above comments and these threads don't create notifications!
…-to-filter-export-report-data
@kathyavini @SayakaOno I think I addressed all the comments/issues! Please take another look when you can |
@antsgar of the 4 date-related bugs I mentioned, I don't see the top two anymore! However, I'm still seeing these two:
date-undefined-moment.mov |
The button should definitely be disabled if the range is not correctly set, I'll fix that! |
@kathyavini thank you for testing thoroughly, dates are hard! 😥 took a second stab at fixing the issues, can you take another look? 🙏 |
a187e67
to
3bc7387
Compare
@antsgar the dates look good! I still don't think the filters are being updated at the right time, but I realized belatedly you might not have seen that comment and video because it was in a thread, sorry! |
Never mind, I could repro now! Will fix 🙂 |
@kathyavini I think I fixed the filters bug, can you re-check please? 🙏 sorry about all the back and forth! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like you to remove one more line, but everything else looks good to me!
Since Joyce had been approved, I merged! |
Description
Jira link:
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: