Slack Integration with OAuth 2.0

Review Request #11289 — Created Nov. 19, 2020 and updated

ruonanjia
rbintegrations
master
rbintegrations, students

Slack Integration workflow now uses OAuth 2.0.
Users fill out the form with the credentials provided
by Slack App and clicks on a button Authorize to bring
up the OAuth 2.0 popup workflow.

  • Manually tested adding Slack integration using OAuth 2.0 workflow
    • Access token is successfully saved into config
    • Slack integration successfully sends notifications to
      linked channel
      with a test Slack App set up
  • Ran unit tests, all works except 2 needing image attachments:
    • test_notify_new_review_request_with_image_file_attachment
    • test_notify_updated_review_request_with_new_image_attachments
Summary Author
Slack Integration with OAuth 2.0
ruonan
Loading file attachments...

Description From Last Updated

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Col: 10 'authorize' is defined but never used.

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Col: 10 'authorize' is defined but never used.

reviewbotreviewbot

Col: 10 'authorize' is defined but never used.

reviewbotreviewbot

I believe that python files, do not follow the camelCase naming scheme.

jblazusijblazusi

Include a line between lines 2 and 3.

jblazusijblazusi

Needs an empty line between line 7 and 8 to separate 3rd party modules and current project codebase

hailanhailan

Lines 10 and 11 should be swapped to maintain alphabetical order.

jblazusijblazusi

Delete 'webhook URL'

hailanhailan

I could be totally wrong, but was it not already in utf-8?

hailanhailan

Remove print statement

hailanhailan

Update Trello to Slack

hailanhailan

Maybe use from django.utils.six.moves.urllib.request

hailanhailan

Include another empty line below line 10.

jblazusijblazusi

Col: 10 'authorize' is defined but never used.

reviewbotreviewbot

I noticed that you are using var here, but are not redeclaring the variable, perhaps this should be const.

jblazusijblazusi

I noticed you are experiencing some extra whitespace errors. I find that these can be hard to find unless you ...

jblazusijblazusi

I could be wrong, but this function does not seem to follow camelCase styling.

jblazusijblazusi

I think we should be using let here.

jblazusijblazusi

Instead of making another variable param, I think that we can just return urlObj.searchParams.get(name);

jblazusijblazusi

I could be wrong about this as well, but I think we could use let here instead of var

jblazusijblazusi

This variable does not seem to be redeclared, so therefore I think it should be const.

jblazusijblazusi

Perhaps it would be useful to console.log or log the error somehow to make the error easier to determine when ...

jblazusijblazusi
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

ruonanjia
ruonanjia
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

ruonanjia
Review request changed

Commits:

Summary Author
-
Slack Integration with OAuth 2.0
ruonan
-
Slack Integration with OAuth 2.0
ruonan
+
Slack Integration with OAuth 2.0
ruonan
+
Slack Integration with OAuth 2.0
ruonan

Diff:

Revision 3 (+917 -317)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

ruonanjia
  1. 
      
  2. 
      
ruonanjia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

ruonanjia
Review request changed

Groups:

+students
hailan
  1. Awesome job, Ruonan! Looking great!
    I'm also learning about OAuth right now, so I'll definitely check this out
    locally at a later time.

  2. rbintegrations/slack/forms.py (Diff revision 4)
     
     

    Needs an empty line between line 7 and 8 to separate 3rd party modules and current project codebase

  3. rbintegrations/slack/integration.py (Diff revision 4)
     
     

    Delete 'webhook URL'

  4. rbintegrations/slack/integration.py (Diff revision 4)
     
     

    I could be totally wrong, but was it not already in utf-8?

  5. rbintegrations/slack/tests.py (Diff revision 4)
     
     

    Remove print statement

  6. rbintegrations/slack/views.py (Diff revision 4)
     
     

    Update Trello to Slack

  7. rbintegrations/slack/views.py (Diff revision 4)
     
     

    Maybe use from django.utils.six.moves.urllib.request

  8. 
      
jblazusi
  1. Awesome Work!
    Unfortunately, I experienced 28 test failures, although it is possible that I may have done something wrong when applying the patch.
    Due to these test failures, I refrained from doing any manual testing.
    I imported all the files, ran ./setup develop then rbtext test -m rbintegrations.slack (For the record I am also using RB4).
    Please let me know if I did something incorrectly on my end.

  2. rbintegrations/slack/authorizeWidget.py (Diff revision 4)
     
     

    I believe that python files, do not follow the camelCase naming scheme.

  3. rbintegrations/slack/forms.py (Diff revision 4)
     
     

    Include a line between lines 2 and 3.

  4. rbintegrations/slack/integration.py (Diff revision 4)
     
     

    Lines 10 and 11 should be swapped to maintain alphabetical order.

  5. rbintegrations/slack/views.py (Diff revision 4)
     
     

    Include another empty line below line 10.

  6. I noticed that you are using var here, but are not redeclaring the variable, perhaps this should be const.

  7. I noticed you are experiencing some extra whitespace errors. I find that these can be hard to find unless you enable whitespaces in your text editor. Then they are easier to find and remove.

    Similar, it does not seem like we are redeclaring win here so perhaps we should use const.

  8. I could be wrong, but this function does not seem to follow camelCase styling.

  9. I think we should be using let here.

  10. Instead of making another variable param, I think that we can just return urlObj.searchParams.get(name);

  11. I could be wrong about this as well, but I think we could use let here instead of var

  12. This variable does not seem to be redeclared, so therefore I think it should be const.

  13. Perhaps it would be useful to console.log or log the error somehow to make the error easier to determine when debugging in the future?

  14. 
      
Loading...