-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add contacts import nitro module #54459
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
base: main
Are you sure you want to change the base?
Add contacts import nitro module #54459
Conversation
…hub.com/margelo/expensify-app-fork; branch 'main' of https://github.com/Expensify/App into @perunt/rerevert-contacts-import-module
I didn't test the Hybrid App as I don't have access yet |
@shawnborton @DylanDylann One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Hey! I've created a PR with adjustments so everything works fine with HybridApp |
Hi! @perunt what's your Xcode version locally? I'm testing PR on HybridApp and I cannot build for simulators (it builds fine on physical device tho). My version is I'm also curious about The key information here is that some people might encounter issues when building on a simulator, while others might not, depending on their Swift compiler version. This can block HybridApp development for people without physical iPhone |
Nevertheless I think this is not a serious issue and we can proceed further with this PR and its HybridApp part 😅 People experiencing issues with the Swift compiler can try downgrading their Xcode version or using a physical device if available. cc: @Julesssss |
I think only remaining thing is to bump Xcode on CI infrastructure. I have this PR that was created before Christmas: #54424 |
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.
Tentatively approving, seems like we need to confirm first that this doesn't break hybrid app for simulator's as per above comments.
Also I think we need to merge main due to xcode being bumped, which is why the validate github actions check is failing. |
…hub.com/margelo/expensify-app-fork; branch 'main' of https://github.com/Expensify/App into @perunt/rerevert-contacts-import-module
I'm still dealing with Android errors after bumping RN. I'll ping you once it's done |
…rerevert-contacts-import-module
…rerevert-contacts-import-module
Android - Contacts - Missing Import contacts button when contact list is emptyVersion Number: 9.1.42-0 PR:54559 Action Performed:Precondition:
Expected Result:In Step 6, Import contacts button should be present when there is no contact. Actual Result:In Step 6, Import contacts button is missing when there is no contact. Workaround:Unknown Platforms:
Screenshots/VideosBug6834291_1747491780709.Screen_Recording_20250517_221907_Expensify_Adhoc.mp4 |
iOS&Android - Members - "Select" widget at bottom not triggered on long pressing member nameVersion Number: 9.1.42-0 PR:54559 Action Performed:
Expected Result:On long pressing member name, "Select" widget at the bottom should be triggered Actual Result:"Select" widget at bottom not triggered on long pressing member name, hence owner cannot select members On Android app - Select widget is displayed at the bottom, but the "Select" checkboxes is not triggered on clicking Select Workaround:Unknown Platforms:
Screenshots/VideosBug6834391_1747506705488.az_recorder_20250517_142128.mp4Bug6834391_1747498004657.ScreenRecording_05-17-2025_12-04-09_1.mp4 |
iOS - Contacts - Searched contact does not appear until background app & reopenVersion Number: 9.1.42-0 PR:54559 Action Performed:
Expected Result:When user searches for a contact in invite member page, the contact should appear immediately to select in contacts below Actual Result:Searched contact does not appear until background app & reopen. Also, infinite loading appears in search field when background app & reopen Workaround:Unknown Platforms:
Screenshots/VideosBug6834341_1747495129102.ScreenRecording_05-17-2025_10-54-09_1.mp4 |
iOS - Invite-User not automatically taken to confirm details page on selecting different rolesVersion Number: 9.1.42-0 PR:54559 Action Performed:
Expected Result:User should be automatically redirected to confirm details page when selecting different roles Actual Result:User remains on role page & not automatically taken to confirm details page on selecting different roles. User has to manually tap back button to go back to confirm details page Workaround:Unknown Platforms:
Screenshots/VideosBug6834360_1747496143479.ScreenRecording_05-17-2025_11-35-00_1.mp4 |
iOS - Invite- Invite button not responsive in confirm details pageVersion Number: 9.1.42-0 PR:54559 Action Performed:
Expected Result:On step 5 - On tapping Invite button, user should be redirected to members page & all listed members displayed Actual Result:Invite button not responsive in confirm details page. User not redirected to members page on tapping invite button. However, the invited user appears in members page and can be seen only after background app & reopening it Workaround:Unknown Platforms:
Screenshots/VideosBug6834377_1747496926150.ScreenRecording_05-17-2025_11-44-59_1.mp4 |
iOS - Profile & Role fields remain highlighted after going back from profile or role pageVersion Number: 9.1.42-0 PR:54559 Action Performed:
Expected Result:Profile & Role fields should not be highlighted after going back from profile or role page Actual Result:Profile & Role fields remain highlighted after going back from profile or role page Workaround:Unknown Platforms:
Screenshots/VideosBug6834434_1747501139771.ScreenRecording_05-17-2025_12-58-19_1.mp4 |
iOS-Profile option does not respond after visiting 1:1 DM & then selecting profile second timeVersion Number: 9.1.42-0 PR:54559 Action Performed:
Expected Result:On step 10 - User able to visit details page on tapping profile option Actual Result:Profile option does not respond after visiting 1:1 DM & then selecting profile second time Workaround:Unknown Platforms:
Screenshots/VideosBug6834451_1747501940568.ScreenRecording_05-17-2025_13-10-51_1.mp4 |
iOS-Notification - No notification is received for 1:1 DMVersion Number: 9.1.42-0 PR:54559 Action Performed:
Expected Result:I should receive a notification. Actual Result:No notification is received for 1:1 DM. The app can be closed or backgrounded. Workaround:Unknown Platforms:
Screenshots/VideosBug6834819_1747550384146.QFXE4461.mp4 |
Android-Contacts - Contact middle name is not displayedVersion Number: 9.1.42-0 PR:54559 Action Performed:Precondition:
Expected Result:The contact middle name will be displayed. Actual Result:The contact middle name is not displayed. Workaround:Unknown Platforms:
Screenshots/VideosBug6835109_1747585616362.Screen_Recording_20250518_003717_Expensify_Adhoc.mp4 |
iOS - Infinite loading on navigating to Private Notes page first timeVersion Number: 9.1.42-0 PR:54559 Action Performed:
Expected Result:Privates page should open when navigated to the page first time Actual Result:Infinite loading on navigating to Private Notes page first time Workaround:Unknown Platforms:
Screenshots/VideosBug6835342_1747619911888.ScreenRecording_05-18-2025_21-56-13_1.mp4 |
QA is completed |
…rerevert-contacts-import-module
|
I'm not sure I am completely following, so I apologize if I'm missing something, but could we include the same little text component beneath the search bar as in your right screenshot, but just remove the first sentence saying "No results found"? So we'd just have a little message under there that says "Import your contacts so your favorite people Thoughts on something like that? |
Sounds good to me. |
Agreed, this PR is big enough, lets not increase it even more.
I think this is fine. I don't think it matters including the middle name. I would not expect this from an app and think it's better without importing the middle name. |
I think this is really close to being merged 🚀! Things left to do:
Only other thing I can think of is putting this behind a beta, which will minimize our chances of having to revert this in case of deploy blockers. @perunt is it possible to put this behind a beta so that we can QA this in staging without exposing it to users? @roryabraham thoughts? |
Good idea moving it to beta. I’ll do it |
@perunt you won't be able to until we create the beta in the back-end |
back-end PR to create the beta: https://github.com/Expensify/Web-Expensify/pull/46918 Note that it must be called |
…rerevert-contacts-import-module
…read that cause an error
Beta was deployed to staging. Should be deployed to production tomorrow. Bumped QA on creating some TC's for this feature here. However, even without the full set of TC's I think we should still be good to merge as long as the beta is in place and in production. So we could merge this as early as tomorrow I think. |
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.
Beta is on production and core e2e regression tests have been written. I think we're good to move forward here 👍 🚀
@perunt one more round of merge conflict resolution 🤞
@roryabraham how do you feel about moving forward?
Explanation of Change
https://github.com/Expensify/Mobile-Expensify/pull/13516 - use this PR for Mobile-Expensify
During the revert of the NitroModules PR, we identified two main issues affecting our iOS builds. First, there was a mismatch between the minimum iOS deployment target (set to 13.4) and our app's requirement (iOS 15). Second, we discovered build failures specific to iOS simulators due to a known Swift compiler issue.
While investigating, we found that our CI environment was running an older version of Xcode (15.2), which contributed to the build failures. We've addressed this by updating our CI configuration to use newer macOS runners and updating the Ruby GitHub Action version.
To move forward cleanly, we'll be splitting this into two separate PRs:
After implementing these changes in isolation, our test builds are now passing successfully.
revert #54421
Fixed Issues
$ #47938
PROPOSAL:
Tests
See QA steps.
Offline tests
QA Steps
Setup Prerequisites
Test Steps
1. Basic Flow - First Time Access
2. Soft Permission Modal - "Not Now" Path
3. Soft Permission Modal - "Continue" Path
4. Contact Permission - Deny Access
5. Contact Permission - Allow Access
6. Search Functionality
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop