flake8
failed.
JSHint
passed.
flake8
-
rbintegrations/microsoftteams/test.py (Diff revision 1) Show all issues -
-
-
Microsoft Teams Integration Option
Review Request #11227 — Created Oct. 16, 2020 and updated
| Information | |
|---|---|
| hailan | |
| rbintegrations | |
| master | |
| Reviewers | |
| 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 |
|---|---|
| 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: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+686) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
-
-
rbintegrations/microsoftteams/integration.py (Diff revision 2) E266 too many leading '#' for block comment
-
-
-
-
-
-
-
-
-
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: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+1588) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
-
-
-
rbintegrations/microsoftteams/integration0.py (Diff revision 3) E501 line too long (81 > 79 characters)
-
-
-
rbintegrations/microsoftteams/integration0.py (Diff revision 3) E501 line too long (80 > 79 characters)
-
-
rbintegrations/microsoftteams/integration0.py (Diff revision 3) E501 line too long (80 > 79 characters)
-
-
-
-
-
-
rbintegrations/microsoftteams/test.py (Diff revision 3) E122 continuation line missing indentation or outdented
-
rbintegrations/microsoftteams/test.py (Diff revision 3) E122 continuation line missing indentation or outdented
-
rbintegrations/microsoftteams/test.py (Diff revision 3) E122 continuation line missing indentation or outdented
-
rbintegrations/microsoftteams/test.py (Diff revision 3) E122 continuation line missing indentation or outdented
-
-
-
rbintegrations/microsoftteams/test.py (Diff revision 3) E122 continuation line missing indentation or outdented
-
Change Summary:
Clean up integrations.py
Description: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||
Diff: |
Revision 4 (+462) |
||||||||||||
Removed Files: |
|||||||||||||
Added Files: |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
rbintegrations/microsoftteams/integration.py (Diff revision 4) E127 continuation line over-indented for visual indent
Change Summary:
Unit tests in tests.py
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+3430) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Change Summary:
Update tests.py
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+3528) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
rbintegrations/microsoftteams/integration.py (Diff revision 6) E501 line too long (83 > 79 characters)
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+3532) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
rbintegrations/microsoftteams/integration.py (Diff revision 7) not a must change but could also use a switch-case statement
-
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. -
rbintegrations/microsoftteams/integration.py (Diff revision 7) Should swap lines 10 and 13, so that they are in alphabetical order.
-
rbintegrations/microsoftteams/integration.py (Diff revision 7) logger = logging.getLogger(__name__)should have 2 lines before and 2 lines after. -
rbintegrations/microsoftteams/integration.py (Diff revision 7) This could also be
if,elif,else. Not sure what the mentors prefer. -
-
-
rbintegrations/microsoftteams/integration.py (Diff revision 7) I think it is good practice to include the
altattribute forimgtags in html. -
rbintegrations/microsoftteams/integration.py (Diff revision 7) Looks like a debug statement, we should remove these.
Change Summary:
Address Ruonan and Jacob's feedbacks
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+3526) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Change Summary:
Address white space
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+3526) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Updated forms.py - the old help text is difficult to read
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+3524) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Capitalize the word Webhook
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+3524) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
rbintegrations/microsoftteams/integration.py (Diff revision 11) 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. -
-
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.
-
-
rbintegrations/microsoftteams/tests.py (Diff revision 11) When we wrap the docstring, we put the final """ on its own line.
-
rbintegrations/microsoftteams/tests.py (Diff revision 11) When we wrap the docstring, we put the final """ on its own line.
-
rbintegrations/microsoftteams/tests.py (Diff revision 11) When we wrap the docstring, we put the final """ on its own line.
-
rbintegrations/microsoftteams/tests.py (Diff revision 11) When we wrap the docstring, we put the final """ on its own line.
-
-
rbintegrations/microsoftteams/tests.py (Diff revision 11) When we wrap the docstring, we put the final """ on its own line.
-
rbintegrations/microsoftteams/tests.py (Diff revision 11) When we wrap the docstring, we put the final """ on its own line.
-
rbintegrations/microsoftteams/tests.py (Diff revision 11) When we wrap the docstring, we put the final """ on its own line.
-
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.
Change Summary:
Addressed David's feedback.
Description: |
|
||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||
Diff: |
Revision 12 (+3504) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Added `.encode('UTF-8')` to our request.
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+3504) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
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
microsoftteamstomsteams? It'll be shorter and imports won't have to wrap. -
rbintegrations/microsoftteams/integration.py (Diff revision 13) 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.
-
-
rbintegrations/microsoftteams/integration.py (Diff revision 13) No need for the parens around the
... is .... -
rbintegrations/microsoftteams/integration.py (Diff revision 13) We should use
format_html(), since it's safer (the arguments going in will be HTML-escaped). -
rbintegrations/microsoftteams/integration.py (Diff revision 13) This should use
.copy()instead ofdict(...). -
-
rbintegrations/microsoftteams/integration.py (Diff revision 13) Docstring summaries must be one line without wrapping.
Change Summary:
Address feedbacks
Commits: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+3498) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.