Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Notifications 2.0 #5071

Closed
2 of 3 tasks
ErisDS opened this issue Mar 25, 2015 · 19 comments · Fixed by #7077 or #9123
Closed
2 of 3 tasks

Update Notifications 2.0 #5071

ErisDS opened this issue Mar 25, 2015 · 19 comments · Fixed by #7077 or #9123
Assignees
Labels
affects:admin Anything relating to Ghost Admin feature [triage] New features we're planning or working on server / core Issues relating to the server or core of Ghost

Comments

@ErisDS
Copy link
Member

ErisDS commented Mar 25, 2015

A little while back, we added a system to Ghost which regularly asks Ghost.org if there's a new version of Ghost available.

Ghost.org replies with a little bit of JSON like this:

{
  "version": "0.5.0",
  "next_check": 1427366737
}

This tells Ghost what the current version is, and when it is allowed to ask again.

Inside of Ghost, this is wired up to our notification system, and if the version number goes above the current version of Ghost, a special notification is shown at the top of Ghost, on all pages, which doesn't go away until the blog is upgraded.

The major problem with this, is that it's incredibly invasive and annoying, so we're currently only using it for major version updates, rather than minors, so as not to infuriate our users when we do fast releases. Clearly, that makes the system significantly less useful.

The second problem is that the message in the notifcation is hardcoded inside of Ghost, and that makes it not particularly useful. There's no way to say 'hey, this interesting thing happened and that's why you should update'. There's also no way to send a notification outside of a version change, which is quite limiting.

To make this whole system SIGNIFICANTLY more useful both to us and to our users, I want to make the following changes:

Move the "update available" notification to the about page.

As this is where the version information is currently shown, lets show the update information here. It means that users have to check the page to find out, but it does at least mean the information is always to hand in an easy to find place.

Add support for custom messages

The updates endpoint on Ghost.org is going to be updated to send back a little bit more JSON, that will look like this by default:

{
  "version": "0.5.0",
  "next_check": 1427366737,
  "messages": []
}

When we want to display a message to our users, it will look like this

{
  "version": "0.5.0",
  "next_check": 1427366737,
  "messages": [{
    "id": ed9dc38c-73e5-4d72-a741-22b11f6e151a,
    "version": "0.5.x",
    "content": "<p>Hey there! 0.6 is available, visit <a href=\"https://ghost.org/download\">Ghost.org</a> to grab your copy now<!/p>" 
  ]}
}

The intention is that Ghost will read these, and display these custom notifications in the top notification spot where update notifications used to go, allowing us to push a particular release or feature, without interfering with the general concept of 'is there an update available'. The 'top' notifications will need to be changed to be dismissible, unlike the existing version.

Note: At some point since 0.5.0, there has been a regression in the notification system, such that top notifications are no longer shown in the correct spot at the top of the screen, but appear in the normal bottom left 'toaster' location. This will need fixing.

Each custom message from Ghost.org will have 3 properties, firstly a GUID which should be stored in an array of seenNotifications in the settings table once a message is dismissed, so that the message is not shown again. Secondly, a version string which adheres to semver - this is to indicate which versions of Ghost the notification should be shown to. Finally the HTML content for the notification is provided in full, but should be sanitized via the caja tools already in the Ghost admin client, to ensure this cannot be abused.

Once we have this in place, we then have 2 different notification concepts driven on versioning - whether or not there is an update available, which is simple factual information and custom messages, which provide an opportunity to communicate important details to our users.

@nuclearpengy
Copy link

+1 :)

@joshkerr
Copy link

+1

@crepererum
Copy link

Please be careful with the content field. Otherwise code injections to ALL running ghost installations are possible in case that the central update notifier gets compromised. That's a way higher security threat than compromised download files, because the admins have no chance to prevent this and aren't even aware of this potential attack scenario.

@ErisDS
Copy link
Member Author

ErisDS commented Mar 26, 2015

@crepererum This is a very good point, and not one that had escaped my attention. I think it's expected that any notification pulled from an external source should be run through the html_sanitizer to remove potentially dangerous code and I've updated the issue to say this explicitly.

@ErisDS ErisDS added help wanted [triage] Ideal issues for contributors to help with feature [triage] New features we're planning or working on labels Apr 3, 2015
@ErisDS ErisDS added this to the Current Backlog milestone Apr 3, 2015
@ErisDS ErisDS modified the milestones: Current Backlog, Next Backlog May 25, 2015
@naz
Copy link
Contributor

naz commented Jul 21, 2015

Hi @ErisDS . Is anyone in particular working on this issue or it could be taken to finish up?
Haven't contributed in a while and would love to start again from something 😃

@ErisDS
Copy link
Member Author

ErisDS commented Jul 21, 2015

Hi @gargol no one is working on this - so it's all yours 👍 Welcome back 😃

@naz
Copy link
Contributor

naz commented Jul 29, 2015

@ErisDS was looking into this issue last night and thought that it's closely related to this refactoring of notification system #5409. After reviewing the changes that are being done there, have couple related questions.

  1. Does update notification on about page fall into the category of Alerts? Or maybe this this should be just completely different concept that is only used on about page (on the mockup it's just a red text that will be configurable) ?
  2. What about those version specific messages? What category should those fall into?

Let me know, if I'm missing something. Maybe there is some guideline for these specific version update notifications that I've missed 😏

@naz
Copy link
Contributor

naz commented Aug 5, 2015

@ErisDS ping.

@ErisDS
Copy link
Member Author

ErisDS commented Aug 6, 2015

Hi @gargol, sorry for the delay. This issue was written before the frontend notification refactor was spec'd and there hasn't been much consideration for how the changes proposed here fit in with the changes being made elsewhere.

  1. Does update notification on about page fall into the category of Alerts? Or maybe this this should be just completely different concept that is only used on about page (on the mockup it's just a red text that will be configurable) ?

The notification on the about page is intended to be something different to an alert. Just a message which appears on that page when there is an update available. In future it'd be cool to have an indicator in the navigation ui that this message is there (like red bubble number indicators that are common in iOS, slack, etc) - that's not expected as part of this issue but just something to keep it in mind that that is how this is being thought of - as a notification which belongs to a specific part of the Ghost interface, instead of a global notification.

  1. What about those version specific messages? What category should those fall into?

They should be alerts. Alerts are the old 'top' notifications, but they are dismissible, which is what was spec'd in the issue here, so that part of the work is done already :)

@naz
Copy link
Contributor

naz commented Aug 20, 2015

While implementing seenNotifications part, had a thought about future clean up of this information. Let's say we are 20 versions upstream and the end user will be having ~20 ids in seenNotifications row in settings table. Should we maybe think about some kind of cleanup for these records, as they should not be able to grow indefinitely?

@ErisDS ErisDS modified the milestone: Next Backlog Oct 9, 2015
@ErisDS
Copy link
Member Author

ErisDS commented Oct 12, 2015

@gargol sorry for the ridiculously slow response here. I don't think cleanup is something that really needs to be thought in the short term, as it would take a long time to get to even 20 ids.

kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Oct 11, 2017
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
- add `utils/semver-compare` that can compare/sort semver version strings
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Oct 11, 2017
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
- add `utils/semver-compare` that can compare/sort semver version strings
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Oct 11, 2017
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
- add `utils/semver-compare` that can compare/sort semver version strings
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Oct 12, 2017
refs TryGhost#5071

- if Ghost received an error from the service e.g. no update notifications available
- we have to add a proper handling in Ghost
- e.g. we don't log 404
- add tests
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Oct 12, 2017
kirrg001 added a commit that referenced this issue Oct 12, 2017
refs #5071

- if Ghost received an error from the service e.g. no update notifications available
- we have to add a proper handling in Ghost
- e.g. we don't log 404
- add tests
- more tests for next update check
@kirrg001
Copy link
Contributor

kirrg001 commented Nov 8, 2017

This was merged into LTS. I had no time so far to finish the PR for master. Simply because of other priorities. I think it's a 1-2 days work to finally get the changes merged into 1.0. And then we are able to close this issue 😁

@kirrg001
Copy link
Contributor

kirrg001 commented Nov 22, 2017

Regarding the pull requests to master (server, admin).

So in the past Ghost fetched release notifications from the update check service with a delay of one day - otherwise too many requests were being made to the service. And the admin client pulled the cached notifications from Ghost.

With notifications 2.0, we have a differentiation between release and custom notifications.

If we put release notifications into the about page for master (see), we can't detect if a user has seen the release notification and it's also not really important. A seen notification tells the server that this notification can be removed. That's why the admin client fetches the update check url from Ghost's configuration endpoint and requests the latest release notification directly from the service, see.

Users do not often visit the about page, that's why there is no urgent reason to add a one day request delay logic. But if we want to add a little badge to show that a new release is available (somewhere over the little menu left-top), we would have to add a delay. But both cc @kevinansfield. Both is no high priority and needs confirmation first, but i still wonder how we communicate that a new release notification is available.

And any custom notification is pulled from Ghost. This can be any notification - even a custom release notification because of a security fix. It's shown as a top bar element and can be configured to be dismissible or not (i think 99% of notifications are dismissible).

The service can also provide a custom notification, which should not be displayed on top, because any parameter (top, dismissible etc.) is just a boolean we can set. But as long as we don't have a clear use case, there is no need to support this feature in the admin. That means we simply don't support none-top custom notifications for now. If we want to support them, we can either show them in the about page or in a separate notification menu section or...or...

@kevinansfield
Copy link
Contributor

But if we want to add a little badge to show that a new release is available (somewhere over the little menu left-top), we would have to add a delay.

This isn't particularly hard to add in the client, we can store the last version and checked date in local storage with the auth data. We then have the option of using the cached value for a badge and forcing a check on the about screen or using the cached value everywhere.

@kirrg001
Copy link
Contributor

This isn't particularly hard to add in the client, we can store the last version and checked date in local storage with the auth data. We then have the option of using the cached value for a badge and forcing a check on the about screen or using the cached value everywhere.

Cool, thanks for feedback. But not for the first version. The first version will simply show if a new release is available or if your blog is up-to-date (on the about page).

kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Nov 23, 2017
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
  - will only perform upgrade check if `config.allowUpgradeCheck` is true
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
  - will only render if `config.allowUpgradeCheck` is true
- add `utils/semver-compare` that can compare/sort semver version strings
@kirrg001
Copy link
Contributor

See #9123 (comment).

kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Dec 6, 2017
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
  - will only perform upgrade check if `config.allowUpgradeCheck` is true
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
  - will only render if `config.allowUpgradeCheck` is true
- add `utils/semver-compare` that can compare/sort semver version strings
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Dec 8, 2017
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
  - will only perform upgrade check if `config.allowUpgradeCheck` is true
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
  - will only render if `config.allowUpgradeCheck` is true
- add `utils/semver-compare` that can compare/sort semver version strings
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Dec 8, 2017
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
  - will only perform upgrade check if `config.allowUpgradeCheck` is true
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
  - will only render if `config.allowUpgradeCheck` is true
- add `utils/semver-compare` that can compare/sort semver version strings
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Jan 4, 2018
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
  - will only perform upgrade check if `config.allowUpgradeCheck` is true
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
  - will only render if `config.allowUpgradeCheck` is true
- add `utils/semver-compare` that can compare/sort semver version strings
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Jan 9, 2018
refs TryGhost/Ghost#5071
- remove old `upgrade-notification` service
- add new `upgrades` service
  - `upgrades.check(version)` is a task that will query the update check service directly
  - `upgrades.upgradeMessage` will contain the last upgrade message received
  - will only perform upgrade check if `config.allowUpgradeCheck` is true
- modify `{{gh-upgrade-notification}}` component to act as a wrapper around the `upgrades` service
  - triggers an upgrade check when rendered
  - yields an object that can be used to indicate the upgrade check status
  - will only render if `config.allowUpgradeCheck` is true
- add `utils/semver-compare` that can compare/sort semver version strings
kirrg001 added a commit that referenced this issue Jan 9, 2018
closes #5071

- Remove hardcoded notification in admin controller
  - NOTE: update check notifications are no longer blocking the admin rendering
  - this is one of the most import changes
  - we remove the hardcoded release message
  - we also remove adding a notification manually in here, because this will work differently from now on
    -> you receive a notification (release or custom) in the update check module and this module adds the notification as is to our database

- Change default core settings keys
  - remove displayUpdateNotification
    -> this was used to store the release version number send from the UCS
    -> based on this value, Ghost creates a notification container with self defined values
    -> not needed anymore

- rename seenNotifications to notifications
  -> the new notifications key will hold both
     1. the notification from the USC
     2. the information about if a notification was seen or not
  - this key hold only one release notification
  - and n custom notifications

- Update Check Module: Request to the USC depends on the privacy configuration
  - useUpdateCheck: true -> does a checkin in the USC (exposes data)
  - useUpdateCheck: false -> does only a GET query to the USC (does not expose any data)
  - make the request handling dynamic, so it depends on the flag
  - add an extra logic to be able to define a custom USC endpoint (helpful for testing)
  - add an extra logic to be able to force the request to the service (helpful for testing)

- Update check module: re-work condition when a check should happen
  - only if the env is not correct
  - remove deprecated config.updateCheck
  - remove isPrivacyDisabled check (handled differently now, explained in last commit)

- Update check module: remove `showUpdateNotification` and readability
  - showUpdateNotification was used in the admin controller to fetch the latest release version number from the db
  - no need to check against semver in general, the USC takes care of that (no need to double check)
  - improve readability of `nextUpdateCheck` condition

- Update check module: refactor `updateCheckResponse`
  - remove db call to displayUpdateNotification, not used anymore
  - support receiving multiple custom notifications
  - support custom notification groups
  - the default group is `all` - this will always be consumed
  - groups can be extended via config e.g. `notificationGroups: ['migration']`

- Update check module: refactor createCustomNotification helper
  - get rid of taking over notification duplication handling (this is not the task of the update check module)
  - ensure we have good fallback values for non present attributes in a notification
  - get rid of semver check (happens in the USC) - could be reconsidered later if LTS is gone

- Refactor notification API
  - reason: get rid of in process notification store
    -> this was an object hold in process
    -> everything get's lost after restart
    -> not helpful anymore, because imagine the following case
      -> you get a notification
      -> you store it in process
      -> you mark this notification as seen
      -> you restart Ghost, you will receive the same notification on the next check again
      -> because we are no longer have a separate seen notifications object
  - use database settings key `notification` instead
  - refactor all api endpoints to support reading and storing into the `notifications` object
  - most important: notification deletion happens via a `seen` property (the notification get's physically deleted 3 month automatically)
    -> we have to remember a seen property, because otherwise you don't know which notification was already received/seen

- Add listener to remove seen notifications automatically after 3 month
  - i just decided for 3 month (we can decrease?)
  - at the end it doesn't really matter, as long as the windows is not tooooo short
  - listen on updates for the notifications settings
  - check if notification was seen and is older than 3 month
  - ignore release notification

- Updated our privacy document
- Updated docs.ghost.org for privacy config behaviour
- contains a migration script to remove old settings keys
kevinansfield added a commit to TryGhost/Admin that referenced this issue Jan 9, 2018
refs TryGhost/Ghost#5071

Upgrade messages are now shown on the About screen rather than as alerts. Notifications that are marked as `top` or `custom` are still shown as alerts to allow for certain upgrade messages to be given more visibility.

- remove old `upgrade-notification` service
- update the `upgrade-status` service:
  - add a `message` property that contains an upgrade notification if any exists
  - add a `handleUpgradeNotification` method that accepts a Notification model instance and extracts the `notification.message` property into a html safe string for use in templates
- when loading server notifications during app boot, pass notifications that aren't marked as `top` or `custom` to the new `handleUpgradeNotification` method
- update the `about.hbs` template to pull the upgrade message from the `upgradeStatus` service
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:admin Anything relating to Ghost Admin feature [triage] New features we're planning or working on server / core Issues relating to the server or core of Ghost
Projects
None yet
7 participants