Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[google_sign_in] Adds support to send clientId as a parameter #3640

Merged
merged 10 commits into from
Mar 18, 2021
Merged

[google_sign_in] Adds support to send clientId as a parameter #3640

merged 10 commits into from
Mar 18, 2021

Conversation

ggirotto
Copy link
Contributor

@ggirotto ggirotto commented Feb 26, 2021

This PR introduces the ability to send clientId as a parameter in google_sign_in plugin. It priorizes the parameterized clientId in iOS and Android platforms. If it wasn't send, then they try to fetch it from GoogleService-info.plist and google-services.json respectively.

List which issues are fixed by this PR. You must list at least one issue.

Pre-launch Checklist

  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@ggirotto
Copy link
Contributor Author

I'm not exactly sure how to update pubspec.yaml and CHANGELOG since the plugin is in the middle of a nullsafety pre-release

@stuartmorgan
Copy link
Contributor

Thanks for the contribution! Looks like this is a pretty straightforward PR, so we can try to get it reviewed quickly. It will need tests though, per the contribution guidelines (you've checked the box, but there are no test files changed in the PR, so if there are test changes they were left out of the PR).

I'm not exactly sure how to update pubspec.yaml and CHANGELOG since the plugin is in the middle of a nullsafety pre-release

The release is now stable, so you can proceed here as normal once you merge in master.

@ggirotto
Copy link
Contributor Author

ggirotto commented Mar 4, 2021

@stuartmorgan I forgot to push my test commit. Now this PR should be complete

@cyanglaz cyanglaz self-assigned this Mar 4, 2021
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

=

@@ -77,7 +77,7 @@ - (void)handleMethodCall:(FlutterMethodCall *)call result:(FlutterResult)result
ofType:@"plist"];
if (path) {
NSMutableDictionary *plist = [[NSMutableDictionary alloc] initWithContentsOfFile:path];
[GIDSignIn sharedInstance].clientID = plist[kClientIdKey];
[GIDSignIn sharedInstance].clientID = [call.arguments objectForKey:@"clientId"] ?: plist[kClientIdKey];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also need to check [[call.arguments objectForKey:@"clientId"] isEqualTo:[NSNull null]]
Maybe refactor it out to a local bool var:

BOOL hasDynamicClientId = [[call.arguments objectForKey:@"clientId"] isKindOfClass:NSString.class]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean checking if clientId is a NSString object? Because ternary operator ?: will already check for nullability

Copy link
Contributor

Choose a reason for hiding this comment

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

So we need to check != nil and != [NSNull null]
Checking is a NSString covers both cases.

@@ -30,6 +30,7 @@ class MethodChannelGoogleSignIn extends GoogleSignInPlatform {
'signInOption': signInOption.toString(),
'scopes': scopes,
'hostedDomain': hostedDomain,
'clientId': clientId,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change need to be published first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyanglaz You mean in a separate PR? I can create a new one with only this change

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we need this in a separate PR and a version bump for the platform_interface. Then google_sign_in needs to dep on the new version.

Copy link
Contributor Author

@ggirotto ggirotto Mar 5, 2021

Choose a reason for hiding this comment

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

@cyanglaz I created a new PR #3686. I didn't add any tests on it, since I suppose the current PR has the necessary tests to cover both PR changes

@cyanglaz
Copy link
Contributor

cyanglaz commented Mar 6, 2021

I landed #3686. Once published, this PR should update the platform interface dependency to 2.0.1 .

@cyanglaz cyanglaz mentioned this pull request Mar 8, 2021
11 tasks
@ggirotto
Copy link
Contributor Author

ggirotto commented Mar 8, 2021

@cyanglaz Updated and Rebased

@cyanglaz
Copy link
Contributor

Looks like there's some issue with the format CI, could you fix it? The lint_darwin ci could be fixed by rebasing master I believe

@ggirotto
Copy link
Contributor Author

@cyanglaz Formatting issues fixed and branched rebased master

@cyanglaz cyanglaz 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 Mar 12, 2021
@fluttergithubbot
Copy link

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

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

1 similar comment
@fluttergithubbot
Copy link

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

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@fluttergithubbot 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 Mar 12, 2021
@ggirotto
Copy link
Contributor Author

@cyanglaz I don't understand the failing output from the CI build:

! webview_flutter 2.0.2 from path /tmp/cirrus-ci-build/packages/webview_flutter

! wifi_info_flutter 2.0.0 from path /tmp/cirrus-ci-build/packages/wifi_info_flutter/wifi_info_flutter

! wifi_info_flutter_platform_interface 2.0.1 from path /tmp/cirrus-ci-build/packages/wifi_info_flutter/wifi_info_flutter_platform_interface
Running "flutter pub get" in all_plugins...                         3.7s

Building without sound null safety
For more information see https://dart.dev/null-safety/unsound-null-safety

Building Linux application...                                   
CMake Error at /usr/share/cmake-3.16/Modules/FindPkgConfig.cmake:463 (message):

  A required package was not found

Call Stack (most recent call first):

  /usr/share/cmake-3.16/Modules/FindPkgConfig.cmake:643 (_pkg_check_modules_internal)

  flutter/CMakeLists.txt:29 (pkg_check_modules)

Could you help me understand what is going wrong here so I can fix it?

@stuartmorgan
Copy link
Contributor

That failure is due to a change in Flutter itself; you'll need to merge master in again to pick up the corresponding fix in flutter/plugins.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 18, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 18, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 20, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 22, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 23, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 23, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 24, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 24, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes p: google_sign_in platform-android platform-ios waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants