Summary:

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

Review Request #9207 — Created Sept. 22, 2017 and submitted Oct. 14, 2017, 2:55 p.m.

Information
Owner:	nicho
Repository:	Review Board
Branch:	master
Bugs:	4592
Depends On:
Commit:	d2db62d...
Reviewers
Groups:	reviewboard, students
People:

Description

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.

Testing Done

Ran JSTests.

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

Issues

Description	From	Last Updated
Also the description should always wrap at 70-75 characters. I recommend writing the description in your commit message. Many editors …	chipx86	Sept. 23, 2017, 9:41 a.m.
The description doesn't make it clear what the source of the problem was or how this fixes it. Descriptions should …	chipx86	Sept. 29, 2017, 3:33 p.m.
Missing period in the second-to-last paragraph of the description.	chipx86	Oct. 4, 2017, 5:58 p.m.
For the summary, "in Review Request" is kind of akwardly-worded and not quite correct. I'd suggest the summary be: Fixed …	chipx86	Oct. 4, 2017, 5:58 p.m.
"This method displays a popup..."	brennie	Sept. 23, 2017, 7:53 a.m.
Only one space between sentences. We're not using typewriters here!	david	Oct. 2, 2017, 7:40 a.m.
The reviewRequest variable is only used once, so we could remove the variable definition and juts move the model.get call …	david	Oct. 3, 2017, 5:44 p.m.
Undo this.	brennie	Oct. 3, 2017, 5:52 p.m.
Lets accept e (type Event) as an argument and call e.stopPropagation() and e.preventDefault() instead.	brennie	Oct. 3, 2017, 5:47 p.m.
Trailing whitespace	brennie	Oct. 3, 2017, 6:03 p.m.
Care to update this to use e.preventDefault() and e.stopPropagation() ? It too can be done at the top of the …	brennie	Oct. 3, 2017, 6:10 p.m.
No longer true. This function doesn't return anything.	brennie	Oct. 3, 2017, 6:15 p.m.
Mind updating this function to use preventDefault and stopPropagation too?	brennie	Oct. 3, 2017, 6:16 p.m.
No longer true. This function doesn't return anything.	brennie	Oct. 3, 2017, 6:16 p.m.
Mind adding Args to all these functions?	brennie	Oct. 3, 2017, 6:33 p.m.
Undo this.	brennie	Oct. 3, 2017, 6:33 p.m.
This is on the wrong class. It needs to be on RB.ReviewablePageView. Can you move it, and then we'll need …	chipx86	Oct. 4, 2017, 6:26 p.m.
This one can do a return false and get rid of e. It's equivalent of these commands. The reason we'd …	chipx86	Oct. 5, 2017, 6:58 a.m.

flake8 passed.

JSHint passed.

Show all issues

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?

david Sept. 23, 2017, 8:09 a.m.

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.

nicho Sept. 23, 2017, 9:24 a.m.

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.

chipx86 Sept. 28, 2017, 11:35 p.m.

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:

Go ahead and move the Upload File handler to ReviewRequestEditorView.
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.

Show all issues

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.

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1)
Show all issues
```
"This method displays a popup..."
```

Change Summary:

Updated description to be more specific.

Summary:

-	Fixed Update -> Add File not doing anything in Review Request
+	Fixed Update -> Add File not doing anything in Review Request.

Description:

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

~		The onUploadFileClicked() function was moved from reviewablePageView.es6.js to reviewRequestEditorView.es6.js, and the element's click function set to it.
	~	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.es6.js
	+	to reviewRequestEditorView.es6.js, and the element's click function set to
	+	it from the setupActions() function. 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.
	+
	+	After the fix, the upload file dialog appears as intended.

Testing Done:

		Ran JSTests.

~		Tested manually by clicking Update -> Add File, where the upload file dialog appears.
	~	Tested manually by clicking Update -> Add File, where the upload file
	+	dialog appears.

Commit:

-	1d2b98d473d4b57b818a35d412b4a81a9181eefd
+	c6825d1f055af43d0a1dd74fac6c50b9fa8df57e

Diff:

Revision 2 (+22 -20)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Ship It!
```

```
Ship It!
```

Change Summary:

Moved the Upload File handler to ReviewRequestEditorView, and the .has-menu handler to ReviewablePageView.

Description:

		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.es6.js
~		to reviewRequestEditorView.es6.js, and the element's click function set to
~		it from the setupActions() function. 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.
	~	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.

Commit:

-	c6825d1f055af43d0a1dd74fac6c50b9fa8df57e
+	55c111a06f5c03487600432c88ed1b8fbc0faf89

Diff:

Revision 3 (+38 -36)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 3)
Show all issues
```
Only one space between sentences. We're not using typewriters here!
```

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3)

Show all issues

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.

nicho Oct. 3, 2017, 5:32 p.m.

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

chipx86 Oct. 3, 2017, 5:38 p.m.

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

Change Summary:

Removed extra space in documentation.

Commit:

-	55c111a06f5c03487600432c88ed1b8fbc0faf89
+	9ef4f4c6fe10d04b99f91f7488f989e28d739bea

Diff:

Revision 4 (+38 -38)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 4)
Show all issues
```
Undo this.
```

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 4)

Show all issues

Lets accept e (type Event) as an argument and call e.stopPropagation() and e.preventDefault() instead.

Change Summary:

Removed redundant variable, readded line break that was accidentally removed and changed how the event propagation was being stopped in onUploadFileClicked.

Commit:

-	9ef4f4c6fe10d04b99f91f7488f989e28d739bea
+	ed9de0f8a786ba4a6ccb9aaab81a8b28e27c0a71

Diff:

Revision 5 (+39 -38)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 5)
Show all issues
```
Trailing whitespace
```

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 5)

Show all issues

Care to update this to use e.preventDefault() and e.stopPropagation() ? It too can be done at the top of the fn.

Change Summary:

Removed trailing whitespace, and changed how event propagation is consumed in onShipItClicked.

Commit:

-	ed9de0f8a786ba4a6ccb9aaab81a8b28e27c0a71
+	2535d454f6bd21bbf5c7a43b683ae9be474d0ecd

Diff:

Revision 6 (+40 -37)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 6)
Show all issues
```
No longer true. This function doesn't return anything.
```

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 6)

Show all issues

Mind updating this function to use preventDefault and stopPropagation too?

reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 6)
Show all issues
```
No longer true. This function doesn't return anything.
```

Change Summary:

Changed how onMenuClicked stops event propagation, removed documentation on return values for all functions that no longer return false to consume events.

Commit:

-	2535d454f6bd21bbf5c7a43b683ae9be474d0ecd
+	5e9d94e75d4452bc0867cbb4776a463bcdfa7e2c

Diff:

Revision 7 (+36 -43)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 7)
Show all issues
```
Mind adding Args to all these functions?
```
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 7)
Show all issues
```
Undo this.
```

Change Summary:

Added args to all functions which were changed to take an Event as a parameter.

Commit:

-	5e9d94e75d4452bc0867cbb4776a463bcdfa7e2c
+	158f8eba460f111e0395b3255ca04c5a02016347

Diff:

Revision 8 (+46 -42)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 8)

Show all issues

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.

chipx86 Oct. 4, 2017, 5:52 p.m.

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.

Show all issues

Missing period in the second-to-last paragraph of the description.

Show all issues

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.

Change Summary:

Moved .has-menu click event registration to RB.ReviewablePageView.

Summary:

-	Fixed Update -> Add File not doing anything in Review Request.
+	Fixed the Update -> Add File review request action not responding to clicks.

Testing Done:

		Ran JSTests.

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

Commit:

-	158f8eba460f111e0395b3255ca04c5a02016347
+	30c6b6892080b96915449b4e77476e2a5b9cb103

Diff:

Revision 9 (+46 -42)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 9)

Show all issues

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.

Change Summary:

Changed onShipItClicked to return false rather than calling methods to stop the event propogation.

Commit:

-	30c6b6892080b96915449b4e77476e2a5b9cb103
+	d2db62dce8215ffc41be63a4a5ad957cfb184b52

Diff:

Revision 10 (+40 -37)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (ccb6ce7)