Add default phone number to the Phone Auth number flow. by yramanchuk · Pull Request #334 · firebase/FirebaseUI-iOS

yramanchuk · 2017-09-12T20:21:49Z

Related issue: #306

morganchen12

Looks mostly good, had a few comments.

morganchen12 · 2017-09-12T20:32:17Z

FirebaseFacebookAuthUI/FUIFacebookAuth.m

+}
+
+- (void)signInWithDefaultValue:(nullable NSString *)defaultValue
+    presentingViewController:(nullable UIViewController *)presentingViewController


nit: colon alignment

morganchen12 · 2017-09-12T20:32:30Z

FirebaseGoogleAuthUI/FUIGoogleAuth.m

+}
+
+- (void)signInWithDefaultValue:(nullable NSString *)defaultValue
+    presentingViewController:(nullable UIViewController *)presentingViewController


nit: colon alignment

morganchen12 · 2017-09-12T20:34:55Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.h

+        a raw phone number and country code.
+    @return object or nil if any of the required parameters is nil.
+*/
+- (instancetype)initWithNormalizedPhoneNumber:(NSString *)normalizedPhoneNumber;


These three initializers need to return nullable instancetype, not instancetype.

morganchen12 · 2017-09-12T20:35:06Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.h

+    @param countryCode              (required) The country code information
+    @return object or nil if any of the required parameters is nil.
+*/
+- (instancetype)initWithRawPhoneNumber:(NSString *)rawPhoneNumber


nullable instancetype

morganchen12 · 2017-09-12T20:35:11Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.h

+    @param countryCode              (required) The country code information
+    @return object or nil if any of the required parameters is nil.
+*/
+- (instancetype)initWithNormalizedPhoneNumber:(NSString *)normalizedPhoneNumber


nullable instancetype

morganchen12 · 2017-09-12T20:35:57Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.h

+
+- (instancetype)init __unavailable;
+
+- (BOOL)validate:(NSError **)error;


Please add docs comments to this method.

morganchen12 · 2017-09-12T20:39:07Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.m

+  NSParameterAssert(rawPhoneNumber);
+  NSParameterAssert(countryCode);
+  if (!normalizedPhoneNumber || !rawPhoneNumber || !countryCode){
+    return nil;


this return statement should be unreachable given the assertions above.

If this is the only point where we return nil and it's unreachable, maybe the method signature should be modified to return a nonnull type and this code should be removed.

morganchen12 · 2017-09-12T20:42:43Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.m

+    if (error) {
+      *error = [NSError errorWithDomain:FUIPhoneNumberValidationErrorDomain
+                                   code:FUIPhoneNumberValidationErrorMissingPlus
+                               userInfo:nil];


We should be adding userInfo dictionaries with description keys, so meaningful error messages appear when the errors are logged.

morganchen12 · 2017-09-12T20:42:56Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.m

+    if (error) {
+      *error = [NSError errorWithDomain:FUIPhoneNumberValidationErrorDomain
+                                   code:FUIPhoneNumberValidationErrorMissingDialCode
+                               userInfo:nil];


morganchen12 · 2017-09-12T20:45:10Z

FirebasePhoneAuthUI/FUIPhoneEntryViewController.m

+- (instancetype)initWithNibName:(nullable NSString *)nibNameOrNil
+                         bundle:(nullable NSBundle *)nibBundleOrNil
+                         authUI:(FUIAuth *)authUI {
+  return [self initWithNibName:NSStringFromClass([self class])


Should this initializer ignore the nib and bundle parameters?

yramanchuk · 2017-09-13T18:56:23Z

Morgan, thanks for all your comments!
PTAL.

morganchen12

Looks good, pending a few comments about assertions/nullability in the initializers.

morganchen12 · 2017-09-13T22:47:31Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.m

+
+@implementation FUIPhoneNumber
+
+- (nullable instancetype)initWithNormalizedPhoneNumber:(NSString *)normalizedPhoneNumber {


This function takes a nonnull normalizedPhoneNumber, so maybe instead of returning nil we can assert and return a nonnull type as well.

morganchen12 · 2017-09-13T22:47:57Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.m

+- (nullable instancetype)initWithNormalizedPhoneNumber:(NSString *)normalizedPhoneNumber
+                                        rawPhoneNumber:(NSString *)rawPhoneNumber
+                                           countryCode:(FUICountryCodeInfo *)countryCode {
+  if (!normalizedPhoneNumber || !rawPhoneNumber || !countryCode){


Same as the other initializer, all of these parameters are expected to be nonnull.

morganchen12 · 2017-09-13T22:48:28Z

FirebasePhoneAuthUI/CountryCode/FUIPhoneNumber.m

+  return self;
+}
+
+- (nullable instancetype)initWithRawPhoneNumber:(NSString *)rawPhoneNumber


If the other two initializers are no longer failable, this one can be marked nonnull as well.

yramanchuk · 2017-09-19T17:09:49Z

@morganchen12 Done!

morganchen12

LGTM, merge whenever

Add default phone number to the Phone Auth number flow.

7cb83f1

yramanchuk requested a review from morganchen12 September 12, 2017 20:21

morganchen12 requested changes Sep 12, 2017

View reviewed changes

yramanchuk mentioned this pull request Sep 12, 2017

Default phone firebase/FirebaseUI-Android#906

Merged

Update code style. Fix phone number validation method.

90e0a0d

Update code style. Fix phone number validation method.

623c011

morganchen12 reviewed Sep 13, 2017

View reviewed changes

Update signature of FUIPhoneNumber init methods.

6608ec1

morganchen12 approved these changes Sep 19, 2017

View reviewed changes

yramanchuk merged commit 5dac6a1 into master Sep 19, 2017

yramanchuk deleted the default_phone branch September 19, 2017 23:40


		- (instancetype)init __unavailable;

		- (BOOL)validate:(NSError **)error;


		@implementation FUIPhoneNumber

		- (nullable instancetype)initWithNormalizedPhoneNumber:(NSString *)normalizedPhoneNumber {

Conversation

yramanchuk commented Sep 12, 2017

Uh oh!

morganchen12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yramanchuk commented Sep 13, 2017

Uh oh!

morganchen12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yramanchuk commented Sep 19, 2017

Uh oh!

morganchen12 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants