Repository navigation
Replace the Auditor write path with MemberActivityRecorder to create one funnel for activity rows - #3068
Conversation
…to create one funnel for activity rows
mroderick
left a comment
mroderick
left a comment
There was a problem hiding this comment.
Needs fixes before merge: the rubocop job fails on two new offenses.
Metrics/MethodLength—MemberActivityRecorder.recordgrew to 11 lines against a max of 10.RSpec/MessageSpies— the new controller spec sets a message expectation withexpect(...).to receive.
Beyond CI, one real defect: the four sponsor.* keys these call sites write are missing from MemberActivityRecorder::KEYS, so every consent toggle logs an unregistered key warning through the funnel this PR consolidates. The rows persist; only the log line suffers.
Two non-blocking test notes: the recorder's new contact_email: branch has no unit coverage, and the feature-spec setups re-derive the audit key with the controller's own ternary.
The migration itself checks out — owner, trackable, recipient, key and the parameters[:note] shape match Auditor::Audit#log_with_note at both call sites, and the sponsor partials read the note unchanged.
| TRACKING_STARTED_ON = Date.new(2026, 9, 8).freeze | ||
|
|
||
| def self.record(actor:, key:, trackable: nil, recipient: nil) | ||
| def self.record(actor:, key:, trackable: nil, recipient: nil, contact_email: nil) |
There was a problem hiding this comment.
This method is now 11 lines against Metrics/MethodLength Max 10, so the lint job fails. Pulling the hash into a local keeps the create! call on one line:
params = contact_email ? { note: contact_email } : {}
PublicActivity::Activity.create!(owner: actor, key:, trackable:, recipient:, parameters: params)|
|
||
| def self.record(actor:, key:, trackable: nil, recipient: nil) | ||
| def self.record(actor:, key:, trackable: nil, recipient: nil, contact_email: nil) | ||
| Rails.logger.warn("MemberActivityRecorder: unregistered key '#{key}'") unless KEYS.include?(key) |
There was a problem hiding this comment.
Both migrated call sites pass keys that KEYS does not list: sponsor.admin_contact_subscribe, sponsor.admin_contact_unsubscribe, sponsor.contact_subscribe and sponsor.contact_unsubscribe. Every consent toggle writes unregistered key here. That warning is the one signal for a genuinely unregistered key.
Adding the four keys follows the comment on KEYS: "extend when adding sites".
| key:, | ||
| trackable:, | ||
| recipient:, | ||
| parameters: contact_email ? { note: contact_email } : {} |
There was a problem hiding this comment.
Nothing in spec/services/member_activity_recorder_spec.rb touches parameters, so this branch is only exercised through the controller. Two examples there would pin both sides: one with contact_email: expecting { note: email }, one without expecting {}.
|
|
||
| describe 'PATCH #update' do | ||
| it 'passes the contact and email to the activity recorder' do | ||
| expect(MemberActivityRecorder).to receive(:record).with( |
There was a problem hiding this comment.
Two problems with this example.
expect(...).to receive trips RSpec/MessageSpies, so the lint job fails. It also restates what the DB-backed examples below already prove through the real recorder (owner, trackable, parameters[:note]), while coupling the test to the exact keyword list, so a harmless new optional kwarg would break it.
Dropping it is enough. If the intent is to pin that no recipient: is passed, assert activity.recipient.nil? in the subscribe example below instead.
| contact = sponsor.contacts.first | ||
| audit = Auditor::Audit.new(sponsor, 'sponsor.admin_contact_subscribe', manager, contact) | ||
| audit.log_with_note(contact.email) | ||
| contact_audit_key = contact.mailing_list_consent ? 'subscribe' : 'unsubscribe' |
There was a problem hiding this comment.
This mirrors the controller's own ternary, so a key-derivation regression there would still satisfy the rendering assertions below. Hardcode it here (sponsor.admin_contact_subscribe, matching the fabricator default) and leave derivation to the form-driven scenarios. Same for the deleted-contact setup further down.
…ember-activity-recorder
No description provided.