Merge branch 'main' into morning-updates

* main: (29 commits)
  Copy edit
  Change alignment
  Clearer assignment icon
  No longer used
  Note that this need to be able to dynamically appear
  Style
  Slim it down
  Trailing spaces
  Clarify a bit
  More natural to fetch that which will be shown first
  Rely on caching of notifications
  Card readings just need to remove the notification
  Dont render inline
  Explain this
  Hide implementation detail of reading
  Fixed up controller test
  Allow for the footer frames to be hidden on the notifications page
  Simplify the containers
  Only broadcast creations
  Dont turbo stream when coming from a read_all redirect
  ...

app/controllers/cards/readings_controller.rb

@@ -2,12 +2,7 @@ class Cards::ReadingsController < ApplicationController
   include CardScoped
 
   def create
-    mark_card_notifications_read
-    @notifications = Current.user.notifications.unread.ordered.limit(20)
+    @notification = Current.user.notifications.find_by(card: @card)
+    @notification.read
   end
-
-  private
-    def mark_card_notifications_read
-      Current.user.notifications.unread.where(card: @card).read_all
-    end
 end

app/controllers/notifications/mark_all_reads_controller.rb

@@ -1,18 +0,0 @@
-class Notifications::MarkAllReadsController < ApplicationController
-  def create
-    Current.user.notifications.unread.read_all
-
-    set_page_and_extract_portion_from Current.user.notifications.read.ordered if request.format.turbo_stream?
-
-    Turbo::StreamsChannel.broadcast_update_to(
-      [ Current.user, :notifications ],
-      target: "notifications",
-      html: ""
-    )
-
-    respond_to do |format|
-      format.turbo_stream
-      format.html
-    end
-  end
-end

app/controllers/notifications/readings_controller.rb

@@ -0,0 +1,11 @@
+class Notifications::ReadingsController < ApplicationController
+  def create
+    @notification = Current.user.notifications.find(params[:id])
+    @notification.read
+  end
+
+  def create_all
+    Current.user.notifications.unread.read_all
+    redirect_to notifications_path
+  end
+end

app/controllers/notifications_controller.rb

@@ -1,19 +1,11 @@
 class NotificationsController < ApplicationController
   def index
+    @unread = Current.user.notifications.unread.ordered unless current_page_param
     set_page_and_extract_portion_from Current.user.notifications.read.ordered
 
-    if @page.first?
-      @unread = Current.user.notifications.unread.ordered
-    end
-  end
-
-  def mark_read
-    @notification = Current.user.notifications.find(params[:id])
-    @notification.update!(read_at: Time.current)
-
     respond_to do |format|
-      format.html { redirect_back fallback_location: notifications_path }
-      format.turbo_stream
+      format.turbo_stream if current_page_param # Allows read-all action to side step pagination
+      format.html
     end
   end
 end

app/helpers/notifications_helper.rb

@@ -32,7 +32,7 @@ module NotificationsHelper
   end
 
   def notification_mark_read_button(notification)
-    button_to mark_read_notification_path(notification),
+    button_to read_notification_path(notification),
       class: "notification__unread_indicator btn borderless",
       title: "Mark as read",
       data: { turbo_frame: "_top" } do

app/models/notification.rb

@@ -9,16 +9,22 @@ class Notification < ApplicationRecord
   scope :ordered, -> { order(read_at: :desc, created_at: :desc) }
 
   delegate :creator, to: :event
+  after_create_commit :broadcast_unread
 
-  broadcasts_to ->(notification) { [ notification.user, :notifications ] }, inserts_by: :prepend
+  def self.read_all
+    update!(read_at: Time.current)
+  end
 
-  class << self
-    def read_all
-      update!(read_at: Time.current)
-    end
+  def read
+    update!(read_at: Time.current)
   end
 
   def read?
     read_at.present?
   end
+
+  private
+    def broadcast_unread
+      broadcast_prepend_later_to user, :notifications, target: "notifications"
+    end
 end

app/views/cards/readings/create.turbo_stream.erb

@@ -1,3 +1 @@
-<%= turbo_stream.update "notifications" do %>
-  <%= render collection: @notifications, partial: "notifications/notification" %>
-<% end %>
+<%= turbo_stream.remove @notification %>

app/views/layouts/application.html.erb

@@ -46,10 +46,12 @@
     <footer id="footer">
       <%= yield :footer %>
 
-      <% if Current.user %>
-        <%= render "notifications/tray" %>
-        <%= render "my/pins/tray" %>
-        <%= turbo_frame_tag "terminal", src: terminal_path, data: { turbo_permanent: true } %>
+      <% if Current.user && !@hide_footer_frames %>
+        <div id="footer_frames" data-turbo-permanent="true">
+          <%= render "notifications/tray" %>
+          <%= render "my/pins/tray" %>
+          <%= turbo_frame_tag "terminal", src: terminal_path %>
+        </div>
       <% end %>
     </footer>
 

app/views/notifications/_tray.html.erb

@@ -1,14 +1,11 @@
 <%= turbo_stream_from Current.user, :notifications %>
 
-<%= tag.dialog id: "notification-tray", class: "tray notification-tray",
-      data: { controller: "dialog", 
-              turbo_permanent: true, 
-              dialog_modal_value: false, 
-              dialog_target: "dialog", 
-              action: "keydown.esc->dialog#close:stop click@document->dialog#closeOnClickOutside" } do %>
-  <%= turbo_frame_tag("notifications", src: notifications_tray_path) -%>
+<dialog class="tray notification-tray"
+    data-controller="dialog" data-dialog-modal-value="false" data-dialog-target="dialog"
+    data-action="keydown.esc->dialog#close:stop click@document->dialog#closeOnClickOutside">
+  <%= turbo_frame_tag "notifications", src: tray_notifications_path %>
 
-  <%= link_to notifications_path, class: "tray__overflow border-radius txt-normal", data: { action: "click->dialog#close"} do %>
+  <%= link_to notifications_path, class: "tray__overflow border-radius txt-normal", data: { action: "click->dialog#close" } do %>
     <div class="notification__content border-radius shadow fill-white flex align-center txt-align-center gap full-width border txt-ink txt-small">
       <div class="flex flex-column txt-tight-lines min-width flex-item-grow">
         <strong>+ More…</strong>
@@ -17,22 +14,18 @@
   <% end %>
 
   <div class="tray__actions notification-tray__actions border border-radius">
-    <%= button_to notifications_mark_all_read_path, 
-        class: "notification-tray__read_action btn btn--reversed fill-transparent borderless flex-item-justify-start shadow txt-xx-small", 
-        data: { action: "click->dialog#close", turbo_frame: "notifications" } do %>
-      <%= icon_tag "check" %>
-      <span class="for-screen-reader">Mark all read</span>
+    <%= button_to read_all_notifications_path,
+          class: "notification-tray__read_action btn btn--reversed fill-transparent borderless flex-item-justify-start shadow txt-xx-small",
+          data: { action: "click->dialog#close", turbo_frame: "notifications" } do %>
+      <%= icon_tag "check" %> <span class="for-screen-reader">Mark all read</span>
     <% end %>
 
-    <%= link_to notifications_path, 
-          class: "btn btn--reversed borderless fill-transparent translucent shadow center txt-uppercase txt-x-small", 
-          data: { action: "click->dialog#close"} do %>
-      <span>Notifications</span>
-    <% end %>
+    <%= link_to tag.span("Notifications"), notifications_path,
+          class: "btn btn--reversed borderless fill-transparent translucent shadow center txt-uppercase txt-x-small",
+          data: { action: "click->dialog#close" } %>
 
     <span class="btn btn--placeholder txt-xx-small borderless"></span>
   </div>
 
-  <button class="tray__expander" data-action="dialog#toggle" aria-label="Expand notifications stack">
-  </button>
-<% end %>
+  <button class="tray__expander" data-action="dialog#toggle" aria-label="Expand notifications stack"></button>
+</dialog>

app/views/notifications/index.html.erb

@@ -1,4 +1,5 @@
 <% @page_title = "Notifications" %>
+<% @hide_footer_frames = true %>
 
 <% content_for :header do %>
   <nav class="align-start">
@@ -6,37 +7,35 @@
 
     <h1 class="txt-x-large"><%= @page_title %></h1>
 
-    <%= link_to notifications_settings_path, class: "btn flex-item-justify-end" do %>
-      <%= icon_tag "settings" %>
-      <span class="for-screen-reader">Notification settings</span>
+    <%= link_to settings_notifications_path, class: "btn flex-item-justify-end" do %>
+      <%= icon_tag "settings" %> <span class="for-screen-reader">Notification settings</span>
     <% end %>
   </nav>
 <% end %>
 
 <section class="panel center borderless unpad flex flex-column gap-half">
-  <%= turbo_frame_tag "notifications_list" do %>
-    <% if @unread.any? %>
-      <div class="flex align-center justify-space-between margin-block-start margin-block-end-half">
-        <h2 class="txt-medium txt-uppercase txt-alert">New for you</h2>
-        <%= button_to notifications_mark_all_read_path, class: "btn txt-small" do %>
-          Mark all as read
-        <% end %>
-      </div>
+  <% if @unread.any? %>
+    <div class="flex align-center justify-space-between margin-block-start margin-block-end-half">
+      <h2 class="txt-medium txt-uppercase txt-alert">New for you</h2>
+      <%= button_to "Mark all as read", read_all_notifications_path, class: "btn txt-small" %>
+    </div>
 
-      <%= render @unread %>
-    <% else %>
-      <div class="pad border-radius border translucent" style="--border-style: dashed; --border-color: var(--color-ink); --border-radius: 0.2em;">
-        <strong>Nothing new for you.</strong>
-      </div>
-    <% end %>
+    <%= render partial: "notifications/notification", collection: @unread, cached: true %>
+  <% else %>
+    <%# FIXME: Need to reveal this section if there are unread notification records in it %>
+    <div class="pad border-radius border translucent" style="--border-style: dashed; --border-color: var(--color-ink); --border-radius: 0.2em;">
+      <strong>Nothing new for you.</strong>
+    </div>
   <% end %>
 </section>
 
-<% if @page.records.any? %>
-  <section id="notifications_list_read" class="notificiations-list--read panel center borderless unpad flex flex-column gap-half margin-block-start">
-    <h2 class="txt-medium margin-block-start-double margin-block-end-half txt-uppercase translucent">Previously seen</h2>
-    <%= render @page.records %>
-  </section>
+<%# FIXME: Need to hide this section if there are no notification records in it, but then it needs to appear when any are added %>
+<section class="notificiations-list--read panel center borderless unpad flex flex-column gap-half margin-block-start">
+  <h2 class="txt-medium margin-block-start-double margin-block-end-half txt-uppercase translucent">Previously seen</h2>
 
-  <%= notifications_next_page_link(@page) %>
-<% end %>
+  <div id="notifications_list_read">
+    <%= render partial: "notifications/notification", collection: @page.records, cached: true %>
+  </div>
+</section>
+
+<%= notifications_next_page_link(@page) if @page.records.any? %>

app/views/notifications/index.turbo_stream.erb

@@ -1,7 +1,2 @@
-<%= turbo_stream.append :notifications_list_read do %>
-  <%= render @page.records %>
-<% end %>
-
-<%= turbo_stream.replace :next_page do %>
-  <%= notifications_next_page_link(@page) %>
-<% end %>
+<%= turbo_stream.append :notifications_list_read, partial: "notifications/notification", collection: @page.records %>
+<%= turbo_stream.replace :next_page, notifications_next_page_link(@page) %>

app/views/notifications/mark_all_reads/create.html.erb

@@ -1 +0,0 @@
-<%= turbo_frame_tag "notifications" %>

app/views/notifications/mark_all_reads/create.turbo_stream.erb

@@ -1,14 +0,0 @@
-<%= turbo_stream.update "notifications_list" do %>
-  <div class="pad border-radius border translucent" style="--border-style: dashed; --border-color: var(--color-ink);">
-    <strong>Nothing new for you.</strong>
-  </div>
-<% end %>
-
-<% if @page&.records&.any? %>
-  <%= turbo_stream.update "notifications_list_read" do %>
-    <h2 class="txt-medium margin-block-start-double margin-block-end-half txt-uppercase translucent">Previously seen</h2>
-    <%= render partial: "notifications/notification", collection: @page.records %>
-  <% end %>
-<% end %>
-
-<%= turbo_stream.update "notifications" %> 

app/views/notifications/mark_read.turbo_stream.erb

@@ -1,9 +0,0 @@
-<%= turbo_stream.remove @notification %>
-
-<% if Current.user.notifications.unread.none? %>
-  <%= turbo_stream.update "notifications_list" do %>
-    <div class="pad border-radius border translucent" style="--border-style: dashed; --border-color: var(--color-ink);">
-      <strong>Nothing new for you.</strong>
-    </div>
-  <% end %>
-<% end %> 

app/views/notifications/readings/create.turbo_stream.erb

@@ -0,0 +1,2 @@
+<%= turbo_stream.remove @notification %>
+<%= turbo_stream.prepend :notifications_list_read, partial: "notifications/notification", locals: { notification: @notification } %>

app/views/notifications/trays/show.html.erb

@@ -1,3 +1,3 @@
 <%= turbo_frame_tag "notifications" do %>
-  <%= render collection: @notifications, partial: "notifications/notification" %>
+  <%= render partial: "notifications/notification", collection: @notifications, cached: true %>
 <% end %>

config/routes.rb

@@ -46,17 +46,13 @@ Rails.application.routes.draw do
   end
 
 
-  resources :notifications, only: :index
-  namespace :notifications do
-    resource :tray, only: :show
-    resource :mark_all_read, only: :create
-    resources :mark_read, only: :create
-    resource :settings, only: :show
-  end
-
   resources :notifications do
-    member do
-      post :mark_read
+    scope module: :notifications do
+      get "tray",     to: "trays#show", on: :collection
+      get "settings", to: "settings#show", on: :collection
+
+      post "readings", to: "readings#create_all", on: :collection, as: :read_all
+      post "reading",  to: "readings#create",     on: :member,     as: :read
     end
   end
 

test/controllers/notifications/mark_all_reads_controller_test.rb

@@ -1,17 +0,0 @@
-require "test_helper"
-
-class Notifications::MarkAllReadsControllerTest < ActionDispatch::IntegrationTest
-  setup do
-    sign_in_as :kevin
-  end
-
-  test "show" do
-    assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do
-      assert_changes -> { notifications(:layout_commented_kevin).reload.read? }, from: false, to: true do
-        post notifications_mark_all_read_path
-      end
-    end
-
-    assert_response :success
-  end
-end

test/controllers/notifications/readings_controller_test.rb

@@ -0,0 +1,23 @@
+require "test_helper"
+
+class Notifications::ReadingsControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    sign_in_as :kevin
+  end
+
+  test "create" do
+    assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do
+      post read_notification_path(notifications(:logo_published_kevin), format: :turbo_stream)
+      assert_response :success
+    end
+  end
+
+  test "create all" do
+    assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do
+      assert_changes -> { notifications(:layout_commented_kevin).reload.read? }, from: false, to: true do
+        post read_all_notifications_path
+        assert_redirected_to notifications_path
+      end
+    end
+  end
+end

test/controllers/notifications/trays_controller_test.rb

@@ -6,7 +6,7 @@ class Notifications::TraysControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "show" do
-    get notifications_tray_path
+    get tray_notifications_path
 
     assert_response :success
     assert_select "div", text: /Layout is broken/