Skip to content

Commit fc6c9bc

Browse files
npfloodUbuntu
andauthored
Add plants:sync_plants, and stop treating convergence as conflict (#133)
Two things the first real run on staging produced. The rake task drives EcPlantFeed, which shipped without one. It refuses to run when any plant in the payload is unlinked, because that is the condition under which SourceSynchronizer creates duplicates instead of matching - the failure mode worth an explicit guard rather than a comment. The synchronizer fix is the interesting one. Its decision table sent "both sides changed" straight to a conflict without checking whether they changed to the SAME value. On the staging run that put 1 of 2 conflicts into the review pile for a record where local_payload and incoming_payload were byte-identical: an earlier migration had written the same text into ECHOcommunity and into the API separately, so both had moved from the 2020 base and arrived at the same place. There is nothing for a reviewer to decide there, and a review pile that contains non-decisions teaches people the pile is noise. Convergence now adopts the value and scores synced. A spec pins it, verified to fail on the previous code. Staging rehearsal, 723 plants: 60 applied, 650 already identical, 1 local edit kept, 2 conflicts - of which this fix removes the spurious one. Zero created, so no duplicates. Co-authored-by: Ubuntu <ubuntu@ip-10-2-1-225.ec2.internal>
1 parent f6b4772 commit fc6c9bc

4 files changed

Lines changed: 116 additions & 1 deletion

File tree

.rubocop.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ Metrics/BlockLength:
2222
- 'lib/tasks/plant_import.rake'
2323
- 'lib/tasks/sync.rake'
2424
- 'lib/tasks/plant_translations.rake'
25+
- 'lib/tasks/plant_sync.rake'
2526
- 'lib/tasks/plant_categories.rake'
2627
Metrics/MethodLength:
2728
Exclude:

app/services/source_synchronizer.rb

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,23 @@ def compare_and_sync(record, incoming_attrs, src_at, report)
229229
local_changed = base_digest != local_digest
230230
incoming_changed = base_digest != incoming_digest
231231

232-
if !local_changed && !incoming_changed
232+
# Convergence, not conflict. Both sides moved from the base, but they moved
233+
# to the SAME value - two people making the same correction independently,
234+
# or an earlier migration writing the same text into both systems. There is
235+
# nothing for a reviewer to decide, so adopting it is the only sensible
236+
# answer; raising a conflict here would put converged records in the review
237+
# pile and teach people that the pile is noise.
238+
if local_changed && incoming_changed && local_digest == incoming_digest
239+
record.update_columns(
240+
'source_snapshot' => incoming_attrs,
241+
'source_digest' => incoming_digest,
242+
'source_updated_at' => src_at,
243+
'last_synced_at' => Time.current,
244+
'sync_state' => 'synced'
245+
)
246+
report.synced += 1
247+
248+
elsif !local_changed && !incoming_changed
233249
# unchanged / unchanged -- touch only
234250
record.update_columns(
235251
last_synced_at: Time.current,

lib/tasks/plant_sync.rake

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
# frozen_string_literal: true
2+
3+
require Rails.root.join('lib/ec_data_source')
4+
require Rails.root.join('lib/ec_plant_feed')
5+
6+
# Runs the ECHOcommunity narrative feed through SourceSynchronizer. The
7+
# reasoning lives in lib/ec_plant_feed.rb; the short version is that this is the
8+
# ENGLISH half of reconciliation, where the genuine conflicts are. Other locales
9+
# are additive and go through plants:backfill_translations instead.
10+
#
11+
# bin/rails plants:sync_plants[tmp/feed.json] # dry run
12+
# APPLY=true bin/rails plants:sync_plants[tmp/feed.json] # write
13+
#
14+
# Payload: {"plants": {"<uuid>": {"uses": "...", ...}}} — flat, one locale, as
15+
# emitted by tools/export_ec_narrative.py --for feed.
16+
namespace :plants do
17+
desc 'Sync English plant narrative from ECHOcommunity (dry run unless APPLY=true)'
18+
task :sync_plants, [:path] => :environment do |_t, args|
19+
path = args[:path] or abort 'usage: bin/rails plants:sync_plants[path/to/feed.json]'
20+
abort "file not found: #{path}" unless File.exist?(path)
21+
22+
data_source = EcDataSource.existing or abort 'run sync:bootstrap first'
23+
plants = JSON.parse(File.read(path))['plants'] or abort "no 'plants' key in #{path}"
24+
apply = ENV['APPLY'] == 'true'
25+
run_id = ENV['RUN_ID'].presence || SecureRandom.hex(8)
26+
27+
unlinked = Plant.unscoped.where(id: plants.keys, data_source_id: nil).count
28+
if unlinked.positive?
29+
abort "#{unlinked} of these plants are not linked to a data source. " \
30+
'Run sync:link_plants first, or the synchronizer will create duplicates.'
31+
end
32+
33+
puts "#{apply ? 'SYNCING' : 'DRY RUN'} #{plants.size} plants, locale en"
34+
puts " data source: #{data_source.name} (#{data_source.id})"
35+
puts " run id: #{run_id}"
36+
puts
37+
38+
result = EcPlantFeed.new(data_source: data_source, run_id: run_id)
39+
.run(plants, apply: apply)
40+
report = result.report
41+
unless report
42+
puts " rows built: #{result.rows.size} (dry run, nothing sent)"
43+
next
44+
end
45+
46+
puts " applied (upstream won): #{report.applied}"
47+
puts " synced (already identical): #{report.synced}"
48+
puts " locally modified (local kept): #{report.locally_modified}"
49+
puts " CONFLICTS raised for review: #{report.conflicts_created}"
50+
puts " conflicts refreshed: #{report.conflicts_updated}"
51+
puts " created (new upstream record): #{report.created}"
52+
puts " invalid: #{report.invalid}"
53+
puts " errored: #{report.errored}"
54+
report.invalid_details.first(10).each { |d| puts " invalid: #{d}" }
55+
report.error_details.first(10).each { |d| puts " error: #{d}" }
56+
puts
57+
puts " open conflicts now: #{SyncConflict.where(status: 'open').count}"
58+
abort 'sync finished with errors' if report.errored.positive?
59+
end
60+
end

spec/services/source_synchronizer_spec.rb

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,4 +472,42 @@ def synced_plant(src_id:, scientific_name: 'Moringa oleifera', family_names: 'Mo
472472
expect(svc_principal.email).to eq "sync+#{data_source.source_system_key}@plant-api.echocommunity.org"
473473
end
474474
end
475+
476+
# Regression from the first real run: two of 723 plants raised conflicts, and
477+
# one of them had local_payload identical to incoming_payload. Both sides had
478+
# moved from the 2020 base to the same text - an earlier migration had written
479+
# it into ECHOcommunity and into the API separately. There was nothing to
480+
# decide, but it still landed in the review pile.
481+
describe 'when both sides changed to the same value' do
482+
let(:org) { create(:organization, :real) }
483+
let(:data_source) { create(:data_source, organization: org) }
484+
485+
def sync(attrs)
486+
described_class.new(data_source: data_source, model: Plant,
487+
source_attributes: attrs, run_id: SecureRandom.hex(4))
488+
end
489+
490+
it 'treats it as converged rather than a conflict' do
491+
plant = create(:plant, owner_organization_id: org.id, source_organization_id: org.id,
492+
scientific_name: 'Agreed name')
493+
plant.update_columns(data_source_id: data_source.id, source_record_id: 'conv-1',
494+
source_snapshot: { 'scientific_name' => 'The old base' },
495+
source_digest: described_class.canonical_digest(
496+
{ 'scientific_name' => 'The old base' }
497+
))
498+
499+
report = sync(%w[scientific_name]).apply(
500+
[{ source_record_id: 'conv-1', deleted: false,
501+
attributes: { 'scientific_name' => 'Agreed name' },
502+
source_updated_at: 1.hour.ago }]
503+
)
504+
505+
aggregate_failures do
506+
expect(report.conflicts_created).to eq(0), 'both sides agree; nothing to review'
507+
expect(report.synced).to eq 1
508+
expect(plant.reload.sync_state).to eq 'synced'
509+
expect(plant.source_snapshot).to eq('scientific_name' => 'Agreed name')
510+
end
511+
end
512+
end
475513
end

0 commit comments

Comments
 (0)