Merge pull request #1109 from Retrospring/feature/pwa-badges

Show notification badges on PWA & cache notification/inbox counters

app/helpers/application_helper.rb

@@ -4,30 +4,6 @@ module ApplicationHelper
   include ApplicationHelper::GraphMethods
   include ApplicationHelper::TitleMethods
 
-  def inbox_count
-    return 0 unless user_signed_in?
-
-    count = Inbox.select("COUNT(id) AS count")
-                 .where(new: true)
-                 .where(user_id: current_user.id)
-                 .group(:user_id)
-                 .order(:count)
-                 .first
-    return nil if count.nil?
-    return nil unless count.count.positive?
-
-    count.count
-  end
-
-  def notification_count
-    return 0 unless user_signed_in?
-
-    count = Notification.for(current_user).where(new: true).count
-    return nil unless count.positive?
-
-    count
-  end
-
   def privileged?(user)
     !current_user.nil? && ((current_user == user) || current_user.mod?)
   end

app/helpers/bootstrap_helper.rb

@@ -5,6 +5,7 @@ module BootstrapHelper
     options = {
       badge:       nil,
       badge_color: nil,
+      badge_attr:  {},
       icon:        nil,
       class:       ""
     }.merge(options)
@@ -29,7 +30,7 @@ module BootstrapHelper
         ("badge-pill" if options[:badge_pill])
       ].compact.join(" ")
 
-      body += " #{content_tag(:span, options[:badge], class: badge_class)}".html_safe
+      body += " #{content_tag(:span, options[:badge], class: badge_class, **options[:badge_attr])}".html_safe
     end
 
     content_tag(:li, link_to(body.html_safe, path, class: "nav-link"), class: classes)

app/javascript/retrospring/controllers/pwa_badge_controller.ts

@@ -0,0 +1,18 @@
+import { Controller } from '@hotwired/stimulus';
+
+export default class extends Controller<HTMLElement> {
+  isPwa: boolean;
+  badgeCapable: boolean;
+
+  initialize(): void {
+    this.isPwa = window.matchMedia('(display-mode: standalone)').matches;
+    this.badgeCapable = "setAppBadge" in navigator;
+  }
+
+  connect(): void {
+    if (this.isPwa && this.badgeCapable) {
+      const count = Number.parseInt(this.element.innerText);
+      navigator.setAppBadge(count);
+    }
+  }
+}

app/javascript/retrospring/initializers/stimulus.ts

@@ -10,6 +10,7 @@ import CapabilitiesController from "retrospring/controllers/capabilities_control
 import CropperController from "retrospring/controllers/cropper_controller";
 import InboxSharingController from "retrospring/controllers/inbox_sharing_controller";
 import ToastController from "retrospring/controllers/toast_controller";
+import PwaBadgeController from "retrospring/controllers/pwa_badge_controller";
 
 /**
  * This module sets up Stimulus and our controllers
@@ -29,6 +30,7 @@ export default function (): void {
   window['Stimulus'].register('cropper', CropperController);
   window['Stimulus'].register('format-popup', FormatPopupController);
   window['Stimulus'].register('inbox-sharing', InboxSharingController);
+  window['Stimulus'].register('pwa-badge', PwaBadgeController);
   window['Stimulus'].register('theme', ThemeController);
   window['Stimulus'].register('toast', ToastController);
 }

app/models/inbox.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class Inbox < ApplicationRecord
-  belongs_to :user
+  belongs_to :user, touch: :inbox_updated_at
   belongs_to :question
 
   attr_accessor :returning
@@ -27,9 +27,10 @@ class Inbox < ApplicationRecord
     self.destroy
   end
 
+  def notification_icon = question.author_is_anonymous ? "/icons/maskable_icon_x128.png" : question.user.profile_picture.url(:small)
+
   def as_push_notification
     {
-      type:  :inbox,
       title: I18n.t(
         "frontend.push_notifications.inbox.title",
         user: if question.author_is_anonymous
@@ -38,8 +39,11 @@ class Inbox < ApplicationRecord
                 question.user.profile.safe_name
               end
       ),
-      icon:  question.author_is_anonymous ? "/icons/maskable_icon_x128.png" : question.user.profile_picture.url(:small),
+      icon:  notification_icon,
-      body:  question.content.truncate(Question::SHORT_QUESTION_MAX_LENGTH)
+      body:  question.content.truncate(Question::SHORT_QUESTION_MAX_LENGTH),
+      data:  {
+        click_url: "/inbox",
+      },
     }
   end
 end

app/models/notification.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class Notification < ApplicationRecord
-  belongs_to :recipient, class_name: "User"
+  belongs_to :recipient, class_name: "User", touch: :notifications_updated_at
   belongs_to :target, polymorphic: true
 
   class << self
@@ -11,11 +11,11 @@ class Notification < ApplicationRecord
     define_cursor_paginator :cursored_for_type, :for_type
 
     def for(recipient, **kwargs)
-      where(kwargs.merge!(recipient:)).includes(:target).order(:created_at).reverse_order
+      where(kwargs.merge!(recipient:)).order(:created_at).reverse_order
     end
 
     def for_type(recipient, type, **kwargs)
-      where(kwargs.merge!(recipient:)).includes(:target).where(type:).order(:created_at).reverse_order
+      where(kwargs.merge!(recipient:)).where(type:).order(:created_at).reverse_order
     end
 
     def notify(recipient, target)

app/models/user.rb

@@ -8,6 +8,7 @@ class User < ApplicationRecord # rubocop:disable Metrics/ClassLength
   include User::AnswerMethods
   include User::BanMethods
   include User::InboxMethods
+  include User::NotificationMethods
   include User::QuestionMethods
   include User::PushNotificationMethods
   include User::ReactionMethods

app/models/user/inbox_methods.rb

@@ -12,4 +12,18 @@ module User::InboxMethods
       .order(:created_at)
       .reverse_order
   end
+
+  def unread_inbox_count
+    Rails.cache.fetch(inbox_cache_key) do
+      count = Inbox.where(new: true, user_id: id).count(:id)
+
+      # Returning +nil+ here in order to not display a counter
+      # at all when there isn't anything in the user's inbox
+      return nil unless count.positive?
+
+      count
+    end
+  end
+
+  def inbox_cache_key = "#{cache_key}/unread_inbox_count-#{inbox_updated_at}"
 end

app/models/user/notification_methods.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module User::NotificationMethods
+  def unread_notification_count
+    Rails.cache.fetch(notification_cache_key) do
+      count = Notification.for(self).where(new: true).count(:id)
+
+      # Returning +nil+ here in order to not display a counter
+      # at all when there aren't any notifications
+      return nil unless count.positive?
+
+      count
+    end
+  end
+
+  def notification_cache_key = "#{cache_key}/unread_notification_count-#{notifications_updated_at}"
+end

app/models/user/push_notification_methods.rb

@@ -9,11 +9,17 @@ module User::PushNotificationMethods
       n.app = app
       n.registration_ids = [s.subscription.symbolize_keys]
       n.data = {
-        message: resource.as_push_notification.to_json
+        message: resource.as_push_notification.merge(notification_data).to_json,
       }
       n.save!
 
       PushNotificationWorker.perform_async(n.id)
     end
   end
+
+  def notification_data = {
+    data: {
+      badge: unread_inbox_count,
+    },
+  }
 end

app/views/navigation/_desktop.html.haml

@@ -10,7 +10,7 @@
           DEV
     %ul.nav.navbar-nav.me-auto
       = nav_entry t("navigation.timeline"), root_path, icon: 'home'
-      = nav_entry t("navigation.inbox"), '/inbox', icon: 'inbox', badge: inbox_count
+      = nav_entry t("navigation.inbox"), "/inbox", icon: "inbox", badge: inbox_count, badge_attr: { data: { controller: "pwa-badge" } }
       - if APP_CONFIG.dig(:features, :discover, :enabled) || current_user.mod?
         = nav_entry t("navigation.discover"), discover_path, icon: 'compass'
     %ul.nav.navbar-nav

app/views/navigation/_main.html.haml

@@ -1,7 +1,10 @@
-- notifications = Notification.for(current_user).where(new: true).includes([:target]).limit(4)
+:ruby
-= render 'navigation/desktop', notifications: notifications
+  notifications = Notification.for(current_user).where(new: true).includes([:target]).limit(4)
-= render 'navigation/mobile', notifications: notifications
+  inbox_count = current_user.unread_inbox_count
+  notification_count = current_user.unread_notification_count
+= render "navigation/desktop", notifications:, inbox_count:, notification_count:
+= render "navigation/mobile", inbox_count:, notification_count:
 
-= render 'modal/ask'
+= render "modal/ask"
 %button.btn.btn-primary.btn-fab.d-block.d-lg-none.d-print-none{ data: { bs_target: "#modal-ask-followers", bs_toggle: :modal }, type: "button" }
   %i.fa.fa-pencil-square-o

db/migrate/20230225143633_add_notification_and_inbox_timestamps_to_users.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class AddNotificationAndInboxTimestampsToUsers < ActiveRecord::Migration[6.1]
+  def change
+    change_table :users, bulk: true do |t|
+      t.timestamp :notifications_updated_at
+      t.timestamp :inbox_updated_at
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2023_02_18_142952) do
+ActiveRecord::Schema.define(version: 2023_02_25_143633) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -364,6 +364,8 @@ ActiveRecord::Schema.define(version: 2023_02_18_142952) do
     t.boolean "sharing_enabled", default: false
     t.boolean "sharing_autoclose", default: false
     t.string "sharing_custom_url"
+    t.datetime "notifications_updated_at"
+    t.datetime "inbox_updated_at"
     t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true
     t.index ["email"], name: "index_users_on_email", unique: true
     t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true

lib/use_case/data_export/user.rb

@@ -13,6 +13,8 @@ module UseCase
         otp_secret_key
         reset_password_sent_at
         reset_password_token
+        inbox_updated_at
+        notifications_updated_at
       ].freeze
 
       IGNORED_FIELDS_PROFILES = %i[

public/service_worker.js

@@ -11,27 +11,22 @@ const OFFLINE_CACHE_PATHS = [
 
 self.addEventListener('push', function (event) {
   if (event.data) {
-    const notification = event.data.json();
+    const contents = event.data.json();
+    navigator.setAppBadge(contents.data.badge);
 
-    event.waitUntil(self.registration.showNotification(notification.title, {
+    event.waitUntil(self.registration.showNotification(contents.title, contents));
-      body: notification.body,
-      tag: notification.type,
-      icon: notification.icon,
-    }));
   } else {
     console.error("Push event received, but it didn't contain any data.", event);
   }
 });
 
 self.addEventListener('notificationclick', async event => {
-  if (event.notification.tag === 'inbox') {
+  if ("click_url" in event.notification.data) {
     event.preventDefault();
-    return clients.openWindow("/inbox", "_blank").then(result => {
+    return clients.openWindow(event.notification.data.click_url, "_blank").then(result => {
       event.notification.close();
       return result;
     });
-  } else {
-    console.warn(`Unhandled notification tag: ${event.notification.tag}`);
   }
 });
 

spec/helpers/application_helper_spec.rb

@@ -3,50 +3,6 @@
 require "rails_helper"
 
 describe ApplicationHelper, type: :helper do
-  describe "#inbox_count" do
-    context "no signed in user" do
-      it "should return 0 as inbox count" do
-        expect(helper.inbox_count).to eq(0)
-      end
-    end
-
-    context "user is signed in" do
-      let(:user) { FactoryBot.create(:user) }
-      let(:question) { FactoryBot.create(:question) }
-
-      before do
-        sign_in(user)
-        Inbox.create(user_id: user.id, question_id: question.id, new: true)
-      end
-
-      it "should return the inbox count" do
-        expect(helper.inbox_count).to eq(1)
-      end
-    end
-  end
-
-  describe "#notification_count" do
-    context "no signed in user" do
-      it "should return 0 as notification count" do
-        expect(helper.notification_count).to eq(0)
-      end
-    end
-
-    context "user is signed in" do
-      let(:user) { FactoryBot.create(:user) }
-      let(:another_user) { FactoryBot.create(:user) }
-
-      before do
-        sign_in(user)
-        another_user.follow(user)
-      end
-
-      it "should return the notification count" do
-        expect(helper.notification_count).to eq(1)
-      end
-    end
-  end
-
   describe "#privileged" do
     context "current user and checked user do not match" do
       let(:user) { FactoryBot.create(:user) }

spec/models/user/inbox_methods_spec.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe User::InboxMethods do
+  context "given a user" do
+    let(:user) { FactoryBot.create(:user) }
+
+    describe "#unread_inbox_count" do
+      subject { user.unread_inbox_count }
+
+      context "user has no questions in their inbox" do
+        it "should return nil" do
+          expect(subject).to eq(nil)
+        end
+      end
+
+      context "user has 1 question in their inbox" do
+        # FactoryBot seems to have issues with setting the +new+ field on inbox entries
+        # so we can create it manually instead
+        let!(:inbox) { Inbox.create(question: FactoryBot.create(:question), user:, new: true) }
+
+        it "should return 1" do
+          expect(subject).to eq(1)
+        end
+      end
+    end
+  end
+end

spec/models/user/notification_methods_spec.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe User::NotificationMethods do
+  context "given a user" do
+    let(:user) { FactoryBot.create(:user) }
+
+    describe "#unread_notification_count" do
+      subject { user.unread_notification_count }
+
+      context "user has no notifications" do
+        it "should return nil" do
+          expect(subject).to eq(nil)
+        end
+      end
+
+      context "user has a notification" do
+        let(:other_user) { FactoryBot.create(:user) }
+
+        before do
+          other_user.follow(user)
+        end
+
+        it "should return 1" do
+          expect(subject).to eq(1)
+        end
+      end
+    end
+  end
+end

spec/workers/question_worker_spec.rb

@@ -78,7 +78,7 @@ describe QuestionWorker do
           user:         receiver,
           subscription: {
             endpoint: "This will not be used",
-            keys:     {}
+            keys:     {},
           }
         )
         receiver.follow(user)