Skip to content

Add on_teams_team_renamed "overload" #1418

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

Merged
merged 2 commits into from
Oct 22, 2020
Merged

Add on_teams_team_renamed "overload" #1418

merged 2 commits into from
Oct 22, 2020

Conversation

mdrichardson
Copy link
Contributor

Fixes #1405

Description

on_teams_team_renamed_activity() was mistakenly named against the pattern of the rest of the activity handlers (which don't end in "activity"`

Specific Changes

  • Add overload method called on_teams_team_renamed()
  • Add deprecation notice for on_teams_team_renamed_activity()

@@ -529,7 +529,7 @@ async def on_conversation_update_activity(self, turn_context: TurnContext):
channel_data.channel, channel_data.team, turn_context
)
if channel_data.event_type == "teamRenamed":
return await self.on_teams_team_renamed_activity(
return await self.on_teams_team_renamed(
Copy link
Contributor Author

@mdrichardson mdrichardson Oct 22, 2020

Choose a reason for hiding this comment

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

This was actually a little trickier to implement than just using a simple overload because of this line. One of on_teams_team_renamed_activity() and on_teams_team_renamed() has to call the other (line 615). I tried to consider how this impacts backwards-compat (below), but please look it over and think it through, letting me know if I missed anything.

Regarding backwards-compat, there's really two ways to do this and I'm open to changing this

1. (current PR) Have on_conversation_update_activity() call on_teams_team_renamed()

The flow works like this:

  1. Activity comes in to on_conversation_update_activity()
  2. on_conversation_update_activity() calls on_teams_team_renamed()
  3. on_teams_team_renamed() calls on_teams_team_renamed_activity(), if it hasn't been overridden

Works for developers who:

  • Have already overridden on_conversation_update_activity()
  • Have already overridden on_teams_team_renamed_activity()
  • Have already overridden both on_conversation_update_activity() and on_teams_team_renamed_activity()
  • Are writing a new bot and simply want to override on_teams_team_renamed()

Does NOT work for developers who:

  • Have created a method on_teams_team_renamed() AND overridden on_teams_team_renamed_activity(), since on_conversation_update_activity() will no longer call on_teams_team_renamed_activity()
    • This is pretty unlikely since creating on_teams_team_renamed() would have accomplished nothing.

1. (alternative PR) Have on_conversation_update_activity() call on_teams_team_renamed_**activity**()

The difference is basically that there are no changes made to on_conversation_update_activity() and on_teams_team_renamed_activity() calls on_teams_team_renamed()

The flow would work like this:

  1. Activity comes in to on_conversation_update_activity()
  2. on_conversation_update_activity() calls on_teams_team_renamed_**activity**()
  3. on_teams_team_renamed_activity() calls on_teams_team_renamed(), if it hasn't been overridden

Works for developers who:

  • Have already overridden on_conversation_update_activity()
  • Have already overridden on_teams_team_renamed_activity()
  • Have already overridden both on_conversation_update_activity() and on_teams_team_renamed_activity()
  • Are writing a new bot and simply want to override on_teams_team_renamed()

Does NOT work for developers who:

  • Works for all since if the developer has created on_teams_team_renamed(), they're already going to use it how they please.

Summary

I went with the first option because it keeps on_conversation_update_activity() aligned through all the methods so that if future developers override it, it makes more "sense". However, option #1 also has potential backwards-compat issues, albeit very minimal. Open to changing things.

@mdrichardson mdrichardson requested a review from axelsrz October 22, 2020 16:53
@tracyboehrer tracyboehrer merged commit 6daed05 into microsoft:main Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teams added, deleted and renamed API quite confusing?
3 participants