-
Notifications
You must be signed in to change notification settings - Fork 1
Web Push delivery service + per-device fan-out (CIRCLE-39) #109
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| require "web-push" | ||
|
|
||
| module CoPlan | ||
| # Delivers one Notification's Web Push payload to one subscription. | ||
| # | ||
| # Per-subscription rather than per-notification so that a single bad | ||
| # endpoint (rate limited, briefly down, etc.) doesn't block delivery to | ||
| # the user's other devices, and so retries / backoff are scoped tightly. | ||
| class WebPushDeliveryJob < ApplicationJob | ||
| queue_as :default | ||
|
|
||
| # SolidQueue retries everything else with backoff. We don't retry the | ||
| # known terminal cases below: ExpiredSubscription / InvalidSubscription | ||
| # are handled inside Deliver and surface as :expired. | ||
| retry_on ::WebPush::PushServiceError, wait: :polynomially_longer, attempts: 5 | ||
| retry_on ::WebPush::TooManyRequests, wait: :polynomially_longer, attempts: 5 | ||
|
|
||
| def perform(notification_id:, subscription_id:) | ||
| notification = Notification.find_by(id: notification_id) | ||
| return unless notification | ||
|
|
||
| subscription = WebPushSubscription.find_by(id: subscription_id) | ||
| return unless subscription | ||
|
|
||
| payload = WebPush::PayloadForNotification.call(notification) | ||
| result = WebPush::Deliver.call(subscription: subscription, payload: payload) | ||
| subscription.destroy if result == :expired | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| require "web-push" | ||
|
|
||
| module CoPlan | ||
| module WebPush | ||
| # Sends a single push payload to a single browser subscription using the | ||
| # configured VAPID key pair. Returns one of: | ||
| # | ||
| # :delivered - push service accepted the message (2xx) | ||
| # :expired - subscription is gone (404 / 410); caller should destroy it | ||
| # | ||
| # Anything else (transient 5xx, rate limiting, network errors) raises so | ||
| # SolidQueue can retry with backoff. | ||
| class Deliver | ||
| def self.call(subscription:, payload:) | ||
| new(subscription: subscription, payload: payload).call | ||
| end | ||
|
|
||
| def initialize(subscription:, payload:) | ||
| @subscription = subscription | ||
| @payload = payload | ||
| end | ||
|
|
||
| def call | ||
| unless CoPlan.configuration.web_push_configured? | ||
| raise ConfigurationError, "Web Push VAPID keys are not configured" | ||
| end | ||
|
|
||
| ::WebPush.payload_send( | ||
| endpoint: @subscription.endpoint, | ||
| p256dh: @subscription.p256dh_key, | ||
| auth: @subscription.auth_key, | ||
| message: @payload.to_json, | ||
| vapid: vapid_options, | ||
| ttl: 24 * 60 * 60, # 24h — push service drops the message after this | ||
| urgency: "normal" | ||
| ) | ||
|
|
||
| @subscription.record_delivery! | ||
| :delivered | ||
| rescue ::WebPush::InvalidSubscription, ::WebPush::ExpiredSubscription | ||
| # Browser unsubscribed or endpoint was rotated. Tell the caller to | ||
| # destroy the row so we don't keep trying. | ||
| :expired | ||
| end | ||
|
|
||
| class ConfigurationError < StandardError; end | ||
|
|
||
| private | ||
|
|
||
| def vapid_options | ||
| { | ||
| subject: CoPlan.configuration.vapid_subject, | ||
| public_key: CoPlan.configuration.vapid_public_key, | ||
| private_key: CoPlan.configuration.vapid_private_key | ||
| } | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| module CoPlan | ||
| module WebPush | ||
| # Builds the { title, body, url, tag } hash the service worker shows for a | ||
| # given Notification. Title/body shape per reason; URL deep-links to the | ||
| # specific thread; tag groups updates for the same thread so successive | ||
| # replies replace each other rather than piling up. | ||
| class PayloadForNotification | ||
| BODY_TRUNCATE = 140 | ||
|
|
||
| def self.call(notification) | ||
| new(notification).call | ||
| end | ||
|
|
||
| def initialize(notification) | ||
| @notification = notification | ||
| @plan = notification.plan | ||
| @thread = notification.comment_thread | ||
| @comment = notification.comment || @thread.comments.order(:created_at).first | ||
| end | ||
|
|
||
| def call | ||
| { | ||
| title: title, | ||
| body: body, | ||
| url: url, | ||
| tag: "comment-thread-#{@thread.id}" | ||
| } | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def title | ||
| case @notification.reason | ||
| when "mention" | ||
| "#{actor_name} mentioned you on #{@plan.title}" | ||
| when "reply" | ||
| "#{actor_name} replied on #{@plan.title}" | ||
| when "new_comment" | ||
| "#{actor_name} commented on #{@plan.title}" | ||
| when "agent_response" | ||
| "Agent updated a thread on #{@plan.title}" | ||
| when "status_change" | ||
| "Thread updated on #{@plan.title}" | ||
| else | ||
| "Update on #{@plan.title}" | ||
| end | ||
| end | ||
|
|
||
| def body | ||
| return "" unless @comment&.body_markdown | ||
|
|
||
| # Strip mention chips and the markdown emphasis/quote/code characters | ||
| # that don't render usefully as plain text in an OS notification. | ||
| # Leave hyphens and `#` alone so co-worker / URL#fragment / -prefix | ||
| # text stays intact. | ||
| text = @comment.body_markdown | ||
| .gsub(/\[@(\w+)\]\(mention:[^)]+\)/, '@\1') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The mention-chip cleanup regex only matches Useful? React with 👍 / 👎. |
||
| .gsub(/[*_`>]/, " ") | ||
| .gsub(/\s+/, " ") | ||
| .strip | ||
| text.truncate(BODY_TRUNCATE, omission: "…") | ||
| end | ||
|
|
||
| def url | ||
| # Relative path is fine — the SW resolves against self.location.origin | ||
| # when opening / focusing the notification target tab. | ||
| CoPlan::Engine.routes.url_helpers.plan_path(@plan, thread: @thread.id) | ||
| end | ||
|
|
||
| def actor_name | ||
| author = @comment&.author | ||
| return "Someone" unless author.respond_to?(:name) && author.name.present? | ||
| author.name | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| require "rails_helper" | ||
|
|
||
| RSpec.describe CoPlan::WebPushDeliveryJob, type: :job do | ||
| let(:user) { create(:coplan_user) } | ||
| let(:subscription) { create(:coplan_web_push_subscription, user: user) } | ||
| let(:plan) { create(:plan, created_by_user: user) } | ||
| let(:thread) do | ||
| create(:comment_thread, | ||
| plan: plan, | ||
| plan_version: plan.current_plan_version, | ||
| created_by_user: user) | ||
| end | ||
| let!(:comment) do | ||
| thread.comments.create!( | ||
| author_type: "human", | ||
| author_id: user.id, | ||
| body_markdown: "Hi" | ||
| ) | ||
| end | ||
| let(:notification) do | ||
| create(:notification, | ||
| user: user, plan: plan, comment_thread: thread, comment: comment, reason: "reply") | ||
| end | ||
|
|
||
| before do | ||
| CoPlan.configuration.vapid_public_key = "pub" | ||
| CoPlan.configuration.vapid_private_key = "priv" | ||
| CoPlan.configuration.vapid_subject = "mailto:test@example.com" | ||
| end | ||
|
|
||
| after do | ||
| CoPlan.configuration.vapid_public_key = nil | ||
| CoPlan.configuration.vapid_private_key = nil | ||
| CoPlan.configuration.vapid_subject = nil | ||
| end | ||
|
|
||
| describe "#perform" do | ||
| it "calls Deliver with the notification's payload and the subscription" do | ||
| allow(CoPlan::WebPush::Deliver).to receive(:call).and_return(:delivered) | ||
|
|
||
| described_class.perform_now(notification_id: notification.id, subscription_id: subscription.id) | ||
|
|
||
| expect(CoPlan::WebPush::Deliver).to have_received(:call).with( | ||
| subscription: subscription, | ||
| payload: hash_including(:title, :body, :url, :tag) | ||
| ) | ||
| end | ||
|
|
||
| it "destroys the subscription when delivery reports :expired" do | ||
| # Eagerly create both rows so the change matcher only counts what the | ||
| # job itself does, not the let-driven setup. | ||
| subscription_id = subscription.id | ||
| notification_id = notification.id | ||
| allow(CoPlan::WebPush::Deliver).to receive(:call).and_return(:expired) | ||
|
|
||
| expect { | ||
| described_class.perform_now(notification_id: notification_id, subscription_id: subscription_id) | ||
| }.to change(CoPlan::WebPushSubscription, :count).by(-1) | ||
| end | ||
|
|
||
| it "no-ops if the notification was already deleted" do | ||
| notification_id = notification.id | ||
| notification.destroy! | ||
|
|
||
| expect { | ||
| described_class.perform_now(notification_id: notification_id, subscription_id: subscription.id) | ||
| }.not_to raise_error | ||
| end | ||
|
|
||
| it "no-ops if the subscription was already deleted" do | ||
| subscription_id = subscription.id | ||
| subscription.destroy! | ||
|
|
||
| expect { | ||
| described_class.perform_now(notification_id: notification.id, subscription_id: subscription_id) | ||
| }.not_to raise_error | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| require "rails_helper" | ||
|
|
||
| RSpec.describe CoPlan::Notification do | ||
| let(:user) { create(:coplan_user) } | ||
| let(:plan) { create(:plan, created_by_user: user) } | ||
| let(:thread) do | ||
| create(:comment_thread, | ||
| plan: plan, | ||
| plan_version: plan.current_plan_version, | ||
| created_by_user: user) | ||
| end | ||
| let(:attrs) do | ||
| { user: user, plan: plan, comment_thread: thread, reason: "reply" } | ||
| end | ||
|
|
||
| describe "after_create web push fan-out" do | ||
| context "when web push is configured" do | ||
| before do | ||
| CoPlan.configuration.vapid_public_key = "pub" | ||
| CoPlan.configuration.vapid_private_key = "priv" | ||
| CoPlan.configuration.vapid_subject = "mailto:test@example.com" | ||
| end | ||
|
|
||
| after do | ||
| CoPlan.configuration.vapid_public_key = nil | ||
| CoPlan.configuration.vapid_private_key = nil | ||
| CoPlan.configuration.vapid_subject = nil | ||
| end | ||
|
|
||
| it "enqueues one delivery job per subscription on create" do | ||
| sub_a = create(:coplan_web_push_subscription, user: user) | ||
| sub_b = create(:coplan_web_push_subscription, user: user) | ||
|
|
||
| expect { | ||
| described_class.create!(attrs) | ||
| }.to have_enqueued_job(CoPlan::WebPushDeliveryJob).twice | ||
|
|
||
| notification = described_class.last | ||
| [sub_a, sub_b].each do |sub| | ||
| expect(CoPlan::WebPushDeliveryJob).to have_been_enqueued.with( | ||
| notification_id: notification.id, | ||
| subscription_id: sub.id | ||
| ) | ||
| end | ||
| end | ||
|
|
||
| it "enqueues nothing when the recipient has no subscriptions" do | ||
| expect { | ||
| described_class.create!(attrs) | ||
| }.not_to have_enqueued_job(CoPlan::WebPushDeliveryJob) | ||
| end | ||
|
|
||
| it "does not enqueue on update" do | ||
| create(:coplan_web_push_subscription, user: user) | ||
| notification = described_class.create!(attrs) | ||
|
|
||
| expect { | ||
| notification.mark_read! | ||
| }.not_to have_enqueued_job(CoPlan::WebPushDeliveryJob) | ||
| end | ||
| end | ||
|
|
||
| context "when web push is not configured" do | ||
| it "does not enqueue any jobs even if subscriptions exist" do | ||
| # Sanity: in test env VAPID keys are nil by default. | ||
| create(:coplan_web_push_subscription, user: user) | ||
|
|
||
| expect { | ||
| described_class.create!(attrs) | ||
| }.not_to have_enqueued_job(CoPlan::WebPushDeliveryJob) | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebPushDeliveryJob#performfetches a subscription by ID and sends immediately, but never checks that it is still owned by the notification’s user. This can leak notifications across accounts becauseWebPushSubscription.upsert_forreassigns an existingendpoint_digestrow to whatever user most recently subscribes on that browser/device; if that reassignment happens before the queued job runs, the payload is delivered to the wrong user’s device. Guard onsubscription.user_id == notification.user_id(or scope the lookup by both IDs) before calling deliver.Useful? React with 👍 / 👎.