Fix various aspects of dispatching webhook with auth credentials

Review Request #11171 — Created Sept. 14, 2020 and discarded

Information

Review Board
master

Reviewers

Fix error dispatching webhook with auth credentials

The error was:

AttributeError: 'SplitResult' object has no attribute 'params'

The 5-tuple used for rebuilding the URL was missing the fragment element instead.


Log webhook server responses on error

If the exception from urlopen represents an HTTP response, then the included
information may be useful to the site admin in troubleshooting.


Always send Authorization header on authenticated webhooks

The problem with HTTPBasicAuthHandler is that it will first attempt to make an
unauthenticated request, and will retry with authentication only if a 401
response was seen. However, some servers respond with 403 if the right
credentials are not supplied right away.

This is a forward-ported version of my patches on top of 3.0.18, which is powering our production site.

Note: It is best to review the changes one commit at a time. The patch series is available publicly on Github.

Description From Last Updated

From the description, it looks like there's multiple independent changes all being introduced. Can you split this into a review …

chipx86chipx86

E501 line too long (81 > 79 characters)

reviewbotreviewbot

Can you reorder the new import to be in alphabetical order? Helps us keep our imports organized.

chipx86chipx86

Now that the list of imports here is short, will it fit on one line?

chipx86chipx86

This can be placed earlier up, in alphabetical order.

chipx86chipx86

We prefer a blank line between statements and new blocks (helps keep blocks from running together, avoiding issues with, say, …

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

YX
chipx86
  1. 
      
  2. Show all issues

    From the description, it looks like there's multiple independent changes all being introduced. Can you split this into a review request per change, so we can more easily get these in and keep them separate?

    1. No problem. I will have to figure out later, what the best way is to submit a patch series for review, like you would with git send-email <rev-list options>.

  3. reviewboard/notifications/webhooks.py (Diff revision 2)
     
     
     
     
    Show all issues

    Can you reorder the new import to be in alphabetical order? Helps us keep our imports organized.

  4. reviewboard/notifications/webhooks.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    Now that the list of imports here is short, will it fit on one line?

    1. As a side note: for new projects I would consider introducing Black into the development workflow.

  5. reviewboard/notifications/webhooks.py (Diff revision 2)
     
     
    Show all issues

    This can be placed earlier up, in alphabetical order.

  6. reviewboard/notifications/webhooks.py (Diff revision 2)
     
     
     
    Show all issues

    We prefer a blank line between statements and new blocks (helps keep blocks from running together, avoiding issues with, say, two if blocks appearing as part of the same set of conditionals).

  7. 
      
YX
Review request changed
Status:
Discarded