Enable Desktop Notifications

Review Request #7660 — Created Sept. 25, 2015 and submitted

kvdgulik
Review Board
master
8072
reviewboard

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.

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.

Description From Last Updated

Col: 80 E501 line too long (90 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

Col: 72 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (101 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (90 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

Col: 72 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (101 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Missing semicolon.

brenniebrennie

Needs doc comment.

brenniebrennie

Single quotes.

brenniebrennie

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

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

We try not to go over 80 characters. We should use gettext() for the string with a %s-interpolation e.g. (gettext("Review ...

brenniebrennie

This should be a constant on the object.

brenniebrennie

No blank line here.

brenniebrennie

Single quotes.

brenniebrennie

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 ...

brenniebrennie

Col: 45 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 17 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

This should go with the first var statement at the beginning, e.g. var x = ..., y = ...; Also, ...

brenniebrennie

This conditional should be wrapped in parentheses.

brenniebrennie

There should be a space between ) and {.

brenniebrennie

This expression should be wrapped in parentheses. Also, the second and third conditions need to be indented four spaces, not ...

brenniebrennie

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

brenniebrennie

Blank line between statement and beginning of block.

brenniebrennie

Update this docstring to refer to the fact that it also can use web notifications.

brenniebrennie

Can you document the arguments to this function? We document them like: /* * ... * * Args: * info ...

brenniebrennie

receive

brenniebrennie

Missing space between ) and {

brenniebrennie

Missing a period. Also, can you document the args as above?

brenniebrennie

Missing a period. Also, can you document the args as above?

brenniebrennie

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

brenniebrennie

This checking should be done in receiveNotification.

brenniebrennie

These need to be declared at the top of the function.

brenniebrennie

Missing a space between ) and {

brenniebrennie

You can use _.delay(_.bind(...), ...)

brenniebrennie

This should be formatted as: } else if (Notification.permission === 'denied' || Notificaiton.permission === 'default') { Also, this can be ...

brenniebrennie

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'}}

brenniebrennie

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

chipx86chipx86

This is missing: from __future__ import unicode_literals which must appear before the other imports, with a blank line in-between.

chipx86chipx86

Needs a trailing comma.

chipx86chipx86

Should be "Show desktop notifications ..."

chipx86chipx86

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

daviddavid

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

chipx86chipx86

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

chipx86chipx86

This also needs a "Returns" block, like: Returns: bool: <description of the return value here>

chipx86chipx86

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

daviddavid

This should be formatted more like: this._shouldUseDesktopNotifications = ( 'Notification' in window && RB.UserSession.instance.get(....));

chipx86chipx86

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

daviddavid

So right now, we have all this code, and some code below, that deals with cross-browser setup and requests for ...

chipx86chipx86

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

daviddavid

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

daviddavid

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

chipx86chipx86

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

daviddavid

We always want to show the in-page bubble. If I get a popup on my screen, I need to be ...

chipx86chipx86

Same as above. The first line should be a summary fitting on a single line.

chipx86chipx86

Same as above.

chipx86chipx86

This can be simplified using JavaScript's || operator: var username = info.user.fullname || info.user.username;

daviddavid

We generally combine var definitions into a single statement: var username = ..., notificationText = ...;

daviddavid

Extra trailing whitespace should be removed.

chipx86chipx86

Space between the , and {

daviddavid

Please indent these two lines one more level.

daviddavid

As I mention below, if you have a local var for this._notification you can skip the extra bind.

daviddavid

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 ...

daviddavid

Col: 44 W291 trailing whitespace

reviewbotreviewbot

This should be indented 4 spaces.

brenniebrennie

No parens needed.

brenniebrennie

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Remove this whitespace.

brenniebrennie

"the user's browser"

CH chronicleyu

Reformat this as: this._canNotify = ( this.notificationType !== undefined && RB.UserSession.instance.get('...')); Also, missing a semi-colon and there is trailing whitespace.

brenniebrennie

Capital 'F' for the false.

CH chronicleyu

Unindent these.

brenniebrennie

No blank line here.

brenniebrennie

Unindent.

brenniebrennie

You don't have to wrap notification.close() in a function, e.g.: _.delay(notification.close, RB.NotificationManager.NOTIFICATION_LIFETIME_MSECS);

brenniebrennie

Col: 44 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

There's an extra ' in here.

daviddavid

Should be a lower-case 'true'

daviddavid

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

daviddavid

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);

daviddavid

I think this should be _showUpdatesBubble

brenniebrennie

Fix indentation

brenniebrennie

Fix indentation

brenniebrennie

This file uses tabs instead of spaces. We use 4 spaces for all our JS.

brenniebrennie

Register

brenniebrennie

Catch

brenniebrennie

Create

brenniebrennie

Show

brenniebrennie

will create.

brenniebrennie

Missing period.

brenniebrennie

Remove this line.

brenniebrennie

"Set up"

brenniebrennie

Dedent by 4

brenniebrennie

Return. Missing period.

brenniebrennie

"Send" The first sentence of a doccomment should be a single line.

brenniebrennie

Missing period.

brenniebrennie

Missing a period. Also doesnt tell us that the following list is a list of keys.

brenniebrennie

Backquotes, not single quotes.

brenniebrennie

"Create" First sentence of a doc comment should fit on a single line. Also, minor typos: ``RB.notificationManager.notifiy

brenniebrennie

second line should be indented one more space

brenniebrennie

should be indented one more space.

brenniebrennie

backquotes, not single quotes.

brenniebrennie

"Call"

brenniebrennie

Call. shouldNotify.

brenniebrennie

"Call". Needs args.

brenniebrennie

"Return" Needs args.

brenniebrennie

API

brenniebrennie

"Otherwise,"

brenniebrennie

"then use their preference" "Otherwise,"

brenniebrennie

Missing a period. Also this should be: `:py:data:`True`

brenniebrennie

No need for an else clause. We can just have return True.

brenniebrennie

The bubble will always be displayed, no?

brenniebrennie

Missing a period.

brenniebrennie

"This function will handle the review updated ..."

brenniebrennie

Missing a period.

brenniebrennie

Missing period.

brenniebrennie

Missing period.

brenniebrennie

Missing period.

brenniebrennie

Missing period.

brenniebrennie

"This function will create ..." "notification"

brenniebrennie

", then we will send"

brenniebrennie

Missing period.

brenniebrennie

"This function will tell.

brenniebrennie

It, not This.

brenniebrennie

``true``

brenniebrennie

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

brenniebrennie

``title`` (``String``) Note the space. Also, missing a period. Capitalization of the.

brenniebrennie

Same as above.

brenniebrennie

Same as above.

brenniebrennie

Same as above.

brenniebrennie

Capitilizaiton and missing period.

brenniebrennie

Same as above.

brenniebrennie

Same as above.

brenniebrennie

Same as above.

brenniebrennie

See my comments in the other doc-comment.

brenniebrennie

Capitilzation and missing period.

brenniebrennie

Same as above.

brenniebrennie

Same as above.

brenniebrennie

Same as ahove.

brenniebrennie

See my comments on the other doc comment.

brenniebrennie

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

This should be in sentence casing, like the other options.

chipx86chipx86

This can be: return (not self.settings or self.settings.get('enable_desktop_notifications', True))

chipx86chipx86

The function name would be better as _onReviewRequestUpdated.

chipx86chipx86

The /* is indented one space too far.

chipx86chipx86

"and send", no "s".

chipx86chipx86

This isn't true anymore, right? We're always sending the bubble now.

chipx86chipx86

Blank line between these.

chipx86chipx86

No need to capitalize "Updates Bubble."

chipx86chipx86

Indentation is wonky here. Make sure you run jshint to catch these.

chipx86chipx86

"review request update," or just "update."

chipx86chipx86

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

chipx86chipx86

Indenation here and below all looks wrong, too. Go through everything and make sure every indentation level is an even ...

chipx86chipx86

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

chipx86chipx86

This should go into more detail on what this is responsible for, requirements, etc.

chipx86chipx86

The entire purpose of what this class does is express visual notifications based off data, so this by definition should ...

chipx86chipx86

"for the user browser" doesn't make sense. "user's browser" ?

chipx86chipx86

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

chipx86chipx86

Most of the stuff in here should be in initialize. Any variable state defaults should be there. The only thing ...

chipx86chipx86

This should just be a constant on the model.

chipx86chipx86

4 space indentation on everything.

chipx86chipx86

Too many parens. They're not needed here.

chipx86chipx86

Indentation is off here as well. Same with ones below. Make sure to check every comment.

chipx86chipx86

Indentation is off here as well.

chipx86chipx86

Blank line between these.

chipx86chipx86

No backticks for the types.

chipx86chipx86

"URL String" isn't a type. It's just a String.

chipx86chipx86

the /* is indented one space too far.

chipx86chipx86

Blank line between each argument.

chipx86chipx86

Blank line between these.

chipx86chipx86

I'm not sure this is useful. This function is just taking positional arguments and turning it into an object of ...

chipx86chipx86

Space before {.

chipx86chipx86

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

chipx86chipx86

We shouldn't have a copy of every single function, since those can all just access the instance. A wrapper function ...

chipx86chipx86

The test suite needs a unit test for every function available to it, under every condition, simulating the ability to ...

chipx86chipx86

Sentence casing.

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

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

brenniebrennie

Remove this blank line.

brenniebrennie

The indentation here is a little off. It should be: NOTIFICATION_TYPE: (window.Notification || window.mozNotification || window.webkitNotification)

brenniebrennie

Single quotes.

brenniebrennie

Single quotes.

brenniebrennie

Can you move this function definition inline into the notify call?

brenniebrennie

There is no var statement for this variable.

brenniebrennie

I'd rather have this all inline: RB.Notificationmanager.instance.notify({ ... });

brenniebrennie

You don't need quotes around JS object keys.

brenniebrennie

Undo this.

brenniebrennie

Should be at top of function.

brenniebrennie
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (90 > 79 characters)
    
  3. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Col: 72
     W291 trailing whitespace
    
  5. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (101 > 79 characters)
    
  6. reviewboard/accounts/models.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  7. 
      
KV
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (90 > 79 characters)
    
  3. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     
    Col: 72
     W291 trailing whitespace
    
  5. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (101 > 79 characters)
    
  6. reviewboard/accounts/models.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  7. 
      
KV
brennie
  1. 
      
  2. We can do this in initialize() and then set e.g., this._shouldUseDesktopNotifications so we only have to do it once.

  3. 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?

    1. 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.

    2. 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

  4. This should be a constant on the object.

  5. 
      
KV
reviewbot
  1. 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
    
    
  2. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
    Col: 45
     E502 the backslash is redundant between brackets
    
  3. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
    Col: 17
     E131 continuation line unaligned for hanging indent
    
  4. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. 
      
KV
brennie
  1. This looks really good! Most things are just style issues.

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

    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?

  3. 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.

  4. This conditional should be wrapped in parentheses.

  5. There should be a space between ) and {.

  6. This expression should be wrapped in parentheses.

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

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

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

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

  8. Blank line between statement and beginning of block.

  9. Update this docstring to refer to the fact that it also can use web notifications.

  10. 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.

    1. 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?

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

    3. 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)?

    4. 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.

  11. Missing space between ) and {

  12. Missing a period.

    Also, can you document the args as above?

  13. Missing a period. Also, can you document the args as above?

  14. 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.

  15. This checking should be done in receiveNotification.

  16. These need to be declared at the top of the function.

  17. Missing a space between ) and {

  18. You can use _.delay(_.bind(...), ...)

  19. This should be formatted as:

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

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

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

    1. 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

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

    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'}}
    
    1. 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.

    2. 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) ?

  22. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 4)
     
     

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

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

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

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

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

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

      What version of IE?

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

  5. 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)
    
  6. These two checks could be combined into the same if statement. Also make sure there's a space before opening {s.

  7. 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);
    }
    
  8. This can be simplified using JavaScript's || operator:

    var username = info.user.fullname || info.user.username;
    
  9. We generally combine var definitions into a single statement:

    var username = ...,
        notificationText = ...;
    
  10. Space between the , and {

  11. Please indent these two lines one more level.

  12. As I mention below, if you have a local var for this._notification you can skip the extra bind.

  13. 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.

  14. 
      
chipx86
  1. <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>

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

  3. This is missing:

    from __future__ import unicode_literals
    

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

  4. Needs a trailing comma.

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

    Should be "Show desktop notifications ..."

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

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

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

    "Return", not "Get".

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

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

    This also needs a "Returns" block, like:

    Returns:
        bool:
        <description of the return value here>
    
  9. This should be formatted more like:

    this._shouldUseDesktopNotifications = (
        'Notification' in window &&
        RB.UserSession.instance.get(....));
    
  10. reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.)

    1. 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.

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

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

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

    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.

  13. Same as above. The first line should be a summary fitting on a single line.

  14. Extra trailing whitespace should be removed.

  15. 
      
KV
reviewbot
  1. 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
    
    
  2. reviewboard/accounts/forms/pages.py (Diff revision 5)
     
     
    Col: 44
     W291 trailing whitespace
    
  3. reviewboard/accounts/models.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. 
      
brennie
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 5)
     
     

    This should be indented 4 spaces.

  3. reviewboard/accounts/models.py (Diff revision 5)
     
     

    No parens needed.

  4. Remove this whitespace.

  5. Reformat this as:

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

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

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

    Unindent these.

    1. 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

    2. Like:

      /*
       * ...
       * Args:
       *     data (Object):
       *         ...
       *     title (String):
       *         ...
       *     body (String):
       *         ...
       */
      
    3. 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

    4. 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``): ...
      
  7. reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Unindent.

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

    _.delay(notification.close,
            RB.NotificationManager.NOTIFICATION_LIFETIME_MSECS);
    
  9. 
      
CH
KV
reviewbot
  1. 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
    
    
  2. reviewboard/accounts/forms/pages.py (Diff revision 6)
     
     
    Col: 44
     W291 trailing whitespace
    
  3. reviewboard/accounts/models.py (Diff revision 6)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. I think this should be _showUpdatesBubble

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

    Fix indentation

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

    Fix indentation

  5. This file uses tabs instead of spaces. We use 4 spaces for all our JS.

  6. 
      
david
  1. 
      
  2. There's an extra ' in here.

  3. Should be a lower-case 'true'

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

  5. 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);
    
  6. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. Doc comments should end in periods.

  2. "Send"

    The first sentence of a doccomment should be a single line.

  3. Missing a period. Also doesnt tell us that the following list is a list of keys.

  4. Backquotes, not single quotes.

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

    1. 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

  6. "Create"

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

    Also, minor typos:

    ``RB.notificationManager.notifiy
    
  7. second line should be indented one more space

  8. should be indented one more space.

  9. backquotes, not single quotes.

  10. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 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!

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

    "Otherwise,"

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

    "then use their preference"

    "Otherwise,"

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

    Missing a period. Also this should be:

    `:py:data:`True`
    
  5. reviewboard/accounts/models.py (Diff revision 9)
     
     
     

    No need for an else clause. We can just have return True.

  6. The bubble will always be displayed, no?

  7. "This function will handle the review updated ..."

  8. "This function will create ..."

    "notification"

  9. ", then we will send"

  10. "This function will tell.

  11. "Otherwise, ``false`` will be returned.
    
  12. ``title`` (``String``)
    

    Note the space.

    Also, missing a period.

    Capitalization of the.

  13. Capitilizaiton and missing period.

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

    See my comments in the other doc-comment.

  15. Capitilzation and missing period.

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

    See my comments on the other doc comment.

  17. 
      
KV
reviewbot
  1. 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
    
    
  2. reviewboard/accounts/models.py (Diff revision 10)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
KV
KV
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 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.

  2. reviewboard/accounts/forms/pages.py (Diff revision 11)
     
     

    This should be in sentence casing, like the other options.

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

    This can be:

    return (not self.settings or
            self.settings.get('enable_desktop_notifications', True))
    
  4. The function name would be better as _onReviewRequestUpdated.

  5. The /* is indented one space too far.

  6. This isn't true anymore, right? We're always sending the bubble now.

  7. Blank line between these.

  8. No need to capitalize "Updates Bubble."

  9. Indentation is wonky here.

    Make sure you run jshint to catch these.

  10. "review request update," or just "update."

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

  12. Indenation here and below all looks wrong, too.

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

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

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

    1. 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?

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

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

  14. This should go into more detail on what this is responsible for, requirements, etc.

  15. 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.

  16. "for the user browser" doesn't make sense. "user's browser" ?

  17. Should be sentence casing.

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

  18. 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.

  19. This should just be a constant on the model.

  20. 4 space indentation on everything.

  21. Too many parens. They're not needed here.

  22. Indentation is off here as well.

    Same with ones below. Make sure to check every comment.

  23. Indentation is off here as well.

  24. Blank line between these.

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

    No backticks for the types.

  26. "URL String" isn't a type. It's just a String.

  27. the /* is indented one space too far.

  28. Blank line between each argument.

  29. Blank line between these.

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

    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.

  31. 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.

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

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

      RB.NotificationManager = Backbone.Model.extend({
          NOTIFICATION_LIFETIME_MSECS: 1000,
          // ...
      
  32. reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.

  33. 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.

    1. 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?

    2. 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.

  34. 
      
KV
reviewbot
  1. 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
    
    
  2. reviewboard/accounts/models.py (Diff revision 12)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/accounts/models.py (Diff revision 12)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  4. 
      
KV
reviewbot
  1. 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
    
    
  2. reviewboard/accounts/models.py (Diff revision 13)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  3. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 15)
     
     

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

    ``True``
    
  3. reviewboard/accounts/models.py (Diff revision 15)
     
     

    Remove this blank line.

  4. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. The indentation here is a little off. It should be:

    NOTIFICATION_TYPE: (window.Notification ||
                        window.mozNotification ||
                        window.webkitNotification)
    
  3. 
      
KV
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. Some nitpicks, then looks good to me.

  2. Can you move this function definition inline into the notify call?

  3. There is no var statement for this variable.

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

    I'd rather have this all inline:

    RB.Notificationmanager.instance.notify({
        ...
    });
    
  5. You don't need quotes around JS object keys.

  6. Should be at top of function.

  7. 
      
KV
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to feature/desktop-notifications (fd3a323)
Loading...