Enable Desktop Notifications

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

Information

Review Board
master

Reviewers

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. Show all issues
    Col: 80
     E501 line too long (90 > 79 characters)
    
  3. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Show all issues
    Col: 72
     W291 trailing whitespace
    
  5. reviewboard/accounts/forms/pages.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (101 > 79 characters)
    
  6. reviewboard/accounts/models.py (Diff revision 1)
     
     
    Show all issues
    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. Show all issues
    Col: 80
     E501 line too long (90 > 79 characters)
    
  3. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     
    Show all issues
    Col: 72
     W291 trailing whitespace
    
  5. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (101 > 79 characters)
    
  6. reviewboard/accounts/models.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  7. 
      
KV
brennie
  1. 
      
  2. Show all issues

    Missing semicolon.

  3. Show all issues

    Needs doc comment.

  4. Show all issues

    Single quotes.

  5. Show all issues

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

  6. Show all issues

    No blank line here.

  7. Show all issues

    No blank line here.

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

    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

  9. Show all issues

    This should be a constant on the object.

  10. Show all issues

    No blank line here.

  11. Show all issues

    Single quotes.

  12. 
      
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)
     
     
    Show all issues
    Col: 45
     E502 the backslash is redundant between brackets
    
  3. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
    Show all issues
    Col: 17
     E131 continuation line unaligned for hanging indent
    
  4. reviewboard/accounts/forms/pages.py (Diff revision 3)
     
     
    Show all issues
    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)
     
     
     
    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?

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

  4. Show all issues

    This conditional should be wrapped in parentheses.

  5. Show all issues

    There should be a space between ) and {.

  6. Show all issues

    This expression should be wrapped in parentheses.

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

  7. Show all issues

    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. Show all issues

    Blank line between statement and beginning of block.

  9. Show all issues

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

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

    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. Show all issues

    receive

  12. Show all issues

    Missing space between ) and {

  13. Show all issues

    Missing a period.

    Also, can you document the args as above?

  14. Show all issues

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

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

  16. Show all issues

    This checking should be done in receiveNotification.

  17. Show all issues

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

  18. Show all issues

    Missing a space between ) and {

  19. Show all issues

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

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

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

  22. reviewboard/templates/base.html (Diff revision 3)
     
     
     
     
     
     
    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'}}
    
    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) ?

  23. 
      
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)
     
     
    Show all issues

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

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

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

  4. Show all issues

    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. 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)
    
  6. Show all issues

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

  7. 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);
    }
    
  8. Show all issues

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

    var username = info.user.fullname || info.user.username;
    
  9. Show all issues

    We generally combine var definitions into a single statement:

    var username = ...,
        notificationText = ...;
    
  10. Show all issues

    Space between the , and {

  11. Show all issues

    Please indent these two lines one more level.

  12. Show all issues

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

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

  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. Show all issues

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

  3. Show all issues

    This is missing:

    from __future__ import unicode_literals
    

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

  4. Show all issues

    Needs a trailing comma.

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

    Should be "Show desktop notifications ..."

  6. reviewboard/accounts/forms/pages.py (Diff revision 4)
     
     
     
    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.

  7. reviewboard/accounts/models.py (Diff revision 4)
     
     
    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.

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

    This also needs a "Returns" block, like:

    Returns:
        bool:
        <description of the return value here>
    
  9. Show all issues

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

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

  12. reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 4)
     
     
     
     
     
     
     
    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.

  13. Show all issues

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

  14. Show all issues

    Same as above.

  15. Show all issues

    Extra trailing whitespace should be removed.

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

    This should be indented 4 spaces.

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

    No parens needed.

  4. Show all issues

    Remove this whitespace.

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

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

    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. Show all issues

    No blank line here.

  8. reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Unindent.

  9. Show all issues

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

    _.delay(notification.close,
            RB.NotificationManager.NOTIFICATION_LIFETIME_MSECS);
    
  10. 
      
CH
  1. 
      
  2. Show all issues

    "the user's browser"

  3. Show all issues

    Capital 'F' for the false.

  4. 
      
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)
     
     
    Show all issues
    Col: 44
     W291 trailing whitespace
    
  3. reviewboard/accounts/models.py (Diff revision 6)
     
     
    Show all issues
    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. Show all issues

    I think this should be _showUpdatesBubble

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

    Fix indentation

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

    Fix indentation

  5. Show all issues

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

  6. 
      
david
  1. 
      
  2. Show all issues

    There's an extra ' in here.

  3. Show all issues

    Should be a lower-case 'true'

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

  5. 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);
    
  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. Show all issues

    Register

  3. Show all issues

    Catch

  4. Show all issues

    Create

  5. Show all issues

    Show

  6. Show all issues

    will create.

  7. Show all issues

    Missing period.

  8. Show all issues

    Remove this line.

  9. Show all issues

    "Set up"

  10. Show all issues

    Dedent by 4

  11. Show all issues

    Return.

    Missing period.

  12. Show all issues

    "Send"

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

  13. Show all issues

    Missing period.

  14. Show all issues

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

  15. Show all issues

    Backquotes, not single quotes.

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

  17. Show all issues

    "Create"

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

    Also, minor typos:

    ``RB.notificationManager.notifiy
    
  18. Show all issues

    second line should be indented one more space

  19. Show all issues

    should be indented one more space.

  20. Show all issues

    backquotes, not single quotes.

  21. Show all issues

    "Call"

  22. Show all issues

    Call. shouldNotify.

  23. Show all issues

    "Call". Needs args.

  24. Show all issues

    "Return"

    Needs args.

  25. 
      
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)
     
     
    Show all issues

    "Otherwise,"

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

    "then use their preference"

    "Otherwise,"

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

    Missing a period. Also this should be:

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

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

  6. Show all issues

    The bubble will always be displayed, no?

  7. Show all issues

    Missing a period.

  8. Show all issues

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

  9. Show all issues

    Missing a period.

  10. Show all issues

    Missing period.

  11. Show all issues

    Missing period.

  12. Show all issues

    Missing period.

  13. Show all issues

    Missing period.

  14. Show all issues

    "This function will create ..."

    "notification"

  15. Show all issues

    ", then we will send"

  16. Show all issues

    Missing period.

  17. Show all issues

    "This function will tell.

  18. Show all issues

    It, not This.

  19. Show all issues

    ``true``
    
  20. Show all issues

    "Otherwise, ``false`` will be returned.
    
  21. Show all issues

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

    Note the space.

    Also, missing a period.

    Capitalization of the.

  22. Show all issues

    Same as above.

  23. Show all issues

    Same as above.

  24. Show all issues

    Same as above.

  25. Show all issues

    Capitilizaiton and missing period.

  26. Show all issues

    Same as above.

  27. Show all issues

    Same as above.

  28. Show all issues

    Same as above.

  29. reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    See my comments in the other doc-comment.

  30. Show all issues

    Capitilzation and missing period.

  31. Show all issues

    Same as above.

  32. Show all issues

    Same as above.

  33. Show all issues

    Same as ahove.

  34. reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    See my comments on the other doc comment.

  35. 
      
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)
     
     
    Show all issues
    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)
     
     
    Show all issues

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

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

    This can be:

    return (not self.settings or
            self.settings.get('enable_desktop_notifications', True))
    
  4. Show all issues

    The function name would be better as _onReviewRequestUpdated.

  5. Show all issues

    The /* is indented one space too far.

  6. Show all issues

    "and send", no "s".

  7. Show all issues

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

  8. Show all issues

    Blank line between these.

  9. Show all issues

    No need to capitalize "Updates Bubble."

  10. Show all issues

    Indentation is wonky here.

    Make sure you run jshint to catch these.

  11. Show all issues

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

  12. Show all issues

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

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

  14. reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 11)
     
     
     
     
     
     
    Show all issues

    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

  15. Show all issues

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

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

  17. Show all issues

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

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

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

  20. Show all issues

    This should just be a constant on the model.

  21. Show all issues

    4 space indentation on everything.

  22. Show all issues

    Too many parens. They're not needed here.

  23. Show all issues

    Indentation is off here as well.

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

  24. Show all issues

    Indentation is off here as well.

  25. Show all issues

    Blank line between these.

  26. reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
     
     
     
     
     
     
     
     
    Show all issues

    No backticks for the types.

  27. Show all issues

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

  28. Show all issues

    the /* is indented one space too far.

  29. Show all issues

    Blank line between each argument.

  30. Show all issues

    Blank line between these.

  31. reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
    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.

  32. Show all issues

    Space before {.

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

    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,
          // ...
      
  34. reviewboard/static/rb/js/ui/managers/notificationManagerModel.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.

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

    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.

  36. Show all issues

    Sentence casing.

  37. 
      
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)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/accounts/models.py (Diff revision 12)
     
     
    Show all issues
    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)
     
     
    Show all issues
    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)
     
     
    Show all issues

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

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

    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. Show all issues

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

    NOTIFICATION_TYPE: (window.Notification ||
                        window.mozNotification ||
                        window.webkitNotification)
    
  3. Show all issues

    Single quotes.

  4. Show all issues

    Single quotes.

  5. 
      
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. Show all issues

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

  3. Show all issues

    There is no var statement for this variable.

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

    I'd rather have this all inline:

    RB.Notificationmanager.instance.notify({
        ...
    });
    
  5. Show all issues

    You don't need quotes around JS object keys.

  6. Show all issues

    Undo this.

  7. Show all issues

    Should be at top of function.

  8. 
      
KV
Review request changed
Status:
Completed
Change Summary:
Pushed to feature/desktop-notifications (fd3a323)