Backbone view for attachment upload

Review Request #5911 — Created May 31, 2014 and submitted

Information

Review Board

Reviewers

A working version of a Backbone view for file uploads (the one that you see when you click Update -> Add File). Previously that popup is used gReviewRequest hack. Now it's a standalone view, as that view is also needed for attachment revisioning.

Side note: The only other thing that needs gReviewRequest hack is "Update Diff" popup. Once someone writes a view for that, gReviewRequest can be removed.

Tested: * Uploading new attachments. * Error when no attachment is specified. * Multiple errors returned from server side (see screenshots).


Description From Last Updated

Styles should be defined in the .less files. That said, a lot of this looks like you're manually copying stuff …

daviddavid

All of the HTML in these strings should be indented 1 space, not 4.

daviddavid

It's not important during prototyping, but we'll eventually want any user-visible strings to be passed into the template as variables, …

daviddavid

You've got some badly defined HTML in here (all the <col> elements are un-closed).

daviddavid

Make sure never to have a trailing comma on the last entry in a dictionary or list. It'll break some …

chipx86chipx86

variables should always be nameed likeThis.

chipx86chipx86

initialize should never call render. That's up to the caller who's constructing the view. Same with setting el.

chipx86chipx86

Instead of assigning that, pass this as the last parameter to save(), and the callbacks will end up having this …

chipx86chipx86

gReviewRequest is deprecated and was supposed to be removed. This view should be associated with a ReviewRequestEditor, which will have …

chipx86chipx86

Would you mind wrapping this to try to keep it under 80 columns? ' <form encoding="multipart/form-data"', ' enctype="multipart/form-data"', ' id="attachment-upload-form">',

daviddavid

Can you wrap this?

daviddavid

Arrays in javascript can't have trailing commas (it breaks on some browsers)

daviddavid

Can you add method comments for this and all the others that don't have them?

daviddavid

If you use this.$(...), it will be more efficient, since it doesn't have to scan the whole document.

daviddavid

This should use gettext()

daviddavid

'errorStr' is not HTML, so this should use .text() instead of .html().

daviddavid

You should be able to use the 'events' mapping for this: events: { ... existing other stuff ... 'click #upload-file-link': …

daviddavid

You shouldn't need to pass el yourself. By default, a new div element will be created.

daviddavid

I just committed a change to common.js that fixes some logic in this code (and simplifies things). Can you look …

daviddavid

Can we name this $list, since it's a jquery object?

daviddavid

Can we swap these (set HTML first, then append?)

daviddavid

No trailing comma (breaks IE)

daviddavid

No trailing comma (breaks IE)

daviddavid

Either you should move the variable definition to the top of this method, or, more likely, just avoid assigning anything: …

daviddavid

You can get rid of the uploadObject parameter by just passing in this.displayErrors as the value for error

daviddavid

Get rid of the rsp parameter here (it's not used)

daviddavid

What is this? I don't see any options anywhere.

daviddavid

What is errors? I don't see where it's defined.

daviddavid

Once the change in send above is made, this anonymous function could be replaced by _.bind(this.send, this).

daviddavid

If you pass this as a second paramater to .save(), you don't have to create a local variable for displayErrors, …

daviddavid

Variable definitions should be the first line in the function.

daviddavid

Can you call this variable self instead of uploadObject?

daviddavid

These two functions should probably be chained: this.$el .append(this.template({ ... })) .modalBox({ ... });

daviddavid
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/staticbundles.py
      Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/staticbundles.py
      Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Styles should be defined in the .less files.

    That said, a lot of this looks like you're manually copying stuff that's autogenerated. Your view should use $.modalBox to create the modal, which will handle the background, etc.

    1. I still have some style="..." stuff left there, but it's a couple of thing only. Is it tolerable or no?

  3. Show all issues

    All of the HTML in these strings should be indented 1 space, not 4.

  4. Show all issues

    It's not important during prototyping, but we'll eventually want any user-visible strings to be passed into the template as variables, so that we can call gettext() on them.

  5. Show all issues

    You've got some badly defined HTML in here (all the <col> elements are un-closed).

  6. 
      
chipx86
  1. 
      
  2. Show all issues

    Make sure never to have a trailing comma on the last entry in a dictionary or list. It'll break some browsers.

    It's good to get in the habit of running 'jshint' within the js/ directory. (You'll need to install that.)

  3. Show all issues

    variables should always be nameed likeThis.

    1. In JavaScript, that is. Python uses underscores (yeah, yeah).

  4. Show all issues

    initialize should never call render. That's up to the caller who's constructing the view.

    Same with setting el.

  5. Show all issues

    Instead of assigning that, pass this as the last parameter to save(), and the callbacks will end up having this set correctly.

  6. Show all issues

    gReviewRequest is deprecated and was supposed to be removed. This view should be associated with a ReviewRequestEditor, which will have a handle to the ReviewRequest model.

    1. Ok, I will look into that.

  7. 
      
VO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
      Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
      Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
  2. 
      
VO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/staticbundles.py
      Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/staticbundles.py
      Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
  2. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Would you mind wrapping this to try to keep it under 80 columns?

    ' <form encoding="multipart/form-data"',
    '       enctype="multipart/form-data"',
    '       id="attachment-upload-form">',
    
  3. Show all issues

    Can you wrap this?

  4. Show all issues

    Arrays in javascript can't have trailing commas (it breaks on some browsers)

  5. Show all issues

    Can you add method comments for this and all the others that don't have them?

  6. Show all issues

    If you use this.$(...), it will be more efficient, since it doesn't have to scan the whole document.

  7. Show all issues

    This should use gettext()

  8. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    'errorStr' is not HTML, so this should use .text() instead of .html().

  3. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. 
      
VO
david
  1. 
      
  2. Show all issues

    You should be able to use the 'events' mapping for this:

    events: {
        ... existing other stuff ...
        'click #upload-file-link': '_onUploadFileClicked'
    }
    
    1. This is the same issue that I keep encountering over and over again - #upload-file-link is not a child of the view element, since it's hidden inside a list that gets displayed when you hover over "Update" button. Therefore I have to assign the handler myself.

      I really wonder why Backbone creators haven't added a way to do this in the 'events' mapping, but most likely I just don't know enough to see all tradeoffs.

    2. I don't understand. You're using this.$(...), which means that #upload-file-link has to be nested (however deeply) within this.$el. Anything that can be accessed through this.$(...) should also be fine to use with events.

    3. It won't work in this case.

      events doesn't attach to those selectors. It only listens on this.$el, and checks the targets of any events that bubble up to it.

      The drop-down menus disable propagation of events, so they'll never reach this.$el.

    4. Ah, it makes sense now.

  3. Show all issues

    You shouldn't need to pass el yourself. By default, a new div element will be created.

  4. reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    I just committed a change to common.js that fixes some logic in this code (and simplifies things). Can you look at that change (76caaa7) and copy the changes into your code here?

  5. Show all issues

    Can we name this $list, since it's a jquery object?

  6. Show all issues

    Can we swap these (set HTML first, then append?)

  7. Show all issues

    No trailing comma (breaks IE)

  8. Show all issues

    No trailing comma (breaks IE)

  9. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Either you should move the variable definition to the top of this method, or, more likely, just avoid assigning anything:

    this.$('#upload-file-link')
        .click(_.bind(this._onUploadFileClicked, this));
    

    This code should probably also go into the render method.

  3. reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    You can get rid of the uploadObject parameter by just passing in this.displayErrors as the value for error

  4. Show all issues

    Get rid of the rsp parameter here (it's not used)

  5. Show all issues

    What is this? I don't see any options anywhere.

  6. Show all issues

    What is errors? I don't see where it's defined.

  7. Show all issues

    Once the change in send above is made, this anonymous function could be replaced by _.bind(this.send, this).

  8. 
      
VO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    If you pass this as a second paramater to .save(), you don't have to create a local variable for displayErrors, because it will use the right context.

    1. Sorry about that (it's not the first time you're telling me about this trick). I think I tried to do this and it didn't work, but I have a feeling I did it in the wrong place. All good now.

  3. Show all issues

    Variable definitions should be the first line in the function.

  4. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. 
      
david
  1. Just a couple trivial coding style comments left. Nice job!

  2. Show all issues

    Can you call this variable self instead of uploadObject?

  3. reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 9)
     
     
     
     
     
     
     
     
    Show all issues

    These two functions should probably be chained:

    this.$el
        .append(this.template({
            ...
        }))
        .modalBox({
            ...
        });
    
  4. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. 
      
david
  1. The code looks good, but the description seems out of date. Can you rewrite that, and then I'll do some final testing and push?

  2. 
      
VO
david
  1. Ship It!

  2. 
      
VO
Review request changed
Status:
Completed
Change Summary:
Pushed to master (8c368ca)