Account::Searchable#clear_search_records called
Search::Record.for(id).destroy_all which deleted every row in the
shard's table, wiping search records belonging to every other account
that happened to hash to the same shard.
ref: https://3.basecamp.com/2914079/buckets/27/card_tables/cards/9782824728
Tag was never actually embedded as an action-text-attachment in
persisted rich text. It was only used in the Fizzy Do command prompt
input which was not stored. The data transfer tests happened to use Tag
as a convenient fixture — switch them to User, which is genuinely
attachable via mentions.
* Extract shared DnsTestHelper from duplicated stub_dns_resolution methods
* Fix flaky WebTest by stubbing DNS resolution for push subscription creation
ActiveStorage::Record and ApplicationRecord used separate connection
pools (from separate `connects_to` calls). This meant `belongs_to
touch: true` on attachments silently failed — the card couldn't join the
attachment's transaction to receive the deferred touch, so fragment
caches were never invalidated after image removal.
Fix by delegating `connection_pool` from ActiveStorage::Record to
ApplicationRecord so they share a single pool.
* Add JSON events API endpoint
* Add regression test for event particulars defaults
* Move JSON events API to a dedicated ActivitiesController
The events endpoint served both the HTML day timeline and the JSON API
feed, but the two paths shared no data or behavior — the HTML side uses
DayTimelinesScoped while the JSON side built its own query. Splitting
into ActivitiesController gives the API its own home at
GET /:account/activities.json without dragging in the timeline
before_actions.
Also preloads comment creator in Event.preloaded to avoid an N+1 when
rendering comment eventables in the JSON feed.
Account incineration could leave orphaned records due to missing cascade
declarations and async jobs failing when the account was already gone.
Cascade fixes:
- Add Account::Searchable concern to clean up Search::Query (delete_all)
and Search::Record (destroy_all, respects SQLite FTS dependent: :destroy)
- Add before_destroy in Account::Storage to delete storage entries
- Suppress storage entry recording during incineration so attachment
purge callbacks don't create entries or enqueue materialize jobs
- Guard Access#clean_inaccessible_data_later with unless user.destroyed?
to avoid enqueuing pointless jobs during user cascade
Job tenancy:
- Extract AccountTenanted concern from the global ActiveJob initializer
into ApplicationJob (include) with targeted prepends for
ActionMailer::MailDeliveryJob and Turbo broadcast jobs
- Defer account resolution from deserialize to perform so that missing
accounts raise DeserializationError inside the execution path where
discard_on can handle it
Tests:
- Comprehensive incineration test covering 35 model types with
before/after assertions and full enqueued job processing
- Mailer deliver_later test verifying account context survives job
serialization for multi-account users
- Turbo broadcast test verifying account-scoped URLs in rendered partials
record.to_json serializes through model accessors. Column::Colored
overrides the color accessor to return a Color struct, which serializes
as {"name":"Blue","value":"var(--color-card-default)"} instead of the
raw CSS string.
Two fixes:
- Export raw DB values via record.attributes.slice(*attributes).to_json
- Color.for_value parses legacy JSON format so columns imported from
old exports self-heal when read
* Exclude export/import blobs and attachments from account exports
Export and import file blobs were being assigned the account's
account_id (via the Current.account default), causing subsequent
exports to include previous export/import ZIP files in the data.
This roughly doubled the export size each time and made imports
fail with an IntegrityError on unrecognized "Account::Export"
record type.
Filter out blobs attached to Export/Import records in
BlobRecordSet, FileRecordSet, and a new AttachmentRecordSet.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Use STI base type for export blob exclusion filter
Rails polymorphic_name returns base_class.name for STI models,
so ActiveStorage stores record_type as "Export" rather than the
concrete subclass names. Filter on the base type accordingly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Fix export filtering for mixed-use Active Storage blobs
Exclude only internal-only blobs for account export metadata/file sets, scope attachment subqueries by account, and add regression coverage for blobs shared between user records and export attachments.
* Simplify export blob filtering
Exports always create fresh blobs, so a blob can never be legitimately
shared between an Export/Import and a real account record. Replace the
internal_only/external split with a single excluded_blob_ids subquery
and drop the contrived shared-blob test.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Fix multi-term SQLite FTS5 search causing a 500 error
When filtering cards by more than one term, `matching` was called
once per term via an association join. Rails did not deduplicate
association joins, resulting in two JOINs to `search_records_fts`
in a single query. SQLite FTS5 then raised "ambiguous column name"
when evaluating the MATCH condition.
Fix by using a string join instead, which Rails deduplicates so
only one JOIN to `search_records_fts` is generated regardless of
how many terms are searched.
Fixes: https://github.com/basecamp/fizzy/discussions/2354
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* test: verify multi-term search requires ALL terms to match
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Mike Dalessio <mike@37signals.com>
Add IMPORTABLE_MODEL_NAMES to RecordSet and verify polymorphic type
columns during import check against this allowlist before calling
constantize. This prevents arbitrary class instantiation from
untrusted import ZIP data.
BlobRecordSet#import_batch preserved blob keys from exported ZIP files
verbatim. A crafted ZIP with a traversal key like "../../config/deploy.yml"
would create a blob whose key resolves to an arbitrary filesystem path
when served by ActiveStorage's DiskService, allowing an attacker to read
local files.
Fix: generate fresh blob keys on import, discarding whatever key was in
the ZIP. This is safe because nothing else in the import pipeline
references blobs by key — attachments use blob_id, ActionText uses GIDs
based on record IDs, and only FileRecordSet used the old key for file
data lookup.
Update FileRecordSet to build an old_key→blob_id mapping from the blob
JSON metadata in the ZIP, then look up blobs by ID instead of by key.
Both check_record and import_batch are now fail-closed: they raise
IntegrityError for unmapped storage files, missing blobs, and duplicate
keys in the export.
Backfill test coverage for BlobRecordSet and FileRecordSet import, and
add a round-trip test verifying blob data survives export/import with
regenerated keys.
* Validate and normalize auto-postpone period to days
- Add Entropy::AUTO_POSTPONE_PERIODS and validate auto_postpone_period against the allowed set
- Introduce auto_postpone_period_in_days for forms and entropy update endpoints
- Fall back to index 0 in knob partial when current value isn't in options
- Remove entropy_auto_close_options helper in favor of model constant
- Update tests to use allowed period values
* Use auto_postpone_period_in_days for JSON entropy updates
The JSON API was accepting auto_postpone_period (seconds) which
bypassed the days normalization and validation. Switch to
auto_postpone_period_in_days consistently and add explicit
wrap_parameters so flat JSON params are wrapped correctly for
the virtual attribute.
* Default to account or 30-day fallback when knob value is invalid
* Address PR review feedback for entropy validation
- Fall back to first knob option (index 0) when persisted value isn't in
the allowed set, preventing nil index errors for legacy values
- Use integer division (1.day.to_i) for consistent day calculations
* Default to account entropy period instead of first option for knob fallback
* Test that default auto-postpone period is in the allowed periods
* Return 422 instead of 500 for invalid entropy auto-postpone values
Rescue ActiveRecord::RecordInvalid in entropy controllers so invalid
auto_postpone_period_in_days values return 422 Unprocessable Entity
instead of raising a 500.
* Fix NoMethodError when board entropy falls back to account default
Use container.account.entropy.auto_postpone_period_in_days instead of
container.account.auto_postpone_period_in_days since Account doesn't
delegate that method.
* Test board entropy fallback to account default for invalid periods
Publishing or unpublishing a board didn't bust the `json.cache! board`
fragment cache because `Board::Publication` didn't touch its parent
board. The board's `updated_at` stayed stale, so subsequent GET requests
continued serving cached JSON without `public_url`.
Clear assignee's existing notifications in setup since Notifier now
uses create_or_find_by instead of create, and reload the association
to avoid caching after destroy_all.
Update fixture references after rebase: use `logo_assignment_kevin`
as base notification and `logo_mentioned_david` for mention tests.
Update source to `logo_published` in tests that need generic card events.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add comprehensive integration test covering card assignment, comments,
mentions, email bundling, and edge cases (system user, inactive user)
- Inline push notification test helpers into the only test that uses them
- Remove separate PushNotificationTestHelper module
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Each target now implements process directly with its own logic,
rather than using processable?/perform_push hooks. The pushable?
check is done once in Notification#push before iterating targets.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
"Push to target" reads naturally - we push the notification to the
target. "Target processes" also makes sense - the target receives
and handles the notification in its own way.
- Add class method PushTarget.process(notification) that instantiates
and calls the instance method
- Rename instance method from push to process
- Add private push_to helper in Pushable for readable iteration
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace separate WebPushJob and NativePushJob with a single PushJob
that calls notification.push, which iterates over registered targets.
Each target handles its own delivery - Web pushes synchronously via
the pool, Native enqueues device-level jobs via deliver_later_to.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PushTarget#push reads more naturally than Push#push. The push target
is the thing that pushes notifications to a specific destination.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace NotificationPusher with a cleaner architecture:
- Add Notification::Pushable concern with push target registry
- Add Notification::Push base class with template methods
- Add Notification::Push::Web for web push (OSS)
- Add Notification::Push::Native for native push (SaaS)
- Add Notification::WebPushJob and Notification::NativePushJob
Key design:
- Registry pattern: Notification.register_push_target(:web)
- Template method: push calls should_push? then perform_push
- Subclasses override should_push? (with super) and perform_push
- Each target handles its own job enqueueing
Also:
- Add Notification#pushable? for checking push eligibility
- Add Notification#identity delegation to user
- Reorganize tests to match new class structure
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tidy up saas engine a bit more
Event notifiers used the `mentionees` DB association to exclude mentioned
users from comment/card notifications. Since mentions are created async
via Mention::CreateJob, a race condition meant the mentionee list could
be empty when the event notification job ran first, causing the user to
receive both a comment and a mention push notification.
Use `scan_mentionees` instead, which scans the rich text body directly
for mentioned users without depending on Mention records existing yet.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rails only applies the last callback when `after_create_commit` and
`after_update_commit` reference the same method name [1]:
> However, if you use the `after_create_commit` and the
`after_update_commit` callback with the same method name, it will only
allow the last callback defined to take effect, as they both internally
alias to `after_commit` which overrides previously defined callbacks
with the same method name.
- Push notifications were never sent when a notification was first
created — only when the source was updated
- Replaced the two callbacks with a single `after_save_commit`, which
fires on both create and update, with the
`source_id_previously_changed?` guard (true for both new records and
source changes)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
[1] https://guides.rubyonrails.org/active_record_callbacks.html#aliases-for-after-commit
Mike Dalessio