Microsoft Teams Integration Option

Review Request #11227 — Created Oct. 16, 2020 and updated

hailan
rbintegrations
master
rbintegrations, students

Added the Microsoft Teams Integration Option. Like the existing Slack
Integration option, this integration will post a message to an MS Teams chat
channel after a review request has been updated.

MS Teams icons are acquired from Adaptive Cards' main webpage:
https://adaptivecards.io/

Unit tests passed and manually tested it in RB3.0 and 4.0.
Summary Author
Microsoft Teams Integration Option
HailanXyouknow
Loading file attachments...

Description From Last Updated

How about renaming the module from microsoftteams to msteams? It'll be shorter and imports won't have to wrap.

chipx86chipx86

E501 line too long (172 > 79 characters)

reviewbotreviewbot

E501 line too long (210 > 79 characters)

reviewbotreviewbot

E262 inline comment should start with '# '

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E266 too many leading '#' for block comment

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E303 too many blank lines (3)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

E262 inline comment should start with '# '

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (210 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E501 line too long (115 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E262 inline comment should start with '# '

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

Should swap lines 10 and 13, so that they are in alphabetical order.

jblazusijblazusi

logger = logging.getLogger(__name__) should have 2 lines before and 2 lines after.

jblazusijblazusi

This could also be if, elif, else. Not sure what the mentors prefer.

jblazusijblazusi

not a must change but could also use a switch-case statement

ruonanjiaruonanjia

Comment should have a period.

jblazusijblazusi

Comment should have a period.

jblazusijblazusi

I think it is good practice to include the alt attribute for img tags in html.

jblazusijblazusi

Looks like a debug statement, we should remove these.

jblazusijblazusi

W293 blank line contains whitespace

reviewbotreviewbot

Please add a module docstring.

daviddavid

This isn't actually used (the call site is using the top-level function). However, I think what I'd do is move ...

daviddavid

What's left to do here?

daviddavid

When we wrap the docstring, we put the final """ on its own line. I think I marked all the ...

daviddavid

What's left to do here?

daviddavid

When we wrap the docstring, we put the final """ on its own line.

daviddavid

When we wrap the docstring, we put the final """ on its own line.

daviddavid

When we wrap the docstring, we put the final """ on its own line.

daviddavid

When we wrap the docstring, we put the final """ on its own line.

daviddavid

There's an extra "!" here.

daviddavid

When we wrap the docstring, we put the final """ on its own line.

daviddavid

When we wrap the docstring, we put the final """ on its own line.

daviddavid

When we wrap the docstring, we put the final """ on its own line.

daviddavid

This should end in a period (since it's not a test case docstring). Please also add "Args" and "Returns" sections.

daviddavid

Blank line after this. We ahould also just have a variable for the emoji name and code, rather than a ...

chipx86chipx86

Blank line between these.

chipx86chipx86

No need for the parens around the ... is ....

chipx86chipx86

We should use format_html(), since it's safer (the arguments going in will be HTML-escaped).

chipx86chipx86

This should use .copy() instead of dict(...).

chipx86chipx86

We standardize on lowercase utf-8.

chipx86chipx86

Docstring summaries must be one line without wrapping.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

hailan
Review request changed

Change Summary:

Started to implement changes in integration.py

Commits:

Summary Author
-
Microsoft Teams Integration Option
HailanXyouknow
+
Microsoft Teams Integration Option
HailanXyouknow

Diff:

Revision 2 (+686)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
Review request changed

Change Summary:

Finished adding all fields to payload.
Manually tested it: looks fine on desktop version of MS Teams.
On the mobile app it doesn't show the attached image_url or thumb_url.
Image

Commits:

Summary Author
-
Microsoft Teams Integration Option
HailanXyouknow
+
Microsoft Teams Integration Option
HailanXyouknow

Diff:

Revision 3 (+1588)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
hailan
hailan
Review request changed

Change Summary:

Clean up integrations.py

Description:

   

Added the Microsoft Teams Integration Option.

~   Will be adding tests.py in the next update.

  ~ MS Teams icons are acquired from Adaptive Cards' main webpage:
  + https://adaptivecards.io/

Commits:

Summary Author
-
Microsoft Teams Integration Option
HailanXyouknow
+
Microsoft Teams Integration Option
HailanXyouknow

Diff:

Revision 4 (+462)

Show changes

Removed Files:

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
Review request changed

Change Summary:

Unit tests in tests.py

Commits:

Summary Author
-
Microsoft Teams Integration Option
HailanXyouknow
+
Microsoft Teams Integration Option
HailanXyouknow

Diff:

Revision 5 (+3430)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
hailan
hailan
ruonanjia
  1. 
      
  2. not a must change but could also use a switch-case statement

  3. 
      
jblazusi
  1. I manually tested the integration, it works great!
    I also ran the automated tests and I had similar issues that the Discord Integration had, which was the 2 test failures.
    Otherwise, everything looks good to me, except for some styling. I did my best to review, although, I am not an expert on the integrations side of Review Board.

  2. Should swap lines 10 and 13, so that they are in alphabetical order.

  3. logger = logging.getLogger(__name__) should have 2 lines before and 2 lines after.

  4. This could also be if, elif, else. Not sure what the mentors prefer.

  5. Comment should have a period.

  6. Comment should have a period.

  7. I think it is good practice to include the alt attribute for img tags in html.

  8. Looks like a debug statement, we should remove these.

  9. 
      
hailan
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
hailan
hailan
david
  1. 
      
  2. Please add a module docstring.

  3. This isn't actually used (the call site is using the top-level function). However, I think what I'd do is move the contents of that function into this one, and then change the call site to use self.format_link.

  4. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    What's left to do here?

  5. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    When we wrap the docstring, we put the final """ on its own line. I think I marked all the places where this happens but please go through and check all the test cases.

  6. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    What's left to do here?

  7. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    When we wrap the docstring, we put the final """ on its own line.

  8. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    When we wrap the docstring, we put the final """ on its own line.

  9. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    When we wrap the docstring, we put the final """ on its own line.

  10. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    When we wrap the docstring, we put the final """ on its own line.

  11. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    There's an extra "!" here.

  12. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    When we wrap the docstring, we put the final """ on its own line.

  13. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    When we wrap the docstring, we put the final """ on its own line.

  14. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    When we wrap the docstring, we put the final """ on its own line.

  15. rbintegrations/microsoftteams/tests.py (Diff revision 11)
     
     

    This should end in a period (since it's not a test case docstring). Please also add "Args" and "Returns" sections.

  16. 
      
hailan
hailan
chipx86
  1. This is great! Thanks for working on this :)

    The semester is over, and you're probably ready for a break. We're ready to release very soon, so it's completely up to you whether you want to take on the review feedback here or leave it up to us.

  2. How about renaming the module from microsoftteams to msteams? It'll be shorter and imports won't have to wrap.

  3. Blank line after this.

    We ahould also just have a variable for the emoji name and code, rather than a dictionary, since they're local. It'll be faster to access a local variable than a dictionary key, and it'll look cleaner.

  4. rbintegrations/microsoftteams/integration.py (Diff revision 13)
     
     
     

    Blank line between these.

  5. No need for the parens around the ... is ....

  6. rbintegrations/microsoftteams/integration.py (Diff revision 13)
     
     
     
     

    We should use format_html(), since it's safer (the arguments going in will be HTML-escaped).

  7. This should use .copy() instead of dict(...).

  8. We standardize on lowercase utf-8.

    1. blah

    2. Oops, I thought this was localhost XD
  9. rbintegrations/microsoftteams/integration.py (Diff revision 13)
     
     
     

    Docstring summaries must be one line without wrapping.

  10. 
      
hailan
Review request changed

Change Summary:

Address feedbacks

Commits:

Summary Author
-
Microsoft Teams Integration Option
HailanXyouknow
+
Microsoft Teams Integration Option
HailanXyouknow

Diff:

Revision 14 (+3498)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...