Summary

Fixed disabled buttons if an error happens updating a review field.

Review Request #10689 — Created Sept. 6, 2019 and updated Sept. 7, 2019, 1:20 p.m.

Information

Owner

amalik2

Repository

Review Board

Branch

master

Bugs

4777

Depends On

Reviewers

Groups

reviewboard, students

People

Description

The publish button ends up being disabled while the changes made to the
model are being saved, so that the user does not repeatedly click the
publish button. If the request fails due to an error with one of the
fields that were changed, then the publish button does not ever end up
being re-enabled. Now, the model emits an event when an error happens
in the above case, and the view listens for that event being published.
When the view is notified of that event, it re-enables the buttons.

Testing Done

I tested this locally by following the instructions listed on the bug
report page (bug #4777).

I also ran the JavaScript unit tests.

Commits

Summary	ID	Author
Fixed publish being disabled when error occurs while saving review request field.	380ddd8571c53a98797f281e55fbbd855f6b79d0	amalik2

Issues

Description	From	Last Updated
Same comment here as on /r/10688 regarding the format we like for commit descriptions. Also note that you should keep …	chipx86	Sept. 6, 2019, 1:43 p.m.
Can you wrap your description at 72 characters?	brennie	Sept. 7, 2019, 8:11 a.m.
We can't currently land review requests that have more than one commit (I never got around to writing that before …	brennie	Sept. 7, 2019, 8:41 a.m.
One small nit and then I'm happy. Markdown is super great in a review request, but becomes the content for …	chipx86	Sept. 7, 2019, 1:15 p.m.
We prefer single quotes over double quotes when possible (exceptions being when the text itself contains a single quote).	chipx86	Sept. 6, 2019, 1:41 p.m.
This should also use single quotes. Lines must also be less than 80 characters in length.	chipx86	Sept. 6, 2019, 1:44 p.m.
Single quotes here as well.	chipx86	Sept. 6, 2019, 1:41 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Same comment here as on /r/10688 regarding the format we like for commit descriptions.

Also note that you should keep lines at around the 72 character point, so make sure your editor is set to wrap there.

reviewboard/static/rb/js/models/reviewRequestEditorModel.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

We prefer single quotes over double quotes when possible (exceptions being when the text itself contains a single quote).

reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
This should also use single quotes.
Lines must also be less than 80 characters in length.
```
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Single quotes here as well.
```

Description:: ~
Fixed publish button being disabled after an invalid user or group is specified.
~
Fixed the publish button being disabled after an invalid user or group is specified.
+
+
The publish button ends up being disabled while the changes made to the model are being saved, so that the user does not repeatedly click the publish button. If the request fails due to an error with one of the fields that were changed, then the publish button does not ever end up being re-enabled. Now, the model emits an event when an error happens in the above case, and the view listens for that event being published. When the view is notified of that event, it re-enables the buttons.

Commits:

Summary	ID	Author
Fixed publish button being disabled when an invalid user or group is specified.	1dfaa19bb9d726c25eebbf618862538ff2241328	amalik2
Fixed publish button being disabled when an invalid user or group is specified.	1dfaa19bb9d726c25eebbf618862538ff2241328	amalik2
Changed ocurrences of double-quotes to single-quotes.	8d241e8dafd90a60b3ce0f6b08b46f1313270c67	amalik2

Diff:

Revision 2 (+37 -3)

Show changes

	reviewboard/static/rb/js/models/reviewRequestEditorModel.es6.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

Summary	ID	Author
Fixed publish button being disabled when an invalid user or group is specified.	1dfaa19bb9d726c25eebbf618862538ff2241328	amalik2
Changed ocurrences of double-quotes to single-quotes.	8d241e8dafd90a60b3ce0f6b08b46f1313270c67	amalik2
Fixed publish button being disabled when an invalid user or group is specified.	1dfaa19bb9d726c25eebbf618862538ff2241328	amalik2
Changed ocurrences of double-quotes to single-quotes.	8d241e8dafd90a60b3ce0f6b08b46f1313270c67	amalik2
Fixed line being longer than 80 characters.	d2c488279f3ed43a265e9f1195156b583951dec0	amalik2

Diff:

Revision 3 (+40 -4)

Show changes

	reviewboard/static/rb/js/models/reviewRequestEditorModel.es6.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you wrap your description at 72 characters?

The issue has been resolved. Show all issues

We can't currently land review requests that have more than one commit (I never got around to writing that before I left ) so can you squash your changes together?

Even if we could, we would rather avoid fixup commits since they don't really add anything to the history.

If you're unsure about how to squash them I'm happy to show you.

Description:: ~
Fixed the publish button being disabled after an invalid user or group is specified.
~
Fixed the publish button being disabled after an invalid user or group
+ is specified.

~
The publish button ends up being disabled while the changes made to the model are being saved, so that the user does not repeatedly click the publish button. If the request fails due to an error with one of the fields that were changed, then the publish button does not ever end up being re-enabled. Now, the model emits an event when an error happens in the above case, and the view listens for that event being published. When the view is notified of that event, it re-enables the buttons.
~
The publish button ends up being disabled while the changes made to the
+ model are being saved, so that the user does not repeatedly click the
+ publish button. If the request fails due to an error with one of the
+ fields that were changed, then the publish button does not ever end up
+ being re-enabled. Now, the model emits an event when an error happens
+ in the above case, and the view listens for that event being published.
+ When the view is notified of that event, it re-enables the buttons.
Testing Done:: ~
I tested this locally by following the instructions listed on the bug report page
~
I tested this locally by following the instructions listed on the bug
+ report page

I also ran the JavaScript unit tests

Commits:

Summary	ID	Author
Fixed publish button being disabled when an invalid user or group is specified.	1dfaa19bb9d726c25eebbf618862538ff2241328	amalik2
Changed ocurrences of double-quotes to single-quotes.	8d241e8dafd90a60b3ce0f6b08b46f1313270c67	amalik2
Fixed line being longer than 80 characters.	d2c488279f3ed43a265e9f1195156b583951dec0	amalik2
Fixed publish being disabled when error occurs while saving review request field.	380ddd8571c53a98797f281e55fbbd855f6b79d0	amalik2

Diff:

Revision 4 (+48 -4)

Show changes

	reviewboard/static/rb/js/models/reviewRequestEditorModel.es6.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.es6.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:: Fixed publish button being disabled when an invalid user or group is specified.
Fixed disabled buttons if an error happens updating a review field.
Description:: -
Fixed the publish button being disabled after an invalid user or group
- is specified.
-

The publish button ends up being disabled while the changes made to the
model are being saved, so that the user does not repeatedly click the
publish button. If the request fails due to an error with one of the
fields that were changed, then the publish button does not ever end up
being re-enabled. Now, the model emits an event when an error happens
in the above case, and the view listens for that event being published.
When the view is notified of that event, it re-enables the buttons.

Fix it!

An issue was opened. Show all issues

One small nit and then I'm happy.

Markdown is super great in a review request, but becomes the content for the commit message, and Markdown is not a standardized thing there. So the link in Testing Done is going to look strange.

Since the bug report is already listed, and will be included in the commit message, just report the link. You can instead say "... listed on bug #4777."

chipx86 Sept. 7, 2019, 1:15 p.m.

Oh, and the sentences should end with a period.

Testing Done:: I tested this locally by following the instructions listed on the bug
~ report page
~ report page (bug #4777).

~
I also ran the JavaScript unit tests
~
I also ran the JavaScript unit tests.

~		Fixed publish button being disabled after an invalid user or group is specified.
	~	Fixed the publish button being disabled after an invalid user or group is specified.
	+
	+	The publish button ends up being disabled while the changes made to the model are being saved, so that the user does not repeatedly click the publish button. If the request fails due to an error with one of the fields that were changed, then the publish button does not ever end up being re-enabled. Now, the model emits an event when an error happens in the above case, and the view listens for that event being published. When the view is notified of that event, it re-enables the buttons.

~		Fixed the publish button being disabled after an invalid user or group is specified.
	~	Fixed the publish button being disabled after an invalid user or group
	+	is specified.

~		The publish button ends up being disabled while the changes made to the model are being saved, so that the user does not repeatedly click the publish button. If the request fails due to an error with one of the fields that were changed, then the publish button does not ever end up being re-enabled. Now, the model emits an event when an error happens in the above case, and the view listens for that event being published. When the view is notified of that event, it re-enables the buttons.
	~	The publish button ends up being disabled while the changes made to the
	+	model are being saved, so that the user does not repeatedly click the
	+	publish button. If the request fails due to an error with one of the
	+	fields that were changed, then the publish button does not ever end up
	+	being re-enabled. Now, the model emits an event when an error happens
	+	in the above case, and the view listens for that event being published.
	+	When the view is notified of that event, it re-enables the buttons.

~		I tested this locally by following the instructions listed on the bug report page
	~	I tested this locally by following the instructions listed on the bug
	+	report page

		I also ran the JavaScript unit tests

		I tested this locally by following the instructions listed on the bug
~		report page
	~	report page (bug #4777).

~		I also ran the JavaScript unit tests
	~	I also ran the JavaScript unit tests.