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

[google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data #4179

Merged
merged 7 commits into from
Oct 8, 2021
Merged

[google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data #4179

merged 7 commits into from
Oct 8, 2021

Conversation

p-shapovalov
Copy link
Contributor

@p-shapovalov p-shapovalov commented Jul 22, 2021 •
edited
Loading

Description

Obtain serverAuthCode on login. This value can be reused then in /pull/4180

Related issues

flutter/flutter#57712
flutter/flutter#57741

Pre-launch Checklist

  • 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 [relevant style guides] and ran [the auto-formatter]. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • 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 added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@google-cla google-cla bot added the cla: yes label Jul 22, 2021
@p-shapovalov p-shapovalov mentioned this pull request Jul 22, 2021
Merged
13 tasks
@stuartmorgan
Copy link
Contributor

Thanks for the contribution!

  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.

You've indicated that you have added tests, but I don't see them. Were they accidentally left out of the PR push?

@p-shapovalov
Copy link
Contributor Author

fixed

@stuartmorgan
Copy link
Contributor

The only change I see is to some mock data. What test here would fail without your changes?

@p-shapovalov
Copy link
Contributor Author

p-shapovalov commented Aug 5, 2021 •
edited
Loading

The only change I see is to some mock data.

Because the only change I did was adding extra serverAuthCode field to GoogleSignInUserData type. And for interface package I can't test how it can be used, I can only test that methods returns previously declared data, and this test already exist and it use overrided == operator declared inside GoogleSignInUserData type

@p-shapovalov
Copy link
Contributor Author

I can of course add line like

expect(response.serverAuthCode, kUser.serverAuthCode);

and the same for each user data field, but that part was already done by

expect(response, kUser)

because of

otherUserData.serverAuthCode == serverAuthCode;

added in user data type

@p-shapovalov
Copy link
Contributor Author

@stuartmorgan

@p-shapovalov
Copy link
Contributor Author

@stuartmorgan anything else I can add to this pull request?

@p-shapovalov
Copy link
Contributor Author

@stuartmorgan please let me know if you need anything else.

@p-shapovalov
Copy link
Contributor Author

Aanyone-nyone-one-one?

@stuartmorgan
Copy link
Contributor

@cyanglaz @bparrishMines You both have some experience with google_sign_in; could one or both of you take a look at this and #4180? It looks straightforward, I'm just not familiar enough with the internals of this plugin to review.

cyanglaz
cyanglaz suggested changes Oct 7, 2021
View reviewed changes
...oogle_sign_in/google_sign_in_platform_interface/test/method_channel_google_sign_in_test.dart
@@ -13,6 +13,8 @@ const Map<String, String> kUserData = <String, String>{
"id": "8162538176523816253123",
"photoUrl": "https://lh5.googleusercontent.com/photo.jpg",
"displayName": "John Doe",
'idToken': '123',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the test data gets validated anywhere. I'd imagine there should be something like;

expect(user.serverAuthCode, kUserData['serverAuthCode']);

(Thanks for adding the idToken)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check #4179 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh make sense, thanks

packages/google_sign_in/google_sign_in_platform_interface/pubspec.yaml Outdated Show resolved Hide resolved
@p-shapovalov p-shapovalov requested a review from cyanglaz October 8, 2021 08:42
cyanglaz
cyanglaz approved these changes Oct 8, 2021
View reviewed changes
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.

LGTM

...oogle_sign_in/google_sign_in_platform_interface/test/method_channel_google_sign_in_test.dart
@@ -13,6 +13,8 @@ const Map<String, String> kUserData = <String, String>{
"id": "8162538176523816253123",
"photoUrl": "https://lh5.googleusercontent.com/photo.jpg",
"displayName": "John Doe",
'idToken': '123',
Copy link
Contributor

Choose a reason for hiding this comment

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

oh make sense, thanks

@cyanglaz cyanglaz added waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. and removed needs tests labels Oct 8, 2021
@fluttergithubbot fluttergithubbot merged commit 5117a3f into flutter:master Oct 8, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 8, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 8, 2021
@engine-flutter-autoroll
5117a3f [google_sign_in] Add serverAuthCode attribute to google_sign_…
7ee0c2e 
…in_platform_interface user data (flutter/plugins#4179)
@p-shapovalov p-shapovalov deleted the fix/fetch-server-auth-code branch October 8, 2021 20:01
fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Oct 8, 2021
@engine-flutter-autoroll
5117a3f [google_sign_in] Add serverAuthCode attribute to google_sign_…
d895afb 
…in_platform_interface user data (flutter/plugins#4179) (#91515)
mgonzalezc pushed a commit to mgonzalezc/plugins that referenced this pull request Oct 12, 2021
Merge branch 'master' into quick_actions_mindapps
419b195 
* master: (1126 commits)
  [webview_flutter] Adjust test URLs again (flutter#4407)
  [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179)
  [camera] Add filter for unsupported cameras on Android (flutter#4418)
  [webview_flutter] Update webview platform interface with new methods for running JavaScript. (flutter#4401)
  [webview_flutter] Add zoomEnabled to webview flutter platform interface (flutter#4404)
  [ci] Remove obsolete Dockerfile (flutter#4405)
  Fix order-dependant platform interface tests (flutter#4406)
  [google_maps_flutter]: LatLng longitude loses precision in constructor #90574 (flutter#4374)
  [google_maps_flutter] Add Marker drag events (flutter#2838)
  [flutter_plugin_tools] Validate pubspec description (flutter#4396)
  Add file_selector to the repo list (flutter#4395)
  [in_app_purchase] Fix in_app_purchase_android/README.md (flutter#4363)
  [google_maps_flutter_web] Add Marker drag events (flutter#4385)
  [webview_flutter] Fixed todos in FlutterWebView.java (flutter#4394)
  Handle `PurchaseStatus.restored` correctly in example. (flutter#4393)
  Handle restored purchases in iOS example app (flutter#4392)
  [file_selector] Remove custom analysis options (flutter#4382)
  [flutter_plugin_tools] Check licenses in Kotlin (flutter#4373)
  Fixed _CastError when running example App (flutter#4390)
  [in_app_purchase] Ensure the introductoryPriceMicros field is transported as a String. (flutter#4370)
  ...

# Conflicts:
#	packages/quick_actions/ios/Classes/FLTQuickActionsPlugin.m
camsim99 pushed a commit to camsim99/plugins that referenced this pull request Oct 18, 2021
@p-shapovalov @camsim99
[google_sign_in] Add serverAuthCode attribute to google_sign_in_platf…
fecfd7e 
…orm_interface user data (flutter#4179)
NickalasB added a commit to NickalasB/plugins that referenced this pull request Oct 21, 2021
@NickalasB
Merge branch 'master' into implement_zoomEnabled_for_ios_and_android
6672b4f 
* master:
  [webview] Fix typos in the README (flutter#4249)
  [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180)
  [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429)
  [shared_preferences] Switch to new analysis options (flutter#4384)
  [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413)
  [camera] Run iOS methods on UI thread by default (flutter#4140)
  [ci] Always run all `format` steps (flutter#4427)
  [flutter_plugin_tools] Fix license-check on Windows (flutter#4425)
  [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400)
  [image_picker][android] suppress unchecked warning (flutter#4408)
  [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436)
  [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality  (flutter#4389)
  Bump compileSdkVersion to 31 (flutter#4432)
  [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426)
  Update integration_test README  (flutter#3824)
  [webview_flutter] Adjust test URLs again (flutter#4407)
  [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179)
  [camera] Add filter for unsupported cameras on Android (flutter#4418)
NickalasB added a commit to NickalasB/plugins that referenced this pull request Oct 21, 2021
@NickalasB
Merge branch 'master' into add_zoom_to_android_webview
eab9765 
* master:
  [webview] Fix typos in the README (flutter#4249)
  [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180)
  [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429)
  [shared_preferences] Switch to new analysis options (flutter#4384)
  [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413)
  [camera] Run iOS methods on UI thread by default (flutter#4140)
  [ci] Always run all `format` steps (flutter#4427)
  [flutter_plugin_tools] Fix license-check on Windows (flutter#4425)
  [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400)
  [image_picker][android] suppress unchecked warning (flutter#4408)
  [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436)
  [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality  (flutter#4389)
  Bump compileSdkVersion to 31 (flutter#4432)
  [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426)
  Update integration_test README  (flutter#3824)
  [webview_flutter] Adjust test URLs again (flutter#4407)
  [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179)
  [camera] Add filter for unsupported cameras on Android (flutter#4418)

# Conflicts:
#	packages/webview_flutter/webview_flutter/CHANGELOG.md
#	packages/webview_flutter/webview_flutter/pubspec.yaml
yasargil added a commit to yasargil/plugins that referenced this pull request Oct 25, 2021
@yasargil
Merge branch 'master' into gmain
d37dd69 
* master: (364 commits)
  Use OpenJDK 11 in CI jobs  (flutter#4419)
  [google_sign_in] remove the commented out code in tests (flutter#4442)
  [webview] Fix typos in the README (flutter#4249)
  [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180)
  [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429)
  [shared_preferences] Switch to new analysis options (flutter#4384)
  [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413)
  [camera] Run iOS methods on UI thread by default (flutter#4140)
  [ci] Always run all `format` steps (flutter#4427)
  [flutter_plugin_tools] Fix license-check on Windows (flutter#4425)
  [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400)
  [image_picker][android] suppress unchecked warning (flutter#4408)
  [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436)
  [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality  (flutter#4389)
  Bump compileSdkVersion to 31 (flutter#4432)
  [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426)
  Update integration_test README  (flutter#3824)
  [webview_flutter] Adjust test URLs again (flutter#4407)
  [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179)
  [camera] Add filter for unsupported cameras on Android (flutter#4418)
  ...

# Conflicts:
#	packages/google_maps_flutter/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
@engine-flutter-autoroll @clocksmith
5117a3f [google_sign_in] Add serverAuthCode attribute to google_sign_…
7afe4d5 
…in_platform_interface user data (flutter/plugins#4179) (flutter#91515)
amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021
@p-shapovalov @amantoux
[google_sign_in] Add serverAuthCode attribute to google_sign_in_platf…
a1b7857 
…orm_interface user data (flutter#4179)
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
@p-shapovalov @KyleFin
[google_sign_in] Add serverAuthCode attribute to google_sign_in_platf…
f893c6b 
…orm_interface user data (flutter#4179)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cyanglaz cyanglaz cyanglaz approved these changes

Assignees
No one assigned
Labels
cla: yes p: google_sign_in 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@p-shapovalov @stuartmorgan @cyanglaz @fluttergithubbot