add 'VERIFY_AND_CHANGE_EMAIL' linkType for generateEmailActionLink#705
Conversation
(cherry picked from commit 8a7ccb6)
| } | ||
|
|
||
| if linkType == verifyAndChangeEmail { | ||
| if newEmail == nil { |
There was a problem hiding this comment.
Do we also need to check for || *newEmail == "" in case an empty string is passed in here?
|
|
||
| if linkType == verifyAndChangeEmail { | ||
| if newEmail == nil { | ||
| return "", errors.New("newEmail must not be nil when linkType is verifyAndChangeEmail") |
There was a problem hiding this comment.
nit: "newEmail must be not be empty when linkType is verifyAndChangeEmail"
|
|
||
| if _, err := client.EmailSignInLink(context.Background(), "", testActionCodeSettings); err == nil { | ||
| t.Errorf("EmailSignInLink('') = nil; want error") | ||
| } |
There was a problem hiding this comment.
Should we add a test case for client.VerifyAndChangeEmailLink being called without an email for the sake of consistency?
| } | ||
| } | ||
|
|
||
| func TestEmailActionLinkNoEmail(t *testing.T) { |
There was a problem hiding this comment.
Should we have a test for when VerifyAndChangeEmailLink is called with empty/nil newEmail?
| } | ||
|
|
||
| // VerifyAndChangeEmailLink generates the out-of-band email action link for email verification and change flows for the | ||
| // specified email address. |
There was a problem hiding this comment.
nit: "// specified current email address and new email address" to clarify there are two email addresses needed for this function.
|
|
||
| func (c *baseClient) generateEmailActionLink( | ||
| ctx context.Context, linkType linkType, email string, settings *ActionCodeSettings) (string, error) { | ||
| ctx context.Context, linkType linkType, email string, settings *ActionCodeSettings, newEmail *string) (string, error) { |
There was a problem hiding this comment.
I wonder if it may make sense to make this a variadic function now, making the last argument something like: "opts ...emailActionLinkOption,"
emailActionLinkOption would be something like "type emailActionLinkOption func(map[string]interface{})"
and then we'd define:
"
func withNewEmail(newEmail string) emailActionLinkOption {
return func(payload map[string]interface{}) {
payload["newEmail"] = newEmail
}
}
"
With this in place, existing call sites of generateEmailActionLink can remain unchanged (no need to pass a trailing nil) .
The new call site (VerifyAndChangeEmailLinkWithSettings) can look like this:
"
func (c *baseClient) VerifyAndChangeEmailLinkWithSettings(
ctx context.Context, email string, newEmail string, settings
*ActionCodeSettings) (string, error) {
if newEmail == "" {
return "", errors.New("newEmail must not be empty")
}
return c.generateEmailActionLink(ctx, verifyAndChangeEmail, email,
settings, withNewEmail(newEmail))
}
"
From my vantage point, the main advantage here is scalability. The tradeoff is the code becomes a bit more verbose and I don't know of any current needs to support additional similar options any time soon, so this may fall into the YAGNI. I'd be curious to hear your thoughts on this!
There was a problem hiding this comment.
Thanks! I tried it out and it worked perfectly, so I'll go with your suggestion.
macastelaz
left a comment
macastelaz
left a comment
There was a problem hiding this comment.
Thanks for the contribution and making the suggested changes!
|
Sorry I didn't catch this earlier - per the contribution guidelines, can you instead merge into the dev branch instead of master (see: https://github.com/firebase/firebase-admin-go?tab=contributing-ov-file) Thanks! |
Ooops - I forgot I could change this myself. None-the-less, if you have future contributions to make, feel free to target the dev branch directly. Thanks again for the contribution here! |
5 participants
Summary
VERIFY_AND_CHANGE_EMAILlinkType in Firebase Auth email action linksVerifyAndChangeEmailLinkandVerifyAndChangeEmailLinkWithSettingsmethodsChanges
verifyAndChangeEmaillinkType constantgenerateEmailActionLinkto accept optionalnewEmailparameterTest plan
This addresses the feature request in #470 by providing the ability to generate email action links for email verification and change flows.