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
Slack Integration with OAuth 2.0
ruonan

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

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

reviewbotreviewbot

Col: 27 Missing semicolon.

reviewbotreviewbot

Please add a module docstring at the top of this file.

daviddavid

Let's use django.util.html.format_html here: return format_html('<button onclick="authorize(\'{0}\')">{1}</button>', server, _('Authorize'))

daviddavid

We should wrap "Authorize" in a gettext call and format it into the HTML.

daviddavid

Add a blank line between these two.

daviddavid

Add a blank line between these two.

daviddavid

Please use single quotes here.

daviddavid

Instead of +, can you use a format operation?

daviddavid

These two lines will need to be swapped.

daviddavid

Can we add some blank lines in here to space things out a bit?

daviddavid

Why are these commented out?

daviddavid

This doesn't appear to be used.

daviddavid

Add another blank line here.

daviddavid

Please add a period in this docstring.

daviddavid

This needs a better docstring.

daviddavid

There's an extra blank line here that can go away.

daviddavid

For this, please use (at the top), from django.utils.six.moves.urllib.parse import parse_qs and then just call parse_qs.

daviddavid

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

reviewbotreviewbot

This file should be named .es6.js, since you're using ES6 features.

daviddavid

This can use an ES6 template string: const redirect = `${redirectServer}rbintegrations/slack/oauth/`;

daviddavid

Please use single quotes throughout.

daviddavid

This can also be a template string.

daviddavid

You've got some trailing whitespace throughout this file.

daviddavid

This can be const.

daviddavid

Instead of the magic number 4, can we compare against XMLHttpRequest.DONE?

daviddavid

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

reviewbotreviewbot

We can get rid of the warning here, and make things more correct and reliable wrt. babel-compilation by assigning this ...

daviddavid

There's still some trailing whitespace within this file. Please look at the diff here on Review board and get rid ...

daviddavid

console.error is probably more approriate than console.log here.

daviddavid

E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 2 Missing semicolon.

reviewbotreviewbot
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
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?

    1. Wasn't working for me till I added this

  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.

    1. I believe scoping would stay the same. What would be the benefit of using let?

    2. I suppose it is up to the mentors, but from my experience, let was always used instead of var, unless var was absolutely needed. This reduces potential naming conflicts which can occur when dealing with a large number of variables as well as unforseen scoping issues. I am not sure whether it has a significant impact on compiling speeds, though it might, to my knowledge const and let are features introduced in ES6. Therefore, I believe they would need to be transpiled by Babel. From my experience, I find it is a more careful approach when working with a language like JavaScript.

    3. In our JS code, we've been moving to always using .es6.js, and preferring let/const over var. It's true that in many cases it's the same, but the new forms are less error-prone than the old. In this case, you can use const since it's assigned only once.

  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

    1. Same comment as before ^

  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. 
      
ruonanjia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

ruonanjia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

ruonanjia
david
  1. 
      
  2. Please add a module docstring at the top of this file.

  3. rbintegrations/slack/authorize_widget.py (Diff revision 6)
     
     
     

    Let's use django.util.html.format_html here:

    return format_html('<button onclick="authorize(\'{0}\')">{1}</button>',
                       server,
                       _('Authorize'))
    
  4. We should wrap "Authorize" in a gettext call and format it into the HTML.

  5. rbintegrations/slack/forms.py (Diff revision 6)
     
     
     

    Add a blank line between these two.

  6. rbintegrations/slack/forms.py (Diff revision 6)
     
     
     

    Add a blank line between these two.

  7. rbintegrations/slack/integration.py (Diff revision 6)
     
     

    Please use single quotes here.

  8. rbintegrations/slack/integration.py (Diff revision 6)
     
     

    Instead of +, can you use a format operation?

  9. rbintegrations/slack/tests.py (Diff revision 6)
     
     
     

    These two lines will need to be swapped.

  10. rbintegrations/slack/tests.py (Diff revision 6)
     
     
     
     

    Can we add some blank lines in here to space things out a bit?

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

    Why are these commented out?

    1. This was addressed in the Slack chat a while back. When I run the tests, I get this error: [No such file or directory: '/var/folders/jm/c43j59q96716g48s49cbw9dc0000gn/T/rb-tests-6lqol9qg/static/rb/images/logo.png']

      Also occurred for the Matrix Integration and Hailan was also having this issue I believe.

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

    This doesn't appear to be used.

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

    Add another blank line here.

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

    Please add a period in this docstring.

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

    This needs a better docstring.

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

    There's an extra blank line here that can go away.

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

    For this, please use (at the top), from django.utils.six.moves.urllib.parse import parse_qs and then just call parse_qs.

  18. This file should be named .es6.js, since you're using ES6 features.

  19. This can use an ES6 template string:

    const redirect = `${redirectServer}rbintegrations/slack/oauth/`;
    
  20. rbintegrations/static/js/slack/authorize.js (Diff revision 6)
     
     
     
     
     

    Please use single quotes throughout.

  21. This can also be a template string.

  22. You've got some trailing whitespace throughout this file.

  23. This can be const.

  24. Instead of the magic number 4, can we compare against XMLHttpRequest.DONE?

  25. 
      
ruonanjia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
  1. 
      
  2. We can get rid of the warning here, and make things more correct and reliable wrt. babel-compilation by assigning this to the window object. Let's also make the name more specific:

    window.authorizeSlack = function(redirectServer) {
      ...
    }
    
  3. There's still some trailing whitespace within this file. Please look at the diff here on Review board and get rid of them.

  4. console.error is probably more approriate than console.log here.

  5. 
      
ruonanjia
Review request changed

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

ruonanjia
ruonanjia
Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
ruonanjia
  1. 
      
  2. 
      
Loading...