Microsoft Teams Integration Option
Review Request #11227 — Created Oct. 16, 2020 and discarded
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 |
---|---|---|
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. |
chipx86 | |
E501 line too long (172 > 79 characters) |
reviewbot | |
E501 line too long (210 > 79 characters) |
reviewbot | |
E262 inline comment should start with '# ' |
reviewbot | |
W292 no newline at end of file |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E266 too many leading '#' for block comment |
reviewbot | |
E231 missing whitespace after ':' |
reviewbot | |
E501 line too long (81 > 79 characters) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E303 too many blank lines (3) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E231 missing whitespace after ':' |
reviewbot | |
E262 inline comment should start with '# ' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E303 too many blank lines (2) |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E501 line too long (81 > 79 characters) |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E501 line too long (210 > 79 characters) |
reviewbot | |
E501 line too long (81 > 79 characters) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E122 continuation line missing indentation or outdented |
reviewbot | |
E122 continuation line missing indentation or outdented |
reviewbot | |
E122 continuation line missing indentation or outdented |
reviewbot | |
E122 continuation line missing indentation or outdented |
reviewbot | |
E501 line too long (115 > 79 characters) |
reviewbot | |
E501 line too long (91 > 79 characters) |
reviewbot | |
E122 continuation line missing indentation or outdented |
reviewbot | |
E262 inline comment should start with '# ' |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E501 line too long (86 > 79 characters) |
reviewbot | |
E501 line too long (81 > 79 characters) |
reviewbot | |
E501 line too long (83 > 79 characters) |
reviewbot | |
Should swap lines 10 and 13, so that they are in alphabetical order. |
jblazusi | |
logger = logging.getLogger(__name__) should have 2 lines before and 2 lines after. |
jblazusi | |
This could also be if, elif, else. Not sure what the mentors prefer. |
jblazusi | |
not a must change but could also use a switch-case statement |
ruonanjia | |
Comment should have a period. |
jblazusi | |
Comment should have a period. |
jblazusi | |
I think it is good practice to include the alt attribute for img tags in html. |
jblazusi | |
Looks like a debug statement, we should remove these. |
jblazusi | |
W293 blank line contains whitespace |
reviewbot | |
Please add a module docstring. |
david | |
This isn't actually used (the call site is using the top-level function). However, I think what I'd do is move … |
david | |
What's left to do here? |
david | |
When we wrap the docstring, we put the final """ on its own line. I think I marked all the … |
david | |
What's left to do here? |
david | |
When we wrap the docstring, we put the final """ on its own line. |
david | |
When we wrap the docstring, we put the final """ on its own line. |
david | |
When we wrap the docstring, we put the final """ on its own line. |
david | |
When we wrap the docstring, we put the final """ on its own line. |
david | |
There's an extra "!" here. |
david | |
When we wrap the docstring, we put the final """ on its own line. |
david | |
When we wrap the docstring, we put the final """ on its own line. |
david | |
When we wrap the docstring, we put the final """ on its own line. |
david | |
This should end in a period (since it's not a test case docstring). Please also add "Args" and "Returns" sections. |
david | |
Blank line after this. We ahould also just have a variable for the emoji name and code, rather than a … |
chipx86 | |
Blank line between these. |
chipx86 | |
No need for the parens around the ... is .... |
chipx86 | |
We should use format_html(), since it's safer (the arguments going in will be HTML-escaped). |
chipx86 | |
This should use .copy() instead of dict(...). |
chipx86 | |
We standardize on lowercase utf-8. |
chipx86 | |
Docstring summaries must be one line without wrapping. |
chipx86 |
- Change Summary:
-
Started to implement changes in integration.py
- Commits:
-
Summary ID Author 63b98a256884f896119f9d618224cd8ef25dd2c0 HailanXyouknow 5bdd71e79b0b216d6a0fca4e18dc4a3a7d622faf HailanXyouknow
Checks run (1 failed, 1 succeeded)
flake8
- 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.
- Commits:
-
Summary ID Author 5bdd71e79b0b216d6a0fca4e18dc4a3a7d622faf HailanXyouknow 284e4f830502d90dfa78854ff579287cb63c0346 HailanXyouknow - Diff:
-
Revision 3 (+1588)
Checks run (1 failed, 1 succeeded)
flake8
- 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 284e4f830502d90dfa78854ff579287cb63c0346 HailanXyouknow 05ab88185485763b58aa3f6fc63a8accc7816ebb HailanXyouknow - Diff:
-
Revision 4 (+462)
- Removed Files:
- Added Files:
- Change Summary:
-
Unit tests in tests.py
- Commits:
-
Summary ID Author 05ab88185485763b58aa3f6fc63a8accc7816ebb HailanXyouknow 804baf13e274da399d79ae09f8b3e79c13a8f9d7 HailanXyouknow
- Change Summary:
-
Update tests.py
- Commits:
-
Summary ID Author 804baf13e274da399d79ae09f8b3e79c13a8f9d7 HailanXyouknow b3952112d0fd5c806cc84a688da237e9089ffc85 HailanXyouknow - Diff:
-
Revision 6 (+3528)
- Commits:
-
Summary ID Author b3952112d0fd5c806cc84a688da237e9089ffc85 HailanXyouknow 8ca4af09382a6baa1af145ef49aa7d7bc82fbcf5 HailanXyouknow - Diff:
-
Revision 7 (+3532)
Checks run (2 succeeded)
-
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. -
-
-
-
-
-
-
- Change Summary:
-
Address Ruonan and Jacob's feedbacks
- Commits:
-
Summary ID Author 8ca4af09382a6baa1af145ef49aa7d7bc82fbcf5 HailanXyouknow bc2825462f681893720180b059ac596376661f75 HailanXyouknow - Diff:
-
Revision 8 (+3526)
- Change Summary:
-
Address white space
- Commits:
-
Summary ID Author bc2825462f681893720180b059ac596376661f75 HailanXyouknow b5cad1541c82eaa81a6b3f160561ec282e7740fd HailanXyouknow - Diff:
-
Revision 9 (+3526)
Checks run (2 succeeded)
- Change Summary:
-
Updated forms.py - the old help text is difficult to read
- Commits:
-
Summary ID Author b5cad1541c82eaa81a6b3f160561ec282e7740fd HailanXyouknow 2a888e3b7ef80588b2bceee92835ab26e669faff HailanXyouknow - Diff:
-
Revision 10 (+3524)
Checks run (2 succeeded)
- Change Summary:
-
Capitalize the word Webhook
- Commits:
-
Summary ID Author 2a888e3b7ef80588b2bceee92835ab26e669faff HailanXyouknow 244565bcc337171c5ccbfdd657551e3f12a6dbff HailanXyouknow - Diff:
-
Revision 11 (+3524)
Checks run (2 succeeded)
-
-
-
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
. -
-
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.
-
-
-
-
-
-
-
-
-
-
This should end in a period (since it's not a test case docstring). Please also add "Args" and "Returns" sections.
- Change Summary:
-
Addressed David's feedback.
- Description:
-
~ Added the Microsoft Teams Integration Option.
~ MS Teams icons are acquired from Adaptive Cards' main webpage: ~ 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/ - Commits:
-
Summary ID Author 244565bcc337171c5ccbfdd657551e3f12a6dbff HailanXyouknow 0d437127a0b3edf449831a39e05d8604342a6958 HailanXyouknow - Diff:
-
Revision 12 (+3504)
Checks run (2 succeeded)
- Change Summary:
-
Added `.encode('UTF-8')` to our request.
- Commits:
-
Summary ID Author 0d437127a0b3edf449831a39e05d8604342a6958 HailanXyouknow 5b8cfb6111e99ac52f5570e53d83849cbadd1ed3 HailanXyouknow - Diff:
-
Revision 13 (+3504)
Checks run (2 succeeded)
-
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.
-
How about renaming the module from
microsoftteams
tomsteams
? It'll be shorter and imports won't have to wrap. -
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.
-
-
-
-
-
-