From 78fc80cf35d6173d7358d8c51a0261a3947bcdf1 Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Fri, 12 Dec 2025 04:06:03 +0100 Subject: [PATCH] API: Allow updates to `last_active_at` (#2076) * Allow Card#last_updated_at to be set This is useful when doing an import from another system. I'm currently working on a script to import our Github issues into Fizzy. This is discussed in https://github.com/basecamp/fizzy/pull/2056#discussion_r2609560246 * Add nil fallback and expand test coverage for last_active_at Adds a safety fallback to Time.current if created_at is unexpectedly nil during card creation. Test coverage to verify: * last_active_at defaults to created_at when not provided * last_active_at can be updated via API on existing cards * import workflow where last_active_at is restored after comments * publishing doesn't overwrite explicit last_active_at values --------- Co-authored-by: Jeremy Daer --- app/controllers/cards_controller.rb | 2 +- app/models/card/eventable.rb | 4 +- docs/API.md | 2 + test/controllers/cards_controller_test.rb | 92 ++++++++++++++++++++--- test/models/card/eventable_test.rb | 42 ++++++++++- 5 files changed, 125 insertions(+), 17 deletions(-) diff --git a/app/controllers/cards_controller.rb b/app/controllers/cards_controller.rb index 8223d3801..83706ddf4 100644 --- a/app/controllers/cards_controller.rb +++ b/app/controllers/cards_controller.rb @@ -61,6 +61,6 @@ class CardsController < ApplicationController end def card_params - params.expect(card: [ :status, :title, :description, :image, :created_at, tag_ids: [] ]) + params.expect(card: [ :status, :title, :description, :image, :created_at, :last_active_at, tag_ids: [] ]) end end diff --git a/app/models/card/eventable.rb b/app/models/card/eventable.rb index 520fd95d4..94544656d 100644 --- a/app/models/card/eventable.rb +++ b/app/models/card/eventable.rb @@ -4,7 +4,7 @@ module Card::Eventable include ::Eventable included do - before_create { self.last_active_at = Time.current } + before_create { self.last_active_at ||= created_at || Time.current } after_save :track_title_change, if: :saved_change_to_title? end @@ -12,7 +12,7 @@ module Card::Eventable def event_was_created(event) transaction do create_system_comment_for(event) - touch_last_active_at + touch_last_active_at unless was_just_published? end end diff --git a/docs/API.md b/docs/API.md index 318e7a1dd..8f85eec6a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -496,6 +496,7 @@ Creates a new card in a board. | `image` | file | No | Header image for the card | | `tag_ids` | array | No | Array of tag IDs to apply to the card | | `created_at` | datetime | No | Override creation timestamp (ISO 8601 format) | +| `last_active_at` | datetime | No | Override last activity timestamp (ISO 8601 format) | __Request:__ @@ -523,6 +524,7 @@ Updates a card. | `status` | string | No | Card status: `drafted`, `published` | | `image` | file | No | Header image for the card | | `tag_ids` | array | No | Array of tag IDs to apply to the card | +| `last_active_at` | datetime | No | Override last activity timestamp (ISO 8601 format) | __Request:__ diff --git a/test/controllers/cards_controller_test.rb b/test/controllers/cards_controller_test.rb index a22018a46..635b2d2e0 100644 --- a/test/controllers/cards_controller_test.rb +++ b/test/controllers/cards_controller_test.rb @@ -21,8 +21,9 @@ class CardsControllerTest < ActionDispatch::IntegrationTest end card = Card.last - assert card.drafted? assert_redirected_to card + + assert card.drafted? end test "create resumes existing draft if it exists" do @@ -30,9 +31,8 @@ class CardsControllerTest < ActionDispatch::IntegrationTest assert_no_difference -> { Card.count } do post board_cards_path(boards(:writebook)) + assert_redirected_to draft end - - assert_redirected_to draft end test "show" do @@ -68,6 +68,7 @@ class CardsControllerTest < ActionDispatch::IntegrationTest boards(:writebook).update! all_access: false boards(:writebook).accesses.revoke_from users(:kevin) + get card_path(cards(:logo)) assert_response :not_found end @@ -75,6 +76,7 @@ class CardsControllerTest < ActionDispatch::IntegrationTest test "admins can see delete button on any card" do get card_path(cards(:logo)) assert_response :success + assert_match "Delete this card", response.body end @@ -83,6 +85,7 @@ class CardsControllerTest < ActionDispatch::IntegrationTest get card_path(cards(:logo)) assert_response :success + assert_match "Delete this card", response.body end @@ -91,6 +94,7 @@ class CardsControllerTest < ActionDispatch::IntegrationTest get card_path(cards(:logo)) assert_response :success + assert_no_match "Delete this card", response.body end @@ -124,10 +128,9 @@ class CardsControllerTest < ActionDispatch::IntegrationTest test "show card with comment containing malformed remote image attachment" do card = cards(:logo) - card.comments.create!( + card.comments.create! \ creator: users(:kevin), body: '' - ) get card_path(card) assert_response :success @@ -140,6 +143,7 @@ class CardsControllerTest < ActionDispatch::IntegrationTest get card_path(card), as: :json assert_response :success + assert_equal card.title, @response.parsed_body["title"] assert_equal 2, @response.parsed_body["steps"].size end @@ -149,12 +153,12 @@ class CardsControllerTest < ActionDispatch::IntegrationTest post board_cards_path(boards(:writebook)), params: { card: { title: "My new card", description: "Big if true", tag_ids: [ tags(:web).id, tags(:mobile).id ] } }, as: :json + assert_response :created end - assert_response :created - assert_equal card_path(Card.last, format: :json), @response.headers["Location"] - card = Card.last + assert_equal card_path(card, format: :json), @response.headers["Location"] + assert_equal "My new card", card.title assert_equal "Big if true", card.description.to_plain_text assert_equal [ tags(:mobile), tags(:web) ].sort, card.tags.sort @@ -167,25 +171,91 @@ class CardsControllerTest < ActionDispatch::IntegrationTest post board_cards_path(boards(:writebook)), params: { card: { title: "Backdated card", created_at: custom_time } }, as: :json + assert_response :created end - assert_response :created assert_equal custom_time, Card.last.created_at end + test "create as JSON with custom last_active_at" do + created_time = Time.utc(2024, 1, 15, 10, 30, 0) + last_active_time = Time.utc(2024, 6, 1, 12, 0, 0) + + assert_difference -> { Card.count }, +1 do + post board_cards_path(boards(:writebook)), + params: { card: { title: "Card with activity", created_at: created_time, last_active_at: last_active_time } }, + as: :json + assert_response :created + end + + card = Card.last + assert_equal created_time, card.created_at + assert_equal last_active_time, card.last_active_at + end + + test "create as JSON defaults last_active_at to created_at when not provided" do + created_time = Time.utc(2024, 1, 15, 10, 30, 0) + + assert_difference -> { Card.count }, +1 do + post board_cards_path(boards(:writebook)), + params: { card: { title: "Backdated card without last_active_at", created_at: created_time } }, + as: :json + assert_response :created + end + + card = Card.last + assert_equal created_time, card.created_at + assert_equal created_time, card.last_active_at + end + + test "update as JSON with custom last_active_at" do + card = cards(:logo) + custom_time = Time.utc(2024, 3, 15, 14, 0, 0) + + put card_path(card, format: :json), params: { card: { last_active_at: custom_time } } + + assert_response :success + assert_equal custom_time, card.reload.last_active_at + end + + test "update as JSON can restore last_active_at after comments overwrite it" do + created_time = Time.utc(2024, 1, 15, 10, 30, 0) + last_active_time = Time.utc(2024, 6, 1, 12, 0, 0) + + # Create a card with custom timestamps (simulating import) + post board_cards_path(boards(:writebook)), + params: { card: { title: "Imported card", created_at: created_time, last_active_at: last_active_time } }, + as: :json + assert_response :created + + card = Card.last + + # Adding a comment overwrites last_active_at (this is expected) + card.comments.create!(creator: users(:kevin), body: "Imported comment") + assert_not_equal last_active_time, card.reload.last_active_at + + # After import, restore the correct last_active_at + put card_path(card, format: :json), params: { card: { last_active_at: last_active_time } } + assert_response :success + + assert_equal last_active_time, card.reload.last_active_at + end + test "update as JSON" do card = cards(:logo) - put card_path(card, format: :json), params: { card: { title: "Update test" } } + put card_path(card, format: :json), params: { card: { title: "Update test" } } assert_response :success + assert_equal "Update test", card.reload.title end test "delete as JSON" do card = cards(:logo) - delete card_path(card, format: :json) + delete card_path(card, format: :json) assert_response :no_content + assert_not Card.exists?(card.id) end end diff --git a/test/models/card/eventable_test.rb b/test/models/card/eventable_test.rb index 0c39ffc91..a92d71f6c 100644 --- a/test/models/card/eventable_test.rb +++ b/test/models/card/eventable_test.rb @@ -5,11 +5,47 @@ class Card::EventableTest < ActiveSupport::TestCase Current.session = sessions(:david) end - test "new cards get the current time as the last activity time" do + test "new cards default last_active_at to created_at" do freeze_time - card = boards(:writebook).cards.create!(title: "Some card card", creator: users(:david)) - assert_equal Time.current, card.last_active_at + card = boards(:writebook).cards.create!(title: "Some card", creator: users(:david)) + assert_equal card.created_at, card.last_active_at + end + + test "new cards with custom created_at default last_active_at to that time" do + custom_time = 1.week.ago.change(usec: 0) + + card = boards(:writebook).cards.create!(title: "Backdated card", creator: users(:david), created_at: custom_time) + assert_equal custom_time, card.created_at + assert_equal custom_time, card.last_active_at + end + + test "new cards preserve explicit last_active_at" do + created_time = 2.weeks.ago.change(usec: 0) + last_active_time = 1.week.ago.change(usec: 0) + + card = boards(:writebook).cards.create! \ + title: "Card with explicit timestamps", + creator: users(:david), + created_at: created_time, + last_active_at: last_active_time + + assert_equal created_time, card.created_at + assert_equal last_active_time, card.last_active_at + end + + test "publishing a card does not overwrite last_active_at" do + created_time = 2.weeks.ago.change(usec: 0) + last_active_time = 1.week.ago.change(usec: 0) + + card = boards(:writebook).cards.create! \ + title: "Published card", + creator: users(:david), + status: :published, + created_at: created_time, + last_active_at: last_active_time + + assert_equal last_active_time, card.last_active_at end test "tracking events update the last activity time" do