Fixed the Update -> Add File review request action not responding to clicks.

Review Request #9207 - Created Sept. 22, 2017 and submitted

Nicholas Mercier
Review Board
master
4592
d2db62d...
reviewboard, students

In the Review Request screen, clicking Update -> Add File would not do
anything.

The Update element's click event is registered before the Add File
element's click event. As a result, the Update element's click event is
firing before, eating up the Add File's click event and thus causing the
upload file dialog to never appear.

The onUploadFileClicked() function was moved from ReviewablePageView to
ReviewRequestEditorView, and the element's handler registered in the
setupActions() function. The .has-menu handler was moved to
ReviewablePageView, better respecting the ownership structure of the
page. This changes the order in which the click events are fired, allowing
the onUploadFileClicked() function to actually fire in response to the
Add File element being clicked.

Furthermore, the has-menu handler was moved from
ReviewRequestEditorView to ReviewablePageView

After the fix, the upload file dialog appears as intended.

Ran JSTests.

Tested manually by clicking Update -> Add File, where the upload file
dialog appears. This was done on both computer and mobile browsers.

  • 0
  • 0
  • 18
  • 0
  • 18
Description From Last Updated
Christian Hammond
  1. 
      
  2. The description doesn't make it clear what the source of the problem was or how this fixes it. Descriptions should always tell a story of what the reported problem was, why it happened, and how it was fixed. Write it like you're telling someone what you did so they can write a report on it.

    As far as I can tell, the event handler really should live on the page's view, not the ReviewRequestEditorView, so I'm not sure why it was moved. What was the source of the problem?

    1. The issue was the generic .has-menu handler eating the event. (Yes, that should be discussed in the description).

      ReviewRequestEditorView has the handlers for almost all of the other review request actions. I'm not sure why it would live on the page view instead.

    2. I originally moved it to match how and where the click event for the Update -> Update Diff element was registered. The source of the problem is the order in which the events are registered The Update tab's event is firing before the Add File's event, which seemed to have been eating it up. There might be an alternative way of doing this, such as reversing the order in which the events are registered, but I suspect by doing that, the Add File would work, but the Update Diff would break.

    3. Okay, looking at this, I think it's fine, but it's worth bringing up the state of how the actions work today.

      The original intent was that ReviewRequestEditorView would own everything involving the editing of a review requerst (fields and such), and the ReviewablePageView would own everything else. As it currently is, ReviewablePageView owns events for the Review, Ship It, and Add Comment actions (none of which are editing operations on a review request itself).

      ReviewRequestEditorView owns actions for updating the close state (which is consistent with owning a review request) and uploading a diff. Uploading a file attachment would indeed be consistent here. It also owns Archive/Unarchive and Mute/Unmute, which is maybe not the right place for this, but I don't know.

      So what I'd suggest is this:

      1. Go ahead and move the Upload File handler to ReviewRequestEditorView.
      2. Move the .has-menu handler to ReviewablePageView, since that's higher up and should be where that handler should go (which just prevents bubbling up further or invoking the default action)

      Later, when actions are redone client-side, we'll need to better manage all this, but it'll probably still be ReviewablePageView (or a sub-view) that's responsible for that management, with ReviewRequestEditorView probably registering actions it needs through that.

  3. 
      
Christian Hammond
  1. 
      
  2. Also the description should always wrap at 70-75 characters. I recommend writing the description in your commit message. Many editors will wrap correctly for Git commits.

  3. 
      
Barret Rennie
  1. 
      
  2. "This method displays a popup..."

  3. 
      
Nicholas Mercier
Barret Rennie
  1. Ship It!

  2. 
      
Nicholas Mercier
  1. Ship It!

  2. 
      
Nicholas Mercier
David Trowbridge
  1. 
      
  2. Only one space between sentences. We're not using typewriters here!

  3. The reviewRequest variable is only used once, so we could remove the variable definition and juts move the model.get call where it's used.

    1. Going to change this on a different review request, as it's widespread.

    2. This one should be changed here, since you're introducing new code.

  4. 
      
Nicholas Mercier
Barret Rennie
  1. 
      
  2. Lets accept e (type Event) as an argument and call e.stopPropagation() and e.preventDefault() instead.

  3. 
      
Nicholas Mercier
Barret Rennie
  1. 
      
  2. Care to update this to use e.preventDefault() and e.stopPropagation() ? It too can be done at the top of the fn.

  3. 
      
Nicholas Mercier
Barret Rennie
  1. 
      
  2. No longer true. This function doesn't return anything.

  3. Mind updating this function to use preventDefault and stopPropagation too?

  4. No longer true. This function doesn't return anything.

  5. 
      
Nicholas Mercier
Barret Rennie
  1. 
      
  2. Mind adding Args to all these functions?

  3. 
      
Nicholas Mercier
Barret Rennie
  1. Ship It!
  2. 
      
Christian Hammond
  1. 
      
  2. This is on the wrong class. It needs to be on RB.ReviewablePageView. Can you move it, and then we'll need to perform more strict testing.

    This particular event handler is from commit 7bfd3ae3, which says:

    Fix clicking on actions for review requests.

    For mobile usage, the actions now live in a top-level <li> with a
    "has-menu" class. This was getting intercepted by an event handler that
    was blocking clicks from triggering links. The event selector now points
    to menus within that class.

    So what you need to do is active the browser's mobile mode. Set it for something like an iPhone 6S size. Reload the page. Then try clicking on actions within the menu. You'll definitely want to try actions a couple levels down, like the Close -> Submitted. Test this both before and after the proper fix.

    You should also try using your phone, along with the above test, to make sure it works on mobile devices. You can get the IP of your laptop and try using it instead of localhost in the browser. You might need to configure your firewall appropriately, if it's interfering.

    1. The reason for the extra testing is that, as currently written, that event handler never did anything on the page, so effectively the event handler has been disabled, regressing behavior in mobile mode.

  3. 
      
Christian Hammond
  1. 
      
  2. Missing period in the second-to-last paragraph of the description.

  3. For the summary, "in Review Request" is kind of akwardly-worded and not quite correct. I'd suggest the summary be:

    Fixed the Update -> Add File review request action not responding to clicks.
    
  4. 
      
Nicholas Mercier
Christian Hammond
  1. 
      
  2. reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     

    This one can do a return false and get rid of e. It's equivalent of these commands. The reason we'd specifically call them is to ensure that they're called before any operations we care about occur, in case ours crash. In this particular case, we don't run any special code, so we can just return false and let jQuery do its thing.

  3. 
      
Nicholas Mercier
David Trowbridge
  1. Ship It!
  2. 
      
Nicholas Mercier
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (ccb6ce7)
Loading...