[local_auth] support localizedFallbackTitle in IOSAuthMessages #3806

furaiev · 2021-04-12T20:36:42Z

Description

Add support localizedFallbackTitle in IOSAuthMessages.

Related Issues

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

google-cla · 2021-04-12T20:36:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

furaiev · 2021-04-12T20:38:28Z

@googlebot I signed it!

furaiev · 2021-04-12T20:39:33Z

@stuartmorgan could you please review?

cyanglaz · 2021-04-13T20:17:46Z

This is pretty similar but does have some improvement of #2616.
CC @stuartmorgan since he already started reviewing this other one.

stuartmorgan-g

Apologies, for the long delay in getting back to this review. Specific comments inline below, but this will also need a test, per the contribution guide. In this case, since it involves native UI, I think the easiest approach would be to add a native unit test that uses a mock LAContext and ensures that the property is set (or not set) correctly based on a constructor method call object. See https://github.com/flutter/plugins/blob/master/packages/local_auth/example/ios/RunnerTests/FLTLocalAuthPluginTests.m

packages/local_auth/ios/Classes/FLTLocalAuthPlugin.m

packages/local_auth/lib/auth_strings.dart

furaiev · 2021-09-18T13:11:14Z

@stuartmorgan thank you for your review.
I've updated the PR according to your comments but faced an issue with tests.
localizedFallbackTitle isn't passed via the constructor, it is just a LAContext property and I didn't found a way to check if it was assigned or not.
Could you please advise other ways to test it?

stuartmorgan-g · 2021-09-18T20:38:27Z

localizedFallbackTitle isn't passed via the constructor, it is just a LAContext property

Sorry, "constructor" was a typo in my comment. That should have said a constructed method call object.

and I didn't found a way to check if it was assigned or not.

See OCMVerify.

furaiev · 2021-09-18T20:51:49Z

Thank you @stuartmorgan ,
I've tried OCMVerify and it is triggered on get but not on set.

OCMVerify([mockAuthContext localizedFallbackTitle]) returns OCMockObject(LAContext): Method localizedFallbackTitle was not invoked.

stuartmorgan-g · 2021-09-18T21:28:33Z

I've tried OCMVerify and it is triggered on get but not on set.

That's because you are providing the method name of the getter. The setter for a property called localizedFallbackTitle is setLocalizedFallbackTitle:

furaiev · 2021-09-19T08:42:23Z

thank you @stuartmorgan !
PR is updated

packages/local_auth/lib/auth_strings.dart

stuartmorgan-g · 2021-10-12T15:37:46Z

Please make sure that you've resolved all of the comment threads before requesting review next time; most of my comments were ignored in the last round.

stuartmorgan-g

Please make sure that you've resolved all of the comment threads before requesting review next time; most of my comments were ignored in the last round.

I don't understand why you marked all of comments as resolved while ignoring most of them, even though I specifically re-surfaced them individually and then also called out here that they needed to be addressed.

This review cannot make progress if you are repeatedly, deliberately ignoring my feedback. There is a large backlog of reviews we're trying to work though; I moved forward with this because I expected it to be straightforward to review, but if that's not going to be the case this will need to move to the backlog.

Apologies, I just realized that the issue is that the issues I called out originally existed in two places in the code, and that the one my comment on was addressed but the other wasn't. So on re-review of the current state, it looked like the code I'd commented on previously was still exactly the same, when that's not the case.

stuartmorgan-g · 2022-02-03T16:19:37Z

@gaaclarke Ping for review.

stuartmorgan-g · 2022-02-15T17:37:08Z

@furaiev If you can resolve the conflicts quickly we can land this ASAP, otherwise it'll need to wait until after #4699 and #4701 land and be reconciled against those changes.

(I don't expect it would be difficult to resolve either way, so no rush.)

furaiev · 2022-02-15T17:59:42Z

@stuartmorgan conflicts are resolved. Thanks!

stuartmorgan-g · 2022-02-15T18:30:55Z

Could you re-run the autoformatter? We're enforcing * alignment in ObjC now.

furaiev · 2022-02-15T18:37:37Z

@stuartmorgan done

stuartmorgan-g · 2022-02-15T18:39:22Z

Thanks!

fluttergithubbot · 2022-02-15T19:40:19Z

This pull request is not suitable for automatic merging in its current state.

This pull request has changes requested by @stuartmorgan. Please resolve those before re-applying the label.

furaiev · 2022-02-16T14:28:17Z

@stuartmorgan it seems like the bot found some unresolved requests but I see only resolved conversations

stuartmorgan-g

I'm not sure why the bot is unhappy. Let's see if approving again helps...

fluttergithubbot · 2022-02-16T15:00:21Z

This pull request is not suitable for automatic merging in its current state.

This pull request has changes requested by @stuartmorgan. Please resolve those before re-applying the label.

stuartmorgan-g · 2022-02-16T15:01:25Z

No clue 🤷🏻

Landing manually.

* google/master: (340 commits) [camera]remove "selfRef" for SavePhotoDelegate and ensure thread safety (flutter#4780) Roll Flutter from 919d205 to adafd66 (5 revisions) (flutter#4876) [google_sign_in] Update platform interface analysis options (flutter#4872) Roll Flutter from b623279 to 919d205 (2 revisions) (flutter#4871) Roll Flutter from 14a2b13 to b623279 (5 revisions) (flutter#4870) [image_picker] Update platform interface analysis options (flutter#4837) [ci] Re-enable stable webview Android tests (flutter#4867) Roll Flutter from 286c975 to 14a2b13 (1 revision) (flutter#4865) [local_auth] support localizedFallbackTitle in IOSAuthMessages (flutter#3806) [image_picker] Update app-facing and web analysis options (flutter#4838) Roll Flutter from 8386344 to 286c975 (4 revisions) (flutter#4864) Roll Flutter from e9f83cf to 8386344 (6 revisions) (flutter#4862) [camera] Switch web package to new analysis options (flutter#4834) [flutter_plugin_tool] Fix iOS/macOS naming (flutter#4861) [webview_flutter] Fix debuggingEnabled on Android (flutter#4859) Roll Flutter from ca2a751 to e9f83cf (10 revisions) (flutter#4857) fix license (flutter#4858) [video_player] add allowBackgroundPlayback option platform interface changes (flutter#4807) [video_player] Updated Pigeon version (flutter#4726) Roll Flutter from 381cb28 to ca2a751 (5 revisions) (flutter#4850) ... # Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/android/build.gradle # packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/Camera.java # packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/MethodCallHandlerImpl.java # packages/camera/camera/ios/Classes/CameraPlugin.m # packages/camera/camera/ios/camera.podspec # packages/camera/camera/lib/camera.dart # packages/camera/camera/lib/src/camera_controller.dart # packages/camera/camera/pubspec.yaml

…ges (flutter/plugins#3806)

Added: hardcoded localizedFallbackTitle

71f3a02

google-cla bot added the cla: no label Apr 12, 2021

github-actions bot added p: local_auth platform-ios labels Apr 12, 2021

google-cla bot added cla: yes and removed cla: no labels Apr 12, 2021

stuartmorgan-g self-requested a review April 20, 2021 20:09

stuartmorgan-g suggested changes Sep 14, 2021

View reviewed changes

packages/local_auth/ios/Classes/FLTLocalAuthPlugin.m Outdated Show resolved Hide resolved

packages/local_auth/ios/Classes/FLTLocalAuthPlugin.m Outdated Show resolved Hide resolved

packages/local_auth/lib/auth_strings.dart Outdated Show resolved Hide resolved

furaiev added 3 commits September 18, 2021 11:42

sync commit

d0f59cb

Merge branch 'flutter-master'

56323ba

localizedFallbackTitle as non nullable param

ad80f96

added: localizedFallbackTitle test

2e52b89

furaiev requested a review from stuartmorgan-g September 19, 2021 08:42

furaiev added 2 commits September 20, 2021 11:50

test formatting fix

1b6c9bb

test formatting fix

399f787

stuartmorgan-g suggested changes Oct 12, 2021

View reviewed changes

packages/local_auth/lib/auth_strings.dart Outdated Show resolved Hide resolved

fixed: localizedFallbackTitle will not sent if it is null

a011421

furaiev force-pushed the master branch from a863378 to a011421 Compare October 12, 2021 15:49

furaiev requested a review from stuartmorgan-g October 13, 2021 06:35

stuartmorgan-g suggested changes Oct 14, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master'

e4ac67f

gaaclarke approved these changes Feb 15, 2022

View reviewed changes

stuartmorgan-g added the last mile label Feb 15, 2022

Merge branch 'master' of https://github.com/flutter/plugins

0474601

ObjC formatted

2ca4cba

stuartmorgan-g added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Feb 15, 2022

fluttergithubbot removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Feb 15, 2022

stuartmorgan-g approved these changes Feb 16, 2022

View reviewed changes

stuartmorgan-g added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Feb 16, 2022

fluttergithubbot removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Feb 16, 2022

stuartmorgan-g merged commit 9318efc into flutter:main Feb 16, 2022

stuartmorgan-g mentioned this pull request Feb 16, 2022

[local_auth] Add native iOS implementation of platform interface #4699

Closed

11 tasks

engine-flutter-autoroll mentioned this pull request Feb 17, 2022

Roll Plugins from 136f49b3e8ec to 0b969a4282dd (10 revisions) flutter/flutter#98659

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2022

9318efc76 [local_auth] support localizedFallbackTitle in IOSAuthMessa…

eeb5758

…ges (flutter/plugins#3806)

engine-flutter-autoroll mentioned this pull request Feb 17, 2022

Roll Plugins from 136f49b3e8ec to ed29396208cd (12 revisions) flutter/flutter#98666

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2022

9318efc76 [local_auth] support localizedFallbackTitle in IOSAuthMessa…

dc58f9b

…ges (flutter/plugins#3806)

This was referenced Apr 15, 2022

[local_auth] IOS FaceID should fallback to Passcode but does not flutter/flutter#96390

Closed

[local_auth] support localizedFallbackTitle in IOSAuthMessages #5296

Closed

[local_auth] support localizedFallbackTitle in IOSAuthMessages #5297

Merged

[local_auth] support localizedFallbackTitle in IOSAuthMessages #3806

[local_auth] support localizedFallbackTitle in IOSAuthMessages #3806

Uh oh!

Conversation

furaiev commented Apr 12, 2021 • edited by stuartmorgan-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Pre-launch Checklist

Uh oh!

google-cla bot commented Apr 12, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

furaiev commented Apr 12, 2021

Uh oh!

furaiev commented Apr 12, 2021

Uh oh!

cyanglaz commented Apr 13, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furaiev commented Sep 18, 2021

Uh oh!

stuartmorgan-g commented Sep 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furaiev commented Sep 18, 2021

Uh oh!

stuartmorgan-g commented Sep 18, 2021

Uh oh!

furaiev commented Sep 19, 2021

Uh oh!

Uh oh!

stuartmorgan-g commented Oct 12, 2021

Uh oh!

stuartmorgan-g left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Feb 3, 2022

Uh oh!

stuartmorgan-g commented Feb 15, 2022

Uh oh!

furaiev commented Feb 15, 2022

Uh oh!

stuartmorgan-g commented Feb 15, 2022

Uh oh!

furaiev commented Feb 15, 2022

Uh oh!

stuartmorgan-g commented Feb 15, 2022

Uh oh!

fluttergithubbot commented Feb 15, 2022

Uh oh!

furaiev commented Feb 16, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Feb 16, 2022

Uh oh!

stuartmorgan-g commented Feb 16, 2022

Uh oh!

Uh oh!

furaiev commented Apr 12, 2021 •

edited by stuartmorgan-g

Loading

stuartmorgan-g commented Sep 18, 2021 •

edited

Loading

stuartmorgan-g left a comment •

edited

Loading