From fb733b9a60522b8f209f5260e44ef39ca6046a4e Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 12 Apr 2025 10:50:25 +0200 Subject: [PATCH] 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")