Add an integration for Microsoft Teams.

Review Request #13742 — Created April 22, 2024 and submitted

Information

rbintegrations
release-4.x

Reviewers

This adds an integration for Microsoft Teams. Similar to our other chat
service integrations, this sends messages to channels in Microsoft Teams when
review requests are created, updated, and reviewed.

We use Microsoft's Adaptive Card format for the messages. Unlike our other chat
integrations, with MS Teams there's unfortunately no ability to add an accent
colour to the message (typically we use yellow, green, and red accents as
a visual indicator for the tone of a message). The styling of the messages
also differ between the MS teams desktop and mobile apps, but this is a
choice by Microsoft and out of our control.

Based on work by Hailan Xu (/r/11227/).

Ran unit tests.

Manually tested on RB5, RB6 and RB7:
- Creating the integration.
- New review requests, with/without diffs and files.
- Updates to review requests.
- Reviews with/without comments, issues, ship its.
- Replies.
- Tested that text which contains Markdown characters
gets properly escaped when being formatted in a link
in a message.
- Tested that URLs which contain ( or ) gets properly
escaped when being formatted in a link in a message.

Summary ID
Add an integration for Microsoft Teams.
26ac22edc2755ce6002f5946f5f69543858fc9bd

Description From Last Updated

Typo in the description on the second line: "intergrations" -> "integrations".

chipx86chipx86

Hmm. Is it possible to scale down the logo here so it doesn't get cropped?

daviddavid

This doesn't have to wrap.

daviddavid

For things that are purely annotations, we can use list instead of List (as long as we make sure to …

daviddavid

We can use dict here. Why the type: ignore?

daviddavid

This is missing a Version Added.

chipx86chipx86

As currently set up, we're including the URL as part of the localized string. Maybe that's okay, but it's also …

chipx86chipx86

Extra space after the comma.

chipx86chipx86

Missing Version Added.

chipx86chipx86

Mentioned this in the base typing change, but we should opt to use Sequence unless we need to mutate a …

chipx86chipx86

What's the type checker upset about? If it's thinking it may be a Sequence and not a List, then maybe …

chipx86chipx86

Let's sort these alphabetically.

chipx86chipx86

While I don't think we do this elsewhere, we may want to think about issues surrounding either the text or …

chipx86chipx86

4.0

chipx86chipx86

Missing Version Added.

chipx86chipx86

line too long (81 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (81 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

To avoid the double-pass through the string, we can do this: escape_map = { '(': '%28', ')': '%29', } path …

chipx86chipx86
david
  1. 
      
  2. Show all issues

    Hmm. Is it possible to scale down the logo here so it doesn't get cropped?

    1. This logo is set by the person who sets up the Incoming Webhook in Microsoft Teams, it can't be set on our end by the RB integration. In my case I just uploaded our logo via the Microsoft Teams app. The admin could choose to not upload a photo, or upload a scaled down one, etc.

    2. Ahh. Perhaps we can provide a recommended image for people?

    3. That's a good idea. I'll try to do this in the docs.

  3. rbintegrations/msteams/forms.py (Diff revision 1)
     
     
     
    Show all issues

    This doesn't have to wrap.

  4. rbintegrations/msteams/integration.py (Diff revision 1)
     
     
    Show all issues

    For things that are purely annotations, we can use list instead of List (as long as we make sure to import from __future__)

  5. rbintegrations/msteams/integration.py (Diff revision 1)
     
     
    Show all issues

    We can use dict here.

    Why the type: ignore?

    1. On the djblets.integrations.Integration parent class, icon_static_urls is defined as an attribute instead of a method:

      icon_static_urls: Dict[str, str] = {}

      Mypy was giving me a "signature incompatible with supertype" error. I saw in RB we have a similar attribute -> method situation on the reviewboard.hostingsvcs.base.paginator.ProxyPaginator class, and there we use # type: ignore as well. I can add a comment here explaining why we do this.

    2. OK, yeah, I've seen that. It seems like python hasn't yet solved the question of how typing should work for things that can either be a plain attribute or a property.

    3. It's not that they haven't solved it, it's that it's considered explicitly to be type-unsafe and discouraged. Although that's an in-theory thing and sadly not practical.

      It's considered a violation of the Liskov Substitution Principle (LSP), which type checkers are built around. The idea there is that you should always be able to substitute in an instance of a subclass in place of a parent of a class without breaking the program.

      When you type attributes, you should be able to have the same types and behavior whether you're the class that defines the attribute or any subclass of that class.

      As soon as you introduce a @property, you violate this. You now have complexity that may or may not guarantee compliance with LSP.

      Three examples would be:

      1. A @property with no setter (you've now removed the ability to set the attribute)
      2. A @property with a setter where the value you set and the value you get are different (e.g., normalization is conducted)
      3. A @property with a setter where the setter and getter work with different types.

      Type checkers can't make guarantees here, so they just flat-out consider this to be a violation of the type.

      That means that, to be compliant, we'd need to rethink some of how we tend to structure our classes. We may want to start moving toward get_*() methods that default to accessing a variable, for instance. Or we may just say, screw it, the theoretical shouldn't impact our practical.

  6. 
      
maubin
maubin
david
  1. Ship It!
  2. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    Typo in the description on the second line: "intergrations" -> "integrations".

  3. rbintegrations/msteams/forms.py (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    This is missing a Version Added.

  4. rbintegrations/msteams/forms.py (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    As currently set up, we're including the URL as part of the localized string. Maybe that's okay, but it's also kind of annoying for localization. But then, we can't lazily-evaluate this here.

    (Also I don't trust this URL to be permanently correct.)

    Wonder if we should continue to do this but link to the integrations entry and have a section on this there instead, which could at least link to the above URL.

    1. Yeah, I can link to our MS Teams integration docs here instead of the MS_TEAMS_WEBHOOK_DOCS_URL.

      In our docs we currently link to this same MS_TEAMS_WEBHOOK_DOCS_URL, which I'd like to keep there because otherwise we have to copy over a lot of information on how to set up the Incoming Webhook in Microsoft Teams (there are steps for the "classic" version of teams, and other steps for the "new" version).

      Plus if the steps ever change, our information will be outdated and we may not notice for a while. In this case people would try our steps, find out that they're wrong, and then seek out the real MS teams docs. If we simply link to the MS Teams docs, and the link is not correct anymore, then people will still have seek out the real MS teams docs, but will do this right away instead of wasting time reading/following our outdated steps first.

    2. I think whatever we link to can just point to Microsoft's docs, instead of duplicating the information there. The important part is to have a stable URL we can control and update, so we don't have to release new builds when Microsoft's URL changes. I'm good with not trying to walk users through this process ourselves.

    3. Oh I think I see what you're saying now. So we should link to our https://www.reviewboard.org/integrations/msteams/ entry instead of https://www.reviewboard.org/docs/manual/latest/admin/integrations/msteams/, because with the latter if the Microsoft URL changes we'd need to do a Review Board release to fix it, but with the former we can just update the rb website right away?

    4. Yeah, and because for the manual we'd need to specify the exact version of the manual, which breaks localization whenever the version changes (it impacts the string to localize).

      We can use an anchor on the integrations page entry to point to some section pointing to the instructions.

      Ideally, what we'd really want is some mechanism on our site for doing redirects. I've wanted that for ages. We just don't have anything for that today.

    5. Got it. Maybe then I'll change the wording to "See here for more information" instead of "See the documentation for more information".

  5. rbintegrations/msteams/forms.py (Diff revision 4)
     
     
    Show all issues

    Extra space after the comma.

  6. rbintegrations/msteams/integration.py (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    Missing Version Added.

  7. rbintegrations/msteams/integration.py (Diff revision 4)
     
     
    Show all issues

    Mentioned this in the base typing change, but we should opt to use Sequence unless we need to mutate a list.

  8. rbintegrations/msteams/integration.py (Diff revision 4)
     
     
    Show all issues

    What's the type checker upset about?

    If it's thinking it may be a Sequence and not a List, then maybe it's worth putting together the column as a JSONList first, and then building pre_text_card using those.

  9. rbintegrations/msteams/integration.py (Diff revision 4)
     
     
     
    Show all issues

    Let's sort these alphabetically.

  10. rbintegrations/msteams/integration.py (Diff revision 4)
     
     
    Show all issues

    While I don't think we do this elsewhere, we may want to think about issues surrounding either the text or the URL breaking out of the [...](...). We may have to escape them.

  11. rbintegrations/msteams/tests.py (Diff revision 4)
     
     
     
    Show all issues

    4.0

  12. rbintegrations/msteams/tests.py (Diff revision 4)
     
     
    Show all issues

    Missing Version Added.

  13. 
      
maubin
Review request changed

Change Summary:

  • Updated comment about ignoring type for icon_static_urls (attribute -> property thing).
  • Link to our own docs in MS Teams integration config pages.
  • Changed the way I build pre_text_card to satisfy type checker.
  • Escapes the text and URL when formatting Markdown links.

Description:

   

This adds an integration for Microsoft Teams. Similar to our other chat

~   service intergrations, this sends messages to channels in Microsoft Teams when
  ~ service integrations, this sends messages to channels in Microsoft Teams when
    review requests are created, updated, and reviewed.

   
   

We use Microsoft's Adaptive Card format for the messages. Unlike our other chat

    integrations, with MS Teams there's unfortunately no ability to add an accent
    colour to the message (typically we use yellow, green, and red accents as
    a visual indicator for the tone of a message). The styling of the messages
    also differ between the MS teams desktop and mobile apps, but this is a
    choice by Microsoft and out of our control.

   
~  

Based on work by Hailan Xu (/r/11227/)

  ~

Based on work by Hailan Xu (/r/11227/).

Testing Done:

   

Ran unit tests.

   
   

Manually tested on RB5, RB6 and RB7:

    - Creating the integration.
    - New review requests, with/without diffs and files.
    - Updates to review requests.
    - Reviews with/without comments, issues, ship its.
~   - Replies.

  ~ - Replies.
  + - Tested that text which contains Markdown characters
  + gets properly escaped when being formatted in a link
  + in a message.
  + - Tested that URLs which contain ( or ) gets properly
  + escaped when being formatted in a link in a message.

Commits:

Summary ID
Add an integration for Microsoft Teams.
31d6dc67e6540b6e7ee03df834a3a1916d12a4f3
Add an integration for Microsoft Teams.
c1886bd3777b5e091f361f410e28758614bbd960

Diff:

Revision 5 (+3384 -16)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
  1. 
      
  2. 
      
maubin
david
  1. Ship It!
  2. 
      
maubin
chipx86
  1. This looks great! I have just one suggestion.

  2. rbintegrations/msteams/integration.py (Diff revision 7)
     
     
    Show all issues

    To avoid the double-pass through the string, we can do this:

    escape_map = {
        '(': '%28',
        ')': '%29',
    }
    
    path = re.sub(
        '[()]',
        lambda: escape_map[m.group(0)],
        path)
    
  3. 
      
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (fb002b0)
Loading...