feat(auth): add options to resendSignUpCode #1422
Conversation
...in/java/com/amplifyframework/auth/cognito/options/AWSCognitoAuthResendSignUpCodeOptions.java
Show resolved
Hide resolved
rxbindings/src/main/java/com/amplifyframework/rx/RxAuthBinding.java
Outdated
Show resolved
Hide resolved
Jordan-Nelson
changed the title
|
Can you please elaborate on what type of testings you have done so that this PR does not introduce regressions? I see some unit tests are done but we are looking for more testing. |
|
Hi @frimfram - For context, I am adding this for use in amplify-flutter. I have tested an end to end resend sign up code flow from a flutter app with the changes in aws-amplify/amplify-flutter#738 and confirmed that I can see the metadata in a lambda trigger. If there is something specific you are looking for from a testing perspective, please let me know. |
|
Please make sure to test edge cases. These are some:
|
|
@frimfram - I have tested the following edge cases. I first tested them with amplify android version 1.21.0 to verify the existing behavior and then tested the same scenarios against this PR (with default options and with options + metadata). If there are other scenarios you would like to see tested, please let me know.
|
| @@ -350,10 +352,17 @@ public void confirmSignUp( | |||
| @Override | |||
| public void resendSignUpCode( | |||
| @NonNull String username, | |||
| @NonNull AuthResendSignUpCodeOptions options, | |||
There was a problem hiding this comment.
Isn't it possible to introduce another overloaded function instead of inserting new NonNull param in a public function? If someone was calling this function, then they would also need to change as a result of this change. For public SDK's it's best to minimize breaking changes.
There was a problem hiding this comment.
This PR does introduce a new overloaded function. The diff here is a possibly a little misleading since it shows resendSignUpCode being modified. resendSignUpCode definied on like 381 below has the same method signature as before, and it calls this newly added method with a default option. This will not be a breaking change.
| suspend fun resendSignUpCode(username: String): AuthSignUpResult | ||
| suspend fun resendSignUpCode( | ||
| username: String, | ||
| options: AuthResendSignUpCodeOptions = AuthResendSignUpCodeOptions.defaults() | ||
| ): AuthSignUpResult |
There was a problem hiding this comment.
Same concern as the above - can you think of a way to introduce this in a backward compatible way?
There was a problem hiding this comment.
The newly added parameter has a default option. This is backwards compatible.
Issue #, if available: n/a
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.