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 |
melvin-bot
bot
requested review from
DylanDylann and
shawnborton
and removed request for
a team
|
@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 |
|
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
…rerevert-contacts-import-module
|
|
Yes, there are a bunch of errors in the tests and ReportUtils, which weren’t touched in this PR. Same with Storybook. I’m working on resolving those. Apart from that, the code is ready, I had some issues with migrating to the latest Nitro on Android, but everything's settled now. |
|
|
|
|
|
|
|
@roryabraham all green. I’ve cleaned up the boilerplate and slightly changed the structure to make it easier to add a new module. Meanwhile it reuses all config from the root, so it should be good to go now |
There was a problem hiding this comment.
LGTM. @roryabraham can you take another look?
We need to resolve merge conflicts but with a PR this big lets just get consensus and then once that is reached we'll make sure to approve and merge right after Taras resolves the most recent round of merge conflicts
|
@DylanDylann can you go through the checklist? |
|
Back to this PR today |
…rerevert-contacts-import-module
|
|
|
|
ios/NewExpensify.xcodeproj/xcshareddata/xcschemes/New Expensify.xcscheme
Outdated
Show resolved
Hide resolved
|
@perunt Could you build Hybrid App successfully? |
It worked for me. Try using https://github.com/Expensify/Mobile-Expensify/pull/13516 for Mobile-Expensify. But in any case, Android should work even without that PR |
|
@DylanDylann or instead of using this PR, you can simply switch 'ContactsModule' to 'NitroUtils' in the Podfile |
| firstName: detail?.firstName ?? '', | ||
| lastName: detail?.lastName ?? '', | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| firstName: (detail?.firstName || ('firstName' in participant ? participant.firstName : '')) ?? '', |
There was a problem hiding this comment.
| firstName: (detail?.firstName || ('firstName' in participant ? participant.firstName : '')) ?? '', | |
| firstName: (detail?.firstName || (participant?.firstName ?? '')) |
There was a problem hiding this comment.
Did you check if the type works here? I'm seeing an error on my end with these changes
That's why I'm checking if the participant has a firstName/lastName property
I'll update the type to include it so everything looks clean
| alternateText: formatPhoneNumber(login) || displayName, | ||
| icons: [ | ||
| { | ||
| source: detail?.avatar ?? FallbackAvatar, | ||
| source: ('avatar' in participant ? participant.avatar : detail?.avatar) ?? FallbackAvatar, |
There was a problem hiding this comment.
TS complains about this as well
I'll update the type to include it
| @@ -14,4 +14,5 @@ web/gtm.js | |||
| src/libs/SearchParser/searchParser.js | |||
| src/libs/SearchParser/autocompleteParser.js | |||
| help/_scripts/** | |||
| modules/** | |||
There was a problem hiding this comment.
it's a bit leftover. It's a bit leftover.
Since we reuse everything from the root and don't use a separate formatting system I need to change it so it ignores only autogenerated nitrogen files
| @@ -65,7 +65,7 @@ | |||
| </Testables> | |||
| </TestAction> | |||
| <LaunchAction | |||
| buildConfiguration = "ReleaseProduction" | |||
| buildConfiguration = "DebugDevelopment" | |||
There was a problem hiding this comment.
This change looks sus to me - why is it introduced?
There was a problem hiding this comment.
I tested it during merge, it wasn't intentional changes, removed
Thanks!
There was a problem hiding this comment.
Can we rename this to modules/ExpensifyNitroUtils/ExpensifyNitroUtils.podspec? That way it shows up in the Podfile.lock as ExpensifyNitroUtils instead of UtilsModule, making it clearer what it is.
|
|
||
| set (PACKAGE_NAME UtilsModule) | ||
| set (CMAKE_VERBOSE_MAKEFILE ON) | ||
| set (CMAKE_CXX_STANDARD 20) |
There was a problem hiding this comment.
Any reason not to use C++23?
There was a problem hiding this comment.
Should this path be modules/ExpensifyNitroUtils/android/src/main/java/com/expensify/nitro/utils/UtilsModulePackage.java?
|
|
||
| private fun requestContactPermission(): Boolean { | ||
| val currentActivity = context.currentActivity | ||
| return if (currentActivity != null) { |
There was a problem hiding this comment.
NAB but I think this would be clearer written in a style more similar to other programming languages:
if (currentActivity == null) {
return false
}
// Request permissions
ActivityCompat.requestPermissions(
currentActivity,
arrayOf(REQUIRED_PERMISSION),
PERMISSION_REQUEST_CODE
)
return true|
|
||
| companion object { | ||
| const val PERMISSION_REQUEST_CODE = 1 | ||
| const val REQUIRED_PERMISSION = Manifest.permission.READ_CONTACTS |
There was a problem hiding this comment.
This is only used once and cast to an array. Why not just initialize it as an array here?
| const val REQUIRED_PERMISSION = Manifest.permission.READ_CONTACTS | |
| const val REQUIRED_PERMISSIONS = arrayOf(Manifest.permission.READ_CONTACTS) |
| } | ||
|
|
||
| private fun hasPhoneContactsPermission(): Boolean { | ||
| val result = ContextCompat.checkSelfPermission(context, Manifest.permission.READ_CONTACTS) == PackageManager.PERMISSION_GRANTED |
There was a problem hiding this comment.
NAB remove the redundant result variable here
| ContactsContract.CommonDataKinds.Email.CONTENT_ITEM_TYPE | ||
| ) | ||
|
|
||
| val sortOrder = "${ContactsContract.Data.CONTACT_ID} ASC" |
There was a problem hiding this comment.
Maybe we should sort by last name rather than CONTACT_ID?
|
|
||
| context.contentResolver?.let { resolver -> | ||
| val projection = arrayOf( | ||
| ContactsContract.Data.MIMETYPE, |
There was a problem hiding this comment.
NAB move projection, selection, selectionArgs, and sortOrder to companion because they're static and never change
| "scripts": { | ||
| "postinstall": "tsc || exit 0;", | ||
| "typecheck": "tsc --noEmit", | ||
| "clean": "del-cli android/build node_modules/**/android/build lib", |
There was a problem hiding this comment.
Why is del-cli being used here? Would rm -rf work the same? Also del-cli isn't listed as a dev dependency, so I'm not sure this would work out of the box.
| @@ -46,7 +46,7 @@ | |||
| "perf-test": "NODE_OPTIONS=--experimental-vm-modules npx reassure", | |||
| "typecheck": "NODE_OPTIONS=--max_old_space_size=8192 tsc", | |||
| "lint": "NODE_OPTIONS=--max_old_space_size=8192 eslint . --max-warnings=0 --cache --cache-location=node_modules/.cache/eslint", | |||
| "lint-changed": "NODE_OPTIONS=--max_old_space_size=8192 eslint --max-warnings=0 --config ./.eslintrc.changed.js $(git diff --diff-filter=AM --name-only origin/main HEAD -- \"*.ts\" \"*.tsx\")", | |||
| "lint-changed": "NODE_OPTIONS=--max_old_space_size=8192 eslint --max-warnings=0 --config ./.eslintrc.changed.js $(git diff --diff-filter=AM --name-only origin/main HEAD -- \"*.ts\" \"*.tsx\" \":!modules/**\")", | |||
There was a problem hiding this comment.
Why disable lint in the modules directory?
| CONST.DEVICE_CONTACT.IMAGE_DATA, | ||
| ]); | ||
| } | ||
| return [] as Contact[]; |
There was a problem hiding this comment.
NAB flip this for better readability:
if (response !== RESULTS.GRANTED) {
return [] as Contact[];
}
return ContactsNitroModule.getAll(...)| @@ -6816,6 +6816,14 @@ const CONST = { | |||
| }, | |||
| }, | |||
|
|
|||
| DEVICE_CONTACT: { | |||
There was a problem hiding this comment.
I feel like this is duplicated from modules/ExpensifyNitroUtils/specs/ContactsModule.nitro.ts - could we just use that as the source of truth?
| export type ContactFields = 'FIRST_NAME' | 'LAST_NAME' | 'MIDDLE_NAME' | 'PHONE_NUMBERS' | 'EMAIL_ADDRESSES' | 'IMAGE_DATA' | 'THUMBNAIL_IMAGE_DATA' | 'GIVEN_NAME_KEY'; | ||
|
|
||
| export interface ContactsModule extends HybridObject<{ios: 'swift'; android: 'kotlin'}> { | ||
| getAll(keys: ContactFields[]): Promise<Contact[]>; |
There was a problem hiding this comment.
I see this is only used once, so you could simplify things by just hard-coding the fields we query to [FIRST_NAME, LAST_NAME, PHONE_NUMBERS, EMAIL_ADDRESSES, IMAGE_DATA] since those are the only fields we deep to be using.
|
@perunt any update here? |
Explanation of Change
https://github.com/Expensify/Mobile-Expensify/pull/13516 - use this PR for Expensify-Mobile
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
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop