fix(templates): harden legacy-migration job per Sourcery review (EVO-1719)#149
Merged
dpaes merged 1 commit intoJun 15, 2026
Conversation
…1719) Addresses the EVO-1234 [6.5] migration-job review (Sourcery follow-up 6.10): - N+1: eager-load :channel in the find_in_batches query (keep_channel_bound? reads source.channel per WhatsApp row). - Rollback blast-radius: scope rollback_legacy_migration to this migration's own provenance prefix (external_legacy_id LIKE 'message_template:%', reusing LEGACY_KEY_PREFIX) so foreign-tagged rows are never deleted. The rollback spec mirrors the filter and asserts a foreign-provenance row survives (guards against a vacuous test). - Consistency: promote the :invalid and :error diagnostic buckets to REASON_VALIDATION_FAILED / REASON_ERROR constants (named distinctly from the existing REASON_INVALID_CONTENT), used in the job, the rake :error gate, and the spec. Symbol values unchanged. require 'set' (4th point) is intentionally NOT added: it triggers Lint/RedundantRequireStatement under Ruby 3.4 (Set is autoloaded), matching Sourcery's own "não procede" verdict. 13/13 specs green; no new RuboCop offenses.
Reviewer's GuideHardens the legacy-to-MessageTemplate migration job and its rake tasks per Sourcery feedback by preloading associations to avoid N+1 queries, tightening rollback scoping to only this job’s provenance keys, and standardizing skip-reason constants and their usage in the job, rake task, and specs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The prefix-based provenance filter (
where('external_legacy_id LIKE ?', "#{prefix}:%")) is now duplicated across the rake task and specs; consider extracting it into a named scope or helper onMessageTemplateto centralize the logic and avoid future drift. - The use of plain hashes for
summary[:skipped]keys now spread across job, rake task, and specs could be further hardened by adding a small value-object or helper method to encapsulate skip-reason handling and reduce reliance on raw symbol keys.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The prefix-based provenance filter (`where('external_legacy_id LIKE ?', "#{prefix}:%")`) is now duplicated across the rake task and specs; consider extracting it into a named scope or helper on `MessageTemplate` to centralize the logic and avoid future drift.
- The use of plain hashes for `summary[:skipped]` keys now spread across job, rake task, and specs could be further hardened by adding a small value-object or helper method to encapsulate skip-reason handling and reduce reliance on raw symbol keys.
## Individual Comments
### Comment 1
<location path="lib/tasks/templates.rake" line_range="36-37" />
<code_context>
+ # (external_legacy_id LIKE 'message_template:%'); channel-bound originals,
+ # admin-created globals (external_legacy_id IS NULL), and any rows tagged by
+ # a future unrelated integration are never touched.
+ prefix = MigrateLegacyTemplatesToMessageTemplateJob::LEGACY_KEY_PREFIX
+ scope = MessageTemplate.where('external_legacy_id LIKE ?', "#{prefix}:%")
count = scope.count
scope.delete_all
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider escaping the prefix for SQL LIKE to avoid future wildcard surprises.
If `LEGACY_KEY_PREFIX` ever includes `%` or `_`, this `LIKE` will treat them as wildcards and could delete unintended rows. Consider escaping the prefix first (e.g. via `ActiveRecord::Base.send(:sanitize_sql_like, prefix)`) before appending `:%` so only the intended namespace is matched.
</issue_to_address>
### Comment 2
<location path="spec/jobs/migrate_legacy_templates_to_message_template_job_spec.rb" line_range="153-159" />
<code_context>
- # Mirrors lib/tasks/templates.rake rollback_legacy_migration.
- MessageTemplate.where.not(external_legacy_id: nil).delete_all
+ # Mirrors lib/tasks/templates.rake rollback_legacy_migration (prefix-scoped).
+ MessageTemplate.where('external_legacy_id LIKE ?', "#{described_class::LEGACY_KEY_PREFIX}:%").delete_all
expect(MessageTemplate.exists?(admin.id)).to be(true)
expect(MessageTemplate.exists?(source.id)).to be(true)
- expect(globals.where.not(external_legacy_id: nil).count).to eq(0)
+ expect(MessageTemplate.exists?(foreign.id)).to be(true)
+ expect(globals.where('external_legacy_id LIKE ?', "#{described_class::LEGACY_KEY_PREFIX}:%").count).to eq(0)
end
end
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a dedicated spec for the `templates:rollback_legacy_migration` rake task to avoid test drift from the job spec.
Because this example now mirrors the rake task, changes to the rake implementation could introduce bugs that the job spec would also mirror, hiding the issue. To reduce this risk, add a small rake-task spec (or shared example used by both specs) that directly invokes `Rake::Task['templates:rollback_legacy_migration'].invoke` and asserts: admin globals kept, channel-bound originals kept, foreign-provenance globals kept, and only `LEGACY_KEY_PREFIX` globals deleted. This helps ensure the tests stay aligned with the real rake behavior over time.
Suggested implementation:
```ruby
shared_examples 'legacy template rollback scope' do
it 'deletes only this migration\'s globals, leaving originals, admin globals, and foreign-provenance rows' do
admin = MessageTemplate.create!(channel: nil, name: 'Kept', content: 'admin')
source = coupled_template(channel: baileys, name: 'Migrated')
# A global tagged by a hypothetical OTHER integration. The old unscoped
# rollback (where.not(external_legacy_id: nil)) would have wrongly deleted
# this; the prefix-scoped delete must spare it. Without this row the test
# is vacuous — it would pass against the old scope too.
```
```ruby
foreign = MessageTemplate.create!(
channel: nil,
name: 'Foreign',
content: 'x',
external_legacy_id: 'other_integration:1'
)
perform_rollback.call
expect(MessageTemplate.exists?(admin.id)).to be(true)
expect(MessageTemplate.exists?(source.id)).to be(true)
expect(MessageTemplate.exists?(foreign.id)).to be(true)
expect(
globals.where(
'external_legacy_id LIKE ?',
"#{described_class::LEGACY_KEY_PREFIX}:%"
).count
).to eq(0)
end
end
describe 'rollback scope (AC7)' do
let(:perform_rollback) do
lambda do
described_class.new.perform
# Mirrors lib/tasks/templates.rake rollback_legacy_migration (prefix-scoped).
MessageTemplate.where(
'external_legacy_id LIKE ?',
"#{described_class::LEGACY_KEY_PREFIX}:%"
).delete_all
end
end
it_behaves_like 'legacy template rollback scope'
```
To fully implement your suggestion and de-couple the job spec from the rake task implementation, add a new spec file, e.g. `spec/tasks/templates_rake_spec.rb`, that:
1. Loads rake tasks (commonly via `Rails.application.load_tasks` or your existing helper).
2. Defines `let(:perform_rollback) { -> { Rake::Task['templates:rollback_legacy_migration'].reenable && Rake::Task['templates:rollback_legacy_migration'].invoke } }`.
3. Includes the shared example with `it_behaves_like 'legacy template rollback scope'` so it runs the same expectations as the job spec, but against the real rake task.
4. Ensures the task is re-enabled between examples if there are multiple invocations.
This keeps the shared behavior in one place while directly exercising the rake task, reducing the risk of the job spec drifting alongside the rake implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+36
to
+37
| prefix = MigrateLegacyTemplatesToMessageTemplateJob::LEGACY_KEY_PREFIX | ||
| scope = MessageTemplate.where('external_legacy_id LIKE ?', "#{prefix}:%") |
There was a problem hiding this comment.
issue (bug_risk): Consider escaping the prefix for SQL LIKE to avoid future wildcard surprises.
If LEGACY_KEY_PREFIX ever includes % or _, this LIKE will treat them as wildcards and could delete unintended rows. Consider escaping the prefix first (e.g. via ActiveRecord::Base.send(:sanitize_sql_like, prefix)) before appending :% so only the intended namespace is matched.
Comment on lines
+153
to
+159
| # Mirrors lib/tasks/templates.rake rollback_legacy_migration (prefix-scoped). | ||
| MessageTemplate.where('external_legacy_id LIKE ?', "#{described_class::LEGACY_KEY_PREFIX}:%").delete_all | ||
|
|
||
| expect(MessageTemplate.exists?(admin.id)).to be(true) | ||
| expect(MessageTemplate.exists?(source.id)).to be(true) | ||
| expect(globals.where.not(external_legacy_id: nil).count).to eq(0) | ||
| expect(MessageTemplate.exists?(foreign.id)).to be(true) | ||
| expect(globals.where('external_legacy_id LIKE ?', "#{described_class::LEGACY_KEY_PREFIX}:%").count).to eq(0) |
There was a problem hiding this comment.
suggestion (testing): Consider adding a dedicated spec for the templates:rollback_legacy_migration rake task to avoid test drift from the job spec.
Because this example now mirrors the rake task, changes to the rake implementation could introduce bugs that the job spec would also mirror, hiding the issue. To reduce this risk, add a small rake-task spec (or shared example used by both specs) that directly invokes Rake::Task['templates:rollback_legacy_migration'].invoke and asserts: admin globals kept, channel-bound originals kept, foreign-provenance globals kept, and only LEGACY_KEY_PREFIX globals deleted. This helps ensure the tests stay aligned with the real rake behavior over time.
Suggested implementation:
shared_examples 'legacy template rollback scope' do
it 'deletes only this migration\'s globals, leaving originals, admin globals, and foreign-provenance rows' do
admin = MessageTemplate.create!(channel: nil, name: 'Kept', content: 'admin')
source = coupled_template(channel: baileys, name: 'Migrated')
# A global tagged by a hypothetical OTHER integration. The old unscoped
# rollback (where.not(external_legacy_id: nil)) would have wrongly deleted
# this; the prefix-scoped delete must spare it. Without this row the test
# is vacuous — it would pass against the old scope too. foreign = MessageTemplate.create!(
channel: nil,
name: 'Foreign',
content: 'x',
external_legacy_id: 'other_integration:1'
)
perform_rollback.call
expect(MessageTemplate.exists?(admin.id)).to be(true)
expect(MessageTemplate.exists?(source.id)).to be(true)
expect(MessageTemplate.exists?(foreign.id)).to be(true)
expect(
globals.where(
'external_legacy_id LIKE ?',
"#{described_class::LEGACY_KEY_PREFIX}:%"
).count
).to eq(0)
end
end
describe 'rollback scope (AC7)' do
let(:perform_rollback) do
lambda do
described_class.new.perform
# Mirrors lib/tasks/templates.rake rollback_legacy_migration (prefix-scoped).
MessageTemplate.where(
'external_legacy_id LIKE ?',
"#{described_class::LEGACY_KEY_PREFIX}:%"
).delete_all
end
end
it_behaves_like 'legacy template rollback scope'To fully implement your suggestion and de-couple the job spec from the rake task implementation, add a new spec file, e.g. spec/tasks/templates_rake_spec.rb, that:
- Loads rake tasks (commonly via
Rails.application.load_tasksor your existing helper). - Defines
let(:perform_rollback) { -> { Rake::Task['templates:rollback_legacy_migration'].reenable && Rake::Task['templates:rollback_legacy_migration'].invoke } }. - Includes the shared example with
it_behaves_like 'legacy template rollback scope'so it runs the same expectations as the job spec, but against the real rake task. - Ensures the task is re-enabled between examples if there are multiple invocations.
This keeps the shared behavior in one place while directly exercising the rake task, reducing the risk of the job spec drifting alongside the rake implementation.
dpaes
approved these changes
Jun 15, 2026
dpaes
left a comment
dpaes
left a comment
Contributor
There was a problem hiding this comment.
Approved. Clean, well-scoped follow-up. Verified all four Sourcery points against origin/develop + the PR head:
- N+1 →
includes(:channel)✅keep_channel_bound?readssource.channel(WhatsApp rows only);belongs_to :channel, polymorphic: true, optional: true→includesdoes a polymorphic preload and preserves thefind_in_batchesPK order. N+1 gone. - Rollback blast-radius ✅ Now scoped to
external_legacy_id LIKE 'message_template:%'reusingLEGACY_KEY_PREFIX(matcheslegacy_key). The spec is non-vacuous: it seeds a foreign-provenance row (other_integration:1) and asserts it survives — the old unscoped delete would have wrongly removed it. require 'set'⚪ Correctly NOT added —Setis autoloaded on Ruby 3.2+ and the require would tripLint/RedundantRequireStatement, matching Sourcery's own "não procede" verdict.- Reason constants ✅
:invalid/:errorpromoted toREASON_VALIDATION_FAILED/REASON_ERRORacross job + rake gate + spec. Symbol values unchanged, so the summary buckets andreason.to_smetrics stay backward-compatible; no other reader of the raw bucket exists in the repo.
Non-blocking nit (cosmetic): the _ chars in 'message_template:%' are single-char LIKE wildcards, so it could in theory match messageXtemplate:.... Practically harmless (no plausible foreign prefix matches; rollback is a manual operator task) — no follow-up needed.
No schema/behavior change. Note: this repo's CI doesn't run RSpec, so the 13/13 green run is the author's local result.
dpaes
deleted the
danilocarneiro/evo-1719-follow-up-610-4-pontos-do-review-sourcery-no-job-de-migracao
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Non-blocking follow-up (6.10) to the Sourcery review of the EVO-1234 [6.5] legacy →
MessageTemplatemigration job. Four points raised; three applied, one intentionally declined.MessageTemplate.where.not(channel_id: nil).find_in_batcheshad no preload, sokeep_channel_bound?issued a per-rowsource.channelquery for every WhatsApp row. Added.includes(:channel). (channelis a polymorphicbelongs_to;includeskeeps it a preload and preservesfind_in_batches' defaultORDER BY id.)rollback_legacy_migrationdeletedwhere.not(external_legacy_id: nil). The column is exclusive to this migration today, but a future integration reusing it would be wiped. Scoped to this migration's provenance prefix:external_legacy_id LIKE 'message_template:%', reusingMigrateLegacyTemplatesToMessageTemplateJob::LEGACY_KEY_PREFIX. The rollback spec mirrors the filter and now seeds a foreign-provenance row (other_integration:1) that must survive — without it the test was vacuous (passed against the old scope too).:invalidand:errorescaped theREASON_*convention. Promoted toREASON_VALIDATION_FAILED/REASON_ERROR(used in the job, the rake:errorgate, and the spec). NamedREASON_VALIDATION_FAILED, notREASON_INVALID, to avoid a near-synonym collision with the existingREASON_INVALID_CONTENT. Symbol values are unchanged, and the comment's deliberate "documented taxonomy vs. diagnostic buckets" two-tier distinction is preserved.require 'set'. Not added. Under Ruby 3.4Setis autoloaded andrequire 'set'tripsLint/RedundantRequireStatement(verified: 1 offense, autocorrectable) — adding it would introduce a lint offense. This matches Sourcery's own "não procede / inócuo" verdict on this point.No migration, no schema change, no behavior change.
Test plan
POSTGRES_PORT=5433 ... bundle exec rspec spec/jobs/migrate_legacy_templates_to_message_template_job_spec.rb→ 13 examples, 0 failures.where.not(external_legacy_id: nil)makes the example FAIL on the foreign-provenance assertion — confirming the test now distinguishes the new scope.developbaseline → zero new offenses;Lint/RedundantRequireStatement= 0.Notes
develop.require 'set'decision is the only one of the 4 points not coded; rationale above is the resolution for the Sourcery thread.Summary by Sourcery
Harden the legacy-to-MessageTemplate migration job and rollback task based on Sourcery review feedback, tightening rollback scoping, naming skip reasons, and reducing query overhead.
Bug Fixes:
Enhancements: