Use a set of datastore objects for all API communication in the JavaScript code

Review Request #1082 — Created Sept. 17, 2009 and submitted

Christian Hammond
Review Board
master
1135
reviewboard
This change introduces datastore.js, which contains all of the actual communication code for our JavaScript (with two exceptions, which I will note in a second).

These new objects give us actual APIs for communicating with objects server-side. The communication logic is hidden inside of these objects, and the rest of the JavaScript code doesn't know a thing about the communication. In the future, this will allow us to add offline support (using either the new client-side storage as part of HTML 5 or Google Gears) much easier.

This is not a complete transition, but it's close. We still have actual queries for two things:

1) Fetching the review form. I plan to rewrite all this separately so it's generated client-side instead of server-side, so this won't be a problem too much longer.

2) Auto-completion of users and groups. We currently go through jQuery.ui.autocomplete, which communicates directly with the server. We may need to write our own autocomplete class, or modify the existing one. As this is no longer shipping with jQuery.ui, we can probably just fork it. This too will happen in a future change.

This also introduces a replacement for our current error banners. We no longer show the error banners on the page, but rather the activity indicator at the top of the screen now turns a reddish color on error and stays on the screen until dismissed or until another operation takes place that would show the banner. The user can click "Show Details" and a form much like the current Server Error Details form will appear. This new form is similar to the old, except it also shows the request data, URL, response code, and response error text.
Tested the following:

* Starring/unstarring review requests and groups
* Modifying review requests
* Publishing and discarding review requests
* Commenting on diffs
* Reading existing comments on diffs
* Publishing new reviews
* Uploading screenshots
* Review request update bubbles
* The new error dialog, accessed from the activity indicator.
David Trowbridge
  1. I'd like to see screenshots of the new error state.
  2. It just keeps going higher and higher!
    1. Yep... Had to be higher than the Djblets one, and I didn't want to change Djblets. *shrug*
  3. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
    These should be on the same line.
    1. Fixing all these. Dunno why I suddenly changed styles like that. I tend to have different styles on different projects and sometimes I switch.
  4. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
     
     
     
     
     
    Can you indent the ?: relative to the test? I think it's a lot more readable like this:
    
    (interfilediff_revision == null
         ? filediff_revision
         : file...)
  5. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
    These should be on the same line.
  6. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
     
     
     
    else goes on the same line as }. Why did you start doing this differently?
  7. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
    These go on the same line.
  8. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
    Same line.
  9. I think I'd prefer if you just put the second param on its own line instead of wrapping it like this.
  10. Any reason you have to use != null? Can draftComment be non-null but still non-true?
    1. I generally like to be explicit, but given that JavaScript has both null and undefined, I suppose I'll be less explicit here.
  11. Same here.
  12. Indentation looks off by one space.
  13. 
      
Christian Hammond
Christian Hammond
Review request changed

Change Summary:

* Fix some error reporting. We were overriding the error handler for setDraftField.
* Remove the errorPrefix values, since they're not used anymore.
* Serialize the HTTP request parameters to text so we can show it.

Diff:

Revision 3 (+1199 -799)

Show changes

David Trowbridge
  1. 
      
  2. 
      
Paul Duran
  1. 
      
  2. this line did not work for me - RB.ReviewRequest.CLOSE_DISCARDED was 'undefined' ; changing to gReviewRequest.CLOSE_DISCARDED worked for me.
    1. Thanks for letting me know. This is fixed in the git repo.
  3. same as above. to gReviewRequest.CLOSE_SUBMITTED worked for me.
  4. 
      
Paul Duran
  1. 
      
    1. Thanks Paul. These should all be fixed now.
  2. this line causes error: "object doesn't support this action" in I.E." (ie8) - to reproduce, start to add comment and then click 'Cancel'
  3. trailing comma causes issues in I.E.
  4. trailing comma causes issues in I.E.
  5. similarly for above, this line causes error: "object doesn't support this action" in I.E." (ie8) - to reproduce, start to add comment to screenshot and then click 'Cancel'
  6. 
      
Loading...