Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions NOTIFICATIONS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# Notifications

This explains how notifications work in OnTrack after the unification.

## The idea

Before, each type of message did its own thing. Emails were sent from many
places. There was no single system.

Now there is one system. You send a notification once. It goes out on all the
channels the user has turned on: in-app, email, and Web Push when the deployment
has VAPID keys and the user has subscribed a browser.

## The flow

something happens in the app
-> you call NotificationService.notify(...)
-> saves an in-app notification (the bell)
-> queues an ID-only email job
-> queues an ID-only push job
-> Sidekiq workers reload the notification and contact providers

You only call one thing. The system handles the rest.

## How to send one

Call this from anywhere in the API code:

NotificationService.notify(
user: project.student,
type: 'feedback',
event: 'task_comment_created',
message: "New feedback is ready for #{task_definition.name}.",
link: "/#/projects/#{project.id}"
)

- user: who gets it.
- type: the category. One of task, feedback, portfolio, extension, general.
This is what the user's on/off setting controls.
- event: the specific thing that happened, as a lower_snake_case string.
Required. Use one event name per ticket, and use the same name every time you
raise that notification, so a notification can always be traced back to the
code that sent it.
- message: the text the user sees. Keep it short, 500 characters at most.
- link: where clicking it should take them. Optional.

type and event are different on purpose. type is the coarse category the user
switches off in their profile. event is the fine-grained reason, and there will
be many events inside one type.

## Types and preferences

Each user already has three on/off settings in their profile:

- receive_task_notifications
- receive_feedback_notifications
- receive_portfolio_notifications

The type you pass maps to one of these settings.

- task uses receive_task_notifications
- feedback uses receive_feedback_notifications
- portfolio uses receive_portfolio_notifications
- extension and general are always sent

If the matching setting is off, nothing is sent. Not the bell, not the email,
not the push. One switch controls all channels. This keeps it simple. We can add
per-channel switches later if we want.

## The pieces

- app/models/notification.rb: the notification record. Has the type, message,
link, and whether it has been read.
- app/services/notification_service.rb: the one entry point. Checks the setting,
saves the record, and queues the email and push channel jobs.
- app/sidekiq/notification_email_job.rb: reloads a notification by id and sends
its email on the `mailers` queue.
- app/sidekiq/push_notification_delivery_job.rb: reloads a notification by id
and hands it to the Web Push delivery channel on the `notifications` queue.
- app/services/push_notification_service.rb: the Web Push delivery channel. It
remains a safe no-op until both VAPID keys are configured.
- app/mailers/notifications_mailer.rb: the email. New method single_notification
with templates in app/views/notifications_mailer.
- app/api/notifications_api.rb: the endpoints the web app calls.
- app/api/entities/notification_entity.rb: the shape of the data sent back.

## The endpoints

GET /api/notifications list my notifications
GET /api/notifications/unread_count how many I have not read
PUT /api/notifications/:id/read mark one as read
PUT /api/notifications/read_all mark all as read
DELETE /api/notifications/:id delete one

GET /api/push_subscriptions list my browser subscriptions
POST /api/push_subscriptions register or update a browser
DELETE /api/push_subscriptions remove the browser identified by endpoint

Every endpoint only ever touches the current user's own notifications.

## What is on now

- In-app: working. The record is saved and the endpoints return it.
- Email: working. Best effort. If email fails, the in-app notification is still
saved.
- Push: implemented and deliberately configuration-gated. It sends only when
VAPID keys are set and that user has opted in from a supported browser. Keep
the production keys blank until browser/device acceptance testing is complete.
17 changes: 17 additions & 0 deletions app/api/additional_notification_email_verification_api.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# frozen_string_literal: true

class AdditionalNotificationEmailVerificationApi < Grape::API
desc 'Verify ownership of an additional notification email'
params do
requires :token, type: String
end
post '/additional_notification_emails/verify' do
header 'Cache-Control', 'private, no-store'
AdditionalNotificationEmailService.verify(token: params[:token])
{ status: 'verified' }
rescue AdditionalNotificationEmailService::AlreadyVerified
error!({ error: 'This verification link has already been used.' }, 409)
rescue AdditionalNotificationEmailService::InvalidToken
error!({ error: 'This verification link is invalid or has expired.' }, 422)
end
end
77 changes: 77 additions & 0 deletions app/api/additional_notification_emails_api.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# frozen_string_literal: true

class AdditionalNotificationEmailsApi < Grape::API
helpers AuthenticationHelpers

before do
authenticated?
header 'Cache-Control', 'private, no-store'
end

helpers do
def own_user!
error!({ error: 'You can only manage your own additional notification email.' }, 403) unless params[:user_id] == current_user.id

current_user
end

def present_additional_email(record)
if record.nil?
{ status: 'none', email: nil, verification_expires_at: nil }
else
{
status: record.verified? ? 'verified' : 'pending',
email: record.email,
verification_expires_at: record.verification_expires_at
}
end
end
end

desc 'Get the current user additional notification email state'
params do
requires :user_id, type: Integer
end
get '/users/:user_id/additional_notification_email' do
user = own_user!
present present_additional_email(user.additional_notification_email)
end

desc 'Request or replace an additional notification email'
params do
requires :user_id, type: Integer
requires :email, type: String
end
put '/users/:user_id/additional_notification_email' do
user = own_user!
record = AdditionalNotificationEmailService.request(user: user, email: params[:email])
present present_additional_email(record)
rescue AdditionalNotificationEmailService::RateLimited
error!({ error: 'Too many verification requests. Try again in one hour.' }, 429)
end

desc 'Resend additional notification email verification'
params do
requires :user_id, type: Integer
end
post '/users/:user_id/additional_notification_email/resend' do
user = own_user!
record = AdditionalNotificationEmailService.resend(user: user)
present present_additional_email(record)
rescue AdditionalNotificationEmailService::AlreadyVerified
error!({ error: 'This additional notification email is already verified.' }, 409)
rescue AdditionalNotificationEmailService::RateLimited
error!({ error: 'Too many verification requests. Try again in one hour.' }, 429)
end

desc 'Remove the current user additional notification email'
params do
requires :user_id, type: Integer
end
delete '/users/:user_id/additional_notification_email' do
user = own_user!
AdditionalNotificationEmailService.remove(user: user)
status 204
body false
end
end
19 changes: 19 additions & 0 deletions app/api/entities/notification_entity.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module Entities
class NotificationEntity < Grape::Entity
expose :id
expose :notification_type
expose :event
expose :message
expose :link
expose :read_at
expose :created_at

# Added after link, and never instead of it. A client that only knows link
# keeps working. A newer client opens the exact page from these ids, and a
# nil means the record is gone, so it can say so instead of opening a
# blank page.
Notification::TARGET_KEYS.each do |key|
expose(key) { |notification, _options| notification.target_ids[key] }
end
end
end
12 changes: 12 additions & 0 deletions app/api/entities/push_subscription_entity.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module Entities
class PushSubscriptionEntity < Grape::Entity
expose :id
expose :endpoint
expose :created_at
expose :updated_at

# p256dh and auth are deliberately not exposed. They are the browser's own
# encryption material, the client already holds them, and nothing in the UI
# needs them read back.
end
end
80 changes: 80 additions & 0 deletions app/api/notifications_api.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
require 'grape'

class NotificationsApi < Grape::API
helpers AuthenticationHelpers
helpers AuthorisationHelpers

before do
authenticated?
end

desc 'Get the current user notifications'
params do
optional :unread_only, type: Boolean, default: false, desc: 'Only return unread notifications'
end
get '/notifications' do
notifications = current_user.notifications.recent_first
notifications = notifications.unread if params[:unread_only]

present notifications, with: Entities::NotificationEntity
end

desc 'Get the current user unread notification count'
get '/notifications/unread_count' do
{ count: current_user.notifications.unread.count }
end

desc 'Mark a notification as read'
params do
requires :id, type: Integer, desc: 'The notification id'
end
put '/notifications/:id/read' do
notification = current_user.notifications.find(params[:id])
notification.mark_read!

present notification, with: Entities::NotificationEntity
end

desc 'Mark all of the current user notifications as read'
put '/notifications/read_all' do
# rubocop:disable Rails/SkipsModelValidations
current_user.notifications.unread.update_all(read_at: Time.zone.now)
# rubocop:enable Rails/SkipsModelValidations

status 200
{ success: true }
end

desc 'Delete the current user notifications up to a confirmed boundary'
params do
requires :through_id,
type: Integer,
values: ->(value) { value.positive? },
desc: 'Delete only notifications whose id is at or below this value'
end
delete '/notifications' do
# The boundary is part of the contract, not an optimisation. A notification
# can arrive after the browser has shown "Delete all" and while the user is
# reading the confirmation. Deleting the unbounded association here would
# remove that unseen notification too. Scoping through current_user keeps
# the operation account-local in the same way as the single-row endpoint.
deleted_count = current_user.notifications
.where('notifications.id <= ?', params[:through_id])
.delete_all

status 200
{ success: true, deleted_count: deleted_count }
end

desc 'Delete a notification'
params do
requires :id, type: Integer, desc: 'The notification id'
end
delete '/notifications/:id' do
notification = current_user.notifications.find(params[:id])
notification.destroy!

status 200
{ success: true }
end
end
64 changes: 64 additions & 0 deletions app/api/push_subscriptions_api.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
require 'grape'

class PushSubscriptionsApi < Grape::API
helpers AuthenticationHelpers
helpers AuthorisationHelpers

before do
authenticated?
end

desc 'Get the push subscriptions belonging to the current user'
get '/push_subscriptions' do
present current_user.push_subscriptions.order(:id), with: Entities::PushSubscriptionEntity
end

# The endpoint posted here is later used as the target of an outbound request
# by PushNotificationService, so it is not accepted as free text.
# PushSubscription validates it against PUSH_SERVICE_HOSTS, and a URL that is
# not an https push service URL fails with a 400 from the handler in
# api_root.rb. PushNotificationService checks again before it sends.
desc 'Register this browser to receive push notifications'
params do
requires :endpoint, type: String, desc: 'The push service URL, from PushSubscription.endpoint'
requires :p256dh, type: String, desc: 'The browser public key, from PushSubscription.getKey("p256dh")'
requires :auth, type: String, desc: 'The browser auth secret, from PushSubscription.getKey("auth")'
end
post '/push_subscriptions' do
subscription = current_user.push_subscriptions.find_by(endpoint: params[:endpoint])

# Not ours, or not stored yet. An endpoint identifies a browser rather than
# a person, so an endpoint held by another user means someone has signed in
# on a machine that account used. Move the registration across instead of
# failing on the unique index.
#
# This is the only lookup in this file that is not scoped to current_user,
# and it is safe. The push service delivers to that browser no matter which
# row owns it, and the payload is encrypted to the keys posted here. Taking
# over someone else's endpoint cannot read their notifications, it can only
# stop them arriving, and you need the endpoint URL to try it at all.
subscription ||= PushSubscription.find_by(endpoint: params[:endpoint]) || PushSubscription.new

subscription.assign_attributes(
user: current_user,
endpoint: params[:endpoint],
p256dh: params[:p256dh],
auth: params[:auth]
)
subscription.save!

present subscription, with: Entities::PushSubscriptionEntity
end

desc 'Stop this browser receiving push notifications'
params do
requires :endpoint, type: String, desc: 'The push service URL to remove'
end
delete '/push_subscriptions' do
subscription = current_user.push_subscriptions.find_by!(endpoint: params[:endpoint])
subscription.destroy!

status 200
{ success: true }
end
end
Loading