feat: [#4349] Add new method to expose same functionality as BotFrameworkAdapter.processActivityDirect #332
Conversation
erquirogasw
requested review from
ceciliaavila,
sw-joelmut and
NahuelPerales
Pull Request Test Coverage Report for Build 3602737297
💛 - Coveralls |
| @@ -145,6 +144,35 @@ export class CloudAdapter extends CloudAdapterBase implements BotFrameworkHttpAd | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Exposes the same functionality as [BotFrameworkAdapter.processActivityDirect](https://github.com/microsoft/botbuilder-js/blob/main/libraries/botbuilder/src/botFrameworkAdapter.ts). | |||
There was a problem hiding this comment.
I don't think we should reference a deprecated method in the documentation of a new method. We should explain what the method does. In any case, we could include a comment in the old method referencing this one.
| const validatedActivity = validateAndFixActivity(ActivityT.parse(activity)); | ||
|
|
||
| if (!validatedActivity.type) { | ||
| console.warn('BadRequest: Missing activity or activity type.'); | ||
| throw new Error('Missing activity or activity type'); | ||
| } |
There was a problem hiding this comment.
This validation seems unnecessary. It was meant to validate the request's body but here you are receiving the activity instead.
| throw new Error('Missing activity or activity type'); | ||
| } | ||
|
|
||
| const authHeader = z.string().parse(authorization ?? ''); |
There was a problem hiding this comment.
I think this is not needed either. The parameter is already a string, why do we need to parse it?
| const authHeader = z.string().parse(authorization ?? ''); | ||
|
|
||
| try { | ||
| await this.processActivity(authHeader, validatedActivity, logic); |
There was a problem hiding this comment.
Shouldn't we return the invoke response?
There was a problem hiding this comment.
Regarding returning the invokeResponse, I'm having the following error. I think it's related to the end const that was created in the previous version. Anyway, in the deprecated class, only the middleware is called. In CloudAdapter, the processActivity method also does it.
| try { | ||
| await this.processActivity(authHeader, validatedActivity, logic); | ||
| } catch (err) { | ||
| throw new Error(err); |
There was a problem hiding this comment.
This doesn't have much sense. We are catching the error to throw it again. Also, be careful with stack exposure (codeQL). I'm not sure if it applies here though.
|
|
||
| const mock = sandbox.mock(adapter); | ||
| mock.expects('processActivity') | ||
| .withArgs('Bearer Authorization', { type: 'invoke', value: 'invoke' }, logic) |
There was a problem hiding this comment.
I would declare the activity at the beginning and re-use it later. This way, we don't have duplicated code, and it's also clearer that we are passing an activity as a parameter to the methods.
| await adapter.processActivityDirect('Bearer Authorization', { type: 'invoke', value: 'invoke' }, logic); | ||
|
|
||
| mock.verify(); | ||
| }); |
There was a problem hiding this comment.
Once you review the try-catch block in the method, you should probably add another test to cover the error case.
erquirogasw
changed the title
| typeof authorization === 'string' | ||
| ? await this.processActivity(authorization, validatedActivity, logic) | ||
| : await this.processActivity(authorization, validatedActivity, logic); |
There was a problem hiding this comment.
Does this if/else do the same thing?
There was a problem hiding this comment.
and also why are we throwing again the error, couldnt it just be await this.processActivity(authorization, validatedActivity, logic)?
There was a problem hiding this comment.
There are two overloads of the method. One receives a string, and the other one, an AuthenticateRequestResult. Regarding the error, there's pending feedback to be applied there.
| * Asynchronously creates a turn context and runs the middleware pipeline for an incoming activity. | ||
| * | ||
| * @param authorization Authorization in format: "Bearer [longString]". |
There was a problem hiding this comment.
| * Asynchronously creates a turn context and runs the middleware pipeline for an incoming activity. | |
| * | |
| * @param authorization Authorization in format: "Bearer [longString]". | |
| * Asynchronously process an activity running the provided logic function. | |
| * | |
| * @param authorization The authorization header in the format: "Bearer [longString]" or the AuthenticateRequestResult for this turn. |
| new Error('CloudAdapter.processActivityDirect(): ERROR\n error stack') | ||
| ); | ||
|
|
||
| sandbox.verify(); |
erquirogasw
force-pushed
the
southworks/add/processActivityDirect-functionality-to-CloudAdapter
branch
from
31fe506 to
cd7f7d5
Compare
erquirogasw
force-pushed
the
southworks/add/processActivityDirect-functionality-to-CloudAdapter
branch
from
cd7f7d5 to
7ade736
Compare
|
Promoted 4380 |
ceciliaavila
deleted the
southworks/add/processActivityDirect-functionality-to-CloudAdapter
branch
Fixes # 4349
#minor
Description
This PR exposes the same functionality as the processActivityDirect method of the deprecated BotFrameworkAdapter class. For that, adds a new method that doesn't use HTTP req and res parameters.
Specific Changes
Testing