-
Notifications
You must be signed in to change notification settings - Fork 248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(auth): Add returnOobLink to the ActionCodeSettings #502
base: dev
Are you sure you want to change the base?
Changes from 14 commits
54b8114
cef91ac
77177c7
a957589
eb0d2a0
05378ef
4121c50
928b104
02cde4f
6b40682
e60757f
bb055ed
e8dfc32
23a1f17
1d24577
f9dc53b
12a4788
4cf413a
54af579
981442d
61c6c04
32af2b8
02300a8
74c9bd5
37c7936
bef15cf
b28ccfb
b04387e
c348341
da18d13
77f51b8
147a277
e5a5134
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,7 @@ var testActionCodeSettings = &ActionCodeSettings{ | |
AndroidPackageName: "com.example.android", | ||
AndroidInstallApp: true, | ||
AndroidMinimumVersion: "6", | ||
ReturnOobLink: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove this in the base testActionCodeSettings struct? We need a test case to verify that if this field is not specified, returnOobLink is still set to true, due to sdk implementation. I suggest adding a new test case in the newly added test, see my other comment. Thanks! |
||
} | ||
var testActionCodeSettingsMap = map[string]interface{}{ | ||
"continueUrl": "https://example.dynamic.link", | ||
|
@@ -49,6 +50,7 @@ var testActionCodeSettingsMap = map[string]interface{}{ | |
"androidPackageName": "com.example.android", | ||
"androidInstallApp": true, | ||
"androidMinimumVersion": "6", | ||
"returnOobLink": true, | ||
} | ||
var invalidActionCodeSettings = []struct { | ||
name string | ||
|
@@ -309,6 +311,35 @@ func TestEmailVerificationLinkError(t *testing.T) { | |
} | ||
} | ||
|
||
func TestEmailVerificationSendEmail(t *testing.T) { | ||
s := echoServer(testActionLinkResponse, t) | ||
defer s.Close() | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we call this "TestEmailVerificationCustomOobLinkSettings" or similar? Then, we can add 2 cases:
It would be great if you can add a similar test for EmailSignInLink and PasswordReset too. Thanks! |
||
testActionCodeSettingsNoReturn := testActionCodeSettings | ||
testActionCodeSettingsNoReturn.ReturnOobLink = false | ||
testActionCodeSettingsMapNoReturn := testActionCodeSettingsMap | ||
testActionCodeSettingsMapNoReturn["returnOobLink"] = false | ||
link, err := s.Client.EmailSignInLink(context.Background(), testEmail, testActionCodeSettingsNoReturn) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
if link != testActionLink { | ||
t.Errorf("EmailSignInLink() = %q; want = %q", link, testActionLink) | ||
} | ||
|
||
want := map[string]interface{}{ | ||
"requestType": "EMAIL_SIGNIN", | ||
"email": testEmail, | ||
"returnOobLink": false, | ||
} | ||
for k, v := range testActionCodeSettingsMapNoReturn { | ||
want[k] = v | ||
} | ||
if err := checkActionLinkRequest(want, s); err != nil { | ||
t.Fatalf("EmailSignInLink() %v", err) | ||
} | ||
} | ||
|
||
func checkActionLinkRequest(want map[string]interface{}, s *mockAuthServer) error { | ||
wantURL := "/projects/mock-project-id/accounts:sendOobCode" | ||
return checkActionLinkRequestWithURL(want, wantURL, s) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Review feedback was to rename this variable to
SendEmailLink
in order to maintain the default value false for booleanThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dal-Papa Would you be able to make this change, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you want but that will break the override that was happening on the key name due to the
toMap
function...