Summary

Enable Desktop Notifications

Review Request #7660 — Created Sept. 25, 2015 and submitted March 22, 2016, 6:52 p.m.

Information

Owner

kvdgulik

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

8072

Reviewers

Groups

reviewboard

People

Description

This task created a desktop notification to send to users when an update has been made to a review request. This involved adding a JSON column to the user profile called settings to represent user settings such as their preference on recieving desktop notifications. Users can enable or disable the desktop notifications using an option in the account setting form.

Testing Done

Tested on Chrome, Firefox, Internet Explorer and Safari.

Test Cases
1. Notifications enabled both in permissions and in account settings, and browser supported
expected: notification is shows
2. Notifications enabled in permissions but not in account settings, and browser supported
expected: updates bubble displayed
3. Notifications disabled in permissions but enabled in account settings, and browser supported
expected: updates bubble displayed
4. Notifications not supported by browser (Internet Explorer)
expected: updates bubble displayed

The JS Unit tests were also run during the testing for this task.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	Sept. 29, 2015, 1:21 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Sept. 29, 2015, 1:22 p.m.
Col: 72 W291 trailing whitespace	reviewbot	Sept. 29, 2015, 1:22 p.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Sept. 29, 2015, 1:22 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Sept. 29, 2015, 1:22 p.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	Sept. 29, 2015, 5:06 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Sept. 29, 2015, 5:06 p.m.
Col: 72 W291 trailing whitespace	reviewbot	Sept. 29, 2015, 5:06 p.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Sept. 29, 2015, 5:06 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Sept. 29, 2015, 5:07 p.m.
Missing semicolon.	brennie	Sept. 29, 2015, 4:52 p.m.
Needs doc comment.	brennie	Sept. 29, 2015, 4:54 p.m.
Single quotes.	brennie	Sept. 29, 2015, 4:54 p.m.
We can do this in initialize() and then set e.g., this._shouldUseDesktopNotifications so we only have to do it once.	brennie	Sept. 29, 2015, 4:57 p.m.
No blank line here.	brennie	Sept. 29, 2015, 4:57 p.m.
No blank line here.	brennie	Sept. 29, 2015, 4:58 p.m.
We try not to go over 80 characters. We should use gettext() for the string with a %s-interpolation e.g. (gettext("Review …	brennie	Sept. 29, 2015, 5 p.m.
This should be a constant on the object.	brennie	Sept. 29, 2015, 5:05 p.m.
No blank line here.	brennie	Sept. 29, 2015, 5:06 p.m.
Single quotes.	brennie	Sept. 29, 2015, 5:06 p.m.
Can we reformat this as follows: 'enable_desktop_notifications': self.profile.settings.get( 'enable_desktop_notifications', True), If not, can we pull this out into a variable …	brennie	Oct. 10, 2015, 5:11 p.m.
Col: 45 E502 the backslash is redundant between brackets	reviewbot	Oct. 13, 2015, 1:36 p.m.
Col: 17 E131 continuation line unaligned for hanging indent	reviewbot	Oct. 13, 2015, 1:36 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 13, 2015, 1:36 p.m.
This should go with the first var statement at the beginning, e.g. var x = ..., y = ...; Also, …	brennie	Oct. 10, 2015, 5:14 p.m.
This conditional should be wrapped in parentheses.	brennie	Oct. 10, 2015, 5:16 p.m.
There should be a space between ) and {.	brennie	Oct. 10, 2015, 5:16 p.m.
This expression should be wrapped in parentheses. Also, the second and third conditions need to be indented four spaces, not …	brennie	Oct. 10, 2015, 5:17 p.m.
This will overwrite window.Notification. Can we set this as a method on the object?	brennie	Oct. 13, 2015, 10:28 a.m.
Blank line between statement and beginning of block.	brennie	Oct. 10, 2015, 5:18 p.m.
Update this docstring to refer to the fact that it also can use web notifications.	brennie	Oct. 10, 2015, 5:19 p.m.
Can you document the arguments to this function? We document them like: /* * ... * * Args: * info …	brennie	Oct. 13, 2015, 2:46 p.m.
receive	brennie	Oct. 10, 2015, 5:22 p.m.
Missing space between ) and {	brennie	Oct. 10, 2015, 5:22 p.m.
Missing a period. Also, can you document the args as above?	brennie	Oct. 13, 2015, 1:41 p.m.
Missing a period. Also, can you document the args as above?	brennie	Oct. 13, 2015, 1:41 p.m.
So this overrides window.Notification again. We should use a method on the object. We should only check for it once …	brennie	Oct. 13, 2015, 10:32 a.m.
This checking should be done in receiveNotification.	brennie	Oct. 13, 2015, 10:32 a.m.
These need to be declared at the top of the function.	brennie	Oct. 10, 2015, 5:30 p.m.
Missing a space between ) and {	brennie	Oct. 10, 2015, 5:31 p.m.
You can use _.delay(_.bind(...), ...)	brennie	Oct. 13, 2015, 2:47 p.m.
This should be formatted as: } else if (Notification.permission === 'denied' \|\| Notificaiton.permission === 'default') { Also, this can be …	brennie	Oct. 10, 2015, 5:37 p.m.
We don't enforce line ending limits for HTML templates. This can be written as enableDesktopNotifications: {{user_profile.settings.enable_desktop_notifications\|yesno:'true,false'}}	brennie	Oct. 13, 2015, 1:44 p.m.
Needs a trailing comma. This is to prevent having to modify this line later when adding a new line.	chipx86	Oct. 23, 2015, 10:47 a.m.
This is missing: from __future__ import unicode_literals which must appear before the other imports, with a blank line in-between.	chipx86	Oct. 23, 2015, 10:48 a.m.
Needs a trailing comma.	chipx86	Oct. 23, 2015, 10:49 a.m.
Should be "Show desktop notifications ..."	chipx86	Oct. 23, 2015, 10:50 a.m.
This should use the property you defined above (self.profile.should_enable_desktop_notifications)	david	Oct. 16, 2015, 3:05 p.m.
This should reuse the convenience function provided on the profile, so that we don't have two places determining a default …	chipx86	Oct. 23, 2015, 10:51 a.m.
"Return", not "Get". "Get" implies that the function is going to get something from somewhere. "Return" implies it's going to …	chipx86	Oct. 23, 2015, 10:51 a.m.
This also needs a "Returns" block, like: Returns: bool: <description of the return value here>	chipx86	Oct. 23, 2015, 10:53 a.m.
Because settings defaults to null, we need to check here that it's not None before calling .get()	david	Oct. 16, 2015, 3:06 p.m.
This should be formatted more like: this._shouldUseDesktopNotifications = ( 'Notification' in window && RB.UserSession.instance.get(....));	chipx86	Oct. 23, 2015, 10:54 a.m.
Probably you can skip the first half of this and just see if this._notificationType is undefined.	david	Oct. 23, 2015, 12:58 p.m.
So right now, we have all this code, and some code below, that deals with cross-browser setup and requests for …	chipx86	Oct. 31, 2015, 12:39 p.m.
Can we align the \|\| with the opening (? We also tend to put operators on the previous line. this._notificationType …	david	Oct. 16, 2015, 3:09 p.m.
These two checks could be combined into the same if statement. Also make sure there's a space before opening {s.	david	Oct. 16, 2015, 3:10 p.m.
First line of function docs should be a summary fitting on one line without wrapping. You can then expand on …	chipx86	Oct. 23, 2015, 11:13 a.m.
Space between if and (. It might also be nice to switch the order of the conditional: if (this._shouldUseDesktopNotifications && …	david	Oct. 16, 2015, 3:11 p.m.
We always want to show the in-page bubble. If I get a popup on my screen, I need to be …	chipx86	Oct. 23, 2015, 11:14 a.m.
Same as above. The first line should be a summary fitting on a single line.	chipx86	Oct. 23, 2015, 11:15 a.m.
Same as above.	chipx86	Oct. 23, 2015, 11:15 a.m.
This can be simplified using JavaScript's \|\| operator: var username = info.user.fullname \|\| info.user.username;	david	Oct. 16, 2015, 3:11 p.m.
We generally combine var definitions into a single statement: var username = ..., notificationText = ...;	david	Oct. 16, 2015, 3:12 p.m.
Extra trailing whitespace should be removed.	chipx86	Oct. 23, 2015, 11:21 a.m.
Space between the , and {	david	Oct. 16, 2015, 3:12 p.m.
Please indent these two lines one more level.	david	Oct. 23, 2015, 10:45 a.m.
As I mention below, if you have a local var for this._notification you can skip the extra bind.	david	Oct. 23, 2015, 10:46 a.m.
This could be formatted much more nicely: _.delay( _.bind(function() { this._notification.close(); }, this), RB.ReviewablePageView.NOTIFICATION_LIFETIME_MSECS); Even better, if you keep a …	david	Oct. 23, 2015, 10:47 a.m.
Col: 44 W291 trailing whitespace	reviewbot	Oct. 31, 2015, 12:41 p.m.
This should be indented 4 spaces.	brennie	Oct. 31, 2015, 12:41 p.m.
No parens needed.	brennie	Oct. 31, 2015, 12:42 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 31, 2015, 12:41 p.m.
Remove this whitespace.	brennie	Oct. 31, 2015, 12:43 p.m.
"the user's browser"	CH chronicleyu	Oct. 31, 2015, 12:48 p.m.
Reformat this as: this._canNotify = ( this.notificationType !== undefined && RB.UserSession.instance.get('...')); Also, missing a semi-colon and there is trailing whitespace.	brennie	Oct. 31, 2015, 12:44 p.m.
Capital 'F' for the false.	CH chronicleyu	Oct. 31, 2015, 12:49 p.m.
Unindent these.	brennie	Nov. 7, 2015, 11:27 a.m.
No blank line here.	brennie	Oct. 31, 2015, 12:47 p.m.
Unindent.	brennie	Oct. 31, 2015, 12:48 p.m.
You don't have to wrap notification.close() in a function, e.g.: _.delay(notification.close, RB.NotificationManager.NOTIFICATION_LIFETIME_MSECS);	brennie	Oct. 31, 2015, 12:48 p.m.
Col: 44 W291 trailing whitespace	reviewbot	Oct. 31, 2015, 12:50 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 31, 2015, 12:50 p.m.
There's an extra ' in here.	david	Nov. 6, 2015, 10:08 a.m.
Should be a lower-case 'true'	david	Nov. 6, 2015, 10:08 a.m.
Actually, the way you had it before was correct (since this is JavaScript, not Python). However, this should end in …	david	Nov. 6, 2015, 10:09 a.m.
I don't think this works (since notification.close() will call immediately). However, you probably can do: _.delay(_.bind(notification.close, notification), RB.NotificationManager.NOTIFICATION_LIFETIME_MSECS);	david	Nov. 6, 2015, 10:15 a.m.
I think this should be _showUpdatesBubble	brennie	Nov. 6, 2015, 10:06 a.m.
Fix indentation	brennie	Nov. 7, 2015, 11:28 a.m.
Fix indentation	brennie	Nov. 7, 2015, 11:28 a.m.
This file uses tabs instead of spaces. We use 4 spaces for all our JS.	brennie	Nov. 6, 2015, 10:07 a.m.
Register	brennie	Nov. 13, 2015, 11:32 a.m.
Catch	brennie	Nov. 13, 2015, 11:32 a.m.
Create	brennie	Nov. 13, 2015, 11:32 a.m.
Show	brennie	Nov. 13, 2015, 11:33 a.m.
will create.	brennie	Nov. 13, 2015, 11:33 a.m.
Missing period.	brennie	Nov. 13, 2015, 11:33 a.m.
Remove this line.	brennie	Nov. 13, 2015, 11:34 a.m.
"Set up"	brennie	Nov. 13, 2015, 11:34 a.m.
Dedent by 4	brennie	Nov. 13, 2015, 11:34 a.m.
Return. Missing period.	brennie	Nov. 13, 2015, 11:34 a.m.
"Send" The first sentence of a doccomment should be a single line.	brennie	Nov. 13, 2015, 11:36 a.m.
Missing period.	brennie	Nov. 13, 2015, 11:36 a.m.
Missing a period. Also doesnt tell us that the following list is a list of keys.	brennie	Nov. 13, 2015, 11:37 a.m.
Backquotes, not single quotes.	brennie	Nov. 13, 2015, 11:39 a.m.
"Create" First sentence of a doc comment should fit on a single line. Also, minor typos: ``RB.notificationManager.notifiy	brennie	Nov. 13, 2015, 11:51 a.m.
second line should be indented one more space	brennie	Nov. 13, 2015, 11:52 a.m.
should be indented one more space.	brennie	Nov. 13, 2015, 11:52 a.m.
backquotes, not single quotes.	brennie	Nov. 13, 2015, 11:53 a.m.
"Call"	brennie	Nov. 13, 2015, 11:53 a.m.
Call. shouldNotify.	brennie	Nov. 13, 2015, 11:53 a.m.
"Call". Needs args.	brennie	Nov. 13, 2015, 11:58 a.m.
"Return" Needs args.	brennie	Nov. 13, 2015, 11:58 a.m.
API	brennie	Nov. 13, 2015, 11:58 a.m.
"Otherwise,"	brennie	Nov. 17, 2015, 10:22 a.m.
"then use their preference" "Otherwise,"	brennie	Nov. 17, 2015, 10:22 a.m.
Missing a period. Also this should be: `:py:data:`True`	brennie	Nov. 17, 2015, 10:23 a.m.
No need for an else clause. We can just have return True.	brennie	Nov. 17, 2015, 10:24 a.m.
The bubble will always be displayed, no?	brennie	Nov. 17, 2015, 10:26 a.m.
Missing a period.	brennie	Nov. 17, 2015, 10:26 a.m.
"This function will handle the review updated ..."	brennie	Nov. 17, 2015, 10:27 a.m.
Missing a period.	brennie	Nov. 17, 2015, 10:27 a.m.
Missing period.	brennie	Nov. 17, 2015, 10:27 a.m.
Missing period.	brennie	Nov. 17, 2015, 10:27 a.m.
Missing period.	brennie	Nov. 17, 2015, 10:27 a.m.
Missing period.	brennie	Nov. 17, 2015, 10:29 a.m.
"This function will create ..." "notification"	brennie	Nov. 17, 2015, 10:30 a.m.
", then we will send"	brennie	Nov. 17, 2015, 10:30 a.m.
Missing period.	brennie	Nov. 17, 2015, 10:30 a.m.
"This function will tell.	brennie	Nov. 17, 2015, 10:31 a.m.
It, not This.	brennie	Nov. 17, 2015, 10:31 a.m.
``true``	brennie	Nov. 17, 2015, 10:32 a.m.
"Otherwise, ``false`` will be returned.	brennie	Nov. 17, 2015, 10:32 a.m.
``title`` (``String``) Note the space. Also, missing a period. Capitalization of the.	brennie	Nov. 17, 2015, 10:33 a.m.
Same as above.	brennie	Nov. 17, 2015, 10:34 a.m.
Same as above.	brennie	Nov. 17, 2015, 10:34 a.m.
Same as above.	brennie	Nov. 17, 2015, 10:34 a.m.
Capitilizaiton and missing period.	brennie	Nov. 17, 2015, 10:35 a.m.
Same as above.	brennie	Nov. 17, 2015, 10:35 a.m.
Same as above.	brennie	Nov. 17, 2015, 10:35 a.m.
Same as above.	brennie	Nov. 17, 2015, 10:35 a.m.
See my comments in the other doc-comment.	brennie	Nov. 17, 2015, 10:35 a.m.
Capitilzation and missing period.	brennie	Nov. 17, 2015, 10:36 a.m.
Same as above.	brennie	Nov. 17, 2015, 10:39 a.m.
Same as above.	brennie	Nov. 17, 2015, 10:39 a.m.
Same as ahove.	brennie	Nov. 17, 2015, 10:39 a.m.
See my comments on the other doc comment.	brennie	Nov. 17, 2015, 10:39 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Nov. 17, 2015, 10:50 a.m.
This should be in sentence casing, like the other options.	chipx86	Nov. 20, 2015, 12:07 p.m.
This can be: return (not self.settings or self.settings.get('enable_desktop_notifications', True))	chipx86	Nov. 20, 2015, 12:08 p.m.
The function name would be better as _onReviewRequestUpdated.	chipx86	Nov. 20, 2015, 12:09 p.m.
The /* is indented one space too far.	chipx86	Nov. 20, 2015, 12:10 p.m.
"and send", no "s".	chipx86	Nov. 20, 2015, 12:10 p.m.
This isn't true anymore, right? We're always sending the bubble now.	chipx86	Nov. 20, 2015, 12:10 p.m.
Blank line between these.	chipx86	Nov. 20, 2015, 12:11 p.m.
No need to capitalize "Updates Bubble."	chipx86	Nov. 20, 2015, 12:11 p.m.
Indentation is wonky here. Make sure you run jshint to catch these.	chipx86	Nov. 20, 2015, 12:13 p.m.
"review request update," or just "update."	chipx86	Nov. 20, 2015, 12:15 p.m.
I'm having a hard time parsing that sentence. I think it's the "used" bit and maybe "the notification."	chipx86	Nov. 20, 2015, 12:15 p.m.
Indenation here and below all looks wrong, too. Go through everything and make sure every indentation level is an even …	chipx86	Nov. 20, 2015, 12:17 p.m.
Most of this looks like stuff that could easily be inline in the call below.	chipx86	Nov. 26, 2015, 11:36 a.m.
This should go into more detail on what this is responsible for, requirements, etc.	chipx86	Nov. 28, 2015, 5:05 p.m.
The entire purpose of what this class does is express visual notifications based off data, so this by definition should …	chipx86	Nov. 20, 2015, 12:19 p.m.
"for the user browser" doesn't make sense. "user's browser" ?	chipx86	Nov. 20, 2015, 12:20 p.m.
Should be sentence casing. Also, this sentence is a bit confusing. I don't think there's a notification telling us about …	chipx86	Nov. 20, 2015, 12:20 p.m.
Most of the stuff in here should be in initialize. Any variable state defaults should be there. The only thing …	chipx86	Nov. 20, 2015, 12:29 p.m.
This should just be a constant on the model.	chipx86	Nov. 20, 2015, 12:34 p.m.
4 space indentation on everything.	chipx86	Nov. 20, 2015, 12:36 p.m.
Too many parens. They're not needed here.	chipx86	Nov. 20, 2015, 12:37 p.m.
Indentation is off here as well. Same with ones below. Make sure to check every comment.	chipx86	Nov. 21, 2015, 11 a.m.
Indentation is off here as well.	chipx86	Nov. 21, 2015, 11:01 a.m.
Blank line between these.	chipx86	Nov. 20, 2015, 12:40 p.m.
No backticks for the types.	chipx86	Nov. 20, 2015, 12:43 p.m.
"URL String" isn't a type. It's just a String.	chipx86	Nov. 20, 2015, 12:44 p.m.
the /* is indented one space too far.	chipx86	Nov. 20, 2015, 12:44 p.m.
Blank line between each argument.	chipx86	Nov. 20, 2015, 12:47 p.m.
Blank line between these.	chipx86	Nov. 20, 2015, 12:47 p.m.
I'm not sure this is useful. This function is just taking positional arguments and turning it into an object of …	chipx86	Nov. 20, 2015, 12:48 p.m.
Space before {.	chipx86	Nov. 20, 2015, 2:26 p.m.
This is only needed by the prototype implementation above, so I'd move it there. Then, update call sites to use …	chipx86	Nov. 26, 2015, 12:10 p.m.
We shouldn't have a copy of every single function, since those can all just access the instance. A wrapper function …	chipx86	Nov. 20, 2015, 12:51 p.m.
The test suite needs a unit test for every function available to it, under every condition, simulating the ability to …	chipx86	Nov. 28, 2015, 5 p.m.
Sentence casing.	chipx86	Nov. 20, 2015, 2:20 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Nov. 20, 2015, 4:13 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Nov. 20, 2015, 4:13 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Nov. 20, 2015, 4:13 p.m.
I've been informed in my work that this should just be: ``True``	brennie	Nov. 28, 2015, 5:01 p.m.
Remove this blank line.	brennie	Nov. 28, 2015, 5:02 p.m.
The indentation here is a little off. It should be: NOTIFICATION_TYPE: (window.Notification \|\| window.mozNotification \|\| window.webkitNotification)	brennie	Dec. 4, 2015, 10:34 a.m.
Single quotes.	brennie	Dec. 4, 2015, 10:35 a.m.
Single quotes.	brennie	Dec. 4, 2015, 10:35 a.m.
Can you move this function definition inline into the notify call?	brennie	March 22, 2016, 5:52 p.m.
There is no var statement for this variable.	brennie	March 22, 2016, 5:52 p.m.
I'd rather have this all inline: RB.Notificationmanager.instance.notify({ ... });	brennie	March 22, 2016, 5:52 p.m.
You don't need quotes around JS object keys.	brennie	March 22, 2016, 5:52 p.m.
Undo this.	brennie	March 22, 2016, 5:52 p.m.
Should be at top of function.	brennie	March 22, 2016, 5:54 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/evolutions/profile_enable_desktop_notifications.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/models.py
    reviewboard/accounts/forms/pages.py

Ignored Files:
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/evolutions/profile_enable_desktop_notifications.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/models.py
    reviewboard/accounts/forms/pages.py

Ignored Files:
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/evolutions/profile_enable_desktop_notifications.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (90 > 79 characters)
```
reviewboard/accounts/forms/pages.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/accounts/forms/pages.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 72
 W291 trailing whitespace
```
reviewboard/accounts/forms/pages.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

Description:

~		Created a desktop notification to send to users on update
~		Added a column to the user profile(enableDeskopNotifications) representing their preference on recieving desktop notifications
~		(WIP) Created an option in the account setting form to enable/disable desktop notifications
	~	- Created a desktop notification to send to users on update
	~	- Added a column to the user profile(enableDeskopNotifications) representing their preference on recieving desktop notifications
	~	- Created an option in the account setting form to enable/disable desktop notifications

Diff:

Revision 2 (+87 -9)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_enable_desktop_notifications.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/templates/base.html

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/evolutions/profile_enable_desktop_notifications.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/models.py
    reviewboard/accounts/forms/pages.py

Ignored Files:
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/evolutions/profile_enable_desktop_notifications.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/models.py
    reviewboard/accounts/forms/pages.py

Ignored Files:
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/evolutions/profile_enable_desktop_notifications.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (90 > 79 characters)
```
reviewboard/accounts/forms/pages.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/accounts/forms/pages.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 72
 W291 trailing whitespace
```
reviewboard/accounts/forms/pages.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/accounts/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

Description:

~		- Created a desktop notification to send to users on update
~		- Added a column to the user profile(enableDeskopNotifications) representing their preference on recieving desktop notifications
~		- Created an option in the account setting form to enable/disable desktop notifications
	~	Created a desktop notification to send to users on update
	~	Added a column to the user profile(enableDeskopNotifications) representing their preference on recieving desktop notifications
	~	Created an option in the account setting form to enable/disable desktop notifications
	+
	+	TODO: Clean up and move enableDesktopNotifications into settings JSON field

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Needs doc comment.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Single quotes.
```

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

The issue has been resolved. Show all issues

We can do this in initialize() and then set e.g., this._shouldUseDesktopNotifications so we only have to do it once.

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line here.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line here.
```

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

The issue has been resolved. Show all issues

We try not to go over 80 characters.

We should use gettext() for the string with a %s-interpolation

e.g. (gettext("Review request submitted by %s", username))

Also, can we rip out the ternary into its own variable to make the new Notification simpler?

kvdgulik Oct. 6, 2015, 3:53 p.m.

In regards to: "We should use gettext() for the string with a %s-interpolation. e.g. (gettext("Review request submitted by %s", username))" How do you guys currently do this in javascript? There doesn't seem to be built in functionality for this.

david Oct. 6, 2015, 3:58 p.m.

Django ships a javascript library for this. It actually involves calling two methods: gettext for the translated format string, and interpolate for the formatting itself.

See https://docs.djangoproject.com/en/1.6/topics/i18n/translation/#using-the-javascript-translation-catalog

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be a constant on the object.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line here.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Single quotes.
```

Diff:

Revision 3 (+107 -8)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/templates/base.html

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/forms/pages.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 45
 E502 the backslash is redundant between brackets
```
reviewboard/accounts/forms/pages.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 17
 E131 continuation line unaligned for hanging indent
```
reviewboard/accounts/forms/pages.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Summary:

-	WIP: Enable Desktop Notifications
+	Enable Desktop Notifications

Description:

~		Created a desktop notification to send to users on update
~		Added a column to the user profile(enableDeskopNotifications) representing their preference on recieving desktop notifications
~		Created an option in the account setting form to enable/disable desktop notifications
	~	- Created a desktop notification to send to users on update
	~	- Added a JSON column to the user profile(settings) to represent users settings such as their preference on recieving desktop notifications
	~	- Created an option in the account setting form to enable/disable desktop notifications
-
-		TODO: Clean up and move enableDesktopNotifications into settings JSON field

Testing Done:

	+	Tested on Chrome, Firefox, Internet Explorer and Safari.
	+
	+	Test Cases
	+	1. Notifications enabled both in permissions and in account settings, and browser supported
	+	expected: notification is shows
	+	2. Notifications enabled in permissions but not in account settings, and browser supported
	+	expected: updates bubble displayed
	+	3. Notifications disabled in permissions but enabled in account settings, and browser supported
	+	expected: updates bubble displayed
	+	4. Notifications not supported by browser (Internet Explorer)
	+	expected: updates bubble displayed

This looks really good! Most things are just style issues.

reviewboard/accounts/forms/pages.py (Diff revision 3)

The issue has been resolved. Show all issues

Can we reformat this as follows:

           'enable_desktop_notifications': self.profile.settings.get(
               'enable_desktop_notifications', True),

If not, can we pull this out into a variable first?

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

The issue has been resolved. Show all issues

This should go with the first var statement at the beginning, e.g.

var x = ...,
    y = ...;


Also, since window's scope is implicit in JS, the real notification (window.Notification) can be accessed as Notification anywhere. We should call this something else to avoid any confusion.

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This conditional should be wrapped in parentheses.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
There should be a space between ) and {.
```

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

The issue has been resolved. Show all issues

This expression should be wrapped in parentheses.

Also, the second and third conditions need to be indented four spaces, not two.

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

The issue has been resolved. Show all issues

This will overwrite window.Notification. Can we set this as a method on the object?

kvdgulik Oct. 10, 2015, 5:23 p.m.

Just for clarification, which object would this be set on?

brennie Oct. 10, 2015, 10:59 p.m.

The RB.ReviewablepageView. e.g. we'd do something like this.notification = ... || ... || ....

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between statement and beginning of block.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Update this docstring to refer to the fact that it also can use web notifications.
```

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

The issue has been resolved. Show all issues

Can you document the arguments to this function?

We document them like:

/*
 * ...
 *
 * Args:
 *     info (info's type):
 *         A brief description of info
 */

Also this first line is indented by one extra space.

kvdgulik Oct. 10, 2015, 7:10 p.m.

Hi, for the arguments to the function, I believe it is the last updates information, and I know that the function is triggered by ReviewRequestModel.js checkForUpdate. However, I'm not sure what type of object this is. Do you happen to know, and if not what should I add to the documentation?

brennie Oct. 10, 2015, 10:59 p.m.

If it is a generic JS object, then it would just be Object.

kvdgulik Oct. 13, 2015, 10:47 a.m.

Okay, one last question. We currently have this open bug: https://hellosplat.com/s/beanbag/tickets/3445/ in which the get_last_updates call returns the review request itself. I'm reading in the output of this call for my recieveNotifications info parameter. Should I document what it currently is(the review request) or what it should be(the last update information)?

david Oct. 13, 2015, 10:56 a.m.

It should be what it should be. Even now, you're still getting the "latest update", it's just that a bug on the backend is updating dates improperly.

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
receive
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing space between ) and {
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a period.
Also, can you document the args as above?
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a period. Also, can you document the args as above?
```

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

The issue has been resolved. Show all issues

So this overrides window.Notification again. We should use a method on the object. We should only check for it once and store it.

This expression should be wrapped in parentheses and the second & third lines need to be indented at 4 spaces each.

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This checking should be done in receiveNotification.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
These need to be declared at the top of the function.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing a space between ) and {
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
You can use _.delay(_.bind(...), ...)
```

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

The issue has been resolved. Show all issues

This should be formatted as:

} else if (Notification.permission === 'denied' ||
           Notificaiton.permission === 'default') {


Also, this can be written as Notification.permission !== 'granted'.

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

Should this be 5000? Isn't chrome 5s?

kvdgulik Oct. 10, 2015, 5:41 p.m.

When I was working on this at the code sprint, I asked how long the notification should last for and Mike said that 10 seconds sounded fine. In Firefox and Safari the notification apparently closes by default faster then this, but this makes sure that the notification closes in a reasonable amount of time on Chrome

reviewboard/templates/base.html (Diff revision 3)

The issue has been resolved. Show all issues

We don't enforce line ending limits for HTML templates.
This can be written as

        enableDesktopNotifications: {{user_profile.settings.enable_desktop_notifications|yesno:'true,false'}}

kvdgulik Oct. 10, 2015, 5:41 p.m.

enableDesktopNotifications: {{user_profile.settings.enable_desktop_notifications|yesno:'true,false'}} does not work because of the case where the property has not yet been defined. I also tried {{user_profile.settings.enable_desktop_notifications|yesno:'true,false, true'}} (the yes,no,none format) and this also did not work.

brennie Oct. 10, 2015, 10:59 p.m.

The reason it doesn't work in that last case becuase user_profile.settings.enable_desktop_notifications is going to throw a KeyError when its not already in the dict.
To combat this, we can provide a property on user_profile that will basically just return settings.get('enable_desktop_notifications', True) ?

Diff:

Revision 4 (+121 -10)

Show changes

	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/templates/base.html

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/forms/pages.py (Diff revision 4)

The issue has been resolved. Show all issues

This should use the property you defined above (self.profile.should_enable_desktop_notifications)

reviewboard/accounts/models.py (Diff revision 4)

The issue has been resolved. Show all issues

Because settings defaults to null, we need to check here that it's not None before calling .get()

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

The issue has been resolved. Show all issues

Probably you can skip the first half of this and just see if this._notificationType is undefined.

kvdgulik Oct. 23, 2015, 11:21 a.m.

Tried this, but unfortunatly it caused notifications to fail on IE

brennie Oct. 23, 2015, 12:41 p.m.

You mean you tried this._shouldUseDesktopNotifications = (this._notificationType !== undefined && RB.UserSession.instance.get('enableDesktopNotifications'))  and it failed?
What version of IE?

kvdgulik Oct. 23, 2015, 12:58 p.m.

Nevermind, Sorry for the false alarm. Re-did this and it worked. Not sure what went wrong the first time

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

The issue has been resolved. Show all issues

Can we align the || with the opening (? We also tend to put operators on the previous line.

this._notificationType = (window.Notification ||
                          window.mozNotification ||
                          window.webkitNotification)

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

The issue has been resolved. Show all issues

These two checks could be combined into the same if statement. Also make sure there's a space before opening {s.

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

The issue has been resolved. Show all issues

Space between if and (.
It might also be nice to switch the order of the conditional:

if (this._shouldUseDesktopNotifications &&
    this._notificationType.permission === 'granted') {
    this._sendDesktopNotification(info);
} else {
    this._sendUpdatesBubble(info);
}

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

The issue has been resolved. Show all issues

This can be simplified using JavaScript's || operator:

var username = info.user.fullname || info.user.username;

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

The issue has been resolved. Show all issues

We generally combine var definitions into a single statement:

var username = ...,
    notificationText = ...;

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Space between the , and {
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Please indent these two lines one more level.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
As I mention below, if you have a local var for this._notification you can skip the extra bind.
```

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

The issue has been resolved. Show all issues

This could be formatted much more nicely:

_.delay(
    _.bind(function() {
        this._notification.close();
    }, this),
    RB.ReviewablePageView.NOTIFICATION_LIFETIME_MSECS);


Even better, if you keep a local var that points to the same thing as this._notification, you can skip the bind entirely.

<p>I'm going to recommend an additional set of changes to the design here.</p>

<p>Right now, we have a bunch of new code in ReviewablePageView for figuring out the</p>

reviewboard/accounts/evolutions/__init__.py (Diff revision 4)

The issue has been resolved. Show all issues

Needs a trailing comma. This is to prevent having to modify this line later when adding a new line.

reviewboard/accounts/evolutions/profile_settings.py (Diff revision 4)

The issue has been resolved. Show all issues

This is missing:

from __future__ import unicode_literals


which must appear before the other imports, with a blank line in-between.

reviewboard/accounts/evolutions/profile_settings.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Needs a trailing comma.
```
reviewboard/accounts/forms/pages.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Should be "Show desktop notifications ..."
```

reviewboard/accounts/forms/pages.py (Diff revision 4)

The issue has been resolved. Show all issues

This should reuse the convenience function provided on the profile, so that we don't have two places determining a default value.

reviewboard/accounts/models.py (Diff revision 4)

The issue has been resolved. Show all issues

"Return", not "Get".
"Get" implies that the function is going to get something from somewhere. "Return" implies it's going to return a value.

reviewboard/accounts/models.py (Diff revision 4)

The issue has been resolved. Show all issues

This also needs a "Returns" block, like:

Returns:
    bool:
    <description of the return value here>

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

The issue has been resolved. Show all issues

This should be formatted more like:

this._shouldUseDesktopNotifications = (
    'Notification' in window &&
    RB.UserSession.instance.get(....));

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

The issue has been resolved. Show all issues

So right now, we have all this code, and some code below, that deals with cross-browser setup and requests for desktop notifications, along with the checks for whether we can use these notifications.

If we later have a new view not based on ReviewablePageView that wants to do the same thing, we're going to have to repeat this.

Instead, I suggest we have a new wrapper object, ui/managers/notificationManager.js, that manages all this. Callers should be able to simply do:

RB.NotificationManager.setup();

...

if (RB.NotificationManager.canNotify) {
    RB.NotificationManager.notify({
        ... // data on the notification
    });
}

The manager would, in setup, figure out the type of object and request permission if needed. It would only do this once, allowing setup to be called by any class expecting to do notifications.

This makes for a nice, reusable interface around notifications that we can later extend, and more easily unit test.

(Also, there should be unit tests for these changes.)

kvdgulik Oct. 23, 2015, 11:21 a.m.

Is there a current example in the code base for unit testing javascript UI features like this? Sorry for forgetting them, but since this was a UI change, I did not believe this was neccesary at first.

brennie Oct. 23, 2015, 12:41 p.m.

There are some examples in the views/tests/ directories. If you need help writing some, feel free to ping us.

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

The issue has been resolved. Show all issues

First line of function docs should be a summary fitting on one line without wrapping. You can then expand on a description.

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

The issue has been resolved. Show all issues

We always want to show the in-page bubble. If I get a popup on my screen, I need to be able to find out what page had the update. If it's not on the page as well, I'll have to guess.

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Same as above. The first line should be a summary fitting on a single line.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Extra trailing whitespace should be removed.
```

Diff:

Revision 5 (+286 -10)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/templates/base.html

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/forms/pages.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 44
 W291 trailing whitespace
```
reviewboard/accounts/models.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

reviewboard/accounts/forms/pages.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This should be indented 4 spaces.
```
reviewboard/accounts/models.py (Diff revision 5)
The issue has been resolved. Show all issues
```
No parens needed.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Remove this whitespace.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)

The issue has been resolved. Show all issues

Reformat this as:

this._canNotify = (
    this.notificationType !== undefined &&
    RB.UserSession.instance.get('...'));


Also, missing a semi-colon and there is trailing whitespace.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)

The issue has been resolved. Show all issues

Unindent these.

kvdgulik Oct. 31, 2015, 12:49 p.m.

Like this?

The last update information for the request
properties:
title (String):
the title of the notification
body (String):
the body text of the notification
icon (URL String):
the URL of the icon to be used in the notification
Icons are not supported in some browsers, and thus
will only be show in supported browsers

brennie Nov. 1, 2015, 9:21 p.m.

Like:

/*
 * ...
 * Args:
 *     data (Object):
 *         ...
 *     title (String):
 *         ...
 *     body (String):
 *         ...
 */

kvdgulik Nov. 6, 2015, 10:15 a.m.

Would this imply that I have 5 arguements though? In this case I was trying to tell the user that data has 4 properties (title, body, iconURL and onclick) and what those properties represent

brennie Nov. 6, 2015, 5:09 p.m.

Good to know I sometimes fail at reading comprehension!

In that case, let's do

/*
 * Args:
 *     data (object):
 *         The last update information for the request. This has the following keys:
 *
 *         * ``title`` (``string``): the title of the notificaiton.
 *         * ``body`` (``String``): ...

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)
The issue has been resolved. Show all issues
```
No blank line here.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)

The issue has been resolved. Show all issues

Unindent.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)

The issue has been resolved. Show all issues

You don't have to wrap notification.close() in a function, e.g.:

_.delay(notification.close,
        RB.NotificationManager.NOTIFICATION_LIFETIME_MSECS);

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)
The issue has been resolved. Show all issues
```
"the user's browser"
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Capital 'F' for the false.
```

Diff:

Revision 6 (+299 -10)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/forms/pages.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 44
 W291 trailing whitespace
```
reviewboard/accounts/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Diff:

Revision 7 (+296 -10)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
I think this should be _showUpdatesBubble
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 7)

The issue has been resolved. Show all issues

Fix indentation

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 7)

The issue has been resolved. Show all issues

Fix indentation

reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js (Diff revision 7)
The issue has been resolved. Show all issues
```
This file uses tabs instead of spaces. We use 4 spaces for all our JS.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revisions 6 - 7)
The issue has been resolved. Show all issues
```
There's an extra ' in here.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revisions 6 - 7)
The issue has been resolved. Show all issues
```
Should be a lower-case 'true'
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revisions 6 - 7)

The issue has been resolved. Show all issues

Actually, the way you had it before was correct (since this is JavaScript, not Python). However, this should end in a period.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revisions 6 - 7)

The issue has been resolved. Show all issues

I don't think this works (since notification.close() will call immediately). However, you probably can do:

_.delay(_.bind(notification.close, notification),
        RB.NotificationManager.NOTIFICATION_LIFETIME_MSECS);

Diff:

Revision 8 (+290 -10)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

```
Doc comments should end in periods.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Register
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Catch
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Create
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Show
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
will create.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Remove this line.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
"Set up"
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Dedent by 4
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Return.
Missing period.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
"Send"
The first sentence of a doccomment should be a single line.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Missing a period. Also doesnt tell us that the following list is a list of keys.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Backquotes, not single quotes.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)

What happens when you call .close() on a closed notification?

kvdgulik Nov. 13, 2015, 12:58 p.m.

Did some testing on this as I was not able to find the answer in the MDN documentation, and it appears as if nothing happens

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)

The issue has been resolved. Show all issues

"Create"

First sentence of a doc comment should fit on a single line.

Also, minor typos:

``RB.notificationManager.notifiy

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
second line should be indented one more space
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
should be indented one more space.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
backquotes, not single quotes.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
"Call"
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Call. shouldNotify.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
"Call". Needs args.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 8)
The issue has been resolved. Show all issues
```
"Return"
Needs args.
```
reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js (Diff revision 8)
The issue has been resolved. Show all issues
```
API
```

Diff:

Revision 9 (+329 -11)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Ship it!

The only comments I have have to do with grammar and word choice. Otherwise, I am fine with this landing.

Could you also rewrite the summary into paragraph form and mention in the testing done that you ran the JS unit tests?

Thanks!

reviewboard/accounts/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
"Otherwise,"
```
reviewboard/accounts/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
"then use their preference"
"Otherwise,"
```

reviewboard/accounts/models.py (Diff revision 9)

The issue has been resolved. Show all issues

Missing a period. Also this should be:

`:py:data:`True`

reviewboard/accounts/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
No need for an else clause. We can just have return True.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
The bubble will always be displayed, no?
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Missing a period.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
"This function will handle the review updated ..."
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Missing a period.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
"This function will create ..."
"notification"
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
", then we will send"
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
"This function will tell.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
It, not This.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
``true``
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)

The issue has been resolved. Show all issues

"Otherwise, ``false`` will be returned.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)

The issue has been resolved. Show all issues

``title`` (``String``)

Note the space.

Also, missing a period.

Capitalization of the.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Capitilizaiton and missing period.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as above.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)

The issue has been resolved. Show all issues

See my comments in the other doc-comment.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Capitilzation and missing period.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as above.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Same as ahove.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)

The issue has been resolved. Show all issues

See my comments on the other doc comment.

Diff:

Revision 10 (+330 -11)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/models.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Description:

~		- Created a desktop notification to send to users on update
	~	This task created a desktop notification to send to users when an update has been made to a review request. This involved adding a JSON column to the user profile called settings to represent user settings such as their preference on recieving desktop notifications. Users can enable or disable the desktop notifications using an option in the account setting form.
-		- Added a JSON column to the user profile(settings) to represent users settings such as their preference on recieving desktop notifications
-		- Created an option in the account setting form to enable/disable desktop notifications

Testing Done:

		Tested on Chrome, Firefox, Internet Explorer and Safari.

		Test Cases
		1. Notifications enabled both in permissions and in account settings, and browser supported
		expected: notification is shows
		2. Notifications enabled in permissions but not in account settings, and browser supported
		expected: updates bubble displayed
		3. Notifications disabled in permissions but enabled in account settings, and browser supported
		expected: updates bubble displayed
		4. Notifications not supported by browser (Internet Explorer)
		expected: updates bubble displayed
	+
	+	The JS Unit tests were also run during the testing for this task.

Diff:

Revision 11 (+330 -11)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Overall, this is looking very good. There's a few things missing, and a few things you don't need, which I've covered below.

The biggest issue that stands out to me, though, is a lack of consistency with indentation levels. This sort of thing must be absolutely consistent, and it feels like maybe your editor just isn't set right to enforce or help notice indentation differences.

We use 4 space indentation, both for JavaScript statements and within documentation. I've highlighted a number of places where I noticed inconsistencies, but please go through and make sure everything is consistent throughout the change.

reviewboard/accounts/forms/pages.py (Diff revision 11)
The issue has been resolved. Show all issues
```
This should be in sentence casing, like the other options.
```

reviewboard/accounts/models.py (Diff revision 11)

The issue has been resolved. Show all issues

This can be:

return (not self.settings or
        self.settings.get('enable_desktop_notifications', True))

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
The issue has been resolved. Show all issues
```
The function name would be better as _onReviewRequestUpdated.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
The issue has been resolved. Show all issues
```
The /* is indented one space too far.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
The issue has been resolved. Show all issues
```
"and send", no "s".
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
The issue has been resolved. Show all issues
```
This isn't true anymore, right? We're always sending the bubble now.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
The issue has been resolved. Show all issues
```
No need to capitalize "Updates Bubble."
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Indentation is wonky here.
Make sure you run jshint to catch these.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
The issue has been resolved. Show all issues
```
"review request update," or just "update."
```

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

The issue has been resolved. Show all issues

I'm having a hard time parsing that sentence. I think it's the "used" bit and maybe "the notification."

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

The issue has been resolved. Show all issues

Indenation here and below all looks wrong, too.

Go through everything and make sure every indentation level is an even multiple of 4.

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

The issue has been resolved. Show all issues

Most of this looks like stuff that could easily be inline in the call below.

kvdgulik Nov. 20, 2015, 4:05 p.m.

I did have it inline, but earlier in the review was asked to move it out for readability. Should I change it back to inline?

brennie Nov. 23, 2015, 3:11 p.m.

Make a judgement call for whichever is more readable (to you).

kvdgulik Nov. 26, 2015, 12:27 p.m.

To me its more readable this way, so I'm going to leave it for now

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
This should go into more detail on what this is responsible for, requirements, etc.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)

The issue has been resolved. Show all issues

The entire purpose of what this class does is express visual notifications based off data, so this by definition should be a View and not a Model.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
"for the user browser" doesn't make sense. "user's browser" ?
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)

The issue has been resolved. Show all issues

Should be sentence casing.

Also, this sentence is a bit confusing. I don't think there's a notification telling us about notifications.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)

The issue has been resolved. Show all issues

Most of the stuff in here should be in initialize. Any variable state defaults should be there. The only thing that should belong in a special setup function would be the part that requests permission.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
This should just be a constant on the model.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)

The issue has been resolved. Show all issues

4 space indentation on everything.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Too many parens. They're not needed here.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Indentation is off here as well.
Same with ones below. Make sure to check every comment.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Indentation is off here as well.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Blank line between these.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)

The issue has been resolved. Show all issues

No backticks for the types.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
"URL String" isn't a type. It's just a String.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
the /* is indented one space too far.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Blank line between each argument.
```
reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Blank line between these.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)

The issue has been resolved. Show all issues

I'm not sure this is useful.
This function is just taking positional arguments and turning it into an object of keys/values. Thing is, when you have a function as complicated as this in JavaScript, argument-wise, it's often better to instead have it take options and to pull from that, emulating Python keyword arguments.
However, that'd defeat the whole point of this function. So, given that, it's probably best not to have this function at all.

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Space before {.
```

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)

The issue has been resolved. Show all issues

This is only needed by the prototype implementation above, so I'd move it there. Then, update call sites to use this.NOTIFICATION_LIFETIME_MSECS.

kvdgulik Nov. 20, 2015, 4:05 p.m.

Sorry, I'm not sure I understand. Where shoud I move it?

brennie Nov. 23, 2015, 3:11 p.m.

I believe Christian is suggesting to move it into the object, i.e.

RB.NotificationManager = Backbone.Model.extend({
    NOTIFICATION_LIFETIME_MSECS: 1000,
    // ...

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)

The issue has been resolved. Show all issues

We shouldn't have a copy of every single function, since those can all just access the instance. A wrapper function like these are only useful if they're doing additional work, like instantiating things.
Same below.
shouldNotify doesn't even do the right thing, as the wrapped function returns a value.

reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js (Diff revision 11)

The issue has been resolved. Show all issues

The test suite needs a unit test for every function available to it, under every condition, simulating the ability to use notifications, inability to use them, different sorts of arguments and interaction, etc.

kvdgulik Nov. 20, 2015, 4:05 p.m.

Currently I have 4 functions: initialize, setup, shouldNotify, and notify. I've uploaded the code with the changes for the review so far, but am not sure what these tests would look like. Any suggestions?

brennie Nov. 23, 2015, 3:11 p.m.

You should have tests for the following:

Fake the preconditions for shouldNotify() to be true and see that it returns true
Fake the preconditions for shouldNotify() to be true and see that RB.NotificationManager.instance.notify does use the native notification (which is the test you have above).
Fake the preconditions for shouldNotify() to be false and see that it returns false
Fake the preconditions for shouldNotify() to be false and see that RB.NotificationManager.instance.notify does not use the native notification.

If you need help with these, feel free to ping me this week.

reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js (Diff revision 11)
The issue has been resolved. Show all issues
```
Sentence casing.
```

Diff:

Revision 12 (+257 -25)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/models.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/models.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Diff:

Revision 13 (+257 -25)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/models.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Diff:

Revision 14 (+257 -25)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Diff:

Revision 15 (+258 -25)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/accounts/models.py (Diff revision 15)

The issue has been resolved. Show all issues

I've been informed in my work that this should just be:

``True``

reviewboard/accounts/models.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Remove this blank line.
```

Diff:

Revision 16 (+309 -26)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Diff:

Revision 17 (+315 -26)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	2 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revisions 15 - 17)

The issue has been resolved. Show all issues

The indentation here is a little off. It should be:

NOTIFICATION_TYPE: (window.Notification ||
                    window.mozNotification ||
                    window.webkitNotification)

reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 17)
The issue has been resolved. Show all issues
```
Single quotes.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 17)
The issue has been resolved. Show all issues
```
Single quotes.
```

Diff:

Revision 18 (+315 -26)

Show changes

	reviewboard/staticbundles.py
	reviewboard/accounts/models.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/profile_settings.py
	reviewboard/accounts/forms/pages.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/forms/pages.py
    reviewboard/accounts/evolutions/profile_settings.py
    reviewboard/accounts/models.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/ui/managers/tests/notificationManagerModelTests.js
    reviewboard/static/rb/js/ui/managers/notificationManagerModel.js
    reviewboard/templates/base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Ship it!

```
Some nitpicks, then looks good to me.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 18)
The issue has been resolved. Show all issues
```
Can you move this function definition inline into the notify call?
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 18)
The issue has been resolved. Show all issues
```
There is no var statement for this variable.
```

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

The issue has been resolved. Show all issues

I'd rather have this all inline:

RB.Notificationmanager.instance.notify({
    ...
});

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 18)
The issue has been resolved. Show all issues
```
You don't need quotes around JS object keys.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 18)
The issue has been resolved. Show all issues
```
Undo this.
```

reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 18)

The issue has been resolved. Show all issues

Should be at top of function.

Status: Closed (submitted)

Change Summary:

Pushed to feature/desktop-notifications (fd3a323)