Skip to content
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

Resolve missing Onfido dependency #6773

Merged
merged 11 commits into from
Jan 14, 2022
Merged

Conversation

Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Dec 15, 2021

Details

Screenshot 2021-12-15 at 17 12 48

  • Also resolves a similar issue with Firebase libraries

Screenshot 2021-12-21 at 14 22 40

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/188764
$ #7190

Tests

Make sure that you clear pods when building to iOS

ENSURE THE ANDROID RELEASE BUILD COMPILES (Android only)

  • Copy the .env.production contents into your .env file
  • Make sure you run npm i from the root App folder
  • Open the App/android folder in Android Studio
  • In Android Studio, click 'Build' > 'Clean project, or run ./gradlew clean from the android folder
  • In Android Studio, open the Build Variant window and select release
  • Wait for Gradle to Sync
  • Generate a signed release build via Build > Generate Signed Bundle / APK, then APK (you will need to create a new on-time keystore and can delete it afterwards)
  • The APK file should generate successfully
  • cd to that folder, then run adb install -r app-release.apk
  • App should run and work as expected

ENSURE THE ONFIDO LIBRARY UPDATE DIDN'T BREAK ONFIDO (Android & iOS)

  • Can be either release OR debug flavour
  • Build to iOS / Android (physical if possible, as Onfido requires hardware camera)
  • Create or navigate to a workspace, then scroll to 'Connect Bank Account'
  • Follow this StackOverflow, adding a verifying bank account
  • Ensure you can upload photos as part of the identity check flow
  • Arrive at the Additional Information (or Let's finish in chat! if you want to fully complete the VBA flow)

QA Steps

Run the ENSURE THE ONFIDO LIBRARY UPDATE DIDN'T BREAK ONFIDO test only.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot 2022-01-11 at 13 18 05

Mobile Web

Simulator Screen Shot - iPhone 7 - 2021-12-21 at 14 44 35

Desktop

Screenshot 2021-12-21 at 14 46 53

iOS

IMG_0072 IMG_0075
IMG_0074

Android

Screenshot_20220111-150535
Screenshot_20220111-150652
Screenshot_20220111-150711

@Julesssss Julesssss self-assigned this Dec 15, 2021
@Julesssss Julesssss force-pushed the jules-fixBrokenOnfidoDependency branch from 73d56f2 to 42411d1 Compare December 15, 2021 17:42
@Julesssss Julesssss marked this pull request as ready for review December 20, 2021 16:07
@Julesssss Julesssss requested a review from a team as a code owner December 20, 2021 16:07
@MelvinBot MelvinBot requested review from aldo-expensify and removed request for a team December 20, 2021 16:08
@aldo-expensify
Copy link
Contributor

Hey @Julesssss ! do you know what I may be missing to test this?

image

Is this the menu entry you are referring to? Do you know why it may be greyed out in my case?

@Julesssss
Copy link
Contributor Author

Is this the menu entry you are referring to? Do you know why it may be greyed out in my case?

Hey @aldo-expensify, I believe it's because you opened the root App folder in Android Studio. Try opening the App/android folder and see if you get the option.

(I just added some more changes too, but that shouldn't have changed anything)

@Julesssss Julesssss mentioned this pull request Dec 20, 2021
1 task
@aldo-expensify
Copy link
Contributor

Is this the menu entry you are referring to? Do you know why it may be greyed out in my case?

Hey @aldo-expensify, I believe it's because you opened the root App folder in Android Studio. Try opening the App/android folder and see if you get the option.

Hmm, nop, I opened it from the App/android folder:

image

Maybe I have something miss-configured in my env. Is it necessary to do the test using Android Studio? I can see the following in my event log:

2021-12-20
6:17 p.m.	Gradle sync started

6:17 p.m.	Android Studio is using the following JDK location when running Gradle:
				/Applications/Android Studio.app/Contents/jre/jdk/Contents/Home
				Using different JDK locations on different processes might cause Gradle to
				spawn multiple daemons, for example, by executing Gradle tasks from a terminal
				while using Android Studio.
				More info...
				Select a JDK from the File System
				Do not show this warning again

6:17 p.m.	Gradle sync failed: Sync failed: reason unknown (39 s 729 ms)

@aldo-expensify
Copy link
Contributor

android/app/build.gradle Show resolved Hide resolved
package.json Show resolved Hide resolved
@Julesssss
Copy link
Contributor Author

Julesssss commented Dec 21, 2021

@aldo-expensify the second screenshot looks better, it looks like the file structure is now being recognized as a project.

That Stack Overflow isn't the solution (yet). Forget the release build for now. The first step is to be able to build successfully the default debug flavour. Once you can successfully generate a build (no need to run/install it), then you can worry about the release build (and follow the Stack Overflow).

The fact that the make project option is disabled tells me that Android Studio is still not configured correctly. Could you try some of the following and let me know how it goes? I can video chat at some point too if this doesn't help:

  • Run npm i again in the root App folder
  • File > Sync project with gradle files
  • Try clicking Add Configuration, as that should have defaulted to something
  • TryFile > Invalidate cache and Restart
  • Open the Problems & Event Log windows, and take a screenshot of any errors

@Julesssss
Copy link
Contributor Author

@marcaaron I took a look through our test cases but couldn't find any related to Onfido/identity checks. Did I miss any, or do we not QA this currently?

I also tried following these steps, but was unable to work out how to get access to the Onfido dashboard. Seeking help in Slack....

@Julesssss
Copy link
Contributor Author

I have to give up on Onfido for now, as I'm struggling to make progress and have external dependencies to work on instead. I have asked for help in engineering-chat here.

@aldo-expensify
Copy link
Contributor

@Julesssss About my miss-configured environment... I fixed the error Gradle sync failed: Sync failed: reason unknown by changing the Android Gradle Plugin Version to 4.2.2. (File > Project Structure > Android Gradle Plugin Version) - more about it in this SO

With that change, the gradle sync finishes without throwing the error and now I can see more stuff in the menu NOT greyed out:

Also I noticed that the Build Variants panel can be accessed from Project Structure (maybe I have a different version of Android Studio.

image

@Julesssss
Copy link
Contributor Author

Julesssss commented Dec 22, 2021

I'm still struggling to test Onfido, currently stuck with an API error at the verify amounts step of the VBA flow. I have updated all repos and have Ngrok secure running 😭

VBAMissingexternalAPIResponse.mov

@aldo-expensify, please leave this for now. I'll have to try again in the new year.

@marcaaron
Copy link
Contributor

What's the status of this PR?

@Julesssss
Copy link
Contributor Author

@marcaaron an external dependency project took priority over this, but that was merged today so I'm back onto this issue. The only blocker here was that I have been unsuccessful in testing Onfido on dev. Will try again today and post my results.

@Julesssss
Copy link
Contributor Author

Okay, I was able to verify the Onfido flow today on both physical iOS and Android devices. Thanks for the help @marcaaron!

@aldo-expensify sorry for the delay, the PR is now ready for testing. I have updated both tests, please let me know if you need any help with testing.

@Julesssss Julesssss changed the title [NOQA] Resolve missing Onfido dependency Resolve missing Onfido dependency Jan 11, 2022
@Julesssss
Copy link
Contributor Author

Bumping this for review, let me know if I can help with anything.

Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested release build on Android device, worked fine!
Tested on IOS simulator, Onfido opened fine, I didn't continue because of camera reasons.

@Julesssss
Copy link
Contributor Author

CC @marcaaron you left comments in the last review, so it would be great to resolve those before we merge.

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can confirm I'm no longer getting the bom issue when running npm run android-build locally

@marcaaron
Copy link
Contributor

Thanks! The only comment I saw unresolved was a "just curious" so NAB if everything's testing good for y'all

@Julesssss Julesssss merged commit ca085ae into main Jan 14, 2022
@Julesssss Julesssss deleted the jules-fixBrokenOnfidoDependency branch January 14, 2022 12:08
@Julesssss
Copy link
Contributor Author

Thanks all!

@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Julesssss in version: 1.1.29-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.30-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants