Status Updates part 5: Add the StatusUpdate model.

Review Request #8406 — Created Sept. 18, 2016 and submitted

Information

Review Board
release-3.0.x
597e12e...

Reviewers

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.

  • Ran syncdb.
  • Used the model via the admin UI (and other changes).
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 …

chipx86chipx86

We should include a file docstring for status_update.py.

chipx86chipx86

Should be "third-party service or extension."

chipx86chipx86

"etc." Can we also flesh out what a status update means? What it comprises of?

chipx86chipx86

We should have doc comments for these, too.

chipx86chipx86

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

chipx86chipx86

No : on static:.

chipx86chipx86

These should use :py:attr: for the constant references.

chipx86chipx86

Can we make this a ForeignKey? I know this is a bigger topic, which we should probably have in more …

chipx86chipx86

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

chipx86chipx86

Static methods should go before normal methods.

chipx86chipx86

Missing Returns.

chipx86chipx86

We don't do this anywhere else.

brenniebrennie

bool

chipx86chipx86
david
chipx86
  1. 
      
  2. reviewboard/reviews/admin.py (Diff revision 1)
     
     
     

    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.

  3. We should include a file docstring for status_update.py.

  4. Should be "third-party service or extension."

  5. "etc."

    Can we also flesh out what a status update means? What it comprises of?

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

    We should have doc comments for these, too.

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

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

    Same below.

  8. No : on static:.

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

    These should use :py:attr: for the constant references.

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

    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.

    1. 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?

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

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

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

    Static methods should go before normal methods.

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

    Missing Returns.

  14. 
      
david
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/admin.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     

    We don't do this anywhere else.

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

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

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

  4. 
      
chipx86
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (619d53a)
Loading...