fix: [#4202][botbuilder] TeamsActivityHandler — Prevent calling event handlers twice

[alexrecuenco]

Fixes #4202

Description

This change prevents calling TeamsActivityHandler event handlers twice for every event received.

Specific Changes

Why were the event handlers getting called twice?

• teamsActivityHandler overwrites dispatchEventActivity
• dispatchEventActivity is only called from within onEventActivity
• onEventActivity already dispatches after calling all handlers
• on teamsActivityHandler, when overwriting, it is also calling all handlers

How to fix it?

• Prevent the handler call inside the dispatchEventHandler
• This should have no effect on any consumer, since they should already be consuming only from within a onEventActivity call.

Testing

A test is added to prevent the behavior from reappearing

Note that to test the issue described, it is enough to run the changed test in main.
The test will fail when run in main currently, since since it is called twice.

[coveralls]

Pull Request Test Coverage Report for Build 2401123034

• 26 of 26 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.5%) to 84.48%

---

Totals
Change from base Build 2387271645:	0.5%
Covered Lines:	19900
Relevant Lines:	22308

---

💛 - Coveralls

[johnataylor]

The original code appears to be the same code as is used to handle the ConversationUpdate activity.

Can you also verify that the behavior of ConversationUpdate? And if it has the same bug. And apply the fix and add a test for that there if necessary.

If this is a bug here and not in the ConversationUpdate scenario then why is that?

[alexrecuenco]

It appears you are right @johnataylor, I adjusted the code accordingly.

I reviewed other activity types, and I don't think they are vulnerable to the same issue.

However, I wonder if we should consider adding a test that runs for each activity type and makes sure they don't have double calls.

[alexrecuenco]

If those extra tests are wanted, I added an experimental branch, experimental/fix-double-call that contains an automated mechanism to test all event types that use the onEventType(handler) structure

[johnataylor]

Thanks for adding the extra test for conversationUpdate is good.

But if you are saying you can verify this isn't a problem in other cases that would be great.

[alexrecuenco]

I wrote some extra tests to verify all others, should I include them?

I am not sure if these tests should be included, since the style of test factory is something I have not seen across botbuilder-s.

[johnataylor]

@alexrecuenco can you send me a point to what you mean by test factory and I'll ask a couple of other folks what they think.

As a general statement, we don't like too many different ways to do things, but if something makes sense and improves coverage then we would always consider it.

[johnataylor]

and we can always consider adding in the extra tests as a separate PR

at this stage the main concern is around this specific bug, and you are confident that this isn't a wider issue?

[alexrecuenco]

I added an extra commit.

Basically, I just mean writing tests inside a for loop dynamically. (You could extract the content of the for loop to a function, making that function a kind of factory)

for(const t of types) {
 it(`should use ${t}`, ...)
}

[alexrecuenco]

But yes, I have done some further testing as demonstrated on that third commit and believe it is not happening anywhere else for the TeamsActivityHandler.

I think it was just an issue with how overwriting a superclass method made it confusing, loosing track of whether the handler chain was called.

Besides, we saw this issue the first time we used a OnEvent handler. It is really hard to miss.

[alexrecuenco]

Hmmm

Error: .github/workflows/tests.yml (Line: 41, Col: 16):
Error: The template is not valid. .github/workflows/tests.yml (Line: 41, Col: 16): hashFiles('**/yarn.lock') couldn't finish within 120 seconds.

I am not sure what that error is referring to. But I will revert back, I guess.

[ghost]

All CLA requirements met.

[alexrecuenco]

Hey @johnataylor ,
Does this PR look good to you or do I need to adjust something else?

(I just rebased)

[gabog]

Hi @EricDahlvang, would you have some time to review this PR?

Thanks

[EricDahlvang]

Hi @alexrecuenco

Thank you for finding this and providing a fix! Please do include your test for the generic 'ActivityTypes handlers should not be called twice'

[alexrecuenco]

Hmmm

Error: .github/workflows/tests.yml (Line: 41, Col: 16):
Error: The template is not valid. .github/workflows/tests.yml (Line: 41, Col: 16): hashFiles('**/yarn.lock') couldn't finish within 120 seconds.

I am not sure what that error is referring to. But I will revert back, I guess.

I got this weird error when trying to create a for loop that would create tests automatically for each type.

Are those tests necessary to prevent regressions in the future? In that case, I would need some help to understand how to prevent this issue. (Maybe it is an issue unrelated to my tests?)