Summary

WebHooks now use the ResourceAPIEncoder

Review Request #7822 — Created Dec. 22, 2015 and submitted Jan. 12, 2016, 11:28 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.5.x

Bugs

4042

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done


Ran unit tests.
Could not reproduce issue 4042 with this patch applied.
The new unit test fails without this patch applied.

Issues

Description	From	Last Updated
Undo this please.	david	Dec. 23, 2015, 5:57 p.m.
Saves -> Save.	david	Dec. 23, 2015, 5:59 p.m.
"serialize it as its URL" is kind of confusing. How about: DiffSet objects are serialized as the link to the …	david	Dec. 23, 2015, 5:59 p.m.
This would be much easier as: self.history.review_request	david	Dec. 23, 2015, 6 p.m.
I'd rather these docstring cleanups be in their own change. It gets harder to track down changes when there are …	chipx86	Jan. 4, 2016, 10:33 a.m.
This will have to go somewhere else. Nothing in diffviewer/ should ever know about review requests. It should only know …	chipx86	Jan. 7, 2016, 3:04 p.m.
The """ is indented one layer too far.	chipx86	Jan. 8, 2016, 12:36 p.m.
How do the other items in the payload look? Can you show me what the payload ends up looking like? …	chipx86	Jan. 8, 2016, 1:22 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/models.py



Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/models.py

reviewboard/diffviewer/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Undo this please.
```
reviewboard/diffviewer/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Saves -> Save.
```

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

The issue has been resolved. Show all issues

"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.

brennie Dec. 23, 2015, 2:08 p.m.

Should this perhaps serialize it as a REST link?

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

The issue has been resolved. Show all issues

This would be much easier as:

self.history.review_request

brennie Dec. 23, 2015, 2:08 p.m.

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

Change Summary:

Address David's issues.

Diff:

Revision 2 (+38 -13)

Show changes

reviewboard/diffviewer/models.py

Ship it!

```
Ship It!
```

chipx86 Dec. 30, 2015, 1:24 a.m.

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."

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

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.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/models.py



Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/models.py

Description:

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

Diff:

Revision 3 (+28)

Show changes

reviewboard/diffviewer/models.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/models.py

Change Summary:

Addressed Christian's issues.

Depends On:

~~7821 - Add support for serializing django Models~~

~~7849 - Allow BasicAPIEncoder specialization~~

Diff:

Revision 4 (+47)

Show changes

reviewboard/notifications/webhooks.py

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/webhooks.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/webhooks.py

Diff:

Revision 5 (+48 -1)

Show changes

reviewboard/notifications/webhooks.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/webhooks.py



Tool: Pyflakes
Processed Files:
    reviewboard/notifications/webhooks.py

David says this looks sane.

Can we get a unit test for it?

Change Summary:

Added a unit test. Fixed a copy-paste bug

Diff:

Revision 6 (+73 -1)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/webhooks.py

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

Testing Done:: Ran unit tests.

Could not reproduce issue 4042 with this patch applied.
+
The new unit test fails without this patch applied.

reviewboard/notifications/webhooks.py (Diff revision 6)
The issue has been resolved. Show all issues
```
The """ is indented one layer too far.
```

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

The issue has been resolved. Show all issues

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.

Summary:

Serialize diffsets in webhooks

WebHooks now use the ResourceAPIEncoder

Description:

		Previously, a when uploading a new diff for a review request, if there
		were webhooks configured, they would fail to execute because `DiffSet`s
		could not be serialized. Now that Djblets supports serialization of
		arbitrary Django models, we implement this behaviour for `DiffSet`s so
~		they can be serialized in webhook payloads (as their diffviewer URL).
	~	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.

Depends On:

~~7849 - Allow BasicAPIEncoder specialization~~

Diff:

Revision 7 (+27 -2)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/webhooks.py

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (0b6bb9a)

		Ran unit tests.
		Could not reproduce issue 4042 with this patch applied.
	+	The new unit test fails without this patch applied.