Skip to content

feat: return deleted items during sync #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 15, 2025
Merged

feat: return deleted items during sync #76

merged 1 commit into from
Jul 15, 2025

Conversation

kofimokome
Copy link
Collaborator

No description provided.

Copy link

sourceant bot commented Jul 11, 2025

Code Review Summary

✨ This pull request primarily focuses on implementing soft deletes across key application models (Category, Group, Party, Transaction, Wallet) and updating related functionalities. This significantly improves data integrity by allowing records to be 'deleted' without permanently removing them from the database, enabling future recovery and better auditing. The changes also refine API queries for syncing and update tests to reflect soft delete behavior.

🚀 Key Improvements

  • Enhanced Data Integrity: Implementation of SoftDeletes provides a robust mechanism for data retention and recovery, which is a significant improvement over hard deletions.
  • Consistent Sync Behavior: The ApiQueryable trait now correctly includes soft-deleted items when querying by synced_since, which is crucial for client-side synchronization logic to accurately reflect all data states, including deletions.
  • Cascading Soft Deletes: The Transaction model's delete() method ensures that related RecurringTransactionRule entries are also soft-deleted, maintaining referential integrity even with soft deletions.
  • Improved Test Coverage: Unit tests were thoughtfully updated to correctly assert soft deletion behavior, making the test suite more accurate and resilient to future changes related to data lifecycle.

💡 Minor Suggestions

  • In app/Http/Traits/ApiQueryable.php, consider chaining the withTrashed() method directly after where() to avoid redundant variable re-assignment, improving conciseness.
  • Ensure API documentation and client-side code are updated for the removal of last_synced_at from the Transaction schema in public/docs/api.json, as this is a breaking change to the API response structure.

Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

@kofimokome kofimokome force-pushed the feat/recent-changes branch from 415dc47 to 06301c3 Compare July 11, 2025 17:41
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

@kofimokome kofimokome force-pushed the feat/recent-changes branch from 06301c3 to c5d8682 Compare July 11, 2025 17:44
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

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

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

@kofimokome kofimokome requested a review from nfebe July 11, 2025 17:48
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

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

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

Needs a rebase.

@kofimokome kofimokome force-pushed the feat/recent-changes branch from c5d8682 to 2aa717f Compare July 11, 2025 20:03
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

There is some inconsistency with using assertNull and assertSoftDeleted it still in mergeable state but delete commented code.

@kofimokome kofimokome force-pushed the feat/recent-changes branch from 2aa717f to 97f2a40 Compare July 11, 2025 21:24
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

# Conflicts:
#	database/migrations/2024_07_20_125117_create_transactions_table.php
#	public/docs/api.json
@kofimokome kofimokome force-pushed the feat/recent-changes branch from 97f2a40 to 3867e7a Compare July 13, 2025 13:20
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

@trakli trakli deleted a comment from sourceant bot Jul 13, 2025
@nfebe nfebe merged commit b75dafe into main Jul 15, 2025
2 checks passed
@nfebe nfebe deleted the feat/recent-changes branch July 15, 2025 20:49
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.

2 participants