WebHooks now use the ResourceAPIEncoder

Review Request #7822 — Created Dec. 22, 2015 and submitted

brennie
Review Board
release-2.5.x
4042
reviewboard

Previously, a when uploading a new diff for a review request, if there
were webhooks configured, they would fail to execute because DiffSets
could not be serialized. Now that Djblets supports serialization of
arbitrary Django models, we implement this behaviour for DiffSets so
they can be serialized in web hook payloads (as their diffviewer URL).

Web hooks have been updated to use the ResourceAPIEncoder so if any
model present in a payload is registered with a resource, it will be
serialized accordingly. This should avoid future serialization issues
with web hooks.

  • Ran unit tests.
  • Could not reproduce issue 4042 with this patch applied.
  • The new unit test fails without this patch applied.
Description From Last Updated

Undo this please.

daviddavid

Saves -> Save.

daviddavid

"serialize it as its URL" is kind of confusing. How about: DiffSet objects are serialized as the link to the …

daviddavid

This would be much easier as: self.history.review_request

daviddavid

I'd rather these docstring cleanups be in their own change. It gets harder to track down changes when there are …

chipx86chipx86

This will have to go somewhere else. Nothing in diffviewer/ should ever know about review requests. It should only know …

chipx86chipx86

The """ is indented one layer too far.

chipx86chipx86

How do the other items in the payload look? Can you show me what the payload ends up looking like? …

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/models.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/models.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/diffviewer/models.py (Diff revision 1)
     
     

    Undo this please.

  3. reviewboard/diffviewer/models.py (Diff revision 1)
     
     

    Saves -> Save.

  4. reviewboard/diffviewer/models.py (Diff revision 1)
     
     

    "serialize it as its URL" is kind of confusing. How about:

    DiffSet objects are serialized as the link to the "View Diff" page at the given revision.
    
    1. Should this perhaps serialize it as a REST link?

  5. reviewboard/diffviewer/models.py (Diff revision 1)
     
     
     
     
     
     
     

    This would be much easier as:

    self.history.review_request

    1. Oops this should have used self.history_id

      Won't doing self.history.review_request require two DB hits (one for the history object, and one for the review request)? Also, we don't need the entire information about the RR here -- just its local site and pk/local_id

  6. 
      
brennie
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
    1. I'm also a little confused by the review request description. Having trouble parsing it. It'd like to see more of a "here's what was wrong, here's what this does to fix it."

  2. reviewboard/diffviewer/models.py (Diff revision 2)
     
     

    I'd rather these docstring cleanups be in their own change. It gets harder to track down changes when there are modifications in a file that are unrelated to the purpose of the commit.

  3. reviewboard/diffviewer/models.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     

    This will have to go somewhere else. Nothing in diffviewer/ should ever know about review requests. It should only know about diffs and the things it needs to access them.

    We'll need a different approach for this particular patch.

  4. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/models.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/models.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/models.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
    
    
  2. 
      
chipx86
  1. David says this looks sane.

    Can we get a unit test for it?

  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
  2. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/notifications/webhooks.py (Diff revision 6)
     
     
     

    The """ is indented one layer too far.

  3. reviewboard/notifications/webhooks.py (Diff revision 6)
     
     
     

    How do the other items in the payload look? Can you show me what the payload ends up looking like?

    The reason I ask is that we don't really use 'url' in our API payloads, and I thought that the webhooks used the same format. I'm not sure I fully know how this should be looking, though.

  4. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/webhooks.py
        reviewboard/notifications/tests.py
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (0b6bb9a)
Loading...