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: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (29284ef)
Loading...