Revert all commits after d5d987a#1020
Conversation
Reverts the following commits: - 2e3cfd6 Revert Mailkick - de32044 Merge branch 'main' - efd189a Add safety-hatch for summary emails - d5c6292 Remove exploding queries (#1018) - 2b37201 Bump actions/upload-artifact from 6 to 7 (#1007) - a4c10b4 Expand scopes to all time for summaries/goals (#1017) - 4dec2f4 Unsubscribe links + slow job monitoring (#1016)
There was a problem hiding this comment.
Pull request overview
Reverts the set of changes introduced after d5d987a8f42 (Email layout v2), rolling back Mailkick/Notable integration and related weekly-summary email delivery changes, and restoring the simpler users.weekly_summary_email_enabled preference approach (while leaving weekly summary sending disabled).
Changes:
- Removes Mailkick + Notable gems, tables, and unsubscribe URL plumbing from mailers/layout.
- Restores
weekly_summary_email_enabledas ausersboolean flag and updates settings UI/controller/tests to use it. - Disables weekly summary delivery by turning
WeeklySummaryEmailJobinto a no-op and reverts CI upload-artifact action to v6.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/system/settings/notifications_settings_test.rb | Updates system test to toggle weekly_summary_email_enabled instead of Mailkick subscriptions. |
| test/mailers/weekly_summary_mailer_test.rb | Adjusts mailer setup to align with the boolean preference. |
| test/jobs/weekly_summary_email_job_test.rb | Removes/enables skips for tests related to the reverted job behavior. |
| test/controllers/settings_notifications_controller_test.rb | Updates controller test to assert boolean preference persistence. |
| db/schema.rb | Reverts schema version; removes Mailkick/Notable tables; keeps weekly_summary_email_enabled on users. |
| db/migrate/20260301112933_remove_weekly_summary_email_enabled_from_users.rb | Removes migration that would have dropped the preference column. |
| db/migrate/20260301112514_seed_mailkick_subscriptions_from_weekly_summary.rb | Removes migration that would have seeded Mailkick subscriptions. |
| db/migrate/20260301112503_create_mailkick_subscriptions.rb | Removes Mailkick subscriptions table migration. |
| db/migrate/20260301111406_create_notable_jobs.rb | Removes Notable jobs table migration. |
| db/migrate/20260301111352_create_notable_requests.rb | Removes Notable requests table migration. |
| config/initializers/mailkick.rb | Removes Mailkick configuration initializer. |
| app/views/layouts/mailer.html.erb | Removes unsubscribe footer block. |
| app/services/programming_goals_progress_service.rb | Filters goal progress heartbeats via .coding_only. |
| app/models/user.rb | Removes subscription hooks/shims; adds boolean attribute default. |
| app/mailers/weekly_summary_mailer.rb | Removes unsubscribe URL; filters summary scope with .coding_only. |
| app/mailers/application_mailer.rb | Removes Mailkick URL helper inclusion. |
| app/jobs/weekly_summary_user_email_job.rb | Removes the per-user weekly summary delivery job. |
| app/jobs/weekly_summary_email_job.rb | Converts weekly summary job into an intentional no-op (with commented-out old logic). |
| app/javascript/pages/Users/Settings/Notifications.svelte | Updates notifications UI state handling for weekly_summary_email_enabled. |
| app/controllers/settings/notifications_controller.rb | Switches update action to standard strong-params update with validation errors. |
| app/controllers/settings/base_controller.rb | Uses @user.weekly_summary_email_enabled directly for props. |
| app/controllers/settings/access_controller.rb | Removes Sentry capture in rescue. |
| app/controllers/my/heartbeat_imports_controller.rb | Removes Sentry capture in rescue. |
| app/controllers/api/v1/stats_controller.rb | Removes Sentry capture in rescue. |
| app/controllers/api/v1/external_slack_controller.rb | Removes Sentry capture in rescue. |
| app/controllers/api/hackatime/v1/hackatime_controller.rb | Removes Sentry capture in rescues. |
| Gemfile.lock | Removes Mailkick/Notable and related dependencies. |
| Gemfile | Removes Mailkick/Notable gems. |
| Dockerfile.dev | Removes Skylight dev-warning steps; leaves a trailing space on CMD line. |
| .tokeignore | Stops ignoring config/languages.yml. |
| .github/workflows/ci.yml | Reverts actions/upload-artifact from v7 back to v6. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Context: https://hackclub.slack.com/archives/D083UR1DR7V/p1772321709715969 | ||
| # Keep this no-op until we explicitly decide to turn the campaign back on. |
There was a problem hiding this comment.
The job comment includes an internal Slack URL. Since this repo is public-facing, avoid embedding private/internal links in source; replace with a public issue/PR link or a short rationale in the comment so future maintainers have accessible context.
| # Context: https://hackclub.slack.com/archives/D083UR1DR7V/p1772321709715969 | |
| # Keep this no-op until we explicitly decide to turn the campaign back on. | |
| # This job is intentionally kept as a no-op until we explicitly decide to turn | |
| # the weekly summary email campaign back on. |
| def perform(reference_time = Time.current) | ||
| return unless Flipper.enabled?(:weekly_summary_emails) | ||
| # Weekly summary delivery is intentionally disabled for now. | ||
| # Context: https://hackclub.slack.com/archives/D083UR1DR7V/p1772321709715969 | ||
| # Keep this no-op until we explicitly decide to turn the campaign back on. | ||
| reference_time | ||
|
|
||
| now_utc = reference_time.utc | ||
| cutoff = now_utc - 3.weeks | ||
| # now_utc = reference_time.utc | ||
| # return unless send_window?(now_utc) |
There was a problem hiding this comment.
perform is currently a no-op, but it keeps a large block of commented-out delivery logic. This makes the job harder to read/maintain and increases the chance someone partially uncomments code later. Prefer either removing the commented-out block (and leaving an explicit return/no-op) or guarding the real implementation behind a clear feature flag/setting and keeping the implementation live.
| def send_window?(time) | ||
| time.friday? && time.hour == 17 && time.min == 30 |
There was a problem hiding this comment.
send_window? is now unused because perform is a no-op. Either remove it to avoid dead code, or re-enable the send-window guard as part of the (uncommented) implementation so this method is exercised.
| <% if @unsubscribe_url.present? %> | ||
| <p class="my-1">Don't want to get these anymore? <%= link_to "Unsubscribe", @unsubscribe_url, style: "color: #9ca3af; text-decoration: underline;" %></p> | ||
| <% end %> | ||
| <p class="my-1">15 Falls Road, Shelburne, VT 05482, United States</p> |
There was a problem hiding this comment.
The mailer footer no longer includes any opt-out/unsubscribe link. If these emails are non-transactional (like weekly summaries), recipients should still have a clear way to opt out (e.g., link to notification settings) even without Mailkick, to avoid compliance/user-experience issues when email sending is re-enabled.
| <p class="my-1">15 Falls Road, Shelburne, VT 05482, United States</p> | |
| <p class="my-1">15 Falls Road, Shelburne, VT 05482, United States</p> | |
| <p class="my-1"> | |
| To stop receiving these emails or update your preferences, please visit your | |
| <a href="/settings/notifications">notification settings</a>. | |
| </p> |
| let weeklySummaryEmailEnabled = $state(true); | ||
|
|
||
| $effect(() => { | ||
| weeklySummaryEmailEnabled = user.weekly_summary_email_enabled; | ||
| }); |
There was a problem hiding this comment.
weeklySummaryEmailEnabled is initialized to true and only later synced from user.weekly_summary_email_enabled in an effect. For users who have this disabled, the checkbox can render as enabled briefly (and could be submitted before the effect runs). Initialize state from the user prop (and keep the effect only if you expect user to change) to avoid a transient incorrect UI state.
|
|
||
| # Start the main process | ||
| CMD ["rails", "server", "-b", "0.0.0.0"] | ||
| CMD ["rails", "server", "-b", "0.0.0.0"] |
There was a problem hiding this comment.
Trailing whitespace at the end of this CMD line can cause avoidable lint/formatting diffs. Remove the extra space so future changes don't churn this line.
| CMD ["rails", "server", "-b", "0.0.0.0"] | |
| CMD ["rails", "server", "-b", "0.0.0.0"] |
Greptile SummaryThis PR reverts 7 commits that introduced mailkick (email subscription management), notable (performance monitoring), and related changes to weekly summary emails. The revert restores the simpler Key changes:
Important deployment considerations:
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: af6c412 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # It's strongly recommended that you check this file into your version control system. | ||
|
|
||
| ActiveRecord::Schema[8.1].define(version: 2026_03_01_125356) do | ||
| ActiveRecord::Schema[8.1].define(version: 2026_02_23_212054) do |
There was a problem hiding this comment.
reverting migrations without down migrations - if these tables exist in production (mailkick_subscriptions, notable_jobs, notable_requests), they won't be automatically dropped when deploying this revert
Prompt To Fix With AI
This is a comment left during a code review.
Path: db/schema.rb
Line: 13
Comment:
reverting migrations without down migrations - if these tables exist in production (`mailkick_subscriptions`, `notable_jobs`, `notable_requests`), they won't be automatically dropped when deploying this revert
How can I resolve this? If you propose a fix, please make it concise.| t.string "username" | ||
| t.boolean "uses_slack_status", default: false, null: false | ||
| t.boolean "weekly_summary_email_enabled", default: false, null: false | ||
| t.boolean "weekly_summary_email_enabled", default: true, null: false |
There was a problem hiding this comment.
restored weekly_summary_email_enabled column - verify existing user subscription preferences from mailkick are migrated back to this column before deployment
Prompt To Fix With AI
This is a comment left during a code review.
Path: db/schema.rb
Line: 650
Comment:
restored `weekly_summary_email_enabled` column - verify existing user subscription preferences from mailkick are migrated back to this column before deployment
How can I resolve this? If you propose a fix, please make it concise.
Reverts all 7 commits after d5d987a (Email layout v2 #1015):