• 
      

    Fix support for displaying desktop notifications.

    Review Request #9453 — Created Dec. 18, 2017 and submitted

    Information

    Review Board
    release-3.0.x
    4bcfa6a...

    Reviewers

    We landed a student project for displaying desktop notifications in
    Review Board 3.0, but unfortunately the final revision did not work.
    There were a handful of problems, starting with bad variable references
    that simply prevented the browser from prompting for permission to
    display notifications and prevented those notifications from ever being
    displayed. There were other issues as well, such as hard-coded strings,
    lack of click support for notifications, and no body text.

    This fixes up the support to properly work. Now, users will be prompted
    for permission when next viewing the review request page, and if
    granted, a notification will be shown whenever the in-page updates
    bubble would be displayed.

    We are limited in what we can show in that bubble, in terms of the
    structure and length of content. For now, the summary of the
    notification matches the summary from the API ("Review request updated",
    "Diff updated", "New review", etc.). The body then contains the review
    request ID and the person who made the update. This isn't perfect, but
    even as-is, the name gets cut off on macOS. Other platforms may impose
    their own limitations as well.

    Still, it's useful when not paying attention to the page. Going forward,
    we may want to tweak the content for the notification (perhaps adding
    more information in the API that we can use), and perhaps add action
    support for things like viewing the diff, for platforms that support it.

    Tested that notifications worked on Chrome and Firefox. I was prompted
    when visiting the page, and then shown notifications any time the
    updates bubble appeared.

    Tested that clicking the notification reloaded the review request page.

    Description From Last Updated

    Trailing comma.

    daviddavid

    Trivial, but maybe use the local notification variable here?

    daviddavid

    Col: 6 Missing semicolon.

    reviewbotreviewbot

    If you put a let instance in the top-level function you can skip similar const definitions in each test case …

    daviddavid

    This blank line is a little weird.

    daviddavid

    Trailing comma.

    daviddavid
    Checks run (1 failed, 1 succeeded)
    flake8 passed.
    JSHint failed.

    JSHint

    david
    1. 
        
    2. Show all issues

      Trailing comma.

    3. Show all issues

      Trivial, but maybe use the local notification variable here?

    4. Show all issues

      If you put a let instance in the top-level function you can skip similar const definitions in each test case (and change a bunch of RB.NotificationManager.instance to instance).

    5. 
        
    chipx86
    david
    1. 
        
    2. Show all issues

      This blank line is a little weird.

    3. Show all issues

      Trailing comma.

    4. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (29284ef)