feat(interactions): call onComplete callback with entire response

[ashwinkumar6]

Description of changes

Once the bot's intent is fulfilled the onComplete callback is called at the end. Currently the callback is called only with the slots data, call onComplete callback with entire response instead of just sending slots

This is useful when there are multiple intent and you also receive session attributes from the Lambda function

onComplete callback before

{
    "slots": "{\"ReturnDate\":\"2022-08-23\",\"PickUpDate\":\"2022-08-22\",\"DriverAge\":\"25\",\"CarType\":\"lucury\",\"PickUpCity\":\"boston\",\"Location\":\"boston\"}"
}

onComplete callback now

{
    "$metadata": {
        "httpStatusCode": 200,
        "requestId": "58e63842-228a-485e-b887-bf8463442b8f",
        "attempts": 1,
        "totalRetryDelay": 0
    },
    "alternativeIntents": "[]",
    "audioStream": {},
    "botVersion": "$LATEST",
    "contentType": "text/plain; charset=utf-8",
    "dialogState": "ReadyForFulfillment",
    "intentName": "BookCar_dev",
    "sessionId": "2022-08-22T17:42:33.055Z-gEppCAoX",
    "slots": "{\"ReturnDate\":\"2022-08-23\",\"PickUpDate\":\"2022-08-22\",\"DriverAge\":\"25\",\"CarType\":\"lucury\",\"PickUpCity\":\"boston\",\"Location\":\"boston\"}"
}

Issue #, if available

#1166

Description of how you validated changes

• Unit test added for the change
• Tested change in the sample app

Checklist

• PR description included
• yarn test passes
• Tests are changed or added

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

[ashwinkumar6]

Update unit test to check if callback is called with the entire response

[ashwinkumar6]

reportBotStatus is internally used to call the onComplete callback.
Can be made private in the next major release

[elorzafe]

I like this! Can you double check on our docs that we don't mention anything about it

[ashwinkumar6]

reportBotStatus isn't mentioned on the docs

[ashwinkumar6]

• data: PostTextCommandOutput | PostContentCommandOutput is a super set of AWSLexProviderV2SendResponse
• This wouldn't be a breaking change
• A deprivation warning is also been added on this method
• please look at AWSLexProviderV2SendResponse type below

export interface PostContentCommandOutputFormatted
	extends Omit<PostContentCommandOutput, 'audioStream'> {
	audioStream?: Uint8Array;
}

export type AWSLexProviderV2SendResponse =
	| PostTextCommandOutput
	| PostContentCommandOutputFormatted;

[elorzafe]

Is weird that is inside of V1 provider. You can define an internal type, that doesn't need to be exported

[ashwinkumar6]

Moved this type inside V1 provider class type itself, we don't expose this type

[ashwinkumar6]

In the response sent back to the customer audioStream is sent in the Uint8Array format

[lgtm-com]

This pull request introduces 1 alert when merging 9663ada into 2b54c1a - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[codecov-commenter]

Codecov Report

Merging #10248 (57319ca) into main (511751a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #10248      +/-   ##
==========================================
- Coverage   84.34%   84.34%   -0.01%     
==========================================
  Files         256      256              
  Lines       18581    18578       -3     
  Branches     3986     3986              
==========================================
- Hits        15673    15670       -3     
  Misses       2818     2818              
  Partials       90       90              

Impacted Files	Coverage Δ
...kages/interactions/src/Providers/AWSLexProvider.ts	93.50% <100.00%> (-0.25%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[elorzafe]

Minor comments, just consider on having the type for that private function on the same module.

[elorzafe]

I like this! Can you double check on our docs that we don't mention anything about it

[elorzafe]

Is weird that is inside of V1 provider. You can define an internal type, that doesn't need to be exported

[elorzafe]

Is weird to mix V2 provider type, can you just rename it to AWSLexProviderSendResponse, if is only internal I would keep it inside of the provider.

[ashwinkumar6]

oh, sorry that needs to be renamed to AWSLexProviderSendResponse.
Updated

[ashwinkumar6]

Moved these types inside provider

[lgtm-com]

This pull request introduces 1 alert when merging dadd80f into 2b54c1a - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[elorzafe]

Looks good!

Thanks @ashwinkumar6 🌮 🎉

[Samaritan1011001]

LGTM, just a few questions. Thank you

[Samaritan1011001]

Three questions here:

• Why is setTimeout with 0 secs used? It seems this is used if there is a race condition of sorts.
• onComplete callback has an error argument that is not used or am I missing it?
• Why call both the onComplete callbacks? i.e config and dynamically assigned one?

[ashwinkumar6]

• Yes that's right, we want to perform the onComplete callback async and not block the main thread
• The Error argument is passed in this case
• In the current implementation 2 independent onComplete callbacks can be added to a bot
  • via the config Interactions.config(botConfig). botConfig will have the callback
  • via the api Interactions.onComplete(botname, callback)

So we call both of them if configured, probably as a breaking change we could make both these ways of adding callbacks refer to the same callback