From a8aa2c4f80c38e73cabdc261cc03f77f7014da03 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 16:49:55 +0200 Subject: [PATCH 01/94] Clean up the controller --- app/controllers/cards/comments_controller.rb | 29 ++++++++------------ app/models/card/messages.rb | 6 ++++ 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/app/controllers/cards/comments_controller.rb b/app/controllers/cards/comments_controller.rb index 4f1745602..58e9f956b 100644 --- a/app/controllers/cards/comments_controller.rb +++ b/app/controllers/cards/comments_controller.rb @@ -1,10 +1,11 @@ class Cards::CommentsController < ApplicationController include CardScoped - before_action :set_comment, only: [ :show, :edit, :update, :destroy ] - before_action :require_own_comment, only: [ :edit, :update, :destroy ] + + before_action :set_comment, only: %i[ show edit update destroy ] + before_action :ensure_authorship, only: %i[ edit update destroy ] def create - @card.capture new_comment + @card.capture Comment.new(comment_params) end def show @@ -23,21 +24,15 @@ class Cards::CommentsController < ApplicationController end private + def set_comment + @comment = @card.comments.find(params[:id]) + end + + def ensure_authorship + head :forbidden if Current.user != @comment.creator + end + def comment_params params.require(:comment).permit(:body) end - - def new_comment - Comment.new(comment_params) - end - - def set_comment - @comment = Comment.joins(:message) - .where(messages: { card_id: @card.id }) - .find(params[:id]) - end - - def require_own_comment - head :forbidden unless Current.user == @comment.creator - end end diff --git a/app/models/card/messages.rb b/app/models/card/messages.rb index 01446905c..f77dd9fd1 100644 --- a/app/models/card/messages.rb +++ b/app/models/card/messages.rb @@ -3,9 +3,15 @@ module Card::Messages included do has_many :messages, -> { chronologically }, dependent: :destroy + has_many :comments, through: :messages, source: :messageable after_save :capture_draft_comment end + def comments + # FIXME: I could have sworn there was a way to declare this as a association? + Comment.joins(:message).where(messages: { card_id: id }) + end + def capture(messageable) messages.create! messageable: messageable end From 5ecf0ce3f6f95d1e6182894a13e17d834801a80f Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 16:57:03 +0200 Subject: [PATCH 02/94] Stick with one term --- app/controllers/cards/comments_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/cards/comments_controller.rb b/app/controllers/cards/comments_controller.rb index 58e9f956b..f2e480100 100644 --- a/app/controllers/cards/comments_controller.rb +++ b/app/controllers/cards/comments_controller.rb @@ -2,7 +2,7 @@ class Cards::CommentsController < ApplicationController include CardScoped before_action :set_comment, only: %i[ show edit update destroy ] - before_action :ensure_authorship, only: %i[ edit update destroy ] + before_action :ensure_creatorship, only: %i[ edit update destroy ] def create @card.capture Comment.new(comment_params) @@ -28,7 +28,7 @@ class Cards::CommentsController < ApplicationController @comment = @card.comments.find(params[:id]) end - def ensure_authorship + def ensure_creatorship head :forbidden if Current.user != @comment.creator end From 16c15831335786ffce00d5c3a588236fa66c7a63 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 16:58:47 +0200 Subject: [PATCH 03/94] Usually the order comes last --- app/controllers/cards/comments/reactions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 00f0ca8a9..8517041b7 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -2,7 +2,7 @@ class Cards::Comments::ReactionsController < ApplicationController before_action :set_comment def index - @reactions = @comment.reactions.ordered.includes(:reacter) + @reactions = @comment.reactions.includes(:reacter).ordered end def new From 256d5360095cc8e0bd8d0d50ebd1a916b9d2e5c0 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 16:58:56 +0200 Subject: [PATCH 04/94] Always use _path unless we are changing domains --- app/controllers/cards/comments/reactions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 8517041b7..392179db3 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -12,7 +12,7 @@ class Cards::Comments::ReactionsController < ApplicationController @reaction = @comment.reactions.create!(reaction_params) broadcast_create - redirect_to card_comment_reactions_url(@comment.card, @comment) + redirect_to card_comment_reactions_path(@comment.card, @comment) end def destroy From 8a4a087a001ba1acc7a25f3bcf3413043de0afbb Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:00:08 +0200 Subject: [PATCH 05/94] Fine to let destroying something destroyed be a no-op Would be an edge-casey race condition, but its possible --- app/controllers/cards/comments/reactions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 392179db3..55e673b21 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -17,7 +17,7 @@ class Cards::Comments::ReactionsController < ApplicationController def destroy @reaction = @comment.reactions.find(params[:id]) - @reaction.destroy! + @reaction.destroy broadcast_remove end From 5084bbf3b7df799fe16b2000890266ffd92d8fd9 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:01:19 +0200 Subject: [PATCH 06/94] Use modern params#expect syntax --- app/controllers/cards/comments/reactions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 55e673b21..1226c605c 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -28,7 +28,7 @@ class Cards::Comments::ReactionsController < ApplicationController end def reaction_params - params.require(:reaction).permit(:content) + params.expect(reaction: :content) end def broadcast_create From dc4e50fba5b0ae7f8d74502bff78e298386a10f1 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:34:49 +0200 Subject: [PATCH 07/94] Move messages partial under cards --- app/views/cards/_messages.html.erb | 2 +- app/views/{ => cards}/messages/_message.html.erb | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename app/views/{ => cards}/messages/_message.html.erb (100%) diff --git a/app/views/cards/_messages.html.erb b/app/views/cards/_messages.html.erb index ea6cbf0df..421ebb663 100644 --- a/app/views/cards/_messages.html.erb +++ b/app/views/cards/_messages.html.erb @@ -1,5 +1,5 @@ <%= messages_tag(card) do %> - <%= render partial: "messages/message", collection: card.messages, cached: true %> + <%= render partial: "cards/messages/message", collection: card.messages, cached: true %> <%= render "cards/comments/new", card: card, cached: true %>
diff --git a/app/views/messages/_message.html.erb b/app/views/cards/messages/_message.html.erb similarity index 100% rename from app/views/messages/_message.html.erb rename to app/views/cards/messages/_message.html.erb From 3ecf150636987d48fd44d3ecbeb5ef5b1f74ef68 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:41:28 +0200 Subject: [PATCH 08/94] Use proper access control Otherwise you could react to any comment on the entire account. And then you might as well just do Comment.find. --- app/controllers/cards/comments/reactions_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 1226c605c..f843d7e42 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -1,4 +1,6 @@ class Cards::Comments::ReactionsController < ApplicationController + include CardScoped + before_action :set_comment def index @@ -24,7 +26,7 @@ class Cards::Comments::ReactionsController < ApplicationController private def set_comment - @comment = Current.account.comments.find(params[:comment_id]) + @comment = @card.comments.find(params[:comment_id]) end def reaction_params From f9bb779b617236a88ef419819f098524ca36d4ff Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:41:53 +0200 Subject: [PATCH 09/94] Use local vars so ivar dependency is only from before actions --- .../cards/comments/reactions_controller.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index f843d7e42..5eee8122f 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -11,17 +11,17 @@ class Cards::Comments::ReactionsController < ApplicationController end def create - @reaction = @comment.reactions.create!(reaction_params) + reaction = @comment.reactions.create!(reaction_params) - broadcast_create - redirect_to card_comment_reactions_path(@comment.card, @comment) + broadcast_create(reaction) + redirect_to card_comment_reactions_path(@card, @comment) end def destroy - @reaction = @comment.reactions.find(params[:id]) - @reaction.destroy + reaction = @comment.reactions.find(params[:id]) + reaction.destroy - broadcast_remove + broadcast_remove(reaction) end private @@ -33,12 +33,12 @@ class Cards::Comments::ReactionsController < ApplicationController params.expect(reaction: :content) end - def broadcast_create - @reaction.broadcast_append_to @reaction.comment, :comments, + def broadcast_create(reaction) + reaction.broadcast_append_to @comment, :comments, target: "reactions_comment_#{@comment.id}", partial: "cards/comments/reactions/reaction", locals: { comment: @comment } end - def broadcast_remove - @reaction.broadcast_remove_to @reaction.comment, :comments + def broadcast_remove(reaction) + reaction.broadcast_remove_to @comment, :comments end end From eec76d0d4f935853057cb5d799a729c1b10fc37f Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:43:31 +0200 Subject: [PATCH 10/94] Use modern params#expect --- app/controllers/cards/comments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/cards/comments_controller.rb b/app/controllers/cards/comments_controller.rb index f2e480100..6c87e89c7 100644 --- a/app/controllers/cards/comments_controller.rb +++ b/app/controllers/cards/comments_controller.rb @@ -33,6 +33,6 @@ class Cards::CommentsController < ApplicationController end def comment_params - params.require(:comment).permit(:body) + params.expect(comment: :body) end end From 79700162fc84bd25f8a456ef720a059804ec0ce9 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:47:35 +0200 Subject: [PATCH 11/94] Use correct stream name comment, :comments makes no sense for a stream for the reactions --- app/controllers/cards/comments/reactions_controller.rb | 4 ++-- app/views/cards/comments/reactions/_reactions.html.erb | 3 ++- test/controllers/cards/comments/reactions_controller_test.rb | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 5eee8122f..2d9a55603 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -34,11 +34,11 @@ class Cards::Comments::ReactionsController < ApplicationController end def broadcast_create(reaction) - reaction.broadcast_append_to @comment, :comments, + reaction.broadcast_append_to @comment, :reactions, target: "reactions_comment_#{@comment.id}", partial: "cards/comments/reactions/reaction", locals: { comment: @comment } end def broadcast_remove(reaction) - reaction.broadcast_remove_to @comment, :comments + reaction.broadcast_remove_to @comment, :reactions end end diff --git a/app/views/cards/comments/reactions/_reactions.html.erb b/app/views/cards/comments/reactions/_reactions.html.erb index 5c9b9f846..8b86e4db0 100644 --- a/app/views/cards/comments/reactions/_reactions.html.erb +++ b/app/views/cards/comments/reactions/_reactions.html.erb @@ -1,5 +1,6 @@ <%= turbo_frame_tag comment, :reacting do %> - <%= turbo_stream_from comment, :comments %> + <%= turbo_stream_from comment, :reactions %> +
<%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions.ordered, locals: { comment: comment }, cached: true %> diff --git a/test/controllers/cards/comments/reactions_controller_test.rb b/test/controllers/cards/comments/reactions_controller_test.rb index 83f556992..01fc8a3e3 100644 --- a/test/controllers/cards/comments/reactions_controller_test.rb +++ b/test/controllers/cards/comments/reactions_controller_test.rb @@ -7,7 +7,7 @@ class Cards::Comments::ReactionsControllerTest < ActionDispatch::IntegrationTest end test "create" do - assert_turbo_stream_broadcasts [ @comment, :comments ], count: 1 do + assert_turbo_stream_broadcasts [ @comment, :reactions ], count: 1 do assert_difference -> { @comment.reactions.count }, 1 do post card_comment_reactions_url(@comment.card, @comment, format: :turbo_stream), params: { reaction: { content: "Great work!" } } assert_redirected_to card_comment_reactions_url(@comment.card, @comment) @@ -16,7 +16,7 @@ class Cards::Comments::ReactionsControllerTest < ActionDispatch::IntegrationTest end test "destroy" do - assert_turbo_stream_broadcasts [ @comment, :comments ], count: 1 do + assert_turbo_stream_broadcasts [ @comment, :reactions ], count: 1 do assert_difference -> { @comment.reactions.count }, -1 do delete card_comment_reaction_url(@comment.card, @comment, reactions(:kevin), format: :turbo_stream) assert_response :success From 0954cfe008484d17066add43f8623ff90b283a0f Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:48:01 +0200 Subject: [PATCH 12/94] Add CR --- app/views/cards/comments/reactions/_reactions.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/cards/comments/reactions/_reactions.html.erb b/app/views/cards/comments/reactions/_reactions.html.erb index 8b86e4db0..8e9198b73 100644 --- a/app/views/cards/comments/reactions/_reactions.html.erb +++ b/app/views/cards/comments/reactions/_reactions.html.erb @@ -1,6 +1,6 @@ <%= turbo_frame_tag comment, :reacting do %> <%= turbo_stream_from comment, :reactions %> - +
<%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions.ordered, locals: { comment: comment }, cached: true %> From 263b3336db0065e3000878c37c18a18a2c471bf6 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:50:53 +0200 Subject: [PATCH 13/94] Add missing CR --- app/views/cards/comments/_body.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/cards/comments/_body.html.erb b/app/views/cards/comments/_body.html.erb index a8f800f1a..89143152f 100644 --- a/app/views/cards/comments/_body.html.erb +++ b/app/views/cards/comments/_body.html.erb @@ -16,6 +16,7 @@ Edit this comment <% end %>
+
<%= sanitize comment.body_html %>
From 7861b7c08722fba61ba4de63f0ed88a2930b3b2c Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:58:11 +0200 Subject: [PATCH 14/94] Just open one turbo stream per card Per comment is excessive and needless --- app/controllers/cards/comments/reactions_controller.rb | 4 ++-- app/views/cards/comments/reactions/_reactions.html.erb | 2 -- app/views/cards/show.html.erb | 1 + test/controllers/cards/comments/reactions_controller_test.rb | 5 +++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 2d9a55603..a23d4c068 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -34,11 +34,11 @@ class Cards::Comments::ReactionsController < ApplicationController end def broadcast_create(reaction) - reaction.broadcast_append_to @comment, :reactions, + reaction.broadcast_append_to @card, target: "reactions_comment_#{@comment.id}", partial: "cards/comments/reactions/reaction", locals: { comment: @comment } end def broadcast_remove(reaction) - reaction.broadcast_remove_to @comment, :reactions + reaction.broadcast_remove_to @card end end diff --git a/app/views/cards/comments/reactions/_reactions.html.erb b/app/views/cards/comments/reactions/_reactions.html.erb index 8e9198b73..06db43d01 100644 --- a/app/views/cards/comments/reactions/_reactions.html.erb +++ b/app/views/cards/comments/reactions/_reactions.html.erb @@ -1,6 +1,4 @@ <%= turbo_frame_tag comment, :reacting do %> - <%= turbo_stream_from comment, :reactions %> -
<%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions.ordered, locals: { comment: comment }, cached: true %> diff --git a/app/views/cards/show.html.erb b/app/views/cards/show.html.erb index 93f338f8a..638b88b2a 100644 --- a/app/views/cards/show.html.erb +++ b/app/views/cards/show.html.erb @@ -1,4 +1,5 @@ <% @page_title = card_title(@card) %> +<%= turbo_stream_from @card %> <% content_for :header do %> <% if @card.creating? && @card.can_recover_abandoned_creation? %> diff --git a/test/controllers/cards/comments/reactions_controller_test.rb b/test/controllers/cards/comments/reactions_controller_test.rb index 01fc8a3e3..51b17daae 100644 --- a/test/controllers/cards/comments/reactions_controller_test.rb +++ b/test/controllers/cards/comments/reactions_controller_test.rb @@ -4,10 +4,11 @@ class Cards::Comments::ReactionsControllerTest < ActionDispatch::IntegrationTest setup do sign_in_as :jz @comment = comments(:logo_agreement_jz) + @card = @comment.card end test "create" do - assert_turbo_stream_broadcasts [ @comment, :reactions ], count: 1 do + assert_turbo_stream_broadcasts @card, count: 1 do assert_difference -> { @comment.reactions.count }, 1 do post card_comment_reactions_url(@comment.card, @comment, format: :turbo_stream), params: { reaction: { content: "Great work!" } } assert_redirected_to card_comment_reactions_url(@comment.card, @comment) @@ -16,7 +17,7 @@ class Cards::Comments::ReactionsControllerTest < ActionDispatch::IntegrationTest end test "destroy" do - assert_turbo_stream_broadcasts [ @comment, :reactions ], count: 1 do + assert_turbo_stream_broadcasts @card, count: 1 do assert_difference -> { @comment.reactions.count }, -1 do delete card_comment_reaction_url(@comment.card, @comment, reactions(:kevin), format: :turbo_stream) assert_response :success From 5aa2b7da84fb2a80252dbc7903b161004cfcb067 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 17:58:48 +0200 Subject: [PATCH 15/94] No need to cache individual reactions Both because the reactions partial actually depends on multiple variables but also because its just not worth it when we are caching the comment. --- .../comments/reactions/_reaction.html.erb | 45 +++++++++---------- .../comments/reactions/_reactions.html.erb | 2 +- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/app/views/cards/comments/reactions/_reaction.html.erb b/app/views/cards/comments/reactions/_reaction.html.erb index d88ed4fae..61a799e32 100644 --- a/app/views/cards/comments/reactions/_reaction.html.erb +++ b/app/views/cards/comments/reactions/_reaction.html.erb @@ -1,25 +1,24 @@ -<% cache reaction do %> -
-
- <%= avatar_tag reaction.reacter, aria: { label: "#{reaction.reacter.name} reacted #{reaction.content}" } %> -
+
+
+ <%= avatar_tag reaction.reacter, aria: { label: "#{reaction.reacter.name} reacted #{reaction.content}" } %> +
- <%= tag.span reaction.content, role: "button", - class: [ "txt-small", { "txt-medium": reaction.all_emoji? } ], - data: { action: "click->reaction-delete#reveal keydown.enter->reaction-delete#reveal:prevent", reaction_delete_target: "content" } %> + <%= tag.span reaction.content, role: "button", + class: [ "txt-small", { "txt-medium": reaction.all_emoji? } ], + data: { action: "click->reaction-delete#reveal keydown.enter->reaction-delete#reveal:prevent", reaction_delete_target: "content" } %> - <%= button_to card_comment_reaction_path(comment.card, comment, reaction), - method: :delete, - class: "btn btn--negative flex-item-justify-end reaction__delete", - data: { action: "reaction-delete#perform", reaction_delete_target: "button" } do %> - <%= icon_tag "minus" %> - Delete this reaction - <% end %> -
- Press enter to delete this reaction -<% end %> + <%= button_to card_comment_reaction_path(comment.card, comment, reaction), + method: :delete, + class: "btn btn--negative flex-item-justify-end reaction__delete", + data: { action: "reaction-delete#perform", reaction_delete_target: "button" } do %> + <%= icon_tag "minus" %> + Delete this reaction + <% end %> +
+ +Press enter to delete this reaction diff --git a/app/views/cards/comments/reactions/_reactions.html.erb b/app/views/cards/comments/reactions/_reactions.html.erb index 06db43d01..9ff21b3d7 100644 --- a/app/views/cards/comments/reactions/_reactions.html.erb +++ b/app/views/cards/comments/reactions/_reactions.html.erb @@ -1,7 +1,7 @@ <%= turbo_frame_tag comment, :reacting do %>
- <%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions.ordered, locals: { comment: comment }, cached: true %> + <%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions.ordered, locals: { comment: comment } %>
<%= turbo_frame_tag comment, :new_reaction do %> From 1145260890846fa4b54ad85202ba69c326c3079b Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:01:21 +0200 Subject: [PATCH 16/94] No idea why this was here --- app/views/cards/comments/reactions/_reactions.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/cards/comments/reactions/_reactions.html.erb b/app/views/cards/comments/reactions/_reactions.html.erb index 9ff21b3d7..396ed4e33 100644 --- a/app/views/cards/comments/reactions/_reactions.html.erb +++ b/app/views/cards/comments/reactions/_reactions.html.erb @@ -1,6 +1,6 @@ <%= turbo_frame_tag comment, :reacting do %>
-
+
<%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions.ordered, locals: { comment: comment } %>
From 3b711161d7c5c0d690b582e53f692fcdcca17109 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:04:21 +0200 Subject: [PATCH 17/94] No need for a manual dom_id turbo_frame_tag can do this directly --- app/views/cards/comments/reactions/new.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/cards/comments/reactions/new.html.erb b/app/views/cards/comments/reactions/new.html.erb index a7945c112..0d85b20ae 100644 --- a/app/views/cards/comments/reactions/new.html.erb +++ b/app/views/cards/comments/reactions/new.html.erb @@ -1,4 +1,4 @@ -<%= turbo_frame_tag dom_id(@comment, :new_reaction) do %> +<%= turbo_frame_tag @comment, :new_reaction do %> <%= form_with url: card_comment_reactions_path(@comment.card, @comment), class: "reaction reaction__form flex-inline postion--relative max-width align-center fill-white gap expanded", html: { aria: { label: "New reaction" } }, From 73f1989861982da398a7c9154f90efc6785ace11 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:05:50 +0200 Subject: [PATCH 18/94] Fix indention --- app/views/cards/comments/reactions/new.html.erb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/views/cards/comments/reactions/new.html.erb b/app/views/cards/comments/reactions/new.html.erb index 0d85b20ae..0a11d9c07 100644 --- a/app/views/cards/comments/reactions/new.html.erb +++ b/app/views/cards/comments/reactions/new.html.erb @@ -7,20 +7,19 @@
<%= avatar_tag Current.user %>
+ <%= form.text_field :content, name: "reaction[content]", autofocus: true, autocomplete: "off", autocorrect: "off", maxlength: 16, required: true, pattern: /\S+.*/, class: "input reaction__input txt-small", aria: { label: "Add a reaction" } %> <%= form.button class: "btn btn--reversed", type: "submit" do %> - <%= icon_tag "check" %> - Submit + <%= icon_tag "check" %> Submit <% end %> <%= link_to card_comment_reactions_path(@comment.card, @comment), role: "button", data: { turbo_frame: dom_id(@comment, :reactions), form_target: "cancel" }, class: "btn btn--negative" do %> - <%= icon_tag "minus" %> - Cancel + <%= icon_tag "minus" %> Cancel <% end %> <% end %> <% end %> From 227d5a8757cfe0cdd281ce29d272104db9dbf561 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:10:26 +0200 Subject: [PATCH 19/94] It is both reacting and reactions So stick with the simpler dom id scope --- app/views/cards/comments/reactions/_reactions.html.erb | 2 +- app/views/cards/comments/reactions/new.html.erb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/cards/comments/reactions/_reactions.html.erb b/app/views/cards/comments/reactions/_reactions.html.erb index 396ed4e33..0e53e817b 100644 --- a/app/views/cards/comments/reactions/_reactions.html.erb +++ b/app/views/cards/comments/reactions/_reactions.html.erb @@ -1,4 +1,4 @@ -<%= turbo_frame_tag comment, :reacting do %> +<%= turbo_frame_tag comment, :reactions do %>
<%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions.ordered, locals: { comment: comment } %> diff --git a/app/views/cards/comments/reactions/new.html.erb b/app/views/cards/comments/reactions/new.html.erb index 0a11d9c07..f5773c470 100644 --- a/app/views/cards/comments/reactions/new.html.erb +++ b/app/views/cards/comments/reactions/new.html.erb @@ -2,7 +2,7 @@ <%= form_with url: card_comment_reactions_path(@comment.card, @comment), class: "reaction reaction__form flex-inline postion--relative max-width align-center fill-white gap expanded", html: { aria: { label: "New reaction" } }, - data: { controller: "form", turbo_frame: dom_id(@comment, :reacting), action: "keydown.esc->form#cancel" } do |form| %> + data: { controller: "form", turbo_frame: dom_id(@comment, :reactions), action: "keydown.esc->form#cancel" } do |form| %> <%= form.button class: "btn btn--reversed", type: "submit" do %> From 6f1c8022925455e5ac2eb49c3ffb54e132aa0af1 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:28:23 +0200 Subject: [PATCH 28/94] Content being required via UI and db constraint is enough We are not using/displaying model validations anywhere --- app/models/reaction.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/reaction.rb b/app/models/reaction.rb index 6eefb7963..45d9c1796 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -4,8 +4,6 @@ class Reaction < ApplicationRecord scope :ordered, -> { order(:created_at) } - validates_presence_of :content - def all_emoji? content.match? /\A(\p{Emoji_Presentation}|\p{Extended_Pictographic}|\uFE0F)+\z/u end From 6effc1568f0918c1451f66abc007430e3dce757e Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:41:26 +0200 Subject: [PATCH 29/94] No need for a separate body partial --- app/helpers/comments_helper.rb | 5 --- app/views/cards/comments/_body.html.erb | 27 ---------------- app/views/cards/comments/_comment.html.erb | 36 +++++++++++++++++++--- app/views/cards/comments/show.html.erb | 2 +- app/views/cards/comments/update.html.erb | 2 +- 5 files changed, 33 insertions(+), 39 deletions(-) delete mode 100644 app/views/cards/comments/_body.html.erb diff --git a/app/helpers/comments_helper.rb b/app/helpers/comments_helper.rb index 77847b53b..122e8d551 100644 --- a/app/helpers/comments_helper.rb +++ b/app/helpers/comments_helper.rb @@ -1,9 +1,4 @@ module CommentsHelper - def comment_tag(comment, &) - tag.div id: dom_id(comment), class: "comment flex align-start full-width", - data: { creator_id: comment.creator_id, created_by_current_user_target: "creation" }, & - end - def new_comment_placeholder(card) if card.creator == Current.user && card.messages.comments.empty? "Next, add some notes, context, pictures, or video about this…" diff --git a/app/views/cards/comments/_body.html.erb b/app/views/cards/comments/_body.html.erb deleted file mode 100644 index 89143152f..000000000 --- a/app/views/cards/comments/_body.html.erb +++ /dev/null @@ -1,27 +0,0 @@ -<% cache comment do %> - <%= turbo_frame_tag dom_id(comment) do %> -
-
- - <%= link_to comment.creator.name, user_path(comment.creator), class: "txt-ink btn btn--plain fill-transparent", data: { turbo_frame: "_top" } %> - - - <%= link_to card_path(comment.card, anchor: "comment_#{comment.id}"), class: "txt-undecorated txt-uppercase" do %> - <%= local_datetime_tag comment.created_at, style: :shortdate, class: "txt-ink translucent" %> - <% end %> - - <%= link_to edit_card_comment_path(comment.card, comment), - class: "comment__edit btn btn--plain txt-xx-small fill-transparent translucent flex-item-justify-end" do %> - <%= icon_tag "menu-dots-horizontal" %> - Edit this comment - <% end %> -
- -
- <%= sanitize comment.body_html %> -
- - <%= render "cards/comments/reactions/reactions", comment: comment %> -
- <% end %> -<% end %> diff --git a/app/views/cards/comments/_comment.html.erb b/app/views/cards/comments/_comment.html.erb index c866f30f0..f2294e2bb 100644 --- a/app/views/cards/comments/_comment.html.erb +++ b/app/views/cards/comments/_comment.html.erb @@ -1,7 +1,33 @@ -<%= comment_tag comment do %> - +<% cache comment do %> + <%= turbo_frame_tag comment do %> +
+ - <%= render "cards/comments/body", comment: comment %> +
+
+ + <%= link_to comment.creator.name, user_path(comment.creator), class: "txt-ink btn btn--plain fill-transparent", data: { turbo_frame: "_top" } %> + + + <%= link_to card_path(comment.card, anchor: "comment_#{comment.id}"), class: "txt-undecorated txt-uppercase" do %> + <%= local_datetime_tag comment.created_at, style: :shortdate, class: "txt-ink translucent" %> + <% end %> + + <%= link_to edit_card_comment_path(comment.card, comment), + class: "comment__edit btn btn--plain txt-xx-small fill-transparent translucent flex-item-justify-end" do %> + <%= icon_tag "menu-dots-horizontal" %> + Edit this comment + <% end %> +
+ +
+ <%= sanitize comment.body_html %> +
+ + <%= render "cards/comments/reactions/reactions", comment: comment %> +
+
+ <% end %> <% end %> diff --git a/app/views/cards/comments/show.html.erb b/app/views/cards/comments/show.html.erb index a17046280..07a5ffbc7 100644 --- a/app/views/cards/comments/show.html.erb +++ b/app/views/cards/comments/show.html.erb @@ -1 +1 @@ -<%= render "cards/comments/body", comment: @comment %> +<%= render "cards/comments/comment", comment: @comment %> diff --git a/app/views/cards/comments/update.html.erb b/app/views/cards/comments/update.html.erb index a17046280..07a5ffbc7 100644 --- a/app/views/cards/comments/update.html.erb +++ b/app/views/cards/comments/update.html.erb @@ -1 +1 @@ -<%= render "cards/comments/body", comment: @comment %> +<%= render "cards/comments/comment", comment: @comment %> From c8fe83e8f3661c538f86ad70f0da06b385bffcfa Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:43:52 +0200 Subject: [PATCH 30/94] Indention --- app/views/cards/comments/_comment.html.erb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/cards/comments/_comment.html.erb b/app/views/cards/comments/_comment.html.erb index f2294e2bb..a08a0ea94 100644 --- a/app/views/cards/comments/_comment.html.erb +++ b/app/views/cards/comments/_comment.html.erb @@ -17,8 +17,7 @@ <%= link_to edit_card_comment_path(comment.card, comment), class: "comment__edit btn btn--plain txt-xx-small fill-transparent translucent flex-item-justify-end" do %> - <%= icon_tag "menu-dots-horizontal" %> - Edit this comment + <%= icon_tag "menu-dots-horizontal" %> Edit this comment <% end %>
From 51341c36d15144240c8a0bf2b33b6a581f5c8d3f Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:45:44 +0200 Subject: [PATCH 31/94] New partial isnt cached --- app/views/cards/_messages.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/cards/_messages.html.erb b/app/views/cards/_messages.html.erb index 421ebb663..4b4298759 100644 --- a/app/views/cards/_messages.html.erb +++ b/app/views/cards/_messages.html.erb @@ -1,6 +1,6 @@ <%= messages_tag(card) do %> <%= render partial: "cards/messages/message", collection: card.messages, cached: true %> - <%= render "cards/comments/new", card: card, cached: true %> + <%= render "cards/comments/new", card: card %>
Subscribers From 09dd073cf7b62532658285f27ded243ce8fb7e72 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 11 Apr 2025 18:47:11 +0200 Subject: [PATCH 32/94] Record lookup works here --- app/views/cards/comments/_comment.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/cards/comments/_comment.html.erb b/app/views/cards/comments/_comment.html.erb index a08a0ea94..427fae8ad 100644 --- a/app/views/cards/comments/_comment.html.erb +++ b/app/views/cards/comments/_comment.html.erb @@ -8,7 +8,7 @@
- <%= link_to comment.creator.name, user_path(comment.creator), class: "txt-ink btn btn--plain fill-transparent", data: { turbo_frame: "_top" } %> + <%= link_to comment.creator.name, comment.creator, class: "txt-ink btn btn--plain fill-transparent", data: { turbo_frame: "_top" } %> <%= link_to card_path(comment.card, anchor: "comment_#{comment.id}"), class: "txt-undecorated txt-uppercase" do %> From 80bc1dd163fcbf8f940a7a9e9c2cd29bc4bd30b6 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 10:15:38 +0200 Subject: [PATCH 33/94] Reactions dont have any callbacks so can just be deleted --- app/models/comment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 519fc9a72..9e1f59a44 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -2,7 +2,7 @@ class Comment < ApplicationRecord include Messageable, Notifiable, Searchable belongs_to :creator, class_name: "User", default: -> { Current.user } - has_many :reactions, dependent: :destroy + has_many :reactions, dependent: :delete_all searchable_by :body_plain_text, using: :comments_search_index, as: :body From 4d2ba5b4323c758c8a279e2084b7a00b81756e42 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 10:27:57 +0200 Subject: [PATCH 34/94] Use the explicit method for now --- app/models/card/messages.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/card/messages.rb b/app/models/card/messages.rb index f77dd9fd1..fed0d55f9 100644 --- a/app/models/card/messages.rb +++ b/app/models/card/messages.rb @@ -3,7 +3,6 @@ module Card::Messages included do has_many :messages, -> { chronologically }, dependent: :destroy - has_many :comments, through: :messages, source: :messageable after_save :capture_draft_comment end From bb64adfd1b876c7f7203c19f91941e0ecda45467 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 10:39:19 +0200 Subject: [PATCH 35/94] Most important methods first --- app/models/card/messages.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/card/messages.rb b/app/models/card/messages.rb index fed0d55f9..aafcca3bb 100644 --- a/app/models/card/messages.rb +++ b/app/models/card/messages.rb @@ -6,15 +6,15 @@ module Card::Messages after_save :capture_draft_comment end + def capture(messageable) + messages.create! messageable: messageable + end + def comments # FIXME: I could have sworn there was a way to declare this as a association? Comment.joins(:message).where(messages: { card_id: id }) end - def capture(messageable) - messages.create! messageable: messageable - end - def draft_comment find_or_build_initial_comment.body.content end From 81f431d94636893aaac7f5e8b15afc3afd3c5842 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 10:39:56 +0200 Subject: [PATCH 36/94] Comment-specific behavior needs to live in the comment This really is a bit of a tangled mess. Everyone knows everything about everyone else. --- app/models/card.rb | 3 ++- app/models/card/commentable.rb | 20 -------------------- app/models/card/eventable.rb | 12 ++++++------ app/models/comment.rb | 17 +++++++++++++++++ app/models/concerns/messageable.rb | 8 ++++++++ app/models/message.rb | 13 ++----------- 6 files changed, 35 insertions(+), 38 deletions(-) delete mode 100644 app/models/card/commentable.rb diff --git a/app/models/card.rb b/app/models/card.rb index 8d4411cf4..2e35957e7 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -1,5 +1,5 @@ class Card < ApplicationRecord - include Assignable, Boostable, Colored, Commentable, Engageable, Eventable, Golden, + include Assignable, Boostable, Colored, Engageable, Eventable, Golden, Messages, Notifiable, Pinnable, Closeable, Scorable, Searchable, Staged, Statuses, Taggable, Watchable @@ -18,6 +18,7 @@ class Card < ApplicationRecord scope :chronologically, -> { order created_at: :asc, id: :asc } scope :latest, -> { order updated_at: :desc, id: :desc } scope :in_collection, ->(collection) { where collection: collection } + scope :ordered_by_comments, -> { order comments_count: :desc } scope :indexed_by, ->(index) do case index diff --git a/app/models/card/commentable.rb b/app/models/card/commentable.rb deleted file mode 100644 index d0308069f..000000000 --- a/app/models/card/commentable.rb +++ /dev/null @@ -1,20 +0,0 @@ -module Card::Commentable - extend ActiveSupport::Concern - - included do - scope :ordered_by_comments, -> { order comments_count: :desc } - end - - def comment_created(comment) - increment! :comments_count - watch_by comment.creator - - track_event :commented, comment_id: comment.id - rescore - end - - def comment_destroyed - decrement! :comments_count - rescore - end -end diff --git a/app/models/card/eventable.rb b/app/models/card/eventable.rb index 038a6d51a..c8098708b 100644 --- a/app/models/card/eventable.rb +++ b/app/models/card/eventable.rb @@ -10,14 +10,14 @@ module Card::Eventable touch :last_active_at end - private - def track_event(action, creator: Current.user, **particulars) - if published? - event = find_or_capture_event_summary.events.create! action: action, creator: creator, card: self, particulars: particulars - event.generate_notifications_later - end + def track_event(action, creator: Current.user, **particulars) + if published? + event = find_or_capture_event_summary.events.create! action: action, creator: creator, card: self, particulars: particulars + event.generate_notifications_later end + end + private def find_or_capture_event_summary transaction do messages.last&.event_summary || capture(EventSummary.new).event_summary diff --git a/app/models/comment.rb b/app/models/comment.rb index 9e1f59a44..3dc91b3ca 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -18,6 +18,23 @@ class Comment < ApplicationRecord card_comments.many? && card_comments_prior.last&.creator != creator end + def created_via(message) + message.card.tap do |card| + card.increment! :comments_count + card.watch_by creator + + card.track_event :commented, comment_id: id + card.rescore + end + end + + def destroyed_via(message) + message.card.tap do |card| + card.decrement! :comments_count + card.rescore + end + end + def to_partial_path "cards/#{super}" end diff --git a/app/models/concerns/messageable.rb b/app/models/concerns/messageable.rb index 219ee1197..4f1fffb30 100644 --- a/app/models/concerns/messageable.rb +++ b/app/models/concerns/messageable.rb @@ -7,4 +7,12 @@ module Messageable has_one :message, as: :messageable, touch: true, dependent: :destroy has_one :card, through: :message end + + def created_via(message) + # Overwrite in Messageable class + end + + def destroyed_via(message) + # Overwrite in Messageable class + end end diff --git a/app/models/message.rb b/app/models/message.rb index bb5dea5a9..67ec3b294 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -5,15 +5,6 @@ class Message < ApplicationRecord scope :chronologically, -> { order created_at: :asc, id: :desc } - after_create :created - after_destroy :destroyed - - private - def created - card.comment_created(comment) if comment? - end - - def destroyed - card.comment_destroyed if comment? - end + after_create -> { messageable.created_via(self) } + after_destroy -> { messageable.destroyed_via(self) } end From fb733b9a60522b8f209f5260e44ef39ca6046a4e Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 10:50:25 +0200 Subject: [PATCH 37/94] No more tracking commenting as an activity for sorting We are doing away with all the automated activity scoring, but for now lets just get rid of it for comments. --- app/models/card.rb | 2 -- app/models/comment.rb | 16 ++-------- app/models/filter/fields.rb | 2 +- ...20250412084635_drop_card_comments_count.rb | 5 +++ db/schema.rb | 3 +- db/schema_cache.yml | 12 +------ test/fixtures/cards.yml | 2 -- test/fixtures/filters.yml | 4 +-- test/models/card/scorable_test.rb | 32 ------------------- test/models/card_test.rb | 4 --- test/models/comment_test.rb | 14 -------- test/models/filter_test.rb | 25 ++------------- 12 files changed, 15 insertions(+), 106 deletions(-) create mode 100644 db/migrate/20250412084635_drop_card_comments_count.rb diff --git a/app/models/card.rb b/app/models/card.rb index 2e35957e7..09d2c0314 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -18,12 +18,10 @@ class Card < ApplicationRecord scope :chronologically, -> { order created_at: :asc, id: :asc } scope :latest, -> { order updated_at: :desc, id: :desc } scope :in_collection, ->(collection) { where collection: collection } - scope :ordered_by_comments, -> { order comments_count: :desc } scope :indexed_by, ->(index) do case index when "most_active" then ordered_by_activity - when "most_discussed" then ordered_by_comments when "most_boosted" then ordered_by_boosts when "newest" then reverse_chronologically when "oldest" then chronologically diff --git a/app/models/comment.rb b/app/models/comment.rb index 3dc91b3ca..79669e9f0 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -19,20 +19,8 @@ class Comment < ApplicationRecord end def created_via(message) - message.card.tap do |card| - card.increment! :comments_count - card.watch_by creator - - card.track_event :commented, comment_id: id - card.rescore - end - end - - def destroyed_via(message) - message.card.tap do |card| - card.decrement! :comments_count - card.rescore - end + message.card.watch_by creator + message.card.track_event :commented, comment_id: id end def to_partial_path diff --git a/app/models/filter/fields.rb b/app/models/filter/fields.rb index e6028e551..b608f2c58 100644 --- a/app/models/filter/fields.rb +++ b/app/models/filter/fields.rb @@ -1,7 +1,7 @@ module Filter::Fields extend ActiveSupport::Concern - INDEXES = %w[ most_discussed most_boosted newest oldest latest stalled ] + INDEXES = %w[ most_boosted newest oldest latest stalled ] delegate :default_value?, to: :class diff --git a/db/migrate/20250412084635_drop_card_comments_count.rb b/db/migrate/20250412084635_drop_card_comments_count.rb new file mode 100644 index 000000000..5a0d15e2f --- /dev/null +++ b/db/migrate/20250412084635_drop_card_comments_count.rb @@ -0,0 +1,5 @@ +class DropCardCommentsCount < ActiveRecord::Migration[8.1] + def change + remove_column :cards, :comments_count + end +end diff --git a/db/schema.rb b/db/schema.rb index 4fa6fa273..cff07b713 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2025_04_10_141409) do +ActiveRecord::Schema[8.1].define(version: 2025_04_12_084635) do create_table "accesses", force: :cascade do |t| t.integer "collection_id", null: false t.datetime "created_at", null: false @@ -115,7 +115,6 @@ ActiveRecord::Schema[8.1].define(version: 2025_04_10_141409) do t.float "activity_score_order", default: 0.0, null: false t.integer "boosts_count", default: 0, null: false t.integer "collection_id", null: false - t.integer "comments_count", default: 0, null: false t.datetime "created_at", null: false t.integer "creator_id", null: false t.date "due_on" diff --git a/db/schema_cache.yml b/db/schema_cache.yml index 1a83eece7..121a82ca3 100644 --- a/db/schema_cache.yml +++ b/db/schema_cache.yml @@ -423,16 +423,6 @@ columns: collation: comment: - *24 - - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column - auto_increment: - name: comments_count - cast_type: *1 - sql_type_metadata: *2 - 'null': false - default: 0 - default_function: - collation: - comment: - *6 - &25 !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column auto_increment: @@ -2164,4 +2154,4 @@ indexes: nulls_not_distinct: comment: valid: true -version: 20250410141409 +version: 20250412084635 diff --git a/test/fixtures/cards.yml b/test/fixtures/cards.yml index 0373030f3..d26288cb0 100644 --- a/test/fixtures/cards.yml +++ b/test/fixtures/cards.yml @@ -5,7 +5,6 @@ logo: due_on: <%= 3.days.from_now %> created_at: <%= 1.week.ago %> boosts_count: 5 - comments_count: 2 activity_score: 7 status: published last_active_at: <%= Time.current %> @@ -15,7 +14,6 @@ layout: creator: david title: Layout is broken created_at: <%= 1.week.ago %> - comments_count: 1 activity_score: 1 status: published last_active_at: <%= Time.current %> diff --git a/test/fixtures/filters.yml b/test/fixtures/filters.yml index e7134b059..b6f73a844 100644 --- a/test/fixtures/filters.yml +++ b/test/fixtures/filters.yml @@ -2,5 +2,5 @@ jz_assignments: creator: david tags: mobile assignees: jz - fields: <%= { indexed_by: :most_discussed }.to_json %> - params_digest: <%= Filter.digest_params({ indexed_by: :most_discussed, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }) %> + fields: <%= { indexed_by: :most_boosted }.to_json %> + params_digest: <%= Filter.digest_params({ indexed_by: :most_boosted, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }) %> diff --git a/test/models/card/scorable_test.rb b/test/models/card/scorable_test.rb index 25ba7e9f9..64d0bb038 100644 --- a/test/models/card/scorable_test.rb +++ b/test/models/card/scorable_test.rb @@ -1,38 +1,6 @@ require "test_helper" class Card::ScorableTest < ActiveSupport::TestCase - test "a card has a score that increases with activity" do - card = cards(:logo) - - score = card.activity_score - assert_operator score, :>, 0 - - with_current_user :kevin do - card.capture Comment.create(body: "This is exciting!") - end - - assert_operator card.activity_score, :>, score - end - - test "commenting on a card boosts its score more than boosting it" do - card = cards(:logo) - card.rescore - - comment_change = capture_change -> { card.activity_score } do - with_current_user :kevin do - card.capture Comment.create(body: "This is exciting!") - end - end - - boost_change = capture_change -> { card.activity_score } do - with_current_user :kevin do - card.boost! - end - end - - assert_operator comment_change, :>, boost_change - end - test "recent activity counts more than older activity in the ordering" do with_current_user :kevin do travel_to 5.days.ago diff --git a/test/models/card_test.rb b/test/models/card_test.rb index ea1aaa062..311470c29 100644 --- a/test/models/card_test.rb +++ b/test/models/card_test.rb @@ -74,10 +74,6 @@ class CardTest < ActiveSupport::TestCase assert_includes Card.search("haggis"), card end - test "ordering by comments" do - assert_equal cards(:logo, :layout, :shipping, :text), Card.ordered_by_comments - end - test "ordering by boosts" do cards(:layout).update! boosts_count: 1_000 assert_equal cards(:layout, :logo, :shipping, :text), Card.ordered_by_boosts diff --git a/test/models/comment_test.rb b/test/models/comment_test.rb index dfe2cee06..9150d2ad8 100644 --- a/test/models/comment_test.rb +++ b/test/models/comment_test.rb @@ -11,20 +11,6 @@ class CommentTest < ActiveSupport::TestCase assert_includes Comment.search("something rustic"), message.comment end - test "updating card counter" do - assert_difference -> { cards(:logo).comments_count } do - assert_changes -> { cards(:logo).activity_score } do - cards(:logo).capture Comment.new(body: "I'd prefer something more rustic") - end - end - - assert_difference -> { cards(:logo).comments_count }, -1 do - assert_changes -> { cards(:logo).activity_score } do - cards(:logo).messages.comments.last.destroy - end - end - end - test "first_by_author_on_card?" do assert_not Comment.new.first_by_author_on_card? diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index 965278600..2c94fe074 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -14,9 +14,6 @@ class FilterTest < ActiveSupport::TestCase assert_not_includes users(:kevin).filters.new.cards, @new_card - filter = users(:david).filters.new indexed_by: "most_discussed", assignee_ids: [ users(:jz).id ], tag_ids: [ tags(:mobile).id ] - assert_equal [ cards(:layout) ], filter.cards - filter = users(:david).filters.new creator_ids: [ users(:david).id ], tag_ids: [ tags(:mobile).id ] assert_equal [ cards(:layout) ], filter.cards @@ -104,34 +101,18 @@ class FilterTest < ActiveSupport::TestCase end test "summary" do - assert_equal "Most discussed, tagged #mobile, and assigned to JZ ", filters(:jz_assignments).summary + assert_equal "Most boosted, tagged #mobile, and assigned to JZ ", filters(:jz_assignments).summary filters(:jz_assignments).update!(stages: workflow_stages(:qa_triage, :qa_in_progress)) - assert_equal "Most discussed, tagged #mobile, assigned to JZ, and staged in Triage or In progress ", filters(:jz_assignments).summary + assert_equal "Most boosted, tagged #mobile, assigned to JZ, and staged in Triage or In progress ", filters(:jz_assignments).summary filters(:jz_assignments).update!(stages: [], assignees: [], tags: [], collections: [ collections(:writebook) ]) - assert_equal "Most discussed in Writebook", filters(:jz_assignments).summary + assert_equal "Most boosted in Writebook", filters(:jz_assignments).summary filters(:jz_assignments).update!(indexed_by: "stalled") assert_equal "Stalled in Writebook", filters(:jz_assignments).summary end - test "params without a key-value pair" do - filter = users(:david).filters.new indexed_by: "most_discussed", assignee_ids: [ users(:jz).id, users(:kevin).id ] - - expected = { indexed_by: "most_discussed", assignee_ids: [ users(:kevin).id ] } - assert_equal expected, filter.as_params_without(:assignee_ids, users(:jz).id).to_h - - expected = { assignee_ids: [ users(:jz).id, users(:kevin).id ] } - assert_equal expected, filter.as_params_without(:indexed_by, "most_discussed").to_h - - expected = { indexed_by: "most_discussed", assignee_ids: [ users(:jz).id, users(:kevin).id ] } - assert_equal expected, filter.as_params_without(:indexed_by, "most_active").to_h - - expected = { indexed_by: "most_discussed", assignee_ids: [ users(:jz).id, users(:kevin).id ] } - assert_equal expected, filter.as_params_without(:assignee_ids, users(:david).id).to_h - end - test "get a clone with some changed params" do seed_filter = users(:david).filters.new indexed_by: "active", terms: [ "haggis" ] filter = seed_filter.with(indexed_by: "closed") From 60a0360203347d066494421456fe8320dfb71bcb Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 10:52:48 +0200 Subject: [PATCH 38/94] Remove more activity scoring related to comments --- app/models/comment.rb | 16 ---------------- test/models/comment_test.rb | 38 ------------------------------------- 2 files changed, 54 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 79669e9f0..859b33941 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -10,14 +10,6 @@ class Comment < ApplicationRecord before_destroy :cleanup_events - def first_by_author_on_card? - card_comments.many? && card_comments_prior.where(creator_id: creator_id).none? - end - - def follows_comment_by_another_author? - card_comments.many? && card_comments_prior.last&.creator != creator - end - def created_via(message) message.card.watch_by creator message.card.track_event :commented, comment_id: id @@ -37,12 +29,4 @@ class Comment < ApplicationRecord # Delete events that reference directly in particulars Event.where(particulars: { comment_id: id }).destroy_all end - - def card_comments_prior - card_comments.where(created_at: ...created_at) - end - - def card_comments - Comment.joins(:message).where(messages: { card: card }) - end end diff --git a/test/models/comment_test.rb b/test/models/comment_test.rb index 9150d2ad8..ae7b505a2 100644 --- a/test/models/comment_test.rb +++ b/test/models/comment_test.rb @@ -10,42 +10,4 @@ class CommentTest < ActiveSupport::TestCase assert_includes Comment.search("something rustic"), message.comment end - - test "first_by_author_on_card?" do - assert_not Comment.new.first_by_author_on_card? - - with_current_user :david do - comment = Comment.new.tap { |c| cards(:logo).capture c } - assert comment.first_by_author_on_card? - - comment = Comment.new.tap { |c| cards(:logo).capture c } - assert_not comment.first_by_author_on_card? - end - - with_current_user :kevin do - comment = Comment.new.tap { |c| cards(:logo).capture c } - assert_not comment.first_by_author_on_card? - end - end - - test "follows_comment_by_another_author?" do - assert_not Comment.new.follows_comment_by_another_author? - - card = collections(:writebook).cards.create! - - with_current_user :david do - comment = Comment.new.tap { |c| card.capture c } - assert_not comment.follows_comment_by_another_author? - end - - with_current_user :kevin do - comment = Comment.new.tap { |c| card.capture c } - assert comment.follows_comment_by_another_author? - end - - with_current_user :david do - comment = Comment.new.tap { |c| card.capture c } - assert comment.follows_comment_by_another_author? - end - end end From 24b02c42fd2a187d3762035d19af40100da4b7e8 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 10:59:59 +0200 Subject: [PATCH 39/94] Comments are dependents of messages, so need to destroy the latter to get the former --- app/controllers/cards/comments_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/cards/comments_controller.rb b/app/controllers/cards/comments_controller.rb index 6c87e89c7..9588646c2 100644 --- a/app/controllers/cards/comments_controller.rb +++ b/app/controllers/cards/comments_controller.rb @@ -19,13 +19,14 @@ class Cards::CommentsController < ApplicationController end def destroy - @comment.destroy + @message.destroy redirect_to @card end private def set_comment @comment = @card.comments.find(params[:id]) + @message = @comment.message end def ensure_creatorship From b698eefa069fe8cc670d4115d24f6f68b8511950 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:00:15 +0200 Subject: [PATCH 40/94] Messageables already have access to the card --- app/models/comment.rb | 7 ++++--- app/models/concerns/messageable.rb | 4 ++-- app/models/message.rb | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 859b33941..e7f094ffb 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -10,9 +10,10 @@ class Comment < ApplicationRecord before_destroy :cleanup_events - def created_via(message) - message.card.watch_by creator - message.card.track_event :commented, comment_id: id + # Called when a new comment is captured as a message during creation + def created_via_message + card.watch_by creator + card.track_event :commented, comment_id: id end def to_partial_path diff --git a/app/models/concerns/messageable.rb b/app/models/concerns/messageable.rb index 4f1fffb30..edf1a002a 100644 --- a/app/models/concerns/messageable.rb +++ b/app/models/concerns/messageable.rb @@ -8,11 +8,11 @@ module Messageable has_one :card, through: :message end - def created_via(message) + def created_via_message # Overwrite in Messageable class end - def destroyed_via(message) + def destroyed_via_message # Overwrite in Messageable class end end diff --git a/app/models/message.rb b/app/models/message.rb index 67ec3b294..550619471 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -5,6 +5,6 @@ class Message < ApplicationRecord scope :chronologically, -> { order created_at: :asc, id: :desc } - after_create -> { messageable.created_via(self) } - after_destroy -> { messageable.destroyed_via(self) } + after_create -> { messageable.created_via_message } + after_destroy -> { messageable.destroyed_via_message } end From 27d90abbd661c65f835c766487db7168b5105643 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:04:16 +0200 Subject: [PATCH 41/94] Reduce the direct knowledge of comments inside the card --- app/controllers/cards/comments/reactions_controller.rb | 2 +- app/controllers/cards/comments_controller.rb | 2 +- app/models/card/messages.rb | 5 ----- app/models/comment.rb | 3 ++- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 0a728fbb2..9c47b5eb8 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -25,7 +25,7 @@ class Cards::Comments::ReactionsController < ApplicationController private def set_comment - @comment = @card.comments.find(params[:comment_id]) + @comment = Comment.via_card(@card).find(params[:comment_id]) end def broadcast_create(reaction) diff --git a/app/controllers/cards/comments_controller.rb b/app/controllers/cards/comments_controller.rb index 9588646c2..c7da8c568 100644 --- a/app/controllers/cards/comments_controller.rb +++ b/app/controllers/cards/comments_controller.rb @@ -25,7 +25,7 @@ class Cards::CommentsController < ApplicationController private def set_comment - @comment = @card.comments.find(params[:id]) + @comment = Comment.via_card(@card).find(params[:id]) @message = @comment.message end diff --git a/app/models/card/messages.rb b/app/models/card/messages.rb index aafcca3bb..01446905c 100644 --- a/app/models/card/messages.rb +++ b/app/models/card/messages.rb @@ -10,11 +10,6 @@ module Card::Messages messages.create! messageable: messageable end - def comments - # FIXME: I could have sworn there was a way to declare this as a association? - Comment.joins(:message).where(messages: { card_id: id }) - end - def draft_comment find_or_build_initial_comment.body.content end diff --git a/app/models/comment.rb b/app/models/comment.rb index e7f094ffb..ff223b5c6 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -4,9 +4,10 @@ class Comment < ApplicationRecord belongs_to :creator, class_name: "User", default: -> { Current.user } has_many :reactions, dependent: :delete_all + has_markdown :body searchable_by :body_plain_text, using: :comments_search_index, as: :body - has_markdown :body + scope :via_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } before_destroy :cleanup_events From 9f7bbbef58a03d53b8d025bf6f0830a4e358cc62 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:05:56 +0200 Subject: [PATCH 42/94] Split draft commenting back out to a dedicated concern --- app/models/card.rb | 2 +- app/models/card/draft_commentable.rb | 33 ++++++++++++++++++++++++++++ app/models/card/messages.rb | 26 ---------------------- 3 files changed, 34 insertions(+), 27 deletions(-) create mode 100644 app/models/card/draft_commentable.rb diff --git a/app/models/card.rb b/app/models/card.rb index 09d2c0314..3688ef4e9 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -1,5 +1,5 @@ class Card < ApplicationRecord - include Assignable, Boostable, Colored, Engageable, Eventable, Golden, + include Assignable, Boostable, Colored, DraftCommentable, Engageable, Eventable, Golden, Messages, Notifiable, Pinnable, Closeable, Scorable, Searchable, Staged, Statuses, Taggable, Watchable diff --git a/app/models/card/draft_commentable.rb b/app/models/card/draft_commentable.rb new file mode 100644 index 000000000..c0e832e6f --- /dev/null +++ b/app/models/card/draft_commentable.rb @@ -0,0 +1,33 @@ +module Card::DraftCommentable + extend ActiveSupport::Concern + + included do + after_save :capture_draft_comment + end + + def draft_comment + find_or_build_initial_comment.body.content + end + + def draft_comment=(body) + if body.present? + @draft_comment = body + else + messages.comments.destroy_all + end + end + + private + def find_or_build_initial_comment + message = messages.comments.first || messages.new(messageable: Comment.new) + message.comment + end + + def capture_draft_comment + if @draft_comment.present? + find_or_build_initial_comment.update! body: @draft_comment, creator: creator + end + @draft_comment = nil + end +end + diff --git a/app/models/card/messages.rb b/app/models/card/messages.rb index 01446905c..89579b78f 100644 --- a/app/models/card/messages.rb +++ b/app/models/card/messages.rb @@ -3,35 +3,9 @@ module Card::Messages included do has_many :messages, -> { chronologically }, dependent: :destroy - after_save :capture_draft_comment end def capture(messageable) messages.create! messageable: messageable end - - def draft_comment - find_or_build_initial_comment.body.content - end - - def draft_comment=(body) - if body.present? - @draft_comment = body - else - messages.comments.destroy_all - end - end - - private - def find_or_build_initial_comment - message = messages.comments.first || messages.new(messageable: Comment.new) - message.comment - end - - def capture_draft_comment - if @draft_comment.present? - find_or_build_initial_comment.update! body: @draft_comment, creator: creator - end - @draft_comment = nil - end end From 735a12abfc2a0a8d2d848acb763c2b85175c9a0a Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:07:02 +0200 Subject: [PATCH 43/94] Better name --- app/models/card.rb | 2 +- app/models/card/{draft_commentable.rb => draft_commenting.rb} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename app/models/card/{draft_commentable.rb => draft_commenting.rb} (95%) diff --git a/app/models/card.rb b/app/models/card.rb index 3688ef4e9..a265378ef 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -1,5 +1,5 @@ class Card < ApplicationRecord - include Assignable, Boostable, Colored, DraftCommentable, Engageable, Eventable, Golden, + include Assignable, Boostable, Colored, DraftCommenting, Engageable, Eventable, Golden, Messages, Notifiable, Pinnable, Closeable, Scorable, Searchable, Staged, Statuses, Taggable, Watchable diff --git a/app/models/card/draft_commentable.rb b/app/models/card/draft_commenting.rb similarity index 95% rename from app/models/card/draft_commentable.rb rename to app/models/card/draft_commenting.rb index c0e832e6f..b2fe3a444 100644 --- a/app/models/card/draft_commentable.rb +++ b/app/models/card/draft_commenting.rb @@ -1,4 +1,4 @@ -module Card::DraftCommentable +module Card::DraftCommenting extend ActiveSupport::Concern included do From ecffdc2af4eab9b267a1c21a4873092e99e4dbb5 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:07:27 +0200 Subject: [PATCH 44/94] Spacing --- app/models/card/draft_commenting.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/card/draft_commenting.rb b/app/models/card/draft_commenting.rb index b2fe3a444..3302928c9 100644 --- a/app/models/card/draft_commenting.rb +++ b/app/models/card/draft_commenting.rb @@ -27,6 +27,7 @@ module Card::DraftCommenting if @draft_comment.present? find_or_build_initial_comment.update! body: @draft_comment, creator: creator end + @draft_comment = nil end end From 63c05220ce38ae67255d166df4e73d73d426deea Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:09:55 +0200 Subject: [PATCH 45/94] Clearer naming --- app/controllers/cards/comments/reactions_controller.rb | 2 +- app/controllers/cards/comments_controller.rb | 2 +- app/models/comment.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/cards/comments/reactions_controller.rb b/app/controllers/cards/comments/reactions_controller.rb index 9c47b5eb8..fb0c4fa55 100644 --- a/app/controllers/cards/comments/reactions_controller.rb +++ b/app/controllers/cards/comments/reactions_controller.rb @@ -25,7 +25,7 @@ class Cards::Comments::ReactionsController < ApplicationController private def set_comment - @comment = Comment.via_card(@card).find(params[:comment_id]) + @comment = Comment.belonging_to_card(@card).find(params[:comment_id]) end def broadcast_create(reaction) diff --git a/app/controllers/cards/comments_controller.rb b/app/controllers/cards/comments_controller.rb index c7da8c568..35f8e13e7 100644 --- a/app/controllers/cards/comments_controller.rb +++ b/app/controllers/cards/comments_controller.rb @@ -25,7 +25,7 @@ class Cards::CommentsController < ApplicationController private def set_comment - @comment = Comment.via_card(@card).find(params[:id]) + @comment = Comment.belonging_to_card(@card).find(params[:id]) @message = @comment.message end diff --git a/app/models/comment.rb b/app/models/comment.rb index ff223b5c6..1eb286454 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -7,7 +7,7 @@ class Comment < ApplicationRecord has_markdown :body searchable_by :body_plain_text, using: :comments_search_index, as: :body - scope :via_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } + scope :belonging_to_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } before_destroy :cleanup_events From 1c4e0d3ccc5b6ca9e33cb620fccf838f948da8be Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:11:30 +0200 Subject: [PATCH 46/94] Not using the destroy callback --- app/models/concerns/messageable.rb | 4 ---- app/models/message.rb | 3 +-- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/app/models/concerns/messageable.rb b/app/models/concerns/messageable.rb index edf1a002a..987cbf784 100644 --- a/app/models/concerns/messageable.rb +++ b/app/models/concerns/messageable.rb @@ -11,8 +11,4 @@ module Messageable def created_via_message # Overwrite in Messageable class end - - def destroyed_via_message - # Overwrite in Messageable class - end end diff --git a/app/models/message.rb b/app/models/message.rb index 550619471..3e5fb37a7 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -5,6 +5,5 @@ class Message < ApplicationRecord scope :chronologically, -> { order created_at: :asc, id: :desc } - after_create -> { messageable.created_via_message } - after_destroy -> { messageable.destroyed_via_message } + after_create -> { messageable.created_via_message } end From 9f1c3ae046cc816566e5f08baf8aa826a146f349 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:17:01 +0200 Subject: [PATCH 47/94] Note my discontent --- app/models/comment.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/comment.rb b/app/models/comment.rb index 1eb286454..dc9c6be33 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -7,6 +7,7 @@ class Comment < ApplicationRecord has_markdown :body searchable_by :body_plain_text, using: :comments_search_index, as: :body + # FIXME: Not a fan of this. Think all references to comment should come directly from the message. scope :belonging_to_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } before_destroy :cleanup_events From cef8899bdae44ec2fd733d1b526df54b351b2276 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:18:59 +0200 Subject: [PATCH 48/94] Comments are not currently used as notifiables --- app/models/comment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index dc9c6be33..0c6a5db60 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -1,5 +1,5 @@ class Comment < ApplicationRecord - include Messageable, Notifiable, Searchable + include Messageable, Searchable belongs_to :creator, class_name: "User", default: -> { Current.user } has_many :reactions, dependent: :delete_all From b8c3ac8145f8411b753594034ff36ea6c523347f Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:22:31 +0200 Subject: [PATCH 49/94] Cache busting should happen as a comment in the partial, not as an actual key component --- app/views/notifications/_notification.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/notifications/_notification.html.erb b/app/views/notifications/_notification.html.erb index 71ea42c39..3c28a305e 100644 --- a/app/views/notifications/_notification.html.erb +++ b/app/views/notifications/_notification.html.erb @@ -1,4 +1,4 @@ -<% cache [notification, "2025-04-09"] do %> +<% cache notification do %> <%= notification_tag notification do %>
<%= avatar_image_tag(notification.creator) %> From 81a89dbda1d8df9c23ed166b0945cb8a592860a2 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:23:45 +0200 Subject: [PATCH 50/94] Comments should always have messages --- app/models/comment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 0c6a5db60..fe134ca2e 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -25,7 +25,7 @@ class Comment < ApplicationRecord private def cleanup_events # Delete events that reference through event_summary - if message&.event_summary.present? + if message.event_summary.present? Event.where(summary: message.event_summary).destroy_all end From b2b9c9974502b8fcea125b0a1fbc17c8f1045e73 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:23:48 +0200 Subject: [PATCH 51/94] Style --- app/views/notifications/_notification.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/notifications/_notification.html.erb b/app/views/notifications/_notification.html.erb index 3c28a305e..e8b94486e 100644 --- a/app/views/notifications/_notification.html.erb +++ b/app/views/notifications/_notification.html.erb @@ -8,7 +8,7 @@ <%= notification_title(notification) %>
<% if notification.event.action == "commented" %> - <%= "#{ strip_tags(notification.event.comment.body_html).blank? ? "#{ notification.event.creator.name } replied" : "#{ notification.event.creator.name }:" } #{ strip_tags(notification.event.comment.body_html).truncate(200) }" %> + <%= "#{strip_tags(notification.event.comment.body_html).blank? ? "#{notification.event.creator.name} replied" : "#{notification.event.creator.name}:" } #{strip_tags(notification.event.comment.body_html).truncate(200)}" %> <% else %> <%= notification_body(notification) %> <% end %> From 9359cac330e02ec6653751b4e5747557e9795369 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:24:02 +0200 Subject: [PATCH 52/94] Style --- app/views/notifications/_notification.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/notifications/_notification.html.erb b/app/views/notifications/_notification.html.erb index e8b94486e..8cb22eeb1 100644 --- a/app/views/notifications/_notification.html.erb +++ b/app/views/notifications/_notification.html.erb @@ -1,7 +1,7 @@ <% cache notification do %> <%= notification_tag notification do %>
- <%= avatar_image_tag(notification.creator) %> + <%= avatar_image_tag notification.creator %>
From 2f8a9bed0eda2cef741f9543bfd6ca92ad61c308 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:30:28 +0200 Subject: [PATCH 53/94] There is only ever one account per database So there is no protection in scoping things by account cc @jorgemanrubia --- app/models/event/particulars.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/event/particulars.rb b/app/models/event/particulars.rb index a9cec0452..389eda8ea 100644 --- a/app/models/event/particulars.rb +++ b/app/models/event/particulars.rb @@ -6,10 +6,10 @@ module Event::Particulars end def assignees - @assignees ||= account.users.where id: assignee_ids + @assignees ||= User.where id: assignee_ids end def comment - @comment ||= account.comments.find_by(id: comment_id) + @comment ||= Comment.find_by id: comment_id end end From 1a9a65fffb03863571fb9e79e3bbecdcc643540c Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:31:50 +0200 Subject: [PATCH 54/94] Unused --- app/models/event.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/event.rb b/app/models/event.rb index e64a8f896..9a4e51495 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -12,7 +12,6 @@ class Event < ApplicationRecord scope :chronologically, -> { order created_at: :asc, id: :desc } scope :non_boosts, -> { where.not action: :boosted } scope :boosts, -> { where action: :boosted } - scope :comments, -> { where action: :commented } after_create -> { card.touch_last_active_at } From d8f9511e08fc327ac9bc26257e20af3e2cda2e1f Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:34:56 +0200 Subject: [PATCH 55/94] Unused --- app/models/event.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/event.rb b/app/models/event.rb index 9a4e51495..a41c3dabe 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -5,7 +5,6 @@ class Event < ApplicationRecord belongs_to :summary, touch: true, class_name: "EventSummary" belongs_to :card - has_one :account, through: :creator has_one :message, through: :summary has_one :comment, through: :message, source: :messageable, source_type: "Comment" From e75d2bcaa1c641197a4a9da24b7dd9392a643e4d Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:37:39 +0200 Subject: [PATCH 56/94] Scopes last --- app/models/message.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/message.rb b/app/models/message.rb index 3e5fb37a7..73dda5fde 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -3,7 +3,7 @@ class Message < ApplicationRecord delegated_type :messageable, types: Messageable::TYPES, inverse_of: :message, dependent: :destroy - scope :chronologically, -> { order created_at: :asc, id: :desc } - after_create -> { messageable.created_via_message } + + scope :chronologically, -> { order created_at: :asc, id: :desc } end From f5b30456a0c828982af32c4a6778fcfb940aca98 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:42:41 +0200 Subject: [PATCH 57/94] Comments arent part of event summaries any more --- app/models/comment.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index fe134ca2e..cba429284 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -24,11 +24,6 @@ class Comment < ApplicationRecord private def cleanup_events - # Delete events that reference through event_summary - if message.event_summary.present? - Event.where(summary: message.event_summary).destroy_all - end - # Delete events that reference directly in particulars Event.where(particulars: { comment_id: id }).destroy_all end From f2c2b0492ea0235c3e36f799bf6c991375e3923b Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:42:50 +0200 Subject: [PATCH 58/94] Have to solve this in an overhaul of events --- app/models/comment.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/comment.rb b/app/models/comment.rb index cba429284..d6f29ae33 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -23,6 +23,7 @@ class Comment < ApplicationRecord end private + # FIXME: This isn't right. We need to introduce an eventable polymorphic association for this. def cleanup_events # Delete events that reference directly in particulars Event.where(particulars: { comment_id: id }).destroy_all From e4efe446e66034a4387225e6fca56bb4e7a3e9e0 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:45:34 +0200 Subject: [PATCH 59/94] Simlpe callback is actually enough Since the Comment already knows about the card. --- app/models/comment.rb | 15 +++++++++------ app/models/concerns/messageable.rb | 4 ---- app/models/message.rb | 2 -- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index d6f29ae33..1d9ca3038 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -11,12 +11,7 @@ class Comment < ApplicationRecord scope :belonging_to_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } before_destroy :cleanup_events - - # Called when a new comment is captured as a message during creation - def created_via_message - card.watch_by creator - card.track_event :commented, comment_id: id - end + after_create_commit :creator_watches_card, :track_commenting_on_card def to_partial_path "cards/#{super}" @@ -28,4 +23,12 @@ class Comment < ApplicationRecord # Delete events that reference directly in particulars Event.where(particulars: { comment_id: id }).destroy_all end + + def creator_watches_card + card.watch_by creator + end + + def track_commenting_on_card + card.track_event :commented, comment_id: id + end end diff --git a/app/models/concerns/messageable.rb b/app/models/concerns/messageable.rb index 987cbf784..219ee1197 100644 --- a/app/models/concerns/messageable.rb +++ b/app/models/concerns/messageable.rb @@ -7,8 +7,4 @@ module Messageable has_one :message, as: :messageable, touch: true, dependent: :destroy has_one :card, through: :message end - - def created_via_message - # Overwrite in Messageable class - end end diff --git a/app/models/message.rb b/app/models/message.rb index 73dda5fde..7d950b3b7 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -3,7 +3,5 @@ class Message < ApplicationRecord delegated_type :messageable, types: Messageable::TYPES, inverse_of: :message, dependent: :destroy - after_create -> { messageable.created_via_message } - scope :chronologically, -> { order created_at: :asc, id: :desc } end From 0031ce34ea85ef22391211acf23b64d4d0135fdc Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:46:17 +0200 Subject: [PATCH 60/94] Correct order --- app/models/comment.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 1d9ca3038..5818f72a9 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -10,20 +10,14 @@ class Comment < ApplicationRecord # FIXME: Not a fan of this. Think all references to comment should come directly from the message. scope :belonging_to_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } - before_destroy :cleanup_events after_create_commit :creator_watches_card, :track_commenting_on_card + before_destroy :cleanup_events def to_partial_path "cards/#{super}" end private - # FIXME: This isn't right. We need to introduce an eventable polymorphic association for this. - def cleanup_events - # Delete events that reference directly in particulars - Event.where(particulars: { comment_id: id }).destroy_all - end - def creator_watches_card card.watch_by creator end @@ -31,4 +25,10 @@ class Comment < ApplicationRecord def track_commenting_on_card card.track_event :commented, comment_id: id end + + # FIXME: This isn't right. We need to introduce an eventable polymorphic association for this. + def cleanup_events + # Delete events that reference directly in particulars + Event.where(particulars: { comment_id: id }).destroy_all + end end From e8a839d5f667850ddf3a75114c01ba66b9c76d0c Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 11:47:08 +0200 Subject: [PATCH 61/94] We have a callback dependency to do this actually --- app/controllers/cards/comments_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/cards/comments_controller.rb b/app/controllers/cards/comments_controller.rb index 35f8e13e7..33437eb24 100644 --- a/app/controllers/cards/comments_controller.rb +++ b/app/controllers/cards/comments_controller.rb @@ -19,14 +19,13 @@ class Cards::CommentsController < ApplicationController end def destroy - @message.destroy + @comment.destroy redirect_to @card end private def set_comment @comment = Comment.belonging_to_card(@card).find(params[:id]) - @message = @comment.message end def ensure_creatorship From 2357c75063623f25c86ab7ed0555100b2b25775d Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 13:09:49 +0200 Subject: [PATCH 62/94] Reads better with a verb first --- app/models/comment.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 5818f72a9..393f38cb4 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -10,7 +10,7 @@ class Comment < ApplicationRecord # FIXME: Not a fan of this. Think all references to comment should come directly from the message. scope :belonging_to_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } - after_create_commit :creator_watches_card, :track_commenting_on_card + after_create_commit :watch_card_by_creator, :track_commenting_on_card before_destroy :cleanup_events def to_partial_path @@ -18,7 +18,7 @@ class Comment < ApplicationRecord end private - def creator_watches_card + def watch_card_by_creator card.watch_by creator end From d4a31fec6d0de850307c19a50207e3acc6c2c282 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 13:33:06 +0200 Subject: [PATCH 63/94] Dont overload the main transaction Even if we are going to get rid of this cleanup_events stuff. --- app/models/comment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 393f38cb4..dd000e703 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -11,7 +11,7 @@ class Comment < ApplicationRecord scope :belonging_to_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } after_create_commit :watch_card_by_creator, :track_commenting_on_card - before_destroy :cleanup_events + after_destroy_commit :cleanup_events def to_partial_path "cards/#{super}" From 706178f3c4df43a65addb1147242a10e5773797d Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 13:33:54 +0200 Subject: [PATCH 64/94] Better naming --- app/models/comment.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index dd000e703..8922b30b7 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -10,7 +10,7 @@ class Comment < ApplicationRecord # FIXME: Not a fan of this. Think all references to comment should come directly from the message. scope :belonging_to_card, ->(card) { joins(:message).where(messages: { card_id: card.id }) } - after_create_commit :watch_card_by_creator, :track_commenting_on_card + after_create_commit :watch_card_by_creator, :track_commented_card after_destroy_commit :cleanup_events def to_partial_path @@ -22,7 +22,7 @@ class Comment < ApplicationRecord card.watch_by creator end - def track_commenting_on_card + def track_commented_card card.track_event :commented, comment_id: id end From 1a70be06c6416e7e11aa7fd3223e8eeecddb1919 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 13:37:31 +0200 Subject: [PATCH 65/94] Fix trailing CR --- app/models/card/draft_commenting.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/card/draft_commenting.rb b/app/models/card/draft_commenting.rb index 3302928c9..615d23916 100644 --- a/app/models/card/draft_commenting.rb +++ b/app/models/card/draft_commenting.rb @@ -31,4 +31,3 @@ module Card::DraftCommenting @draft_comment = nil end end - From 19c537af64f230134c14a3b21e89740bf62365be Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 13:48:03 +0200 Subject: [PATCH 66/94] Inline anemic method --- app/controllers/cards_controller.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/controllers/cards_controller.rb b/app/controllers/cards_controller.rb index 558f13d69..d6fba8376 100644 --- a/app/controllers/cards_controller.rb +++ b/app/controllers/cards_controller.rb @@ -26,7 +26,7 @@ class CardsController < ApplicationController def destroy @card.destroy! - redirect_to cards_path(collection_ids: [ @card.collection ]), notice: deleted_notice + redirect_to cards_path(collection_ids: [ @card.collection ]), notice: ("Card deleted" unless @card.creating?) end def update @@ -50,8 +50,4 @@ class CardsController < ApplicationController def card_params params.expect(card: [ :status, :title, :color, :due_on, :image, :draft_comment, tag_ids: [] ]) end - - def deleted_notice - "Card deleted" unless @card.creating? - end end From b36b2f04a993a82b139ced5c61880c760c5a0821 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 13:49:23 +0200 Subject: [PATCH 67/94] Too clever to have the redirect_to directly on the factory method return --- app/controllers/cards_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/cards_controller.rb b/app/controllers/cards_controller.rb index d6fba8376..e62146d4e 100644 --- a/app/controllers/cards_controller.rb +++ b/app/controllers/cards_controller.rb @@ -15,7 +15,8 @@ class CardsController < ApplicationController end def create - redirect_to @collection.cards.create! + card = @collection.cards.create! + redirect_to card end def show From a9c6f8bceb365fdf6bc495bdfc9021d2671a1151 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 13:51:47 +0200 Subject: [PATCH 68/94] No reuse likely --- app/controllers/first_runs_controller.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/controllers/first_runs_controller.rb b/app/controllers/first_runs_controller.rb index 6b59db593..5441bab17 100644 --- a/app/controllers/first_runs_controller.rb +++ b/app/controllers/first_runs_controller.rb @@ -7,7 +7,7 @@ class FirstRunsController < ApplicationController end def create - user = FirstRun.create!(user_params) + user = FirstRun.create!(params.expect(user: [ :name, :email_address, :password ])) start_new_session_for user redirect_to root_path end @@ -16,8 +16,4 @@ class FirstRunsController < ApplicationController def prevent_repeats redirect_to root_path unless Account.none? end - - def user_params - params.expect(user: [ :name, :email_address, :password ]) - end end From d3c5c86b217009e31d33cf531445fb4c4343cab8 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 14:06:11 +0200 Subject: [PATCH 69/94] Style --- app/controllers/notifications_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 403828c2b..6cb5e5985 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -1,10 +1,10 @@ class NotificationsController < ApplicationController def index - set_page_and_extract_portion_from Current.user.notifications.read.ordered - if @page.first? @unread = Current.user.notifications.unread.ordered end + + set_page_and_extract_portion_from Current.user.notifications.read.ordered end def mark_read From 4723b7b5041645647fdd0aa88a93201b903414aa Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 14:13:20 +0200 Subject: [PATCH 70/94] Style --- app/helpers/application_helper.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index a4d9e9ec5..52783033c 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -4,9 +4,6 @@ module ApplicationHelper end def icon_tag(name, **options) - classes = class_names "icon icon--#{name}", options.delete(:class) - options["aria-hidden"] = true - - content_tag :span, "", class: classes, **options + tag.span class: class_names("icon icon--#{name}", options.delete(:class)), "aria-hidden": true, **options end end From 120e8da1d5d414de74c2b1b4b156edaebc99a54b Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 14:14:34 +0200 Subject: [PATCH 71/94] Unused --- app/helpers/cards_helper.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/helpers/cards_helper.rb b/app/helpers/cards_helper.rb index 1546efd8e..08165c2d5 100644 --- a/app/helpers/cards_helper.rb +++ b/app/helpers/cards_helper.rb @@ -1,16 +1,8 @@ module CardsHelper - CARD_ROTATION = %w[ 75 60 45 35 25 5 ] - def card_title(card) card.title.presence || "Untitled" end - def card_rotation(card) - value = CARD_ROTATION[Zlib.crc32(card.to_param) % CARD_ROTATION.size] - - "--card-rotate: #{value}deg;" - end - def cards_next_page_link(target, page:, filter:, fetch_on_visible: false, data: {}, **options) url = cards_previews_path(target: target, page: page.next_param, **filter.as_params) From 1b732c41c7471c4f04c75d14b935abc4fcc791ca Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 18:15:05 +0200 Subject: [PATCH 72/94] Use render svg --- app/controllers/qr_codes_controller.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/controllers/qr_codes_controller.rb b/app/controllers/qr_codes_controller.rb index 707166930..42917d64b 100644 --- a/app/controllers/qr_codes_controller.rb +++ b/app/controllers/qr_codes_controller.rb @@ -2,10 +2,7 @@ class QrCodesController < ApplicationController allow_unauthenticated_access def show - qr_code_link = QrCodeLink.from_signed(params[:id]) - svg = RQRCode::QRCode.new(qr_code_link.url).as_svg(viewbox: true, fill: :white, color: :black) - expires_in 1.year, public: true - render plain: svg, content_type: "image/svg+xml" + render svg: RQRCode::QRCode.new(QrCodeLink.from_signed(params[:id]).url).as_svg(viewbox: true, fill: :white, color: :black) end end From 760cbb6c9974bf184d325e6b56aabd289c3cf783 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 18:52:54 +0200 Subject: [PATCH 73/94] Drop boosts Killed by design. --- app/assets/stylesheets/cards.css | 31 ------------- app/controllers/cards/boosts_controller.rb | 12 ----- app/controllers/concerns/events_timeline.rb | 14 ++---- app/helpers/events_helper.rb | 4 -- app/models/card.rb | 3 +- app/models/card/boostable.rb | 18 -------- app/models/card/scorable.rb | 3 +- app/models/event.rb | 6 --- app/models/event_summary.rb | 14 +----- app/models/filter/fields.rb | 2 +- app/views/cards/boosts/_boosts.html.erb | 10 ---- .../cards/boosts/create.turbo_stream.erb | 7 --- config/routes.rb | 1 - ...12162339_remove_boosts_count_from_cards.rb | 5 ++ db/schema.rb | 4 +- db/schema_cache.yml | 29 +----------- .../cards/boosts_controller_test.rb | 27 ----------- test/fixtures/cards.yml | 1 - test/fixtures/events.yml | 35 -------------- test/fixtures/filters.yml | 4 +- test/integration/card_messages.rb | 7 --- test/models/boost_test.rb | 7 --- test/models/card/scorable_test.rb | 46 ------------------- test/models/card_test.rb | 13 ------ test/models/event_summary_test.rb | 5 +- test/models/filter_test.rb | 6 +-- test/models/notifier_test.rb | 8 ---- 27 files changed, 23 insertions(+), 299 deletions(-) delete mode 100644 app/controllers/cards/boosts_controller.rb delete mode 100644 app/models/card/boostable.rb delete mode 100644 app/views/cards/boosts/_boosts.html.erb delete mode 100644 app/views/cards/boosts/create.turbo_stream.erb create mode 100644 db/migrate/20250412162339_remove_boosts_count_from_cards.rb delete mode 100644 test/controllers/cards/boosts_controller_test.rb delete mode 100644 test/models/boost_test.rb diff --git a/app/assets/stylesheets/cards.css b/app/assets/stylesheets/cards.css index 57024929f..8f0e2ee10 100644 --- a/app/assets/stylesheets/cards.css +++ b/app/assets/stylesheets/cards.css @@ -471,37 +471,6 @@ } } - .input.boost__input { - --hover-size: 0; - --input-border-radius: 0; - --input-border-size: 0; - --input-padding: 0; - --outline-size: 0; - - color: inherit; - font-size: inherit; - font-weight: inherit; - inline-size: min-content; - max-inline-size: 3ch; - min-inline-size: 1ch; - outline: none; - - @supports (field-sizing: content) { - field-sizing: content; - max-inline-size: unset; - } - - &:focus { - background-color: var(--color-highlight); - } - - &::-webkit-outer-spin-button, - &::-webkit-inner-spin-button { - -webkit-appearance: none; - margin: 0; - } - } - .card__closed { align-items: center; aspect-ratio: 7/4; diff --git a/app/controllers/cards/boosts_controller.rb b/app/controllers/cards/boosts_controller.rb deleted file mode 100644 index bebe888fc..000000000 --- a/app/controllers/cards/boosts_controller.rb +++ /dev/null @@ -1,12 +0,0 @@ -class Cards::BoostsController < ApplicationController - include CardScoped - - def create - count = if params[:boost_count].to_i == @card.boosts_count - @card.boosts_count + 1 - else - params[:boost_count].to_i - end - @card.boost!(count) - end -end diff --git a/app/controllers/concerns/events_timeline.rb b/app/controllers/concerns/events_timeline.rb index d71928b4d..0946907bb 100644 --- a/app/controllers/concerns/events_timeline.rb +++ b/app/controllers/concerns/events_timeline.rb @@ -19,13 +19,9 @@ module EventsTimeline end def events_for_activity_day - user_events.where(created_at: @activity_day.all_day). - group_by { |event| [ event.created_at.hour, helpers.event_column(event) ] }. - map { |hour_col, events| - [ hour_col, - events.uniq { |e| e.action == "boosted" ? [ e.creator_id, e.card_id ] : e.id } - ] - } + user_events.where(created_at: @activity_day.all_day) + .group_by { |event| [ event.created_at.hour, helpers.event_column(event) ] } + .map { |hour_col, events| [ hour_col, events.uniq { |e| e.id } ] } end def latest_event_before_activity_day @@ -38,7 +34,7 @@ module EventsTimeline def user_cards Current.user.accessible_cards - .published_or_drafted_by(Current.user) - .where(collection_id: collection_filter) + .published_or_drafted_by(Current.user) + .where(collection_id: collection_filter) end end diff --git a/app/helpers/events_helper.rb b/app/helpers/events_helper.rb index 4af17cbb0..ee0a908c2 100644 --- a/app/helpers/events_helper.rb +++ b/app/helpers/events_helper.rb @@ -72,8 +72,6 @@ module EventsHelper end when "unassigned" "#{ event.creator == Current.user ? "You" : event.creator.name } unassigned #{ event.assignees.include?(Current.user) ? "yourself" : event.assignees.pluck(:name).to_sentence } from #{ card_title(event.card) }".html_safe - when "boosted" - "#{ event.creator == Current.user ? "You" : event.creator.name } boosted #{ card_title(event.card) }".html_safe when "commented" "#{ event.creator == Current.user ? "You" : event.creator.name } commented on #{ card_title(event.card) }".html_safe when "published" @@ -99,8 +97,6 @@ module EventsHelper case event.action when "assigned" "assigned" - when "boosted" - "thumb-up" when "staged" "bolt" when "unstaged" diff --git a/app/models/card.rb b/app/models/card.rb index a265378ef..8b826e73b 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -1,5 +1,5 @@ class Card < ApplicationRecord - include Assignable, Boostable, Colored, DraftCommenting, Engageable, Eventable, Golden, + include Assignable, Colored, DraftCommenting, Engageable, Eventable, Golden, Messages, Notifiable, Pinnable, Closeable, Scorable, Searchable, Staged, Statuses, Taggable, Watchable @@ -22,7 +22,6 @@ class Card < ApplicationRecord scope :indexed_by, ->(index) do case index when "most_active" then ordered_by_activity - when "most_boosted" then ordered_by_boosts when "newest" then reverse_chronologically when "oldest" then chronologically when "latest" then latest diff --git a/app/models/card/boostable.rb b/app/models/card/boostable.rb deleted file mode 100644 index 9e7c404c8..000000000 --- a/app/models/card/boostable.rb +++ /dev/null @@ -1,18 +0,0 @@ -module Card::Boostable - extend ActiveSupport::Concern - - included do - scope :ordered_by_boosts, -> { order boosts_count: :desc } - end - - def boost!(count = 1) - count = count.to_i - count = 1 if count < 1 - - transaction do - track_event :boosted, count: count - update! boosts_count: count - rescore - end - end -end diff --git a/app/models/card/scorable.rb b/app/models/card/scorable.rb index 3bad86a74..91b0856b5 100644 --- a/app/models/card/scorable.rb +++ b/app/models/card/scorable.rb @@ -36,7 +36,6 @@ module Card::Scorable def event_weight(event) case - when event.boosted? then 1 when event.comment&.first_by_author_on_card? then 20 when event.comment&.follows_comment_by_another_author? then 15 when event.commented? then 10 @@ -60,6 +59,6 @@ module Card::Scorable end def scorable_events - events.where(action: [ :commented, :boosted ]) + events.where(action: :commented) end end diff --git a/app/models/event.rb b/app/models/event.rb index a41c3dabe..ad17a5e8c 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -9,15 +9,9 @@ class Event < ApplicationRecord has_one :comment, through: :message, source: :messageable, source_type: "Comment" scope :chronologically, -> { order created_at: :asc, id: :desc } - scope :non_boosts, -> { where.not action: :boosted } - scope :boosts, -> { where action: :boosted } after_create -> { card.touch_last_active_at } - def boosted? - action == "boosted" - end - def commented? action == "commented" end diff --git a/app/models/event_summary.rb b/app/models/event_summary.rb index b74a5a10c..fcfaabd83 100644 --- a/app/models/event_summary.rb +++ b/app/models/event_summary.rb @@ -5,16 +5,12 @@ class EventSummary < ApplicationRecord # FIXME: Consider persisting the body and compute at write time. def body - "#{main_summary} #{boosts_summary}".squish + events.map { |event| summarize(event) }.join(" ") end private delegate :time_ago_in_words, to: "ApplicationController.helpers" - def main_summary - events.non_boosts.map { |event| summarize(event) }.join(" ") - end - def summarize(event) case event.action when "published" @@ -39,12 +35,4 @@ class EventSummary < ApplicationRecord "#{event.creator.name} changed title from '#{event.particulars.dig('particulars', 'old_title')}' to '#{event.particulars.dig('particulars', 'new_title')}'." end end - - def boosts_summary - if tally = events.boosts.group(:creator).count.presence - tally.map do |creator, count| - "#{creator.name} +#{count}" - end.to_sentence + "." - end - end end diff --git a/app/models/filter/fields.rb b/app/models/filter/fields.rb index b608f2c58..9ff9764d4 100644 --- a/app/models/filter/fields.rb +++ b/app/models/filter/fields.rb @@ -1,7 +1,7 @@ module Filter::Fields extend ActiveSupport::Concern - INDEXES = %w[ most_boosted newest oldest latest stalled ] + INDEXES = %w[ newest oldest latest stalled ] delegate :default_value?, to: :class diff --git a/app/views/cards/boosts/_boosts.html.erb b/app/views/cards/boosts/_boosts.html.erb deleted file mode 100644 index 7c496fee7..000000000 --- a/app/views/cards/boosts/_boosts.html.erb +++ /dev/null @@ -1,10 +0,0 @@ -<%= form_with url: card_boosts_path(card), class: "pad-inline flex align-center gap" do %> - - <%= number_field_tag :boost_count, card.boosts_count, min: 1, class: "boost__input input", autocomplete: "off" %> - <%= card.boosts_count == 1 ? "boost" : "boosts" %> - - <%= tag.button class: "btn", type: :submit, style: "font-size: 0.4em" do %> - <%= icon_tag "add" %> - Boost - <% end %> -<% end %> \ No newline at end of file diff --git a/app/views/cards/boosts/create.turbo_stream.erb b/app/views/cards/boosts/create.turbo_stream.erb deleted file mode 100644 index 41ad2443e..000000000 --- a/app/views/cards/boosts/create.turbo_stream.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%= turbo_stream.update dom_id(@card, :boosts) do %> - <%= render "cards/boosts/boosts", card: @card %> -<% end %> - -<%= turbo_stream.replace dom_id(@card, :messages) do %> - <%= render "cards/messages", card: @card %> -<% end %> diff --git a/config/routes.rb b/config/routes.rb index b7b2615c6..ffd343916 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -35,7 +35,6 @@ Rails.application.routes.draw do resource :goldness resources :assignments - resources :boosts resources :stagings resources :taggings diff --git a/db/migrate/20250412162339_remove_boosts_count_from_cards.rb b/db/migrate/20250412162339_remove_boosts_count_from_cards.rb new file mode 100644 index 000000000..f6cb4aa66 --- /dev/null +++ b/db/migrate/20250412162339_remove_boosts_count_from_cards.rb @@ -0,0 +1,5 @@ +class RemoveBoostsCountFromCards < ActiveRecord::Migration[8.1] + def change + remove_column :cards, :boosts_count + end +end diff --git a/db/schema.rb b/db/schema.rb index cff07b713..643eef5da 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2025_04_12_084635) do +ActiveRecord::Schema[8.1].define(version: 2025_04_12_162339) do create_table "accesses", force: :cascade do |t| t.integer "collection_id", null: false t.datetime "created_at", null: false @@ -113,7 +113,6 @@ ActiveRecord::Schema[8.1].define(version: 2025_04_12_084635) do t.float "activity_score", default: 0.0, null: false t.datetime "activity_score_at" t.float "activity_score_order", default: 0.0, null: false - t.integer "boosts_count", default: 0, null: false t.integer "collection_id", null: false t.datetime "created_at", null: false t.integer "creator_id", null: false @@ -292,7 +291,6 @@ ActiveRecord::Schema[8.1].define(version: 2025_04_12_084635) do t.datetime "created_at", null: false t.string "title" t.datetime "updated_at", null: false - t.index ["account_id", "title"], name: "index_tags_on_account_id_and_title", unique: true t.index ["account_id"], name: "index_tags_on_account_id" end diff --git a/db/schema_cache.yml b/db/schema_cache.yml index 121a82ca3..612dd12be 100644 --- a/db/schema_cache.yml +++ b/db/schema_cache.yml @@ -412,16 +412,6 @@ columns: default_function: collation: comment: - - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column - auto_increment: - name: boosts_count - cast_type: *1 - sql_type_metadata: *2 - 'null': false - default: 0 - default_function: - collation: - comment: - *24 - *6 - &25 !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column @@ -2005,23 +1995,6 @@ indexes: comment: valid: true tags: - - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition - table: tags - name: index_tags_on_account_id_and_title - unique: true - columns: - - account_id - - title - lengths: {} - orders: {} - opclasses: {} - where: - type: - using: - include: - nulls_not_distinct: - comment: - valid: true - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition table: tags name: index_tags_on_account_id @@ -2154,4 +2127,4 @@ indexes: nulls_not_distinct: comment: valid: true -version: 20250412084635 +version: 20250412162339 diff --git a/test/controllers/cards/boosts_controller_test.rb b/test/controllers/cards/boosts_controller_test.rb deleted file mode 100644 index 6866fa459..000000000 --- a/test/controllers/cards/boosts_controller_test.rb +++ /dev/null @@ -1,27 +0,0 @@ -require "test_helper" - -class Cards::BoostsControllerTest < ActionDispatch::IntegrationTest - setup do - sign_in_as :kevin - end - - test "create" do - boost_count = cards(:logo).boosts_count - - assert_difference "cards(:logo).reload.boosts_count", +1 do - post card_boosts_path(cards(:logo), params: { boost_count: boost_count }, format: :turbo_stream) - end - - assert_turbo_stream action: :update, target: dom_id(cards(:logo), :boosts) - end - - test "create with value" do - boost_count = 10 - - assert_changes "cards(:logo).reload.boosts_count", to: boost_count do - post card_boosts_path(cards(:logo), params: { boost_count: boost_count }, format: :turbo_stream) - end - - assert_turbo_stream action: :update, target: dom_id(cards(:logo), :boosts) - end -end diff --git a/test/fixtures/cards.yml b/test/fixtures/cards.yml index d26288cb0..f49179eb3 100644 --- a/test/fixtures/cards.yml +++ b/test/fixtures/cards.yml @@ -4,7 +4,6 @@ logo: title: The logo isn't big enough due_on: <%= 3.days.from_now %> created_at: <%= 1.week.ago %> - boosts_count: 5 activity_score: 7 status: published last_active_at: <%= Time.current %> diff --git a/test/fixtures/events.yml b/test/fixtures/events.yml index 672fed176..61746d7fd 100644 --- a/test/fixtures/events.yml +++ b/test/fixtures/events.yml @@ -13,13 +13,6 @@ logo_assignment_jz: particulars: <%= { assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }.to_json %> created_at: <%= 1.week.ago + 1.hour %> -logo_boost_dhh: - creator: david - card: logo - action: boosted - summary: logo_initial_activity - created_at: <%= 1.week.ago + 2.hours %> - logo_assignment_km: creator: david card: logo @@ -28,34 +21,6 @@ logo_assignment_km: particulars: <%= { assignee_ids: [ ActiveRecord::FixtureSet.identify(:kevin) ] }.to_json %> created_at: <%= 1.day.ago %> -logo_boost_km1: - creator: kevin - card: logo - action: boosted - summary: logo_second_activity - created_at: <%= 1.day.ago + 1.hour %> - -logo_boost_km2: - creator: kevin - card: logo - action: boosted - summary: logo_second_activity - created_at: <%= 1.day.ago + 2.hours %> - -logo_boost_jz1: - creator: jz - card: logo - action: boosted - summary: logo_second_activity - created_at: <%= 1.day.ago + 3.hours %> - -logo_boost_jz2: - creator: jz - card: logo - action: boosted - summary: logo_third_activity - created_at: <%= 1.hour.ago %> - layout_published: creator: david card: layout diff --git a/test/fixtures/filters.yml b/test/fixtures/filters.yml index b6f73a844..f68bb6327 100644 --- a/test/fixtures/filters.yml +++ b/test/fixtures/filters.yml @@ -2,5 +2,5 @@ jz_assignments: creator: david tags: mobile assignees: jz - fields: <%= { indexed_by: :most_boosted }.to_json %> - params_digest: <%= Filter.digest_params({ indexed_by: :most_boosted, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }) %> + fields: <%= { indexed_by: :newest }.to_json %> + params_digest: <%= Filter.digest_params({ indexed_by: :newest, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }) %> diff --git a/test/integration/card_messages.rb b/test/integration/card_messages.rb index 464133fd7..6569d67d8 100644 --- a/test/integration/card_messages.rb +++ b/test/integration/card_messages.rb @@ -13,13 +13,6 @@ class CardMessagesTest < ActionDispatch::IntegrationTest assert_predicate card.messages.last, :event_summary? assert_equal "created", card.messages.last.messageable.events.sole.action - # Boost it - post card_boosts_path(card, format: :turbo_stream) - assert_equal 1, card.messages.count - assert_predicate card.messages.last, :event_summary? - assert_equal 2, card.messages.last.event_summary.events.count - assert_equal "boosted", card.messages.last.messageable.events.last.action - # Comment on it post collection_card_comments_url(collections(:writebook), card), params: { comment: { body: "Agreed." } } assert_equal 2, card.messages.count diff --git a/test/models/boost_test.rb b/test/models/boost_test.rb deleted file mode 100644 index 8c31db440..000000000 --- a/test/models/boost_test.rb +++ /dev/null @@ -1,7 +0,0 @@ -require "test_helper" - -class BoostTest < ActiveSupport::TestCase - # test "the truth" do - # assert true - # end -end diff --git a/test/models/card/scorable_test.rb b/test/models/card/scorable_test.rb index 64d0bb038..f24785e63 100644 --- a/test/models/card/scorable_test.rb +++ b/test/models/card/scorable_test.rb @@ -1,52 +1,6 @@ require "test_helper" class Card::ScorableTest < ActiveSupport::TestCase - test "recent activity counts more than older activity in the ordering" do - with_current_user :kevin do - travel_to 5.days.ago - card_old = collections(:writebook).cards.create! status: :published, title: "old" - card_mid = collections(:writebook).cards.create! status: :published, title: "mid" - card_new = collections(:writebook).cards.create! status: :published, title: "new" - - card_old.boost! - card_old.boost! - - travel_back - travel_to 2.days.ago - card_mid.boost! - - travel_back - card_new.boost! - - assert_equal %w[ new mid old ], Card.where(id: [ card_old, card_mid, card_new ]).ordered_by_activity.map(&:title) - end - end - - test "items with old activity are more stale than those with none, or with new activity" do - with_current_user :kevin do - travel_to 20.days.ago - card_old = collections(:writebook).cards.create! status: :published, title: "old" - card_new = collections(:writebook).cards.create! status: :published, title: "new" - card_none = collections(:writebook).cards.create! status: :published, title: "none" - - card_old.boost! - card_old.boost! - - travel_back - travel_to 2.days.ago - card_new.boost! - card_new.boost! - - travel_back - - assert_equal %w[ old new none ], Card.where(id: [ card_none, card_old, card_new ]).ordered_by_staleness.map(&:title) - - card_old.boost! - - assert_equal %w[ new old none ], Card.where(id: [ card_none, card_old, card_new ]).ordered_by_staleness.map(&:title) - end - end - test "cards with no activity have a valid activity_score_order" do card = Card.create! collection: collections(:writebook), creator: users(:kevin) diff --git a/test/models/card_test.rb b/test/models/card_test.rb index 311470c29..5b6d7087f 100644 --- a/test/models/card_test.rb +++ b/test/models/card_test.rb @@ -13,14 +13,6 @@ class CardTest < ActiveSupport::TestCase assert_equal "Agreed.", cards(:logo).messages.comments.last.messageable.body.to_plain_text.chomp end - test "boosting" do - assert_changes -> { cards(:logo).activity_score } do - assert_difference -> { cards(:logo).boosts_count }, +1 do - cards(:logo).boost!(cards(:logo).boosts_count+ 1) - end - end - end - test "assignment states" do assert cards(:logo).assigned_to?(users(:kevin)) assert_not cards(:logo).assigned_to?(users(:david)) @@ -74,11 +66,6 @@ class CardTest < ActiveSupport::TestCase assert_includes Card.search("haggis"), card end - test "ordering by boosts" do - cards(:layout).update! boosts_count: 1_000 - assert_equal cards(:layout, :logo, :shipping, :text), Card.ordered_by_boosts - end - test "closed" do assert_equal [ cards(:shipping) ], Card.closed end diff --git a/test/models/event_summary_test.rb b/test/models/event_summary_test.rb index ed78b4e1c..a438c492c 100644 --- a/test/models/event_summary_test.rb +++ b/test/models/event_summary_test.rb @@ -2,9 +2,8 @@ require "test_helper" class EventSummaryTest < ActiveSupport::TestCase test "body" do - assert_equal "Added by David. Assigned to JZ. David +1.", event_summaries(:logo_initial_activity).body - assert_equal "Assigned to Kevin. Kevin +2 and JZ +1.", event_summaries(:logo_second_activity).body - assert_equal "JZ +1.", event_summaries(:logo_third_activity).body + assert_equal "Added by David. Assigned to JZ.", event_summaries(:logo_initial_activity).body + assert_equal "Assigned to Kevin.", event_summaries(:logo_second_activity).body assert_equal "Added by Kevin.", event_summaries(:text_initial_activity).body end end diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index 2c94fe074..1a1ab882b 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -101,13 +101,13 @@ class FilterTest < ActiveSupport::TestCase end test "summary" do - assert_equal "Most boosted, tagged #mobile, and assigned to JZ ", filters(:jz_assignments).summary + assert_equal "Newest, tagged #mobile, and assigned to JZ ", filters(:jz_assignments).summary filters(:jz_assignments).update!(stages: workflow_stages(:qa_triage, :qa_in_progress)) - assert_equal "Most boosted, tagged #mobile, assigned to JZ, and staged in Triage or In progress ", filters(:jz_assignments).summary + assert_equal "Newest, tagged #mobile, assigned to JZ, and staged in Triage or In progress ", filters(:jz_assignments).summary filters(:jz_assignments).update!(stages: [], assignees: [], tags: [], collections: [ collections(:writebook) ]) - assert_equal "Most boosted in Writebook", filters(:jz_assignments).summary + assert_equal "Newest in Writebook", filters(:jz_assignments).summary filters(:jz_assignments).update!(indexed_by: "stalled") assert_equal "Stalled in Writebook", filters(:jz_assignments).summary diff --git a/test/models/notifier_test.rb b/test/models/notifier_test.rb index 7f2c7b5ac..058f455d7 100644 --- a/test/models/notifier_test.rb +++ b/test/models/notifier_test.rb @@ -5,14 +5,6 @@ class NotifierTest < ActiveSupport::TestCase assert_kind_of Notifier::Published, Notifier.for(events(:logo_published)) end - test "for does not raise an error when the event is not notifiable" do - assert_nothing_raised do - assert_no_difference -> { Notification.count } do - Notifier.for(events(:logo_boost_dhh)) - end - end - end - test "generate does not create notifications if the event was system-generated" do cards(:logo).drafted! events(:logo_published).update!(creator: accounts("37s").users.system) From 6bbf68a4f979ae2765a3513be7524aa212f2f60a Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 20:06:34 +0200 Subject: [PATCH 74/94] The tenanted db is the account scope --- .../accounts/join_codes_controller.rb | 6 +- app/controllers/accounts/users_controller.rb | 8 +- app/controllers/cards/stagings_controller.rb | 2 +- app/controllers/cards/taggings_controller.rb | 2 +- .../collections/workflows_controller.rb | 2 +- app/controllers/collections_controller.rb | 6 +- app/controllers/concerns/events_timeline.rb | 2 +- app/controllers/concerns/workflow_scoped.rb | 2 +- app/controllers/uploads_controller.rb | 3 +- app/controllers/users/avatars_controller.rb | 2 +- app/controllers/users_controller.rb | 10 +- app/controllers/workflows_controller.rb | 10 +- app/models/account.rb | 18 +-- app/models/account/closure_reasons.rb | 29 ---- app/models/card/closeable.rb | 4 +- app/models/card/taggable.rb | 2 +- app/models/closure/reason.rb | 21 ++- app/models/collection.rb | 1 - app/models/collection/accessible.rb | 2 +- app/models/collection/broadcastable.rb | 2 +- app/models/current.rb | 1 - app/models/first_run.rb | 5 +- app/models/tag.rb | 2 - app/models/user.rb | 2 - app/models/user/accessor.rb | 2 +- app/models/user/role.rb | 6 +- app/models/workflow.rb | 1 - app/views/accounts/users/_invite.html.erb | 2 +- app/views/cards/_closure_toggle.html.erb | 2 +- app/views/collections/edit.html.erb | 2 +- app/views/events/index.html.erb | 4 +- app/views/filters/_assignees.html.erb | 2 +- app/views/filters/_broadcasts.html.erb | 2 +- app/views/filters/_creators.html.erb | 2 +- app/views/filters/_dialog.html.erb | 8 +- app/views/filters/_stages.html.erb | 5 +- app/views/filters/_tags.html.erb | 2 +- app/views/workflows/index.html.erb | 2 +- config/routes.rb | 7 +- .../20250412170620_remove_account_ids.rb | 9 ++ db/schema.rb | 14 +- db/schema_cache.yml | 127 ++---------------- script/closureulate/collections_and_cards.rb | 2 +- script/populate/cards_for_pagination.rb | 4 +- .../accounts/users_controller_test.rb | 14 +- .../collections_controller_test.rb | 4 +- test/fixtures/collections.yml | 1 - test/fixtures/tags.yml | 2 - test/fixtures/users.yml | 3 - test/fixtures/workflows.yml | 2 - test/models/account/closure_reasons_test.rb | 8 -- test/models/card_test.rb | 4 +- test/models/collection_test.rb | 8 +- test/models/filter_test.rb | 2 +- test/models/notifier_test.rb | 2 +- test/models/tag_test.rb | 16 +-- test/models/user_test.rb | 5 +- 57 files changed, 123 insertions(+), 297 deletions(-) delete mode 100644 app/models/account/closure_reasons.rb create mode 100644 db/migrate/20250412170620_remove_account_ids.rb delete mode 100644 test/models/account/closure_reasons_test.rb diff --git a/app/controllers/accounts/join_codes_controller.rb b/app/controllers/accounts/join_codes_controller.rb index 52e909de8..b6f08bf3c 100644 --- a/app/controllers/accounts/join_codes_controller.rb +++ b/app/controllers/accounts/join_codes_controller.rb @@ -1,10 +1,10 @@ class Accounts::JoinCodesController < ApplicationController def show - render svg: RQRCode::QRCode.new(join_url(Current.account.join_code)).as_svg(viewbox: true, fill: :white, color: :black) + render svg: RQRCode::QRCode.new(join_url(Account.sole.join_code)).as_svg(viewbox: true, fill: :white, color: :black) end def update - Current.account.reset_join_code - redirect_to account_users_path + Account.sole.reset_join_code + redirect_to users_path end end diff --git a/app/controllers/accounts/users_controller.rb b/app/controllers/accounts/users_controller.rb index 7243677de..1ee4a15c6 100644 --- a/app/controllers/accounts/users_controller.rb +++ b/app/controllers/accounts/users_controller.rb @@ -3,22 +3,22 @@ class Accounts::UsersController < ApplicationController before_action :ensure_permission_to_administer_user, only: %i[ update destroy ] def index - @users = Current.account.users.active + @users = User.active end def update @user.update(role_params) - redirect_to account_users_path + redirect_to users_path end def destroy @user.deactivate - redirect_to account_users_path + redirect_to users_path end private def set_user - @user = Current.account.users.active.find(params[:id]) + @user = User.active.find(params[:id]) end def ensure_permission_to_administer_user diff --git a/app/controllers/cards/stagings_controller.rb b/app/controllers/cards/stagings_controller.rb index af524fa52..02cbd61af 100644 --- a/app/controllers/cards/stagings_controller.rb +++ b/app/controllers/cards/stagings_controller.rb @@ -3,7 +3,7 @@ class Cards::StagingsController < ApplicationController def create if params[:stage_id].present? - @card.toggle_stage Current.account.stages.find(params[:stage_id]) + @card.toggle_stage Stage.find(params[:stage_id]) else @card.update!(stage: nil) end diff --git a/app/controllers/cards/taggings_controller.rb b/app/controllers/cards/taggings_controller.rb index f75681c9d..bf17628fd 100644 --- a/app/controllers/cards/taggings_controller.rb +++ b/app/controllers/cards/taggings_controller.rb @@ -2,7 +2,7 @@ class Cards::TaggingsController < ApplicationController include CardScoped def new - @tags = Current.account.tags.alphabetically + @tags = Tag.alphabetically end def create diff --git a/app/controllers/collections/workflows_controller.rb b/app/controllers/collections/workflows_controller.rb index 963feb595..31a8cb715 100644 --- a/app/controllers/collections/workflows_controller.rb +++ b/app/controllers/collections/workflows_controller.rb @@ -11,6 +11,6 @@ class Collections::WorkflowsController < ApplicationController private def set_workflow - @workflow = Current.account.workflows.find(params.expect(collection: [ :workflow_id ]).require(:workflow_id)) + @workflow = Workflow.find(params.expect(collection: [ :workflow_id ]).require(:workflow_id)) end end diff --git a/app/controllers/collections_controller.rb b/app/controllers/collections_controller.rb index 3eb8cb507..36698afdf 100644 --- a/app/controllers/collections_controller.rb +++ b/app/controllers/collections_controller.rb @@ -2,11 +2,11 @@ class CollectionsController < ApplicationController before_action :set_collection, except: %i[ new create ] def new - @collection = Current.account.collections.build + @collection = Collection.new end def create - @collection = Current.account.collections.create! collection_params + @collection = Collection.create! collection_params redirect_to cards_path(collection_ids: [ @collection ]) end @@ -37,7 +37,7 @@ class CollectionsController < ApplicationController end def grantees - Current.account.users.active.where id: grantee_ids + User.active.where id: grantee_ids end def revokees diff --git a/app/controllers/concerns/events_timeline.rb b/app/controllers/concerns/events_timeline.rb index 0946907bb..0d50d2093 100644 --- a/app/controllers/concerns/events_timeline.rb +++ b/app/controllers/concerns/events_timeline.rb @@ -29,7 +29,7 @@ module EventsTimeline end def user_events - Event.where(card: user_cards, creator: Current.account.users) + Event.where(card: user_cards) end def user_cards diff --git a/app/controllers/concerns/workflow_scoped.rb b/app/controllers/concerns/workflow_scoped.rb index ca2e7398c..8e358733a 100644 --- a/app/controllers/concerns/workflow_scoped.rb +++ b/app/controllers/concerns/workflow_scoped.rb @@ -7,6 +7,6 @@ module WorkflowScoped private def set_workflow - @workflow = Current.account.workflows.find params[:workflow_id] + @workflow = Workflow.find(params[:workflow_id]) end end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 1c2af5a53..35e587a99 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -5,7 +5,8 @@ class UploadsController < ApplicationController before_action :set_attachment, only: :show def create - @upload = Current.account.uploads_attachments.create! blob: create_blob! + # FIXME: Try to get upload attachments on root + @upload = Account.first.uploads_attachments.create! blob: create_blob! end def show diff --git a/app/controllers/users/avatars_controller.rb b/app/controllers/users/avatars_controller.rb index e48a1f7f2..46deb691e 100644 --- a/app/controllers/users/avatars_controller.rb +++ b/app/controllers/users/avatars_controller.rb @@ -24,7 +24,7 @@ class Users::AvatarsController < ApplicationController end def set_user - @user = Current.account.users.find(params[:user_id]) + @user = User.find(params[:user_id]) end def render_avatar_or_initials diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 32e5c340c..f460b4d5c 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -5,11 +5,11 @@ class UsersController < ApplicationController before_action :set_account_from_join_code, only: %i[ new create ] def new - @user = @account.users.build + @user = User.new end def create - user = @account.users.create!(user_params) + user = User.create!(user_params) start_new_session_for user redirect_to root_path end @@ -21,8 +21,8 @@ class UsersController < ApplicationController end def update - @user.update user_params - redirect_to user_path(@user) + @user.update! user_params + redirect_to @user end private @@ -31,7 +31,7 @@ class UsersController < ApplicationController end def set_user - @user = Current.account.users.active.find(params[:id]) + @user = User.active.find(params[:id]) end def user_params diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb index 390cedcdb..fb2682988 100644 --- a/app/controllers/workflows_controller.rb +++ b/app/controllers/workflows_controller.rb @@ -2,15 +2,15 @@ class WorkflowsController < ApplicationController before_action :set_workflow, only: %i[ show edit update destroy ] def index - @workflows = Current.account.workflows + @workflows = Workflow.all end def new - @workflow = Current.account.workflows.new + @workflow = Workflow.new end def create - @workflow = Current.account.workflows.create! workflow_params + @workflow = Workflow.create! workflow_params # FIXME: this should definitely change. [ "Triage", "In progress", "On Hold", "Review" ].each { |name| @workflow.stages.create! name: name } redirect_to workflows_path @@ -34,10 +34,10 @@ class WorkflowsController < ApplicationController private def set_workflow - @workflow = Current.account.workflows.find params[:id] + @workflow = Workflow.find params[:id] end def workflow_params - params.expect workflow: [ :name ] + params.expect workflow: :name end end diff --git a/app/models/account.rb b/app/models/account.rb index 10ba9f773..f562ef665 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1,21 +1,5 @@ class Account < ApplicationRecord - include ClosureReasons, Joinable - - has_many :collections, dependent: :destroy - has_many :cards, through: :collections - - has_many :users, dependent: :destroy do - def system - find_or_create_system_user(proxy_association.owner) - end - end - - has_many :comments, through: :users - - has_many :workflows, dependent: :destroy - has_many :stages, through: :workflows, class_name: "Workflow::Stage" - - has_many :tags, dependent: :destroy + include Joinable has_many_attached :uploads end diff --git a/app/models/account/closure_reasons.rb b/app/models/account/closure_reasons.rb deleted file mode 100644 index e5bdf25a1..000000000 --- a/app/models/account/closure_reasons.rb +++ /dev/null @@ -1,29 +0,0 @@ -module Account::ClosureReasons - extend ActiveSupport::Concern - - DEFAULT_LABELS = [ - "Completed", - "Duplicate", - "Maybe later", - "Working as intended" - ] - - FALLBACK_LABEL = "Done" - - included do - has_many :closure_reasons, dependent: :destroy, class_name: "Closure::Reason" do - def labels - pluck(:label).presence || [ FALLBACK_LABEL ] - end - end - - after_create :create_default_closure_reasons - end - - private - def create_default_closure_reasons - DEFAULT_LABELS.each do |label| - closure_reasons.create! label: label - end - end -end diff --git a/app/models/card/closeable.rb b/app/models/card/closeable.rb index 3f98cb719..ce0a63e33 100644 --- a/app/models/card/closeable.rb +++ b/app/models/card/closeable.rb @@ -16,7 +16,7 @@ module Card::Closeable class_methods do def auto_close_all_due due_to_be_closed.find_each do |card| - card.close(user: card.collection.account.users.system, reason: "Closed") + card.close(user: User.system, reason: "Closed") end end end @@ -41,7 +41,7 @@ module Card::Closeable closure&.created_at end - def close(user: Current.user, reason: Account::ClosureReasons::FALLBACK_LABEL) + def close(user: Current.user, reason: Closure::Reason::FALLBACK_LABEL) unless closed? transaction do create_closure! user: user, reason: reason diff --git a/app/models/card/taggable.rb b/app/models/card/taggable.rb index a419a4cae..5ad75a593 100644 --- a/app/models/card/taggable.rb +++ b/app/models/card/taggable.rb @@ -9,7 +9,7 @@ module Card::Taggable end def toggle_tag_with(title) - tag = collection.account.tags.find_or_create_by!(title: title) + tag = Tag.find_or_create_by!(title: title) transaction do if tagged_with?(tag) diff --git a/app/models/closure/reason.rb b/app/models/closure/reason.rb index aa1f0161b..1472d09d1 100644 --- a/app/models/closure/reason.rb +++ b/app/models/closure/reason.rb @@ -1,3 +1,22 @@ class Closure::Reason < ApplicationRecord - belongs_to :account + DEFAULT_LABELS = [ + "Completed", + "Duplicate", + "Maybe later", + "Working as intended" + ] + + FALLBACK_LABEL = "Done" + + class << self + def labels + pluck(:label).presence || [ FALLBACK_LABEL ] + end + + def create_defaults + DEFAULT_LABELS.each do |label| + create! label: label + end + end + end end diff --git a/app/models/collection.rb b/app/models/collection.rb index 3820af5a1..319b2bb7d 100644 --- a/app/models/collection.rb +++ b/app/models/collection.rb @@ -1,7 +1,6 @@ class Collection < ApplicationRecord include Accessible, Broadcastable, Filterable - belongs_to :account belongs_to :creator, class_name: "User", default: -> { Current.user } belongs_to :workflow, optional: true diff --git a/app/models/collection/accessible.rb b/app/models/collection/accessible.rb index 81ed9b5b1..fc99cc217 100644 --- a/app/models/collection/accessible.rb +++ b/app/models/collection/accessible.rb @@ -34,6 +34,6 @@ module Collection::Accessible private def grant_access_to_everyone - accesses.grant_to(account.users) if all_access_previously_changed?(to: true) + accesses.grant_to(User.all) if all_access_previously_changed?(to: true) end end diff --git a/app/models/collection/broadcastable.rb b/app/models/collection/broadcastable.rb index 5e36898d6..82d548f33 100644 --- a/app/models/collection/broadcastable.rb +++ b/app/models/collection/broadcastable.rb @@ -3,6 +3,6 @@ module Collection::Broadcastable included do broadcasts_refreshes - broadcasts_refreshes_to ->(collection) { [ collection.account, :collections ] } + broadcasts_refreshes_to ->(collection) { :collections } end end diff --git a/app/models/current.rb b/app/models/current.rb index e5546b93b..a4d0f0864 100644 --- a/app/models/current.rb +++ b/app/models/current.rb @@ -2,5 +2,4 @@ class Current < ActiveSupport::CurrentAttributes attribute :session delegate :user, to: :session, allow_nil: true - delegate :account, to: :user, allow_nil: true end diff --git a/app/models/first_run.rb b/app/models/first_run.rb index 4115b8715..aeee8c8a2 100644 --- a/app/models/first_run.rb +++ b/app/models/first_run.rb @@ -1,6 +1,7 @@ class FirstRun def self.create!(user_attributes) - account = Account.create!(name: "Fizzy") - account.users.member.create!(user_attributes) + Account.create!(name: "Fizzy") + User.member.create!(user_attributes) + Closure::Reason.create_defaults end end diff --git a/app/models/tag.rb b/app/models/tag.rb index 128a9aaac..eadaa72f9 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -1,8 +1,6 @@ class Tag < ApplicationRecord include Filterable - belongs_to :account, default: -> { Current.account }, touch: true - has_many :taggings, dependent: :destroy has_many :cards, through: :taggings diff --git a/app/models/user.rb b/app/models/user.rb index f1b6a0d1f..a22841c42 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,8 +1,6 @@ class User < ApplicationRecord include Accessor, Assignee, Avatar, Role, Transferable - belongs_to :account - has_many :sessions, dependent: :destroy has_secure_password validations: false diff --git a/app/models/user/accessor.rb b/app/models/user/accessor.rb index f6c902c50..89534b777 100644 --- a/app/models/user/accessor.rb +++ b/app/models/user/accessor.rb @@ -11,6 +11,6 @@ module User::Accessor private def grant_access_to_collections - Access.insert_all account.collections.all_access.pluck(:id).collect { |collection_id| { collection_id: collection_id, user_id: id } } + Access.insert_all Collection.all_access.pluck(:id).collect { |collection_id| { collection_id: collection_id, user_id: id } } end end diff --git a/app/models/user/role.rb b/app/models/user/role.rb index 625361037..1b5648679 100644 --- a/app/models/user/role.rb +++ b/app/models/user/role.rb @@ -10,10 +10,8 @@ module User::Role end class_methods do - def find_or_create_system_user(account) - account.users.find_or_create_by!(role: :system) do |user| - user.name = "System" - end + def system + find_or_create_by!(role: :system) { it.name = "System" } end end diff --git a/app/models/workflow.rb b/app/models/workflow.rb index 164d3eb47..df896cf09 100644 --- a/app/models/workflow.rb +++ b/app/models/workflow.rb @@ -1,4 +1,3 @@ class Workflow < ApplicationRecord - belongs_to :account has_many :stages, dependent: :delete_all end diff --git a/app/views/accounts/users/_invite.html.erb b/app/views/accounts/users/_invite.html.erb index e064d74bb..d6e99af78 100644 --- a/app/views/accounts/users/_invite.html.erb +++ b/app/views/accounts/users/_invite.html.erb @@ -1,5 +1,5 @@
- <% url = join_url(Current.account.join_code) %> + <% url = join_url(Account.sole.join_code) %>
- <% Current.account.users.active.order(:name).each do |user| %> + <% User.active.order(:name).each do |user| %>
<%= form.check_box "assignee_ids[]", { checked: filter.assignees.include?(user), diff --git a/app/views/filters/_broadcasts.html.erb b/app/views/filters/_broadcasts.html.erb index ff1777e7c..a432280e6 100644 --- a/app/views/filters/_broadcasts.html.erb +++ b/app/views/filters/_broadcasts.html.erb @@ -3,5 +3,5 @@ <%= turbo_stream_from collection %> <% end %> <% else %> - <%= turbo_stream_from Current.account, :collections %> + <%= turbo_stream_from :collections %> <% end %> diff --git a/app/views/filters/_creators.html.erb b/app/views/filters/_creators.html.erb index 420ef97eb..e46931110 100644 --- a/app/views/filters/_creators.html.erb +++ b/app/views/filters/_creators.html.erb @@ -37,7 +37,7 @@ <%= icon_tag "check", class: "checked flex-item-justify-end" %>
- <% Current.account.users.active.without(Current.user).order(:name).each do |user| %> + <% User.active.without(Current.user).order(:name).each do |user| %>
<%= form.check_box :creator_ids, { multiple: true, diff --git a/app/views/filters/_dialog.html.erb b/app/views/filters/_dialog.html.erb index e633630c4..e20219364 100644 --- a/app/views/filters/_dialog.html.erb +++ b/app/views/filters/_dialog.html.erb @@ -78,7 +78,7 @@ <% end %> - <% if workflow = Current.account.workflows.first %> + <% if workflow = Workflow.first %>
  • In Stage
    • <% workflow.stages.each do |stage| %> @@ -93,7 +93,7 @@     <% end %> - <% if (tags = Current.account.tags.order(:title)).any? %> + <% if (tags = Tag.all.order(:title)).any? %>
    Tagged
    @@ -133,7 +133,7 @@
    - <% Current.account.users.active.without(Current.user).order(:name).each do |user| %> + <% User.active.without(Current.user).order(:name).each do |user| %>
    - <% Current.account.users.active.without(Current.user).order(:name).each do |user| %> + <% User.active.without(Current.user).order(:name).each do |user| %>