add bug fix for #1404, more tests

libraries/botbuilder-core/src/transcriptLogger.ts

@@ -47,8 +47,24 @@ export class TranscriptLoggerMiddleware implements Middleware {
 
         // hook up onSend pipeline
         context.onSendActivities(async (ctx: TurnContext, activities: Partial<Activity>[], next2: () => Promise<ResourceResponse[]>) => {
-            // run full pipeline
-            const responses: ResourceResponse[] = await next2();
+            // Run full pipeline and create a pointer to the return value from the pipeline.
+            let nextValue: ResourceResponse[] = await next2();
+            let responses: ResourceResponse[] = nextValue;
+
+            // If the return value from the pipeline is not an Array, point `responses` to an array of empty ResourceResponses.
+            // This allows for the following:
+            //      1. Allows the transcript logging to progress and not error out.
+            //         See https://github.com/microsoft/botbuilder-js/issues/1404
+            //      1. This prevents TranscriptLoggers from throwing an error if no activity.id is set.
+            //         See https://github.com/microsoft/botbuilder-js/issues/1122
+            //      2. By mocking an array of ResourceResponses, we still return the value from `next2()`.
+            //         This prevents any pre-4.6.0 bots with multiple SendActivityHandlers that rely
+            //         on values that are not an array of ResourceResponses.
+            if (!Array.isArray(responses)) {
+                responses = activities.map(() => {
+                    return {} as ResourceResponse;
+                });
+            }
 
             activities.map((a: Partial<Activity>, index: number) => {
                 const clonedActivity = this.cloneActivity(a);
@@ -72,7 +88,7 @@ export class TranscriptLoggerMiddleware implements Middleware {
                 this.logActivity(transcript, clonedActivity);
             });
 
-            return responses;
+            return nextValue;
         });
 
         // hook up update activity pipeline

libraries/botbuilder-core/tests/transcriptMiddleware.test.js

@@ -235,6 +235,100 @@ describe(`TranscriptLoggerMiddleware`, function () {
         });
     });
 
+    it(`should not error for sent activities if another handler does not return an array`, async () => {
+        class NoResourceResponseMiddleware {
+            async onTurn(context, next) {
+                context.onSendActivities(async (context, activities, next) => {
+                    // In SendActivitiesHandlers developers are supposed to call:
+                    //      return next();
+                    // If this is not returned, then the next handler will not have the ResourceResponses[].
+                    const responses = await next();
+
+                    return {};
+                });
+
+                // Run the bot's application logic.
+                await next();
+            }
+        }
+
+        let conversationId = null;
+        const transcriptStore = new MemoryTranscriptStore();
+        const adapter = new TestAdapter(async (context) => {
+            conversationId = context.activity.conversation.id;
+            await context.sendActivity(`echo:${context.activity.text}`);
+        });
+
+        // Register both middleware
+        adapter.use(new TranscriptLoggerMiddleware(transcriptStore));
+        adapter.use(new NoResourceResponseMiddleware());
+
+        await adapter.send('foo')
+            .assertReply('echo:foo');
+
+        const pagedResult = await transcriptStore.getTranscriptActivities('test', conversationId);
+        assert.equal(pagedResult.items.length, 2);
+        assert.equal(pagedResult.items[0].text, 'foo');
+        assert.equal(pagedResult.items[1].text, 'echo:foo');
+        pagedResult.items.forEach(a => {
+            assert(a.id);
+            assert(a.timestamp);
+        });
+    });
+
+    it(`should not error when logging sent activities and return the actual value from next()`, async () => {
+        // This middleware should receive 1 from `next()`
+        class AssertionMiddleware {
+            async onTurn(context, next) {
+                context.onSendActivities(async (context, activities, next) => {
+                    const notResourceResponses = await next();
+                    assert.strictEqual(notResourceResponses, 1);
+                });
+
+                await next();
+            }
+        }
+        // This middleware returns the value 1 from its registered SendActivitiesHandler.
+        // The TranscriptLoggerMiddleware should return this value to the next registered SendActivitiesHandler.
+        class Returns1Middleware {
+            async onTurn(context, next) {
+                context.onSendActivities(async (context, activities, next) => {
+                    // In SendActivitiesHandlers developers are supposed to call:
+                    //      return next();
+                    // If this is not returned, then the next handler will not have the ResourceResponses[].
+                    const responses = await next();
+
+                    return 1;
+                });
+
+                await next();
+            }
+        }
+
+        let conversationId = null;
+        const transcriptStore = new MemoryTranscriptStore();
+        const adapter = new TestAdapter(async (context) => {
+            conversationId = context.activity.conversation.id;
+            await context.sendActivity(`echo:${context.activity.text}`);
+        });
+
+        adapter.use(new AssertionMiddleware());
+        adapter.use(new TranscriptLoggerMiddleware(transcriptStore));
+        adapter.use(new Returns1Middleware());
+
+        await adapter.send('foo')
+            .assertReply('echo:foo');
+
+        const pagedResult = await transcriptStore.getTranscriptActivities('test', conversationId);
+        assert.equal(pagedResult.items.length, 2);
+        assert.equal(pagedResult.items[0].text, 'foo');
+        assert.equal(pagedResult.items[1].text, 'echo:foo');
+        pagedResult.items.forEach(a => {
+            assert(a.id);
+            assert(a.timestamp);
+        });
+    });
+
     describe('\'s error handling', function () {
         const originalConsoleError = console.error;
 
@@ -340,10 +434,11 @@ describe(`TranscriptLoggerMiddleware`, function () {
                     })
                     done();
                 });
-            });
+            })
+            .catch(err => done(err));
     });
 
-    it(`should use outgoing activity's timestamp for activity id when activity id and resourceResponse is empty`, function(done) {
+    it(`should use outgoing activity's timestamp for activity id when activity id and resourceResponse is empty`, async () => {
         let conversationId, timestamp;
         const transcriptStore = new MemoryTranscriptStore();
 
@@ -364,34 +459,30 @@ describe(`TranscriptLoggerMiddleware`, function () {
             text: 'foo'
         };
 
-        adapter
-            .send(fooActivity)
+        await adapter.send(fooActivity)
             // sent activities do not contain the id returned from the service, so it should be undefined here
             .assertReply(activity => {
                 assert.equal(activity.id, undefined);
                 assert.equal(activity.text, 'echo:foo');
                 assert.equal(activity.timestamp, timestamp);
-            }) 
-            .then(() => {
-                transcriptStore.getTranscriptActivities('test', conversationId).then(pagedResult => {
-                    assert.equal(pagedResult.items.length, 2);
-                    assert.equal(pagedResult.items[0].text, 'foo');
-                    // Transcript activities should have the id present on the activity when received
-                    assert.equal(pagedResult.items[0].id, 'testFooId');
-
-                    assert.equal(pagedResult.items[1].text, 'echo:foo');
-                    // Sent Activities in the transcript store should use the timestamp on the bot's outgoing activities
-                    // to log the activity when the following cases are true:
-                    // 1. The outgoing Activity.id is falsey
-                    // 2. The ResourceResponse.id is falsey (some channels may not emit a ResourceResponse with an id value)
-                    // 3. The outgoing Activity.timestamp exists
-                    assert.equal(pagedResult.items[1].id, timestamp.getTime().toString());
-                    pagedResult.items.forEach(a => {
-                        assert(a.timestamp);
-                    });
-                    done();
-                });
             });
+
+        const pagedResult = await transcriptStore.getTranscriptActivities('test', conversationId);
+        assert.equal(pagedResult.items.length, 2);
+        assert.equal(pagedResult.items[0].text, 'foo');
+        // Transcript activities should have the id present on the activity when received
+        assert.equal(pagedResult.items[0].id, 'testFooId');
+
+        assert.equal(pagedResult.items[1].text, 'echo:foo');
+        // Sent Activities in the transcript store should use the timestamp on the bot's outgoing activities
+        // to log the activity when the following cases are true:
+        // 1. The outgoing Activity.id is falsey
+        // 2. The ResourceResponse.id is falsey (some channels may not emit a ResourceResponse with an id value)
+        // 3. The outgoing Activity.timestamp exists
+        assert.equal(pagedResult.items[1].id, timestamp.getTime().toString());
+        pagedResult.items.forEach(a => {
+            assert(a.timestamp);
+        });
     });
 
     it(`should use current server time for activity id when activity and resourceResponse id is empty and no activity timestamp exists`, function(done) {