• 
      

    Microsoft Teams Integration Option

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

    Information

    rbintegrations
    master

    Reviewers

    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 ID Author
    Microsoft Teams Integration Option
    fd6fe6409a66b76ca6d933ae842651a12fcbe6d8 HailanXyouknow

    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 ID Author
    Microsoft Teams Integration Option
    63b98a256884f896119f9d618224cd8ef25dd2c0 HailanXyouknow
    Microsoft Teams Integration Option
    5bdd71e79b0b216d6a0fca4e18dc4a3a7d622faf HailanXyouknow

    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 ID Author
    Microsoft Teams Integration Option
    5bdd71e79b0b216d6a0fca4e18dc4a3a7d622faf HailanXyouknow
    Microsoft Teams Integration Option
    284e4f830502d90dfa78854ff579287cb63c0346 HailanXyouknow

    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 ID Author
    Microsoft Teams Integration Option
    284e4f830502d90dfa78854ff579287cb63c0346 HailanXyouknow
    Microsoft Teams Integration Option
    05ab88185485763b58aa3f6fc63a8accc7816ebb HailanXyouknow
    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 ID Author
    Microsoft Teams Integration Option
    05ab88185485763b58aa3f6fc63a8accc7816ebb HailanXyouknow
    Microsoft Teams Integration Option
    804baf13e274da399d79ae09f8b3e79c13a8f9d7 HailanXyouknow

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    hailan
    Review request changed
    Change Summary:
    Update tests.py
    Commits:
    Summary ID Author
    Microsoft Teams Integration Option
    804baf13e274da399d79ae09f8b3e79c13a8f9d7 HailanXyouknow
    Microsoft Teams Integration Option
    b3952112d0fd5c806cc84a688da237e9089ffc85 HailanXyouknow

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    hailan
    hailan
    hailan
    ruonanjia
    1. 
        
    2. Show all issues

      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. Show all issues

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

    3. Show all issues

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

    4. Show all issues

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

    5. Show all issues

      Comment should have a period.

    6. Show all issues

      Comment should have a period.

    7. Show all issues

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

    8. Show all issues

      Looks like a debug statement, we should remove these.

    9. 
        
    hailan
    Review request changed
    Change Summary:
    Address Ruonan and Jacob's feedbacks
    Commits:
    Summary ID Author
    Microsoft Teams Integration Option
    8ca4af09382a6baa1af145ef49aa7d7bc82fbcf5 HailanXyouknow
    Microsoft Teams Integration Option
    bc2825462f681893720180b059ac596376661f75 HailanXyouknow

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    hailan
    hailan
    hailan
    david
    1. 
        
    2. Show all issues

      Please add a module docstring.

    3. Show all issues

      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)
       
       
      Show all issues

      What's left to do here?

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

      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)
       
       
      Show all issues

      What's left to do here?

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

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

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

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

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

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

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

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

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

      There's an extra "!" here.

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

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

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

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

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

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

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

      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. Show all issues

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

    3. Show all issues

      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)
       
       
       
      Show all issues

      Blank line between these.

    5. Show all issues

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

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

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

    7. Show all issues

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

    8. Show all issues

      We standardize on lowercase utf-8.

      1. blah

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

      Docstring summaries must be one line without wrapping.

    10. 
        
    hailan
    maubin
    Review request changed
    Status:
    Discarded
    Change Summary:

    Moved to /r/13742/.