Adds automated test for manual phone sign-in #376

Ivan164 · 2017-10-13T22:19:24Z

Added automation for phone manual test.

Ivan164 · 2017-10-13T22:19:53Z

Example/Auth/Sample/MainViewController.m

@@ -428,6 +428,11 @@
 */
 static NSString *const kAutoAccountLinking = @"Link with Google";

+/** @var kAutoPhoneNumberSignIn
+ @brief The button title for automated account linking.


Will fix indentation on next push.

protocol86 · 2017-10-13T23:32:45Z

Example/Auth/Sample/MainViewController.m

@@ -680,7 +685,7 @@ - (void)updateTable {
                                           action:^{ [weakSelf linkPhoneNumber]; }],
        [StaticContentTableViewCell cellWithTitle:kUnlinkPhoneNumber
                                           action:^{
-          [weakSelf unlinkFromProvider:FIRPhoneAuthProviderID];
+                     [weakSelf unlinkFromProvider:FIRPhoneAuthProviderID completion:nil];


no indentation is needed here.

protocol86 · 2017-10-15T01:35:04Z

Example/Auth/Sample/MainViewController.m

@@ -428,6 +428,11 @@
 */
 static NSString *const kAutoAccountLinking = @"Link with Google";

+/** @var kAutoPhoneNumberSignIn
+ @brief The button title for automated account linking.


The "brief" should be related to Phone Number Sign In and not account linking.

protocol86 · 2017-10-15T01:37:42Z

Example/Auth/Sample/MainViewController.m

+           [self logFailedTest:@"Could not update phone number."];
+         }
+         [self logSuccess:@"update phone number test succeeded."];
+         [self unlinkFromProvider:FIRPhoneAuthProviderID completion:^ {


remove space after "^"

protocol86 · 2017-10-15T01:40:12Z

Example/Auth/Sample/MainViewController.m

@@ -2050,14 +2107,15 @@ - (void)showEmailPasswordDialogWithCompletion:(ShowEmailPasswordDialogCompletion
    @brief Unlinks the current user from the provider with the specified provider ID.
    @param provider The provider ID of the provider to unlink the current user's account from.


Please add "completion" parameter to this comment block, please make sure this is added to all the functions where you added a completion.

Example:
@param completion A completion block to be executed after the provider is unlinked.

protocol86 · 2017-10-15T01:41:29Z

Example/Auth/Sample/MainViewController.m

+    @param phoneNumber Number pass in string for automation.
+ */
+- (void)signInWithPhoneNumberRecaptchaWithString:(NSString *_Nullable)phoneNumber
+                                completion:(void(^)(NSError *_Nullable))completion {


Please align ":" by indenting this line.

protocol86 · 2017-10-15T02:01:10Z

Example/Auth/Sample/MainViewController.m

+    @brief Allows sign in with phone number using reCAPTCHA and completion.
+    @param phoneNumber Number pass in string for automation.
+ */
+- (void)signInWithPhoneNumberRecaptchaWithString:(NSString *_Nullable)phoneNumber


I'd prefer the name signInWithPhoneNumber:(NSString *_Nullable)phoneNumber
completion:(void(^)(NSError *_Nullable))completion...

The reasons are:

ReCAPTCHA is now the correct way to sign in with phone number so it is no longer a special case.

We try out best to make sure the last part of the function name is the name of the actual variable (starting with a lowercase character);
Example:
- (void) doSomethingWithSomethingElse:(NSString *)somethingElse;

Notice that the SomethingElse at the end of the function name matches the "somethingelse" which is the variable name.

Same for other functions (update, link).

protocol86 · 2017-10-15T02:03:18Z

Example/Auth/Sample/MainViewController.m

+    @param phoneNumber Number pass in string for automation.
+ */
+- (void)linkPhoneNumberWithString:(NSString *_Nullable)phoneNumber
+             completion:(void(^)(void))completion{


protocol86 · 2017-10-15T02:17:25Z

Example/Auth/Sample/MainViewController.m

@@ -2653,13 +2725,27 @@ - (void)linkPhoneNumber {
                    return;
                  }
                  [self logSuccess:@"link phone number succeeded."];
+                  if (completion) completion();


As convention we use curly braces for conditionals even when unnecessary.

if (completion) { completion(); }

protocol86 · 2017-10-15T02:20:35Z

Example/Auth/Sample/MainViewController.m

+    @param phoneNumber Number pass in string for automation.
+ */
+- (void)linkPhoneNumberWithString:(NSString *_Nullable)phoneNumber
+             completion:(void(^)(void))completion{


The completion should take an error parameter just like signInWithPhoneNumberRecaptchaWithString.

protocol86 · 2017-10-15T02:22:32Z

Example/Auth/Sample/MainViewController.m

+      [self hideSpinner:^{
+        if (error) {
+          [self logFailure:@"failed to send verification code" error:error];
+          [self showMessagePrompt:error.localizedDescription];


In case of error, you need to call the completion block with the error before returning.

Example:

completion(error); return;

protocol86

In the future please reply to comment indicating that they are addressed, just the word "Done" is usually enough. :)

protocol86 · 2017-10-16T23:28:43Z

Example/Auth/Sample/MainViewController.m

+/** @fn signInWithPhoneNumberPrompt
+    @brief Allows sign in with phone number via popup prompt.
+ */
+- (void)signInWithPhoneNumberPrompt {


signInWithPhoneNumberWithPrompt sounds better. The current name makes the method sound like a it only shows a prompt for signing in with phone number but not actually doing the sign in. Same for other methods.

…gnin Adds automated test for manual phone sign-in.

Adds automated test for manual phone sign-in

2643598

Ivan164 requested a review from protocol86 October 13, 2017 22:19

googlebot added the cla: yes label Oct 13, 2017

Ivan164 commented Oct 13, 2017

View reviewed changes

protocol86 reviewed Oct 15, 2017

View reviewed changes

Adds automated test for manual phone sign-in

d8bcaf0

protocol86 approved these changes Oct 16, 2017

View reviewed changes

Ivan164 added 2 commits October 16, 2017 16:58

Adds automated test for manual phone sign-in

ed4b9d4

Adds automated test for manual phone sign-in

cd112a8

Ivan164 merged commit bb681a0 into master Oct 17, 2017

paulb777 deleted the automate-phonenumber-signin branch November 8, 2017 20:27

minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018

Merge pull request firebase#376 from firebase/automate-phonenumber-si…

69d2dcd

…gnin Adds automated test for manual phone sign-in.

firebase locked and limited conversation to collaborators Nov 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adds automated test for manual phone sign-in #376

Adds automated test for manual phone sign-in #376

Uh oh!

Ivan164 commented Oct 13, 2017 •

edited

Loading

Uh oh!

Ivan164 Oct 13, 2017 •

edited

Loading

Uh oh!

protocol86 Oct 13, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 Oct 15, 2017

Uh oh!

protocol86 left a comment

Uh oh!

protocol86 Oct 16, 2017

Uh oh!

Ivan164 Oct 16, 2017

Uh oh!

Uh oh!

		@@ -2050,14 +2107,15 @@ - (void)showEmailPasswordDialogWithCompletion:(ShowEmailPasswordDialogCompletion
		@brief Unlinks the current user from the provider with the specified provider ID.
		@param provider The provider ID of the provider to unlink the current user's account from.

Adds automated test for manual phone sign-in #376

Adds automated test for manual phone sign-in #376

Uh oh!

Conversation

Ivan164 commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivan164 Oct 13, 2017 • 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!

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!

protocol86 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!

Uh oh!

Ivan164 commented Oct 13, 2017 •

edited

Loading

Ivan164 Oct 13, 2017 •

edited

Loading