Skip to content
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

fix(@aws-amplify/auth): incorrect return type for Auth.resendSignUp #5112

Merged
merged 8 commits into from
Aug 19, 2020
Merged

fix(@aws-amplify/auth): incorrect return type for Auth.resendSignUp #5112

merged 8 commits into from
Aug 19, 2020

Conversation

mousedownmike
Copy link
Contributor

Partial fix for #5029

Updated Auth.resendSignUp signature to return any type instead of string. Revised Spy to be reflective of an actual CodeDeliveryDetails response.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #5112 into main will decrease coverage by 0.17%.
The diff coverage is 85.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5112      +/-   ##
==========================================
- Coverage   73.48%   73.31%   -0.18%     
==========================================
  Files         205      208       +3     
  Lines       12621    12922     +301     
  Branches     2460     2526      +66     
==========================================
+ Hits         9275     9474     +199     
- Misses       3157     3257     +100     
- Partials      189      191       +2     
Impacted Files Coverage Δ
packages/api/src/API.ts 95.12% <ø> (ø)
packages/auth/src/OAuth/OAuth.ts 56.11% <0.00%> (+6.47%) ⬆️
packages/datastore/src/sync/index.ts 15.38% <0.00%> (-0.06%) ⬇️
packages/datastore/src/sync/processors/mutation.ts 13.19% <0.00%> (ø)
...ages/datastore/src/sync/processors/subscription.ts 42.58% <0.00%> (-0.28%) ⬇️
...pubsub/src/Providers/AWSAppSyncRealTimeProvider.ts 18.54% <0.00%> (ø)
...ges/analytics/src/Providers/AWSPinpointProvider.ts 74.69% <20.00%> (-0.84%) ⬇️
...kages/aws-amplify-react/src/Auth/Authenticator.tsx 71.85% <20.00%> (-3.74%) ⬇️
packages/storage/src/providers/AWSS3Provider.ts 86.97% <66.66%> (-0.33%) ⬇️
...torage/src/providers/AWSS3ProviderManagedUpload.ts 92.66% <75.00%> (-2.44%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d3d7a8...7cc8966. Read the comment docs.

@ericclemmons ericclemmons linked an issue Mar 23, 2020 that may be closed by this pull request
Copy link
Contributor

@ericclemmons ericclemmons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Thanks for this.

We still need to validate the PR, but I had a question about Promise<any> first. 🤞

*/
public resendSignUp(
username: string,
clientMetadata: ClientMetaData = this._config.clientMetadata
): Promise<string> {
): Promise<any> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It possible to re-use CodeDeliveryDetails here?

export interface CodeDeliveryDetails {
AttributeName: string;
DeliveryMedium: string;
Destination: string;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericclemmons are we sure it always resolves to CodeDeliveryDetails? I wasn't positive so I chose any. The code builds fine with CodeDelieryDetails in there and I can push that if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch with CodeDeliveryDetails is here:
https://github.com/mousedownmike/amplify-js/tree/resendsignup_codedeliverydetails

Happy to close this PR and start a new one or merge the change into this one. Whichever solution works best and whenever it works.

Cheers,
Mike

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to get this merged in once tests pass.

We'll do some more tests to confirm the payload shape, but for now this is a great step forward 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I went ahead & opened #6561 for comparison)

@ericclemmons
Copy link
Contributor

ericclemmons commented Mar 27, 2020

Everything looks good, though we'll need to manually validate the response payload against the types to be sure whether any is the correct type.

The underlying call indicates a string, but we have to be certain to avoid run-time errors during this crucial flow:

/**
* This is used by a user to resend a confirmation code
* @param {nodeCallback<string>} callback Called on success or error.
* @param {ClientMetadata} clientMetadata object which is passed from client to Cognito Lambda trigger
* @returns {void}
*/
resendConfirmationCode(callback, clientMetadata) {

@amhinson amhinson changed the base branch from master to main June 18, 2020 18:55
@f22hd
Copy link

f22hd commented Jul 22, 2020

Exactly what we added to our project to resolve this mistype of the return type.
Hope this PR gets approved. Thanks.

*/
public resendSignUp(
username: string,
clientMetadata: ClientMetaData = this._config.clientMetadata
): Promise<string> {
): Promise<any> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to get this merged in once tests pass.

We'll do some more tests to confirm the payload shape, but for now this is a great step forward 👍

@amhinson amhinson merged commit 9164b37 into aws-amplify:main Aug 19, 2020
nubpro pushed a commit to nubpro/amplify-js that referenced this pull request Oct 2, 2020
iartemiev pushed a commit that referenced this pull request Oct 13, 2020
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent CodeDeliveryDetails usage.
6 participants