Fix discarding of review replies, and several issues with drafts.

Review Request #4023 — Created April 5, 2013 and submitted

Information

Review Board
master

Reviewers

Fix discarding of review replies, and several issues with drafts.

Review replies weren't given the same draft logic that reviews were
given, but since they're a variant on a review, it needed the same
logic. Furthermore, replies couldn't be discarded.

A new mixin was added that contains all the draft logic. It's based on
what was in ReviewModel, but modified. Both ReviewModel and
ReviewReplyModel use it for all their draft handling now.

The logic was changed to handle the case where an object goes from
unloaded to loaded and back to unloaded (such as after a destruction).
Instead of overriding its url() and ready() functions, it now just
includes some state indicating whether to use its own functions or other
ones.

Since the mixin needs to call parent functions, but may not know what
the parent is, a _.super() function was added, which will compute the
super prototype for an instance.

BaseResource got some fixes to handle parentObject loading properly when
using an old-style resource object. It also now resets the object state
after a destroy(), so it can be used again.

In order to work around some Chrome bugs I encountered, I've changed the
draft resources on the server side to return HTTP 302s instead of 301s,
as the latter is meant to be more permanent. I've also updated the Last
Modified time when updating a reply. I'm not sure this solved all the
problems yet (time will tell, as it was intermittent), but it's probably
more correct anyway.
Lots of manual testing of discarding, and with an upcoming change
to fix the "discard if empty" (which went missing in the rewrite,
but also was pretty broken before it anyway).

Unit tests all passed (server-side and JavaScript).
Description From Last Updated

This isn't about using get(), it's about the object form of ready() instead of the function form.

daviddavid

Should this use _.result in case 'url' isn't a function?

daviddavid

subtly

daviddavid

resource

daviddavid
david
  1. 
      
  2. This isn't about using get(), it's about the object form of ready() instead of the function form.
  3. Should this use _.result in case 'url' isn't a function?
    1. I was going to say yeah, good idea, but it doesn't work in practice. We can't control the context for 'this', and so it ends up calling it on the parent prototype without using this instance. Kind of annoying. We'd need our own _.result.
      
      Of course, in both cases where we care today, they're functions, so this is more academic.
  4. 
      
chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
        reviewboard/webapi/resources.py
      Ignored Files:
        reviewboard/static/rb/js/views/tests/screenshotThumbnailViewTests.js
        reviewboard/static/rb/js/models/reviewReplyModel.js
        reviewboard/static/rb/js/models/baseResourceModel.js
        reviewboard/static/rb/js/utils/underscoreUtils.js
        reviewboard/static/rb/js/models/draftResourceModelMixin.js
        reviewboard/static/rb/js/models/draftReviewModel.js
        reviewboard/static/rb/js/models/tests/reviewReplyModelTests.js
        reviewboard/static/rb/js/models/reviewModel.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
        reviewboard/webapi/resources.py
      Ignored Files:
        reviewboard/static/rb/js/views/tests/screenshotThumbnailViewTests.js
        reviewboard/static/rb/js/models/reviewReplyModel.js
        reviewboard/static/rb/js/models/baseResourceModel.js
        reviewboard/static/rb/js/utils/underscoreUtils.js
        reviewboard/static/rb/js/models/draftResourceModelMixin.js
        reviewboard/static/rb/js/models/draftReviewModel.js
        reviewboard/static/rb/js/models/tests/reviewReplyModelTests.js
        reviewboard/static/rb/js/models/reviewModel.js
    
    
  2. 
      
david
  1. Just a couple spelling issues.
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (c6db61f)
Loading...