-
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
LHN - RBR appears on the wrong workspace chat for an error occurring on another workspace #49202
base: main
Are you sure you want to change the base?
Conversation
…error occurring on another workspace Expensify#47874
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
@SIMalik, can you please upload the videos for all other platforms? We need to do this to ensure all platforms pass, and I will also upload my own tests for all platforms again when I fill the check list later. :) |
Hi @ntdiary, Thank you for reviewing my PR. I wanted to ask for some guidance on how to test the changes across all six platforms (Android Native, Android mWeb Chrome, iOS Native, iOS mWeb Safari, MacOS Chrome/Safari, and MacOS Desktop). Currently, I'm using Linux for development, and as this is my first PR, I'm not entirely sure how to test on all these platforms. Could you please advise on the best approach for this? Any help or resources you can provide would be greatly appreciated. Thank you! |
https://github.com/Expensify/App/blob/main/contributingGuides/TESTING_MACOS_AND_IOS.md |
@ntdiary Hi |
Reviewer Checklist
Screenshots/Videos |
src/libs/OptionsListUtils.ts
Outdated
@@ -471,7 +471,7 @@ function uniqFast(items: string[]): string[] { | |||
function getAllReportErrors(report: OnyxEntry<Report>, reportActions: OnyxEntry<ReportActions>): OnyxCommon.Errors { | |||
const reportErrors = report?.errors ?? {}; | |||
const reportErrorFields = report?.errorFields ?? {}; | |||
const reportActionsArray = Object.values(reportActions ?? {}); | |||
const reportActionsArray = Object.values(reportActions ?? {}).filter(action => !ReportActionUtils.isDeletedAction(action)); |
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.
const reportActionsArray = Object.values(reportActions ?? {}).filter(action => !ReportActionUtils.isDeletedAction(action)); | |
const reportActionsArray = Object.values(reportActions ?? {}).filter((action) => !ReportActionUtils.isDeletedAction(action)); |
tiny lint issue. :)
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.
@SIMalik, can you please fix this tiny issue? I will fill the check list soon. :)
Addtionally, @SIMalik, did you encounter any difficulties while setting up the environment? :) |
@ntdiary |
@SIMalik, Have you run |
@ntdiary |
@SIMalik, can you please provide a clearer screenshot of the emulator error? I couldn't make out the error message on it. 😂 Also, if you can test the Android mWeb using As for security error, it’s usually due to not having the correct local HTTPS setup. BTW, we have a time zone difference, so my responses may not be timely. :) |
@ntdiary Could you please confirm if this meets the requirements? |
@SIMalik, LGTM! And if the video is too large, you can also upload screenshots like I did. 😄 |
@SIMalik, any updates for the other platforms? :) |
@ntdiary |
Hi @ntdiary, I initially installed macOS 10.15 Catalina, but I encountered several unsupported command issues, which affected its performance. Realizing it was an outdated version, I decided to remove it. Currently, I am in the process of installing macOS 15 Sequoia. Since this is my first time installing macOS, it’s taking some time, but I'm making progress and am not stuck. Thank you for your understanding! |
Hi @ntdiary MacOS: Chrome / Safari Working on other platforms. |
Hi @ntdiary macOS: Ventura 13.7 iOS Setup: I’ve completed the following steps: Step 1: Install Xcode 14.3.1 Step 3: Set Up CocoaPods Step 4: Configure Mapbox Token Step 5: Install Project Dependencies Step 6: Run the iOS Simulator Issue: When I open Expensify/ios/NewExpensify.xcworkspace in Xcode, I encounter the following error message: "The target 'NewExpensifyTests' contains source code developed with Swift 3.x. This version of Xcode does not support building or migrating Swift 3.x targets." Could you please advise on how to proceed or correct any steps to successfully run the iOS app? Thank you! |
@SIMalik, this message? Don't worry, just click "OK" to ignore it. I see it every time I open the project in Xcode. :) |
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'm about to approve the changes now. @SIMalik, please continue testing on the other platforms, and feel free to let me know if you encounter any problems. This will definitely help you understand and solve other issues in the future. :)
Hi @ntdiary, Yes, I'm currently testing on the other platforms and will ensure everything is completed. If I encounter any issues, I’ll be sure to let you know. Thank you for your support! |
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.
Great thanks!
@SIMalik looks like the automated tests found an error that will need to be fixed. We might need to re-evaluate the approach. |
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.
Requesting changes to fix the failing tests
You're absolutely right; I’m currently working on addressing the failing tests and will make the necessary adjustments. |
App/tests/unit/DebugUtilsTest.ts Lines 1310 to 1317 in abdfb72
The mock So we need to add the
As for the ESLint and performance errors, based on the logs, they don't seem related to this PR. Perhaps we can try running the tests again after merging the latest |
Test Suites: 1 passed, 1 total |
Details
Fixed Issues
$ #47874
PROPOSAL: #47874 (comment)
Tests
Offline tests
NA
QA Steps
Same as Tests above
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
47874-NoErrorShown-2.mp4
Android: Native
Android: mWeb Chrome
Video
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop