-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
deprecate ReportActionUtils.getAllReportActions()
(Issue 39091)
#40046
deprecate ReportActionUtils.getAllReportActions()
(Issue 39091)
#40046
Conversation
This reverts commit 2e950e3.
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
Can't get android running on my machine. Any advice? Will try again later or when I get access to the Slack. |
@EzraEllette if you merge main the android build issues should be fixed. |
Reviewer Checklist
Screenshots/VideosAndroid: Native01_Android_Native.mp4Android: mWeb Chrome02_Android_Chrome.mp4iOS: Native03_iOS_Native.mp4iOS: mWeb Safari04_iOS_Safari.mp4MacOS: Chrome / Safari05_MacOS_Chrome.mp4MacOS: Desktop06_MacOS_Desktop.mp4 |
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.
LGTM.
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 great. Thank you!
Looks like a couple of the tests are failing. Can you see if you can get the CLA bot to recheck again? It also looks like the checklist in the PR description might be out-of-date. |
recheck |
@tgolen The branch is updated and the tests are passing. 🎉 |
@@ -56,6 +59,19 @@ function clearReportActionErrors(reportID: string, reportAction: ReportAction, k | |||
}); | |||
} | |||
|
|||
const reportActionsByReport: OnyxCollection<ReportActions> = {}; | |||
Onyx.connect({ | |||
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, |
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.
@tgolen Is there a reason why waitForCollectionCallback
is not being used here?
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.
None that I know of 😅
Thanks for catching that. It probably should be using waitForCollectionCallback
since that will have a slight performance improvement.
@EzraEllette Could you do a follow-up PR to modify that?
Sure thing
…On Fri, Apr 12, 2024 at 11:57 AM Tim Golen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/libs/actions/ReportActions.ts
<#40046 (comment)>:
> @@ -56,6 +59,19 @@ function clearReportActionErrors(reportID: string, reportAction: ReportAction, k
});
}
+const reportActionsByReport: OnyxCollection<ReportActions> = {};
+Onyx.connect({
+ key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
None that I know of 😅
Thanks for catching that. It probably should be using
waitForCollectionCallback since that will have a slight performance
improvement.
@EzraEllette <https://github.com/EzraEllette> Could you do a follow-up PR
to modify that?
—
Reply to this email directly, view it on GitHub
<#40046 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AL7TPLNPZ27HCFGURW6W77DY5AHAJAVCNFSM6AAAAABGBF35L2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOJXHE2TMMRRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
🚀 Deployed to staging by https://github.com/tgolen in version: 1.4.63-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
Details
Removed
ReportActionUtils.getAllReportActions()
from list of exports. Using Onyx, Subscribed to report action data in all other files that require a list ofReportActions
by report ID.Fixed Issues
$ #39091
PROPOSAL: #39091 (comment)
Tests
getAllReportActions
is not exportedOffline tests
Ensure that no reports may be created or modified while offline.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android.1.mov
Android.2.mov
Android: mWeb Chrome
iOS: Native
Please note that the submit button failure was already present in the main branch when I started this PR.iOS.Native.P1.mov
iOS.Native.P2.mov
iOS: mWeb Safari
iOS.Web.mov
MacOS: Chrome / Safari
Web.1.mov
Web.2.mov
MacOS: Desktop
Desktop.1.mov
Desktop.2.mov