admin/ledgerr
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Implement recurring interactors (#3) #10

Merged
admin merged 2 commits from feat/3-implement-recurring-interactors into main 2026-06-28 09:56:39 +00:00
Conversation 1 Commits 2 Files Changed 5
+102
admin commented 2026-06-28 09:33:46 +00:00
Owner
Copy Link
Copy Source

Summary

  • Implement the 4 recurring interactors (GetRecurringExpenses, UpsertRecurringExpense, DeleteRecurringExpense, ProcessDueRecurringExpenses) under domain/recurring/interactor/
  • Wire the 4 new factories in di/DomainModule.kt

Test plan

  • ./gradlew assembleDebug succeeds

Closes #3

## Summary - Implement the 4 recurring interactors (`GetRecurringExpenses`, `UpsertRecurringExpense`, `DeleteRecurringExpense`, `ProcessDueRecurringExpenses`) under `domain/recurring/interactor/` - Wire the 4 new factories in `di/DomainModule.kt` ## Test plan - [x] `./gradlew assembleDebug` succeeds Closes #3
admin added 1 commit 2026-06-28 09:33:46 +00:00
feat(#3): implement recurring interactors 6a11284212
admin reviewed 2026-06-28 09:45:23 +00:00
admin left a comment
Author
Owner
Copy Link
Copy Source

Solid PR — DAO contract is respected, signatures match docs/03-function-todos.md, and the implementation is consistent with PR #2's established patterns. Left inline comments for the items that need to be addressed or followed up on before this merges cleanly.

Must address:

  1. ProcessDueRecurringExpenses.await is not atomic — see inline comment at the insert + advance pair.

Worth a quick check:
2. GetRecurringExpenses.awaitOne returns RecurringExpense? without a category — confirm the edit screen handles it.

Follow-up issues (out of scope for this PR per the no-implementation rule):
3. UpsertRecurringExpense (and UpsertExpense in PR #2) diverge from the @Upsert convention in AGENTS.md:302 / docs/03-function-todos.md:26. Converge the DAOs and the docs in a follow-up.
4. GetRecurringExpenses is single-DAO but docs/02-interfaces.md:107 and the original issue #3 step 3 specify two-DAO. The simplification is correct, but the docs are stale. Update in a follow-up.
5. No unit tests for ProcessDueRecurringExpenses — the per-row advance and the isActive filter in getDue would benefit from in-memory Room tests. Worth a follow-up.

Solid PR — DAO contract is respected, signatures match `docs/03-function-todos.md`, and the implementation is consistent with PR #2's established patterns. Left inline comments for the items that need to be addressed or followed up on before this merges cleanly. **Must address:** 1. `ProcessDueRecurringExpenses.await` is not atomic — see inline comment at the insert + advance pair. **Worth a quick check:** 2. `GetRecurringExpenses.awaitOne` returns `RecurringExpense?` without a category — confirm the edit screen handles it. **Follow-up issues (out of scope for this PR per the no-implementation rule):** 3. `UpsertRecurringExpense` (and `UpsertExpense` in PR #2) diverge from the `@Upsert` convention in `AGENTS.md:302` / `docs/03-function-todos.md:26`. Converge the DAOs and the docs in a follow-up. 4. `GetRecurringExpenses` is single-DAO but `docs/02-interfaces.md:107` and the original issue #3 step 3 specify two-DAO. The simplification is correct, but the docs are stale. Update in a follow-up. 5. No unit tests for `ProcessDueRecurringExpenses` — the per-row advance and the `isActive` filter in `getDue` would benefit from in-memory Room tests. Worth a follow-up.
app/src/main/java/dev/achmad/ledgerr/di/DomainModule.kt
@@ -15,2 +19,4 @@
factory { ReassignExpenseCategory(get()) }
factory { GetRecurringExpenses(get()) }
admin commented 2026-06-28 09:45:23 +00:00
Author
Owner
Copy Link
Copy Source

This is single-DAO, but docs/02-interfaces.md:107 and the original issue #3 step 3 both specify GetRecurringExpenses(get(), get()) with a CategoryDao. The simplification is correct — RecurringExpenseDao.subscribeAll() uses @Relation to join the category in one query (RecurringExpenseDao.kt:27-29), and awaitOne only needs the recurring entity — so a second CategoryDao would be redundant. GetExpenses keeps its two-DAO shape because ExpenseDao.getById / search do not use @Relation, so the asymmetry between the two interactors is justified by the DAO design, not a doc bug.

Suggested follow-up (out of scope here per the no-implementation rule)

Open a follow-up issue to update docs/02-interfaces.md:107 (and the original issue #3 if it is still open) to match the actual signature. Optionally add a one-line note in the doc explaining why GetRecurringExpenses is one-DAO while GetExpenses is two-DAO.

This is single-DAO, but `docs/02-interfaces.md:107` and the original issue #3 step 3 both specify `GetRecurringExpenses(get(), get())` with a `CategoryDao`. The simplification is correct — `RecurringExpenseDao.subscribeAll()` uses `@Relation` to join the category in one query (`RecurringExpenseDao.kt:27-29`), and `awaitOne` only needs the recurring entity — so a second `CategoryDao` would be redundant. `GetExpenses` keeps its two-DAO shape because `ExpenseDao.getById` / `search` do not use `@Relation`, so the asymmetry between the two interactors is justified by the DAO design, not a doc bug. ### Suggested follow-up (out of scope here per the no-implementation rule) Open a follow-up issue to update `docs/02-interfaces.md:107` (and the original issue #3 if it is still open) to match the actual signature. Optionally add a one-line note in the doc explaining *why* `GetRecurringExpenses` is one-DAO while `GetExpenses` is two-DAO.
app/src/main/java/dev/achmad/ledgerr/domain/recurring/interactor/GetRecurringExpenses.kt
@@ -0,0 +21,4 @@
}
suspend fun awaitOne(id: Long): RecurringExpense? =
recurringDao.getById(id)?.toModel()
admin commented 2026-06-28 09:45:23 +00:00
Author
Owner
Copy Link
Copy Source

awaitOne returns RecurringExpense? without a category. This matches docs/02-interfaces.md:109 and docs/03-function-todos.md:108, so it's intentional — but worth a quick check before this lands: does the edit-recurring screen (AddEditRecurringScreen from issue #6) load the category separately via GetCategories, or doesn't it need it on this path? If the screen reads the category from its own GetCategories flow, no change is needed; if it relied on this returning a category, that's a downstream bug.

`awaitOne` returns `RecurringExpense?` without a category. This matches `docs/02-interfaces.md:109` and `docs/03-function-todos.md:108`, so it's intentional — but worth a quick check before this lands: does the edit-recurring screen (`AddEditRecurringScreen` from issue #6) load the category separately via `GetCategories`, or doesn't it need it on this path? If the screen reads the category from its own `GetCategories` flow, no change is needed; if it relied on this returning a category, that's a downstream bug.
app/src/main/java/dev/achmad/ledgerr/domain/recurring/interactor/ProcessDueRecurringExpenses.kt Outdated
@@ -0,0 +29,4 @@
val advanced = template.copy(
nextDueDate = template.interval.advance(template.nextDueDate),
)
recurringDao.update(advanced.toEntity())
admin commented 2026-06-28 09:45:23 +00:00
Author
Owner
Copy Link
Copy Source

The insert + advance pair is not atomic. If the process is killed (or a coroutine is cancelled) between expenseDao.insert and recurringDao.update, the template's nextDueDate is not advanced and the next call will re-process the same template → duplicate expense with no idempotency key (the only link back to the template is recurringExpenseId, which is not unique).

The codebase already uses database.withTransaction { } for ClearAllData (per docs/03-function-todos.md:181), so the pattern is established.

Suggested fix

Inject AppDatabase and wrap the per-row loop in database.withTransaction { ... }. The cleanest shape drops the two DAO constructor params and resolves them from the database inside the transaction block:

class ProcessDueRecurringExpenses(
    private val database: AppDatabase,
) {
    suspend fun await(today: LocalDate = LocalDate.now()): List<Expense> =
        database.withTransaction {
            val recurringDao = database.recurringExpenseDao()
            val expenseDao = database.expenseDao()
            // ... existing loop body, unchanged ...
        }
}

Then DomainModule becomes factory { ProcessDueRecurringExpenses(get()) } (the AppDatabase is already a single in DataModule).

Trade-off

Per AGENTS.md, interactors normally take DAOs and not the database. ClearAllData is the existing exception because it also needs cross-DAO atomicity. Same justification applies here.

The insert + advance pair is **not atomic**. If the process is killed (or a coroutine is cancelled) between `expenseDao.insert` and `recurringDao.update`, the template's `nextDueDate` is not advanced and the next call will re-process the same template → **duplicate expense** with no idempotency key (the only link back to the template is `recurringExpenseId`, which is not unique). The codebase already uses `database.withTransaction { }` for `ClearAllData` (per `docs/03-function-todos.md:181`), so the pattern is established. ### Suggested fix Inject `AppDatabase` and wrap the per-row loop in `database.withTransaction { ... }`. The cleanest shape drops the two DAO constructor params and resolves them from the database inside the transaction block: ```kotlin class ProcessDueRecurringExpenses( private val database: AppDatabase, ) { suspend fun await(today: LocalDate = LocalDate.now()): List<Expense> = database.withTransaction { val recurringDao = database.recurringExpenseDao() val expenseDao = database.expenseDao() // ... existing loop body, unchanged ... } } ``` Then `DomainModule` becomes `factory { ProcessDueRecurringExpenses(get()) }` (the `AppDatabase` is already a `single` in `DataModule`). ### Trade-off Per AGENTS.md, interactors normally take DAOs and not the database. `ClearAllData` is the existing exception because it also needs cross-DAO atomicity. Same justification applies here.
app/src/main/java/dev/achmad/ledgerr/domain/recurring/interactor/UpsertRecurringExpense.kt
@@ -0,0 +15,4 @@
dao.update(entity)
recurring.id
}
}
admin commented 2026-06-28 09:45:23 +00:00
Author
Owner
Copy Link
Copy Source

AGENTS.md:302 says "Use @Upsert Room annotation for insert-or-update operations" and docs/03-function-todos.md:26 says the upsert path "Uses Room @Upsert", but neither RecurringExpenseDao nor ExpenseDao defines an @Upsert method. The manual if (id == 0L) insert else update pattern works, but it diverges from the documented convention.

Same gap exists in PR #2's UpsertExpense.kt:10-16 (both DAOs have the same @Insert(REPLACE) + @Update only, no @Upsert).

Suggested follow-up (out of scope here per the no-implementation rule)

Open a follow-up issue to converge the two: either

  • add @Upsert methods to both ExpenseDao and RecurringExpenseDao and use them in UpsertExpense / UpsertRecurringExpense (and expenseDao.upsert(...) inside the ProcessDueRecurringExpenses loop), or
  • relax AGENTS.md:302 and docs/03-function-todos.md:26 to match the actual insert/update pattern.

This PR is fine as-is — flagging for traceability.

`AGENTS.md:302` says "Use `@Upsert` Room annotation for insert-or-update operations" and `docs/03-function-todos.md:26` says the upsert path "Uses Room `@Upsert`", but neither `RecurringExpenseDao` nor `ExpenseDao` defines an `@Upsert` method. The manual `if (id == 0L) insert else update` pattern works, but it diverges from the documented convention. Same gap exists in PR #2's `UpsertExpense.kt:10-16` (both DAOs have the same `@Insert(REPLACE)` + `@Update` only, no `@Upsert`). ### Suggested follow-up (out of scope here per the no-implementation rule) Open a follow-up issue to converge the two: either - add `@Upsert` methods to both `ExpenseDao` and `RecurringExpenseDao` and use them in `UpsertExpense` / `UpsertRecurringExpense` (and `expenseDao.upsert(...)` inside the `ProcessDueRecurringExpenses` loop), or - relax `AGENTS.md:302` and `docs/03-function-todos.md:26` to match the actual `insert`/`update` pattern. This PR is fine as-is — flagging for traceability.
admin added 1 commit 2026-06-28 09:52:09 +00:00
fix(#3): make ProcessDueRecurringExpenses atomic via withTransaction 7bb65025a2 
Address review: insert + advance pair must run in one DB transaction to
prevent duplicate expenses if the process is killed mid-loop.
admin merged commit ec21462d03 into main 2026-06-28 09:56:39 +00:00
admin deleted branch feat/3-implement-recurring-interactors 2026-06-28 15:02:22 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
admin
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: admin/ledgerr#10
Delete Branch "feat/3-implement-recurring-interactors"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?