Conversation
|
| key = [info.installment_id, info.purchase.email.to_s.downcase] | ||
| next if key.last.blank? | ||
| # TODO(send_origin_snapshot): replace the editable draft marker with immutable send-time provenance. | ||
| origin = info.installment.json_data[SUGGESTED_EMAIL_KEY].present? ? :suggested : :hand_written |
There was a problem hiding this comment.
This classification depends entirely on marketing_launch_written_copy_digest, but this branch has no production writer for that marker; only the test sets it manually. As a result, production deliveries are all classified as hand_written, leaving the suggested-email safety denominator empty and making the suggested-versus-hand-written read-out unusable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/services/marketing/metrics_report.rb
Line: 147
Comment:
**Suggested origin is never set**
This classification depends entirely on `marketing_launch_written_copy_digest`, but this branch has no production writer for that marker; only the test sets it manually. As a result, production deliveries are all classified as `hand_written`, leaving the suggested-email safety denominator empty and making the suggested-versus-hand-written read-out unusable.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| groups = ARMS.index_with { [] } | ||
| Marketing::HoldoutAssignment.where(prior_sales_bucket: COHORTS.fetch(@cohort)) | ||
| .where("marketing_holdout_assigned_at < ?", @window_end).find_each do |assignment| | ||
| groups[assignment.marketing_holdout? ? :holdout : :treatment] << seller_metrics(assignment) |
There was a problem hiding this comment.
The report processes every historical holdout assignment and runs several seller-specific queries for each one, including delivery and cart-recovery queries for every assignment. Because assignments accumulate and both cohorts run serially, the weekly job's query count and database load grow by multiple round trips per assigned creator. Please aggregate or batch these metrics so the job remains practical as the assignment table grows.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/services/marketing/metrics_report.rb
Line: 29
Comment:
**Report work grows unbounded**
The report processes every historical holdout assignment and runs several seller-specific queries for each one, including delivery and cart-recovery queries for every assignment. Because assignments accumulate and both cohorts run serially, the weekly job's query count and database load grow by multiple round trips per assigned creator. Please aggregate or batch these metrics so the job remains practical as the assignment table grows.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Moved this to the Gumclaw cron |
Stage
What
Adds
Marketing::MetricsReport, weekly cohort snapshots, and a queryable plain-text read. Draft handoff for Gianfranco; do not merge automatically. Tracks antiwork/gumroad-private#2723.Why
Compare treatment with the frozen creator holdout without dropping creators who never use a marketing action. Missing telemetry must not masquerade as a safe rollout: complaint counts remain unavailable and
blocks_rolloutstays true until the safety data is complete.How
Monday 12:00 UTC on the low queue; a trailing 90-day, end-exclusive UTC window. The existing dashboard is a static generated site, not a marketing POST endpoint, so this writes one
Marketing::MetricsSnapshotper window/cohort. No Telegram integration in Rails. Existing assignments and feature gates are untouched.email_infos/sent_post_emailsprovide a separate email-linked count.price_centsminus effective partial refunds; full refunds, unreversed chargebacks and bundle child duplicates excluded through existing scopes. Includes subscription revenue and zero-activity creators assigned by window start. Refund state is current at execution, not historical as-of reconstruction.The buyer/email/cart tests are unadjusted exploratory read-outs, not independent creator-randomized causal estimates. Cohorts are frozen
zeroversusunder_100+at_least_100; newer assignments are counted separately rather than given partial 90-day outcomes.Sibling contracts inspected: #7734 uses
installments.json_data.marketing_launch_written_copy_digest; #7735 persistsWorkflow.first_published_atbut no auto-enabled origin; #7724 adds no metric columns. Named TODO hooks cover immutable send origin, timestamped complaint events, and auto-enabled workflow provenance; no guessed columns or holdout changes.Before / after
Before: no durable five-read-out report. After: weekly aggregate JSON and plain text available through
Marketing::MetricsSnapshot.order(window_end: :desc).first.report_text. Walkthrough evidence pending.Test results
bin/rspec spec/services/marketing/metrics_report_spec.rb spec/services/marketing/metrics_statistics_spec.rb spec/sidekiq/marketing/weekly_metrics_report_job_spec.rb: 26 examples, 0 failures, with isolated test services.bin/rubocop -ais absent in this repository; equivalentbundle exec rubocop -apassed on all eight changed Ruby implementation/spec files.bin/check-migration-versions: passed; the new-table migration was exercised locally.bin/test-confidence: exit 0; seven selected files passed. It skipped a request-spec failure as “likely pre-existing (unverified)”; that skip is not claimed as a verified baseline.Mutation proofs
Pending execution before final draft handoff.
Review focus
Weekly read message contract (folded in 2026-09-17)
The delivered surface is the
no_agentcronmarketing-holdout-metrics-weekly(script stdoutverbatim), so the message shape lives in the renderer, not in Rails. The whole post is:
Auto-marketing weekly read — <window end>; assigned <treatment>/<holdout> (holdout N%).Assignments are N days old; first read with real comparisons lands <date>.7-day first sale: 4.2% (n=312) vs 3.4% (n=88), p=0.31.Not yet readable: cart recovery (need ≥25 creators per arm with a complete 7-day window); 90-day repeat purchase, 90-day net revenue (need ≥25 creators per arm past 90 days); unsubscribe proxy (needs complete delivery telemetry).(+1 point for 7-day first sale and 90-day repeat purchase, +$1.00/creator for net revenue,
two-sided p < 0.05) or when complete unsubscribe/complaint telemetry deteriorates — that gate
blocks rollout.
A metric is readable only when both arms have at least 25 creators with complete follow-up;
below that it is omitted rather than printed as a near-empty rate. With data in both cohorts a
metric gets one labelled line per cohort (
(new creators)/(existing sellers)); with data inone cohort it is reported unlabelled instead of repeating every metric twice. No unavailable-vs-
unavailable rows, no "unavailable is not zero" disclaimers and no methodology footer — the
method is the table above.
Dry run — what the next fire posts (2026-09-17 data, rendered from the audited snapshot, no production query)
Two lines, down from roughly forty. The date is the first Monday 12:00 UTC fire at least seven
complete days after the earliest assignment.
--dry-runrenders the latest snapshot and writesnothing; the weekly read itself is an audited, read-only production query. 18 Python tests cover
the message contract, the readability floor, the verdict gate, fail-closed parsing and the UTC
schedule.