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

feat(auth, android): add android for disable app verification feature #6069

Conversation

Duell10111
Copy link
Contributor

Description

At the moment it is only possible to disable the app verification on the iOS platform for testing.
As I wanted to create e2e tests for the android version of my app, I created the necessary code changes to make this flag also possible on the Android platform. 😄

Related issues

No related Issue as far as I now.

Release Summary

auth: appVerificationDisabledForTesting now also settable for Android platform

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

🔥

@CLAassistant
Copy link

CLAassistant commented Feb 13, 2022

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Feb 13, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/5zAQrdo2uSU9FSHYEpFtc3zVuF41
✅ Preview: Canceled

[Deployment for de4d89b canceled]

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/BqGMEJF4c8tZSLs63jWaYcbXig18
✅ Preview: https://react-native-firebase-git-fork-duell10111-duel-0fa39d-invertase.vercel.app

@Duell10111 Duell10111 force-pushed the @duell10111/android-auth-disableappverification branch from c032a61 to a2a4105 Compare February 13, 2022 15:39
@vercel vercel bot temporarily deployed to Preview – react-native-firebase-next February 13, 2022 15:39 Inactive
mikehardy
mikehardy previously approved these changes Feb 13, 2022
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Oh that looks really clean! Thanks for posting this. Approving the CI runs here, hopefully it goes right through, if not I'll work with you to get all the various CI checks cleared.

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Feb 13, 2022
@codecov
Copy link

codecov bot commented Feb 13, 2022

Codecov Report

Merging #6069 (4c9a5ea) into main (55dc55c) will decrease coverage by 0.46%.
The diff coverage is 22.23%.

❗ Current head 4c9a5ea differs from pull request most recent head de4d89b. Consider uploading reports for the commit de4d89b to get more accurate results

@@             Coverage Diff              @@
##               main    #6069      +/-   ##
============================================
- Coverage     53.37%   52.92%   -0.45%     
+ Complexity      632      622      -10     
============================================
  Files           208      208              
  Lines         10214    10220       +6     
  Branches       1626     1625       -1     
============================================
- Hits           5451     5408      -43     
- Misses         4489     4558      +69     
+ Partials        274      254      -20     

@mikehardy
Copy link
Collaborator

/home/runner/work/react-native-firebase/react-native-firebase/packages/auth/lib/Settings.j
Warning: 18:21 warning 'isIOS' is defined but never used @typescript-eslint/no-unused-vars

This one makes sense - now that auth is no longer ios-only for the method, the import itself (at top of file) is no longer needed

Don't worry about tacking on little follow-on commits, squash-merging this one is fine since it's a focused change even with follow-ons and it'll leave a clean commit trail post-merge once squashed

@mikehardy
Copy link
Collaborator

I reached into the source tree for the PR and just removed the , isIOS - so trivial a fix, not much point in waiting, if lint goes green this is 💯

@mikehardy mikehardy merged commit 48c7842 into invertase:main Feb 13, 2022
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Feb 13, 2022
@mikehardy
Copy link
Collaborator

Publishing it now, new version will be out on npmjs.com in a minute - thanks again

@Duell10111
Copy link
Contributor Author

Duell10111 commented Feb 13, 2022

Thanks for the lint fix and the fast processing of the PR 😄

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.

3 participants