Skip to content

fix(disputes): shared Agreement side effects for dispute status changes#100

Merged
Kalchaqui merged 2 commits into
Thalos-Infrastructure:mainfrom
naninu123:fix/issue-58-dispute-side-effects
Jul 24, 2026
Merged

fix(disputes): shared Agreement side effects for dispute status changes#100
Kalchaqui merged 2 commits into
Thalos-Infrastructure:mainfrom
naninu123:fix/issue-58-dispute-side-effects

Conversation

@naninu123

Copy link
Copy Markdown
Contributor

Summary

Closes #58.

Dispute-driven Agreement status changes now go through the same side-effect path as normal Agreement updates:

  • AgreementActivityService — single shared writer for agreement_activity (replaces 3 private logActivity copies in Agreements / Disputes / Webhooks)
  • AgreementsService.applyStatusChange — shared status update + activity log (status_changed_to_* with {status, from, to}) + domain events (FUNDED / COMPLETED)
  • DisputesService open / resolve / cancel call applyStatusChange then log dispute-specific actions (dispute_opened, dispute_resolved, dispute_cancelled)
  • Dispute-specific DISPUTE_OPENED / DISPUTE_RESOLVED emits kept

Acceptance criteria

AC Status
Unit: dispute open/resolve emit expected domain events + activity logs
Integration: end-to-end dispute → Agreement update still works ✅ migrated-flows
Parity: dispute-driven vs normal updateStatus share status_changed_to_* shape
Regression: listByWallet creator+participant scoping unchanged

Test plan

  • npm test280 passed
  • npx tsc --noEmit — clean
  • npm run lint0 errors (pre-existing warnings only)
  • New: agreement-activity.service.spec.ts, dispute-side-effects.spec.ts
  • Updated: agreement-lifecycle.spec.ts, webhooks.spec.ts, integration DI

Notes

No public API / DTO / route changes. Backward compatible.

@vercel

vercel Bot commented Jul 21, 2026

Copy link
Copy Markdown

@naninu123 is attempting to deploy a commit to the ManuelJG's projects Team on Vercel.

A member of the Team first needs to authorize it.

@Kalchaqui

Copy link
Copy Markdown
Collaborator

Review — LGTM (rebase before merge)

Solid fix for #58. Scope matches the issue: shared activity logging + dispute status changes go through a common side-effect path, without public API churn.

What looks good

  • AgreementActivityService consolidates the 3 private logActivity copies (Agreements / Disputes / Webhooks).
  • DisputesService open / resolve / cancel call applyStatusChange then still emit DISPUTE_OPENED / DISPUTE_RESOLVED (notifications stay wired).
  • Dispute-specific activity actions (dispute_opened, dispute_resolved, dispute_cancelled) preserved alongside status_changed_to_*.
  • listByWallet untouched.
  • New unit coverage in dispute-side-effects.spec.ts / agreement-activity.service.spec.ts is meaningful.

Before merge

  1. Please rebase on current main — branch is ~8 commits behind (includes dispute-milestone + KYB). Re-run npm test after rebase.

Non-blocking nits (optional)

After rebase + green tests, this is good to merge from my side.

@naninu123
naninu123 force-pushed the fix/issue-58-dispute-side-effects branch from 29f705f to 67f1030 Compare July 23, 2026 05:28
@naninu123

Copy link
Copy Markdown
Contributor Author

Rebased on current main. Ready for re-review.

@Kalchaqui

Copy link
Copy Markdown
Collaborator

Re-review after rebase

Thanks @naninu123 — confirmed force-push 67f1030 and the #58 design still looks right (AgreementActivityService + applyStatusChange + dispute activity/events).

CI on this SHA: Test / Lint / Build / Format green (Vercel fail expected).

Blocker for merge right now

GitHub reports merge conflicts with main in:

  • src/agreements/agreements.service.ts
  • src/disputes/disputes.service.ts

Please rebase/resolve and push again before we merge.

Maintainer note (also relevant)

This PR overlaps open #104 (activity logging + previous_state / new_state). Prefer merge #100 first once conflicts are clear, then rebase #104 onto the shared AgreementActivityService.

Optional nit (still non-blocking)

Check applyStatusChange { success } before emitting dispute events.

Happy to give a final LGTM as soon as conflicts are resolved and CI is green again.

…side effects

Closes Thalos-Infrastructure#58

Consolidate agreement_activity writes into AgreementActivityService and make
DisputesService open/resolve/cancel go through AgreementsService.applyStatusChange
so dispute-driven status updates emit the same activity logs and domain events
as normal updateStatus. Wire WebhooksService to the shared activity helper.
…fields into shared activity

Resolve conflicts with Thalos-Infrastructure#104 (previous_state/new_state):
- AgreementActivityService accepts optional state columns
- applyStatusChange returns fromStatus + writes state fields
- Disputes open/resolve/cancel go through applyStatusChange and
  check success before dispute events (maintainer nit)
- Drop private logActivity / logAgreementActivity duplicates
- Update lifecycle expectations for status_changed_to_* entries
@naninu123
naninu123 force-pushed the fix/issue-58-dispute-side-effects branch from 67f1030 to cb08d30 Compare July 23, 2026 23:39
@naninu123

Copy link
Copy Markdown
Contributor Author

Rebased + conflicts resolved

@Kalchaqui force-pushed rebased branch onto current main (includes #104).

Conflict resolution

  • Kept shared AgreementActivityService + applyStatusChange path from this PR
  • Merged feat(agreements): extend activity logging with previous/new state + dispute events (#61) #104 previous_state / new_state into AgreementActivityService.logActivity (optional 5th arg)
  • Dropped private logActivity / logAgreementActivity duplicates in AgreementsService
  • Dispute open/resolve/cancel → applyStatusChange then dispute-specific activity with state fields
  • Maintainer nit: check applyStatusChange { success } before emitting dispute events — done

Verify

  • jest full suite: 345 passed
  • Prettier: clean
  • ESLint on touched files: 0 errors (pre-existing warnings only)
  • Build: green

Ready for final LGTM / merge.

@Kalchaqui

Copy link
Copy Markdown
Collaborator

Re-review — LGTM

Thanks @naninu123cb08d30 clears the prior merge conflicts and cleanly folds #104 previous_state / new_state into AgreementActivityService + dispute side-effects.

Design for #58 still looks right (applyStatusChange + shared activity writer + dispute-specific actions/events). Early-return on failed status update is in place.

Merge-ready once CI is green on this SHA (Vercel auth fail expected / ignore).

@Kalchaqui
Kalchaqui merged commit c6d29f9 into Thalos-Infrastructure:main Jul 24, 2026
6 of 7 checks passed
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.

Ensure dispute-driven Agreement changes trigger notifications & activity logs

2 participants