-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Feat: isEmptyReport derived value #60697
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
Changes from all commits
98520e7
fce72a1
e0b446d
b8205dc
23f04eb
e8b7bca
2f93cd3
743f8b7
0e86159
7f7b90d
7c836f7
a706bb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import {generateReportAttributes, generateReportName, isValidReport} from '@libs/ReportUtils'; | ||
| import {generateIsEmptyReport, generateReportAttributes, generateReportName, isValidReport} from '@libs/ReportUtils'; | ||
| import SidebarUtils from '@libs/SidebarUtils'; | ||
| import createOnyxDerivedValueConfig from '@userActions/OnyxDerived/createOnyxDerivedValueConfig'; | ||
| import hasKeyTriggeredCompute from '@userActions/OnyxDerived/utils'; | ||
|
|
@@ -107,6 +107,7 @@ export default createOnyxDerivedValueConfig({ | |
|
|
||
| acc[report.reportID] = { | ||
| reportName: generateReportName(report), | ||
| isEmpty: generateIsEmptyReport(report), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you contribute to the unit test for Derived value here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! I tried to add another test, but I've noticed that the tests are not working as expected (they are not failing regardless of assertion made). Will address it (and contact Tomek to double check if there is indeed a regression) and get back to you when resolved 👍
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It turns out is something with async nature of tests because they are not correctly fulfilled (the async callback is not even ran). Also, what would might help is waiting for We are still trying to figure out 100% what's happening. |
||
| brickRoadStatus, | ||
| }; | ||
|
|
||
|
|
||
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.
NAB, but if all that
isEmptyReportdoes is add caching on topgenerateEmptyReport, wouldn't it make more sense to concatenate them into a single function? Something like: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.
Hi @mjasikowski, thanks for the comment!
Why we need to separate them into two is that when generating
reportAttributesunderOnyxDerived, we need to make sure it is fresh value. In other words, we need to force cache refresh when cache expires.I have followed the pattern that was used with
generateReportName + getReportNamecombo.Let me know if that makes sense, thanks!