Summary

Status Updates part 5: Add the StatusUpdate model.

Review Request #8406 — Created Sept. 18, 2016 and submitted Sept. 29, 2016, 10:53 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

~~8405~~

Blocks

8407

Commit

597e12e...

Reviewers

Groups

reviewboard

People

Description

This change adds the model for status updates. A status update has some display
fields (the state, summary, description, url, etc.), and some relations to
other models to link things up as appropriate.

Testing Done


Ran syncdb.
Used the model via the admin UI (and other changes).

Issues

Description	From	Last Updated
What does this look like in the admin UI? We should probably flesh this out with some state that'd help …	chipx86	Sept. 19, 2016, 10:21 a.m.
We should include a file docstring for status_update.py.	chipx86	Sept. 19, 2016, 10:09 a.m.
Should be "third-party service or extension."	chipx86	Sept. 19, 2016, 10:09 a.m.
"etc." Can we also flesh out what a status update means? What it comprises of?	chipx86	Sept. 19, 2016, 10:11 a.m.
We should have doc comments for these, too.	chipx86	Sept. 19, 2016, 10:17 a.m.
Doc comments follow the same format as docstrings: Single-line summary, multi-line description. Same below.	chipx86	Sept. 19, 2016, 10:17 a.m.
No : on static:.	chipx86	Sept. 19, 2016, 10:17 a.m.
These should use :py:attr: for the constant references.	chipx86	Sept. 19, 2016, 10:17 a.m.
Can we make this a ForeignKey? I know this is a bigger topic, which we should probably have in more …	chipx86	Sept. 28, 2016, 12:14 p.m.
We should be representing these as fields in the ModelAdmin, instead of in __str__ (we recently fixed this in another …	chipx86	Sept. 19, 2016, 10:21 a.m.
Static methods should go before normal methods.	chipx86	Sept. 19, 2016, 10:21 a.m.
Missing Returns.	chipx86	Sept. 19, 2016, 10:21 a.m.
We don't do this anywhere else.	brennie	Sept. 28, 2016, 12:14 p.m.
bool	chipx86	Sept. 28, 2016, 2:28 p.m.

Description:: This change adds the model for status updates. A status update has some display
fields (the state, summary, description, url, etc.), and some relations to
other models to link things up as appropriate.
-
-
Testing done:
- - Ran syncdb.
- - Used the model via the admin UI (and other changes).
Testing Done:: +
Ran syncdb.
+
Used the model via the admin UI (and other changes).

reviewboard/reviews/admin.py (Diff revision 1)

The issue has been resolved. Show all issues

What does this look like in the admin UI? We should probably flesh this out with some state that'd help us locate and debug statuses.

reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We should include a file docstring for status_update.py.
```
reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be "third-party service or extension."
```
reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"etc."
Can we also flesh out what a status update means? What it comprises of?
```
reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We should have doc comments for these, too.
```

reviewboard/reviews/models/status_update.py (Diff revision 1)

The issue has been resolved. Show all issues

Doc comments follow the same format as docstrings: Single-line summary, multi-line description.

Same below.

reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No : on static:.
```
reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
These should use :py:attr: for the constant references.
```

reviewboard/reviews/models/status_update.py (Diff revision 1)

The issue has been dropped. Show all issues

Can we make this a ForeignKey?

I know this is a bigger topic, which we should probably have in more detail offline (I have a lot of thoughts to go over here), but the more I think about it the more I firmly believe we should be consolidating reviews on publish. To leave that option open as a possibility, for now or down the road, I'd like to not box ourselves in to a one-to-one relationship.

david Sept. 19, 2016, 9:29 a.m.

Let's keep discussion online for the benefit of any third parties who happen to be listening.

I don't understand what it buys us to consolidate things into a single review, but I see a lot of downsides:

It loses provenance. There's now no way to know which comments are owned by which status update. We could theoretically put this into extra_data, but then we're essentially storing relation information in there. That isn't queryable, and pushes a lot of implementation knowledge out into the client. The ID that was used by the client to create the review is also lost.
It loses ownership. The clients making the reviews will probably not all be owned by a single endpoint in the way that Review Bot handles it. Those reviews may be created via the API with different automation users for each service. As soon as we merge them, the "user" who created the merged-from review gets lost. This is extra bad in the case where all of this is being done via the API rather than an extension.
It makes e-mail really hard. If one review is published today, and another is published tomorrow after running some massive test suite, how does the second e-mail look? What do we do for the message IDs?
It creates the possibility for all sorts of bad race condition cases. "Find an existing public review and move all the comments over" is not an atomic operation. If two processes are trying to publish reviews at roughly the same time (which doesn't seem all that unlikely given the nature of this feature), we'd still end up with two reviews.

What's the upside?

chipx86 Sept. 28, 2016, 2:06 p.m.

For the benefits of third parties, I concede these points. I had a different view on downsides vs. benefits, but I'm pretty much convinced by these arguments. I think a lot of the concerns I had (the annoyance with having multiple reply drafts and having that leak into the UI, even with the default view of the new draft banner showing these combined) can be dealt with in other ways without merging reviews under the hood or exposing these as one review in the API.

reviewboard/reviews/models/status_update.py (Diff revision 1)

The issue has been resolved. Show all issues

We should be representing these as fields in the ModelAdmin, instead of in __str__ (we recently fixed this in another model).

reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Static methods should go before normal methods.
```
reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing Returns.
```

Commit:

f520bc8e0d65027acb05c4631ed5a8b2214e39d7

597e12e12b65b9ef5eb4845fed16a442ecbe336e

Diff:

Revision 2 (+209 -4)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/models/__init__.py
	reviewboard/reviews/models/status_update.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/models/status_update.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/models/status_update.py
    reviewboard/reviews/admin.py
    reviewboard/reviews/models/__init__.py

Ship it!

reviewboard/reviews/admin.py (Diff revision 2)

The issue has been dropped. Show all issues

We don't do this anywhere else.

david Sept. 28, 2016, 12:15 p.m.

Sure we do: https://github.com/reviewboard/reviewboard/blob/master/reviewboard/reviews/views.py

reviewboard/reviews/admin.py (Diff revision 2)

Not related to this change, but we should consider a @model_admin_short_description() decorator for stuff like this.

chipx86 Sept. 28, 2016, 2:01 p.m.

There's a few other like this, so maybe something like @add_attributes(short_description=_('Review request ID'), something='blah') would be neat.

Ship it!

reviewboard/reviews/models/status_update.py (Diff revision 2)
The issue has been resolved. Show all issues
```
bool
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (619d53a)

		This change adds the model for status updates. A status update has some display
		fields (the state, summary, description, url, etc.), and some relations to
		other models to link things up as appropriate.
-
-		Testing done:
-		- Ran syncdb.
-		- Used the model via the admin UI (and other changes).

	+	Ran syncdb.
	+	Used the model via the admin UI (and other changes).