From 58c525e5a6dce81fbb3adbf8466447218b99efec Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 10 Sep 2025 09:50:22 +0200 Subject: [PATCH 1/6] Broadcast notifications that are deleted To prevent 404s when someone doesn't refresh the page --- app/models/notification.rb | 1 + test/models/notification_test.rb | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/app/models/notification.rb b/app/models/notification.rb index 3f5a00aac..0fa2350ee 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -10,6 +10,7 @@ class Notification < ApplicationRecord scope :ordered, -> { order(read_at: :desc, created_at: :desc) } after_create_commit :broadcast_unread + after_destroy_commit :broadcast_read after_create :bundle delegate :notifiable_target, to: :source diff --git a/test/models/notification_test.rb b/test/models/notification_test.rb index 7e28aca7d..cb230070a 100644 --- a/test/models/notification_test.rb +++ b/test/models/notification_test.rb @@ -38,4 +38,13 @@ class NotificationTest < ActiveSupport::TestCase notification.read end end + + test "deleting notification broadcasts its removal" do + notification = notifications(:logo_published_kevin) + notification.update!(read_at: nil) + + assert_turbo_stream_broadcasts([ notification.user, :notifications ], count: 1) do + notification.destroy + end + end end From b9c7ac225ac5d7013b0a260b9e35e713289b7a8e Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 10 Sep 2025 10:07:58 +0200 Subject: [PATCH 2/6] Extract method --- app/models/card.rb | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/app/models/card.rb b/app/models/card.rb index d8dc388d2..c9dea378c 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -61,17 +61,18 @@ class Card < ApplicationRecord def handle_collection_change transaction do - old_collection = Collection.find_by(id: collection_id_before_last_save) - if old_collection.present? - track_event "collection_changed", particulars: { - old_collection: old_collection.name, - new_collection: collection.name - } - end + track_collection_change_event grant_access_to_assignees unless collection.all_access? end end + def track_collection_change_event + old_collection = Collection.find_by(id: collection_id_before_last_save) + if old_collection.present? + track_event "collection_changed", particulars: { old_collection: old_collection.name, new_collection: collection.name } + end + end + def grant_access_to_assignees collection.accesses.grant_to(assignees) end From 2e51a34694a7f4dcac96f3760cbdd1eec1262d7b Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 10 Sep 2025 10:08:14 +0200 Subject: [PATCH 3/6] Extract method to fetch all the notifications for a card for a given user We'll need to handle access changes --- app/models/card/readable.rb | 18 ++++++++-------- test/models/card/readable_test.rb | 34 +++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/app/models/card/readable.rb b/app/models/card/readable.rb index 84db015ed..26b25b720 100644 --- a/app/models/card/readable.rb +++ b/app/models/card/readable.rb @@ -7,17 +7,15 @@ module Card::Readable end end + def notifications_for(user) + scope = user.notifications.unread + scope.where(source: event_notification_sources) + .or(scope.where(source: mention_notification_sources)) + end + private - def notifications_for(user) - user.notifications.unread.where(source: notification_sources) - end - - def notification_sources - event_notification_sources + mention_notification_sources - end - def event_notification_sources - events + comment_creation_events + events.or(comment_creation_events) end def comment_creation_events @@ -25,7 +23,7 @@ module Card::Readable end def mention_notification_sources - mentions + comment_mentions + mentions.or(comment_mentions) end def comment_mentions diff --git a/test/models/card/readable_test.rb b/test/models/card/readable_test.rb index c53d3f44d..ffbfdcc44 100644 --- a/test/models/card/readable_test.rb +++ b/test/models/card/readable_test.rb @@ -26,4 +26,38 @@ class Card::ReadableTest < ActiveSupport::TestCase cards(:layout).read_by(users(:kevin)) end end + + test "notifications for a given user" do + # Returns card event notifications + kevin_notifications = cards(:logo).notifications_for(users(:kevin)) + assert_includes kevin_notifications, notifications(:logo_published_kevin) + assert_includes kevin_notifications, notifications(:logo_assignment_kevin) + + # Returns comment creation event notifications + layout_notifications = cards(:layout).notifications_for(users(:kevin)) + assert_includes layout_notifications, notifications(:layout_commented_kevin) + + # Returns card mention notifications + david_notifications = cards(:logo).notifications_for(users(:david)) + assert_includes david_notifications, notifications(:logo_card_david_mention_by_jz) + + # Returns comment mention notifications + assert_includes david_notifications, notifications(:logo_comment_david_mention_by_jz) + + # Only returns unread notifications + notifications(:logo_published_kevin).read + kevin_notifications_after_read = cards(:logo).notifications_for(users(:kevin)) + assert_not_includes kevin_notifications_after_read, notifications(:logo_published_kevin) + assert_includes kevin_notifications_after_read, notifications(:logo_assignment_kevin) + + # Does not include notifications from other cards + other_event = events(:text_published) + other_notification = users(:kevin).notifications.create!(source: other_event, creator: users(:david)) + logo_notifications = cards(:logo).notifications_for(users(:kevin)) + assert_not_includes logo_notifications, other_notification + + # Does not include notifications for other users + assert_not_includes kevin_notifications, notifications(:logo_card_david_mention_by_jz) + assert_not_includes kevin_notifications, notifications(:logo_comment_david_mention_by_jz) + end end From 7f7ecf0346dc6f24669bc2e0cc13db1626ee6cea Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 10 Sep 2025 10:24:20 +0200 Subject: [PATCH 4/6] Delete notifications when the user loses access to the collection --- .../remove_inaccessible_notifications_job.rb | 5 ++ app/models/card.rb | 4 ++ app/models/card/readable.rb | 18 +++++-- ...ove_inaccessible_notifications_job_test.rb | 11 +++++ test/models/card/readable_test.rb | 47 ++++++++----------- 5 files changed, 54 insertions(+), 31 deletions(-) create mode 100644 app/jobs/card/remove_inaccessible_notifications_job.rb create mode 100644 test/jobs/card/remove_inaccessible_notifications_job_test.rb diff --git a/app/jobs/card/remove_inaccessible_notifications_job.rb b/app/jobs/card/remove_inaccessible_notifications_job.rb new file mode 100644 index 000000000..db7dec4e3 --- /dev/null +++ b/app/jobs/card/remove_inaccessible_notifications_job.rb @@ -0,0 +1,5 @@ +class Card::RemoveInaccessibleNotificationsJob < ApplicationJob + def perform(card) + card.remove_inaccessible_notifications + end +end diff --git a/app/models/card.rb b/app/models/card.rb index c9dea378c..00214fa5d 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -39,6 +39,8 @@ class Card < ApplicationRecord end end + delegate :accessible_to?, to: :collection + def cache_key [ super, collection.name ].compact.join("/") end @@ -64,6 +66,8 @@ class Card < ApplicationRecord track_collection_change_event grant_access_to_assignees unless collection.all_access? end + + remove_inaccessible_notifications_later end def track_collection_change_event diff --git a/app/models/card/readable.rb b/app/models/card/readable.rb index 26b25b720..06c811715 100644 --- a/app/models/card/readable.rb +++ b/app/models/card/readable.rb @@ -7,13 +7,23 @@ module Card::Readable end end - def notifications_for(user) - scope = user.notifications.unread - scope.where(source: event_notification_sources) - .or(scope.where(source: mention_notification_sources)) + def remove_inaccessible_notifications + User.find_each do |user| + notifications_for(user).destroy_all unless accessible_to?(user) + end end private + def remove_inaccessible_notifications_later + Card::RemoveInaccessibleNotificationsJob.perform_later(self) + end + + def notifications_for(user) + scope = user.notifications.unread + scope.where(source: event_notification_sources) + .or(scope.where(source: mention_notification_sources)) + end + def event_notification_sources events.or(comment_creation_events) end diff --git a/test/jobs/card/remove_inaccessible_notifications_job_test.rb b/test/jobs/card/remove_inaccessible_notifications_job_test.rb new file mode 100644 index 000000000..38b76314e --- /dev/null +++ b/test/jobs/card/remove_inaccessible_notifications_job_test.rb @@ -0,0 +1,11 @@ +require "test_helper" + +class Card::RemoveInaccessibleNotificationsJobTest < ActiveJob::TestCase + test "calls remove_inaccessible_notifications on the card" do + card = cards(:logo) + + Card.any_instance.expects(:remove_inaccessible_notifications) + + Card::RemoveInaccessibleNotificationsJob.perform_now(card) + end +end diff --git a/test/models/card/readable_test.rb b/test/models/card/readable_test.rb index ffbfdcc44..ff4a44662 100644 --- a/test/models/card/readable_test.rb +++ b/test/models/card/readable_test.rb @@ -27,37 +27,30 @@ class Card::ReadableTest < ActiveSupport::TestCase end end - test "notifications for a given user" do - # Returns card event notifications - kevin_notifications = cards(:logo).notifications_for(users(:kevin)) - assert_includes kevin_notifications, notifications(:logo_published_kevin) - assert_includes kevin_notifications, notifications(:logo_assignment_kevin) + test "remove inaccessible notifications" do + card = cards(:logo) + kevin = users(:kevin) + david = users(:david) - # Returns comment creation event notifications - layout_notifications = cards(:layout).notifications_for(users(:kevin)) - assert_includes layout_notifications, notifications(:layout_commented_kevin) + assert card.accessible_to?(kevin) + kevin_notifications = [ notifications(:logo_published_kevin), notifications(:logo_assignment_kevin) ] + david_notifications = [ notifications(:logo_card_david_mention_by_jz), notifications(:logo_comment_david_mention_by_jz) ] - # Returns card mention notifications - david_notifications = cards(:logo).notifications_for(users(:david)) - assert_includes david_notifications, notifications(:logo_card_david_mention_by_jz) + # Kevin loses access + card.collection.accesses.find_by(user: kevin).destroy + assert_not card.accessible_to?(kevin) + assert card.accessible_to?(david) - # Returns comment mention notifications - assert_includes david_notifications, notifications(:logo_comment_david_mention_by_jz) + card.remove_inaccessible_notifications - # Only returns unread notifications - notifications(:logo_published_kevin).read - kevin_notifications_after_read = cards(:logo).notifications_for(users(:kevin)) - assert_not_includes kevin_notifications_after_read, notifications(:logo_published_kevin) - assert_includes kevin_notifications_after_read, notifications(:logo_assignment_kevin) + # Kevin's notifications removed + kevin_notifications.each do |notification| + assert_not Notification.exists?(notification.id) + end - # Does not include notifications from other cards - other_event = events(:text_published) - other_notification = users(:kevin).notifications.create!(source: other_event, creator: users(:david)) - logo_notifications = cards(:logo).notifications_for(users(:kevin)) - assert_not_includes logo_notifications, other_notification - - # Does not include notifications for other users - assert_not_includes kevin_notifications, notifications(:logo_card_david_mention_by_jz) - assert_not_includes kevin_notifications, notifications(:logo_comment_david_mention_by_jz) + # David's notifications preserved + david_notifications.each do |notification| + assert Notification.exists?(notification.id) + end end end From 4e725668563ca11184191b06ea5b394fccf59fba Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 10 Sep 2025 11:24:23 +0200 Subject: [PATCH 5/6] Flatten card resource We only need the parent collection for creating cards. Carrying it in the rest of actions is a hassle. --- app/controllers/cards_controller.rb | 10 +- app/helpers/cards_helper.rb | 2 +- app/models/ai/list_cards_tool.rb | 2 +- app/models/ai/list_comments_tool.rb | 2 +- app/models/card/promptable.rb | 2 +- app/models/comment/promptable.rb | 2 +- app/models/notification_pusher.rb | 2 +- app/views/cards/container/_image.html.erb | 2 +- app/views/cards/container/_title.html.erb | 4 +- .../cards/container/footer/_draft.html.erb | 2 +- app/views/cards/display/_preview.html.erb | 2 +- app/views/cards/edit.html.erb | 4 +- app/views/cards/show.html.erb | 2 +- app/views/prompts/cards/_card.html.erb | 2 +- config/routes.rb | 10 +- test/controllers/cards_controller_test.rb | 12 +- ...nerate_highlights_for_existing_periods.yml | 295 +++++++++++++++++ ...t_test-test_generate_period_highlights.yml | 300 ++++++++++++++++++ ...nerate_highlights_for_existing_periods.yml | 248 +++++++++++++++ ...s_test-test_generate_weekly_highlights.yml | 252 +++++++++++++++ 20 files changed, 1126 insertions(+), 31 deletions(-) diff --git a/app/controllers/cards_controller.rb b/app/controllers/cards_controller.rb index e52926955..7f1377e17 100644 --- a/app/controllers/cards_controller.rb +++ b/app/controllers/cards_controller.rb @@ -1,9 +1,9 @@ require "ostruct" class CardsController < ApplicationController - include CollectionScoped, Collections::ColumnsScoped + include Collections::ColumnsScoped - skip_before_action :set_collection, only: :index + before_action :set_collection, only: %i[ create ] before_action :set_card, only: %i[ show edit update destroy ] enable_collection_filtering only: :index @@ -47,8 +47,12 @@ class CardsController < ApplicationController end private + def set_collection + @collection = Current.user.collections.find(params[:collection_id]) + end + def set_card - @card = @collection.cards.find params[:id] + @card = Current.user.accessible_cards.find params[:id] end def card_params diff --git a/app/helpers/cards_helper.rb b/app/helpers/cards_helper.rb index 9d332794b..cdf5c5d7b 100644 --- a/app/helpers/cards_helper.rb +++ b/app/helpers/cards_helper.rb @@ -49,7 +49,7 @@ module CardsHelper end def button_to_delete_card(card) - button_to collection_card_path(card.collection, card), + button_to card_path(card), method: :delete, class: "btn txt-negative borderless txt-small", data: { turbo_frame: "_top", turbo_confirm: "Are you sure you want to permanently delete this card?" } do concat(icon_tag("trash")) concat(tag.span("Delete this card")) diff --git a/app/models/ai/list_cards_tool.rb b/app/models/ai/list_cards_tool.rb index ec105ba73..a32456ddf 100644 --- a/app/models/ai/list_cards_tool.rb +++ b/app/models/ai/list_cards_tool.rb @@ -102,7 +102,7 @@ class Ai::ListCardsTool < Ai::Tool creator: card.creator.as_json(only: [ :id, :name ]), assignees: card.assignees.as_json(only: [ :id, :name ]), description: card.description.to_plain_text.truncate(1000), - url: collection_card_url(card.collection, card) + url: card_url(card) } end end diff --git a/app/models/ai/list_comments_tool.rb b/app/models/ai/list_comments_tool.rb index 8bc7a875a..20fa21417 100644 --- a/app/models/ai/list_comments_tool.rb +++ b/app/models/ai/list_comments_tool.rb @@ -89,7 +89,7 @@ class Ai::ListCommentsTool < Ai::Tool reacter: reaction.reacter.as_json(only: [ :id, :name ]) } end, - url: collection_card_url(comment.card.collection_id, comment.card, anchor: "comment_#{comment.id}") + url: card_url(comment.card, anchor: "comment_#{comment.id}") } end end diff --git a/app/models/card/promptable.rb b/app/models/card/promptable.rb index b9a00597e..c0374759b 100644 --- a/app/models/card/promptable.rb +++ b/app/models/card/promptable.rb @@ -27,7 +27,7 @@ module Card::Promptable * Collection id: #{collection_id} * Collection name: #{collection.name} * Number of comments: #{comments.count} - * Path: #{collection_card_path(collection, self, script_name: Account.sole.slug)} + * Path: #{card_path(self, script_name: Account.sole.slug)} END OF CARD #{id} PROMPT diff --git a/app/models/comment/promptable.rb b/app/models/comment/promptable.rb index eb11fde80..93c931432 100644 --- a/app/models/comment/promptable.rb +++ b/app/models/comment/promptable.rb @@ -20,7 +20,7 @@ module Comment::Promptable * Card title: #{card.title} * Created by: #{creator.name}} * Created at: #{created_at}} - * Path: #{collection_card_path(card.collection, card, anchor: ActionView::RecordIdentifier.dom_id(self), script_name: Account.sole.slug)} + * Path: #{card_path(card, anchor: ActionView::RecordIdentifier.dom_id(self), script_name: Account.sole.slug)} END OF COMMENT #{id} PROMPT end diff --git a/app/models/notification_pusher.rb b/app/models/notification_pusher.rb index 6fd81e0a5..b5926e2be 100644 --- a/app/models/notification_pusher.rb +++ b/app/models/notification_pusher.rb @@ -113,6 +113,6 @@ class NotificationPusher end def card_path(card) - "#{account_prefix}#{collection_card_path(card.collection, card)}" + "#{account_prefix}#{Rails.application.routes.url_helpers.card_path(card)}" end end diff --git a/app/views/cards/container/_image.html.erb b/app/views/cards/container/_image.html.erb index b38f05c07..de2341aa1 100644 --- a/app/views/cards/container/_image.html.erb +++ b/app/views/cards/container/_image.html.erb @@ -4,7 +4,7 @@ Remove background image <% end %> <% elsif !card.closed? %> - <%= form_with model: card, url: collection_card_path(card.collection, card), data: { controller: "form" } do |form| %> + <%= form_with model: card, url: card_path(card), data: { controller: "form" } do |form| %>