diff --git a/backend/bills-indexer/internal/adapter/sqlite/writer_test.go b/backend/bills-indexer/internal/adapter/sqlite/writer_test.go index 78d677c9..e92117e7 100644 --- a/backend/bills-indexer/internal/adapter/sqlite/writer_test.go +++ b/backend/bills-indexer/internal/adapter/sqlite/writer_test.go @@ -8,6 +8,7 @@ import ( "time" "epac/bills-indexer/internal/domain" + "epac/bills-indexer/internal/usecase" _ "modernc.org/sqlite" ) @@ -114,6 +115,173 @@ func TestWriterCreatesBillsRelationalSchema(t *testing.T) { } } +func TestWriterStoresMultipleBillVersionDiffPairs(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "bills_pairs.db") + writer := NewWriter(WithClock(fixedClock{})) + + v1 := domain.BillVersion{ + ID: "v1", + SortOrder: 1, + Sections: []domain.VersionSection{ + {Label: "1", Text: "A"}, + {Label: "2", Text: "B"}, + }, + TextHash: ptrString("h1"), + TextSourceURL: ptrString("s1"), + } + v2 := domain.BillVersion{ + ID: "v2", + SortOrder: 2, + Sections: []domain.VersionSection{ + {Label: "1", Text: "A modified"}, + {Label: "2", Text: "B"}, + }, + TextHash: ptrString("h2"), + TextSourceURL: ptrString("s2"), + } + v3 := domain.BillVersion{ + ID: "v3", + SortOrder: 3, + Sections: []domain.VersionSection{ + {Label: "1", Text: "A modified"}, + {Label: "3", Text: "C"}, + }, + TextHash: ptrString("h3"), + TextSourceURL: ptrString("s3"), + } + + // Compute all pairs using the use case policy + diffs := usecase.ComputeBillVersionDiff("C-2", []domain.BillVersion{v1, v2, v3}, "https://example.test/bill") + + bill := domain.Bill{ + ID: "13543613", + Number: "C-2", + Title: "Border bill", + Versions: []domain.BillVersion{v1, v2, v3}, + Diffs: diffs, + } + + stats, err := writer.Write(context.Background(), dbPath, domain.Batch{Bills: []domain.Bill{bill}}) + if err != nil { + t.Fatalf("Write: %v", err) + } + if stats.DiffCount != 3 { + t.Errorf("expected 3 diffs written to stats, got %d", stats.DiffCount) + } + + db, err := sql.Open("sqlite", dbPath) + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + defer db.Close() + + // Assert pair count (should be 3 pairs: v1->v2, v1->v3, v2->v3) + var pairCount int + if err := db.QueryRow("SELECT COUNT(*) FROM bill_diffs WHERE bill_id = ?", "13543613").Scan(&pairCount); err != nil { + t.Fatalf("query bill_diffs count: %v", err) + } + if pairCount != 3 { + t.Fatalf("expected 3 diff pairs, got %d", pairCount) + } + + // Assert stable IDs and presence of adjacent and non-adjacent pairs + expectedPairs := map[string]struct { + from string + to string + }{ + "diff-c-2-v1-v2": {from: "v1", to: "v2"}, + "diff-c-2-v1-v3": {from: "v1", to: "v3"}, + "diff-c-2-v2-v3": {from: "v2", to: "v3"}, + } + + rows, err := db.Query("SELECT id, from_version_id, to_version_id FROM bill_diffs WHERE bill_id = ?", "13543613") + if err != nil { + t.Fatalf("query bill_diffs: %v", err) + } + defer rows.Close() + + for rows.Next() { + var id, fromVal, toVal string + if err := rows.Scan(&id, &fromVal, &toVal); err != nil { + t.Fatalf("scan bill_diff: %v", err) + } + expected, found := expectedPairs[id] + if !found { + t.Errorf("unexpected diff ID stored: %q", id) + continue + } + if expected.from != fromVal || expected.to != toVal { + t.Errorf("mismatched versions for diff %q: expected %s->%s, got %s->%s", id, expected.from, expected.to, fromVal, toVal) + } + } + + // Assert representative bill_clause_diffs rows + // For v1->v2 (adjacent): clause 1 modified, clause 2 unchanged + var v1v2C1Change, v1v2C2Change string + err = db.QueryRow("SELECT change_type FROM bill_clause_diffs WHERE diff_id = ? AND label = ?", "diff-c-2-v1-v2", "1").Scan(&v1v2C1Change) + if err != nil { + t.Fatalf("query v1->v2 clause 1 change: %v", err) + } + if v1v2C1Change != "modified" { + t.Errorf("expected v1->v2 clause 1 to be modified, got %q", v1v2C1Change) + } + err = db.QueryRow("SELECT change_type FROM bill_clause_diffs WHERE diff_id = ? AND label = ?", "diff-c-2-v1-v2", "2").Scan(&v1v2C2Change) + if err != nil { + t.Fatalf("query v1->v2 clause 2 change: %v", err) + } + if v1v2C2Change != "unchanged" { + t.Errorf("expected v1->v2 clause 2 to be unchanged, got %q", v1v2C2Change) + } + + // For v1->v3 (non-adjacent): clause 1 modified, clause 2 removed, clause 3 added + var v1v3C1Change, v1v3C2Change, v1v3C3Change string + err = db.QueryRow("SELECT change_type FROM bill_clause_diffs WHERE diff_id = ? AND label = ?", "diff-c-2-v1-v3", "1").Scan(&v1v3C1Change) + if err != nil { + t.Fatalf("query v1->v3 clause 1 change: %v", err) + } + if v1v3C1Change != "modified" { + t.Errorf("expected v1->v3 clause 1 to be modified, got %q", v1v3C1Change) + } + err = db.QueryRow("SELECT change_type FROM bill_clause_diffs WHERE diff_id = ? AND label = ?", "diff-c-2-v1-v3", "2").Scan(&v1v3C2Change) + if err != nil { + t.Fatalf("query v1->v3 clause 2 change: %v", err) + } + if v1v3C2Change != "removed" { + t.Errorf("expected v1->v3 clause 2 to be removed, got %q", v1v3C2Change) + } + err = db.QueryRow("SELECT change_type FROM bill_clause_diffs WHERE diff_id = ? AND label = ?", "diff-c-2-v1-v3", "3").Scan(&v1v3C3Change) + if err != nil { + t.Fatalf("query v1->v3 clause 3 change: %v", err) + } + if v1v3C3Change != "added" { + t.Errorf("expected v1->v3 clause 3 to be added, got %q", v1v3C3Change) + } + + // For v2->v3 (adjacent): clause 1 unchanged, clause 2 removed, clause 3 added + var v2v3C1Change, v2v3C2Change, v2v3C3Change string + err = db.QueryRow("SELECT change_type FROM bill_clause_diffs WHERE diff_id = ? AND label = ?", "diff-c-2-v2-v3", "1").Scan(&v2v3C1Change) + if err != nil { + t.Fatalf("query v2->v3 clause 1 change: %v", err) + } + if v2v3C1Change != "unchanged" { + t.Errorf("expected v2->v3 clause 1 to be unchanged, got %q", v2v3C1Change) + } + err = db.QueryRow("SELECT change_type FROM bill_clause_diffs WHERE diff_id = ? AND label = ?", "diff-c-2-v2-v3", "2").Scan(&v2v3C2Change) + if err != nil { + t.Fatalf("query v2->v3 clause 2 change: %v", err) + } + if v2v3C2Change != "removed" { + t.Errorf("expected v2->v3 clause 2 to be removed, got %q", v2v3C2Change) + } + err = db.QueryRow("SELECT change_type FROM bill_clause_diffs WHERE diff_id = ? AND label = ?", "diff-c-2-v2-v3", "3").Scan(&v2v3C3Change) + if err != nil { + t.Fatalf("query v2->v3 clause 3 change: %v", err) + } + if v2v3C3Change != "added" { + t.Errorf("expected v2->v3 clause 3 to be added, got %q", v2v3C3Change) + } +} + type fixedClock struct{} func (fixedClock) Now() time.Time { diff --git a/backend/bills-indexer/internal/usecase/compute_bill_version_diff.go b/backend/bills-indexer/internal/usecase/compute_bill_version_diff.go index df3937c8..3adfc94c 100644 --- a/backend/bills-indexer/internal/usecase/compute_bill_version_diff.go +++ b/backend/bills-indexer/internal/usecase/compute_bill_version_diff.go @@ -27,39 +27,52 @@ func ComputeBillVersionDiff(number string, versions []domain.BillVersion, detail } ordered := append([]domain.BillVersion(nil), versions...) sort.SliceStable(ordered, func(i, j int) bool { return ordered[i].SortOrder < ordered[j].SortOrder }) - diffs := make([]domain.BillDiff, 0, len(ordered)-1) - for i := 1; i < len(ordered); i++ { - fromVer := ordered[i-1] - toVer := ordered[i] - diffID := stableID("diff", number, fromVer.ID, toVer.ID) - - var clauseDiffs []domain.BillClauseDiff - if len(fromVer.Sections) > 0 && len(toVer.Sections) > 0 { - rawDiffs := DiffClauses(fromVer.Sections, toVer.Sections) - clauseDiffs = make([]domain.BillClauseDiff, 0, len(rawDiffs)) - for idx, rd := range rawDiffs { - clauseID := stableID("clause", number, diffID, rd.Label) - if rd.Label == "" { - clauseID = stableID("clause", number, diffID, strconv.Itoa(idx)) + n := len(ordered) + diffs := make([]domain.BillDiff, 0, n*(n-1)/2) + for i := 0; i < n; i++ { + for j := i + 1; j < n; j++ { + fromVer := ordered[i] + toVer := ordered[j] + + // We only emit a diff artifact if: + // 1. It is an adjacent pair (to keep compatibility and stability for existing smoke tests), OR + // 2. Both versions have clause text (len(Sections) > 0). + isAdjacent := (j == i + 1) + hasTextBothSides := (len(fromVer.Sections) > 0 && len(toVer.Sections) > 0) + if !isAdjacent && !hasTextBothSides { + continue + } + + diffID := stableID("diff", number, fromVer.ID, toVer.ID) + + var clauseDiffs []domain.BillClauseDiff + if len(fromVer.Sections) > 0 && len(toVer.Sections) > 0 { + rawDiffs := DiffClauses(fromVer.Sections, toVer.Sections) + clauseDiffs = make([]domain.BillClauseDiff, 0, len(rawDiffs)) + for idx, rd := range rawDiffs { + clauseID := stableID("clause", number, diffID, rd.Label) + if rd.Label == "" { + clauseID = stableID("clause", number, diffID, strconv.Itoa(idx)) + } + clauseDiffs = append(clauseDiffs, domain.BillClauseDiff{ + ID: clauseID, + Label: rd.Label, + ChangeType: rd.ChangeType, + FromText: rd.FromText, + ToText: rd.ToText, + HansardAnchorURL: nil, + }) } - clauseDiffs = append(clauseDiffs, domain.BillClauseDiff{ - ID: clauseID, - Label: rd.Label, - ChangeType: rd.ChangeType, - FromText: rd.FromText, - ToText: rd.ToText, - HansardAnchorURL: nil, - }) } - } - diffs = append(diffs, domain.BillDiff{ - ID: diffID, - FromVersionID: fromVer.ID, - ToVersionID: toVer.ID, - SourceURL: detailURL, - Clauses: clauseDiffs, - }) + diffs = append(diffs, domain.BillDiff{ + ID: diffID, + FromVersionID: fromVer.ID, + ToVersionID: toVer.ID, + SourceURL: detailURL, + Clauses: clauseDiffs, + }) + } } return diffs } diff --git a/backend/bills-indexer/internal/usecase/compute_bill_version_diff_test.go b/backend/bills-indexer/internal/usecase/compute_bill_version_diff_test.go index e52c9a02..3b918f1e 100644 --- a/backend/bills-indexer/internal/usecase/compute_bill_version_diff_test.go +++ b/backend/bills-indexer/internal/usecase/compute_bill_version_diff_test.go @@ -55,7 +55,7 @@ func TestDiffClauses(t *testing.T) { } func TestComputeBillVersionDiffCases(t *testing.T) { - // Case 1: Multi-version bill with text available + // Case 1: Multi-version bill (3 versions) with text available v1 := domain.BillVersion{ ID: "v1", SortOrder: 1, @@ -74,16 +74,43 @@ func TestComputeBillVersionDiffCases(t *testing.T) { TextHash: ptrString("hash-2"), TextSourceURL: ptrString("https://example.test/xml2"), } + v3 := domain.BillVersion{ + ID: "v3", + SortOrder: 3, + Sections: []domain.VersionSection{ + {Label: "1", Text: "Hello Universe"}, + }, + TextHash: ptrString("hash-3"), + TextSourceURL: ptrString("https://example.test/xml3"), + } - diffs := ComputeBillVersionDiff("C-2", []domain.BillVersion{v1, v2}, "https://example.test/bill") - if len(diffs) != 1 { - t.Fatalf("expected 1 diff, got %d", len(diffs)) + diffs := ComputeBillVersionDiff("C-2", []domain.BillVersion{v1, v2, v3}, "https://example.test/bill") + if len(diffs) != 3 { + t.Fatalf("expected 3 diffs, got %d", len(diffs)) } + + // Check v1 -> v2 if diffs[0].FromVersionID != "v1" || diffs[0].ToVersionID != "v2" { - t.Errorf("incorrect version pair in diff: %+v", diffs[0]) + t.Errorf("incorrect version pair in diff 0: %+v", diffs[0]) } if len(diffs[0].Clauses) != 1 || diffs[0].Clauses[0].Label != "1" || diffs[0].Clauses[0].ChangeType != "modified" { - t.Errorf("expected modified clause, got: %+v", diffs[0].Clauses) + t.Errorf("expected modified clause in diff 0, got: %+v", diffs[0].Clauses) + } + + // Check v1 -> v3 + if diffs[1].FromVersionID != "v1" || diffs[1].ToVersionID != "v3" { + t.Errorf("incorrect version pair in diff 1: %+v", diffs[1]) + } + if len(diffs[1].Clauses) != 1 || diffs[1].Clauses[0].Label != "1" || diffs[1].Clauses[0].ChangeType != "modified" { + t.Errorf("expected modified clause in diff 1, got: %+v", diffs[1].Clauses) + } + + // Check v2 -> v3 + if diffs[2].FromVersionID != "v2" || diffs[2].ToVersionID != "v3" { + t.Errorf("incorrect version pair in diff 2: %+v", diffs[2]) + } + if len(diffs[2].Clauses) != 1 || diffs[2].Clauses[0].Label != "1" || diffs[2].Clauses[0].ChangeType != "modified" { + t.Errorf("expected modified clause in diff 2, got: %+v", diffs[2].Clauses) } // Case 2: One-version bill -> no diff records should be built @@ -92,25 +119,62 @@ func TestComputeBillVersionDiffCases(t *testing.T) { t.Errorf("expected 0 diffs for single version, got %d", len(diffsOne)) } - // Case 3: Multi-version bill with missing text -> creates diff records but no clauses - v1Missing := domain.BillVersion{ + // Case 3: Multi-version bill with missing text on some versions + // v1 and v3 have text, v2 has missing text. + // - v1 -> v2 (adjacent): emitted, 0 clauses + // - v2 -> v3 (adjacent): emitted, 0 clauses + // - v1 -> v3 (non-adjacent, both have text): emitted, 1 clause + v1Partial := domain.BillVersion{ ID: "v1", SortOrder: 1, - TextHash: nil, - TextSourceURL: nil, + Sections: []domain.VersionSection{{Label: "1", Text: "Hello"}}, + TextHash: ptrString("hash-1"), + TextSourceURL: ptrString("https://example.test/xml1"), } - v2Missing := domain.BillVersion{ + v2Partial := domain.BillVersion{ ID: "v2", SortOrder: 2, TextHash: nil, TextSourceURL: nil, } - diffsMissing := ComputeBillVersionDiff("C-2", []domain.BillVersion{v1Missing, v2Missing}, "https://example.test/bill") - if len(diffsMissing) != 1 { - t.Fatalf("expected 1 diff, got %d", len(diffsMissing)) + v3Partial := domain.BillVersion{ + ID: "v3", + SortOrder: 3, + Sections: []domain.VersionSection{{Label: "1", Text: "Hello Universe"}}, + TextHash: ptrString("hash-3"), + TextSourceURL: ptrString("https://example.test/xml3"), + } + diffsPartial := ComputeBillVersionDiff("C-2", []domain.BillVersion{v1Partial, v2Partial, v3Partial}, "https://example.test/bill") + if len(diffsPartial) != 3 { + t.Fatalf("expected 3 diffs, got %d", len(diffsPartial)) + } + // Check v1 -> v2 (adjacent, no clauses) + if diffsPartial[0].FromVersionID != "v1" || diffsPartial[0].ToVersionID != "v2" || len(diffsPartial[0].Clauses) != 0 { + t.Errorf("expected empty adjacent v1->v2, got: %+v", diffsPartial[0]) + } + // Check v1 -> v3 (non-adjacent, has clauses) + if diffsPartial[1].FromVersionID != "v1" || diffsPartial[1].ToVersionID != "v3" || len(diffsPartial[1].Clauses) != 1 { + t.Errorf("expected populated non-adjacent v1->v3, got: %+v", diffsPartial[1]) + } + // Check v2 -> v3 (adjacent, no clauses) + if diffsPartial[2].FromVersionID != "v2" || diffsPartial[2].ToVersionID != "v3" || len(diffsPartial[2].Clauses) != 0 { + t.Errorf("expected empty adjacent v2->v3, got: %+v", diffsPartial[2]) + } + + // Case 4: Multi-version bill with all versions missing text + // Only adjacent pairs (v1->v2, v2->v3) should be emitted. Non-adjacent (v1->v3) should be skipped. + v1NoText := domain.BillVersion{ID: "v1", SortOrder: 1} + v2NoText := domain.BillVersion{ID: "v2", SortOrder: 2} + v3NoText := domain.BillVersion{ID: "v3", SortOrder: 3} + diffsNoText := ComputeBillVersionDiff("C-2", []domain.BillVersion{v1NoText, v2NoText, v3NoText}, "https://example.test/bill") + if len(diffsNoText) != 2 { + t.Fatalf("expected 2 diffs (adjacent only) when all lack text, got %d: %+v", len(diffsNoText), diffsNoText) + } + if diffsNoText[0].FromVersionID != "v1" || diffsNoText[0].ToVersionID != "v2" { + t.Errorf("expected v1->v2 as first diff, got %+v", diffsNoText[0]) } - if len(diffsMissing[0].Clauses) != 0 { - t.Errorf("expected 0 clauses in diff when text is missing, got %d", len(diffsMissing[0].Clauses)) + if diffsNoText[1].FromVersionID != "v2" || diffsNoText[1].ToVersionID != "v3" { + t.Errorf("expected v2->v3 as second diff, got %+v", diffsNoText[1]) } } diff --git a/backend/bills/internal/usecase/bills.go b/backend/bills/internal/usecase/bills.go index 19fb7d16..8ff16fbf 100644 --- a/backend/bills/internal/usecase/bills.go +++ b/backend/bills/internal/usecase/bills.go @@ -97,6 +97,10 @@ func (u *LoadBillVersionDiff) Execute(ctx context.Context, input LoadBillVersion return nil, ErrDiffMissingTo } + if fromVersionID == toVersionID { + return nil, nil + } + diff, err := u.repo.GetBillVersionDiff(ctx, billID, fromVersionID, toVersionID) if err != nil || diff == nil { return diff, err @@ -104,6 +108,20 @@ func (u *LoadBillVersionDiff) Execute(ctx context.Context, input LoadBillVersion if len(diff.Clauses) == 0 { return nil, nil } + + // Check if all clauses are unchanged. If they are, return the diff but with + // an empty clauses slice so the client receives HTTP 200 with an empty clauses array. + allUnchanged := true + for _, c := range diff.Clauses { + if c.ChangeType != "unchanged" { + allUnchanged = false + break + } + } + if allUnchanged { + diff.Clauses = []domain.BillClauseDiff{} + } + return diff, nil } diff --git a/backend/bills/internal/usecase/bills_test.go b/backend/bills/internal/usecase/bills_test.go index 216624ff..03426033 100644 --- a/backend/bills/internal/usecase/bills_test.go +++ b/backend/bills/internal/usecase/bills_test.go @@ -100,6 +100,62 @@ func TestLoadBillVersionDiffMapsEmptyClauseDiffToUnavailable(t *testing.T) { } } +func TestLoadBillVersionDiffReturns204ForSameVersion(t *testing.T) { + repo := &fakeBillRepository{} + diff, err := NewLoadBillVersionDiff(repo).Execute(context.Background(), LoadBillVersionDiffInput{ + BillID: "C-2", + FromVersionID: "v1", + ToVersionID: "v1", + }) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + if diff != nil { + t.Fatalf("expected nil diff for same version comparison, got %+v", diff) + } + if repo.called { + t.Fatal("repository should not be called for same version comparison") + } +} + +func TestLoadBillVersionDiffReturnsEmptyClausesForIdenticalText(t *testing.T) { + repo := &fakeBillRepository{ + diff: &domain.BillVersionDiff{ + From: domain.BillVersion{ID: "v1"}, + To: domain.BillVersion{ID: "v2"}, + Clauses: []domain.BillClauseDiff{ + { + ID: "c1", + ChangeType: "unchanged", + FromText: "Same", + ToText: "Same", + }, + { + ID: "c2", + ChangeType: "unchanged", + FromText: "Also Same", + ToText: "Also Same", + }, + }, + }, + } + + diff, err := NewLoadBillVersionDiff(repo).Execute(context.Background(), LoadBillVersionDiffInput{ + BillID: "C-2", + FromVersionID: "v1", + ToVersionID: "v2", + }) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + if diff == nil { + t.Fatal("expected non-nil diff for identical versions") + } + if len(diff.Clauses) != 0 { + t.Fatalf("expected empty clauses slice, got %d clauses: %+v", len(diff.Clauses), diff.Clauses) + } +} + type fakeBillRepository struct { diff *domain.BillVersionDiff err error diff --git a/ios/epac/Views/Bills/BillVersionsDiffView.swift b/ios/epac/Views/Bills/BillVersionsDiffView.swift index f05c5819..3795dd8a 100644 --- a/ios/epac/Views/Bills/BillVersionsDiffView.swift +++ b/ios/epac/Views/Bills/BillVersionsDiffView.swift @@ -235,6 +235,15 @@ struct BillVersionsDiffView: View { diff = nil return } + let sorted = Self.sortVersions(versions) + if let fromIndex = sorted.firstIndex(where: { $0.id == fromVersionID }), + let toIndex = sorted.firstIndex(where: { $0.id == toVersionID }), + fromIndex > toIndex { + let temp = fromVersionID + fromVersionID = toVersionID + toVersionID = temp + return + } isLoading = true loadFailed = false defer { isLoading = false } diff --git a/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_a11y.png b/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_a11y.png new file mode 100644 index 00000000..f599ae20 Binary files /dev/null and b/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_a11y.png differ diff --git a/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_dark.png b/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_dark.png new file mode 100644 index 00000000..827c02a0 Binary files /dev/null and b/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_dark.png differ diff --git a/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_light.png b/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_light.png new file mode 100644 index 00000000..cb170531 Binary files /dev/null and b/ios/epacTests/__Snapshots__/SnapshotTests/testRecentlyBecameLawCard.RecentlyBecameLawCard_light.png differ