Skip to content

Conversation

@zensgit
Copy link
Owner

@zensgit zensgit commented Oct 15, 2025

Purpose / Problem

  • Lock JSON contract: all budget money fields serialize as string; percentage fields remain numeric.
  • Test is marked #[ignore] until budget Decimal migration lands, so it won’t affect CI now.

Summary of Approach

  • Add test file only: jive-api/tests/contract_decimal_budget.rs.
  • Mirrors pattern of merged serialization contract tests for market/account/transactions.
  • Validates:
    • BudgetProgress: budgeted_amount, spent_amount, remaining_amount → string
    • BudgetReport: total_budgeted, total_spent, total_remaining, unbudgeted_spending → string
    • BudgetSummary: budgeted, spent, remaining → string
    • Numeric: percentage_used, overall_percentage, BudgetSummary.percentage
  • No business logic changes.

Testing Evidence

  • Local build with SQLX offline:
    • SQLX_OFFLINE=true cargo test --no-run → ok (tests compile; new test is ignored)
    • SQLX_OFFLINE=true cargo clippy -D warnings → ok (no issues related to this change)

Migration Notes

  • None for DB/schema. When migrating budget to Decimal:
    • Move f64 money fields to Decimal and ensure serde as string (consistent with market/account/transaction).
    • Remove #[ignore] on this test to enforce contract.

Rollback Plan

  • Safe to revert by removing the new test file if needed; no code paths touched.

Related Context

Checklist

  • Adds test-only change; CI unaffected now
  • Matches crate path: jive_money_api
  • No API or handler logic change
  • Remove #[ignore] immediately after budget Decimal migration lands
  • Verify Flutter client continues parsing money as string for budget endpoints
  • Add rounding/formatting edge-case tests post-migration

@zensgit
Copy link
Owner Author

zensgit commented Oct 16, 2025

Re-triggered CI by pushing a no-op commit.

  • This PR is test-only and marked #[ignore], so CI should remain green.
  • Once checks pass, I'll auto-merge to chore/invitations-audit-align-dev-mock.

Post-merge follow-ups (recommendation):

  • Migrate budget money fields to Decimal with string serialization parity.
  • Remove #[ignore] in contract_decimal_budget.rs to enforce the contract.
  • Update Flutter budget parsing to accept money as string (consistent across modules).
  • Add rounding/format edge-case tests in a follow-up PR.

@zensgit zensgit marked this pull request as ready for review October 16, 2025 00:47
@Copilot Copilot AI review requested due to automatic review settings October 16, 2025 00:47
@zensgit zensgit merged commit 0cbafe2 into chore/invitations-audit-align-dev-mock Oct 16, 2025
@zensgit zensgit deleted the chore/decimal-contract-tests-budget branch October 16, 2025 00:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds an ignored test to lock the JSON serialization contract for budget endpoints: money fields should serialize as strings and percentage fields remain numeric.

  • Introduces jive-api/tests/contract_decimal_budget.rs with a comprehensive serialization contract test.
  • Asserts string serialization for key money fields; asserts numeric serialization for percentage fields.
  • Test is #[ignore] pending the Decimal migration for budget models.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

categories: vec![],
};

let val: Value = serde_json::to_value(&progress).expect("serialize BudgetProgress");
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] val is shadowed later (see line 57) when serializing the report, which slightly reduces clarity. Consider renaming to progress_json and report_json to make the two JSON values distinct.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant