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
5 changed files with 102 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/java/dev/achmad/ledgerr/di/DomainModule.kt
+9
View File
@@ -5,6 +5,10 @@ import dev.achmad.ledgerr.domain.category.interactor.GetCategories
import dev.achmad.ledgerr.domain.category.interactor.SeedDefaultCategories import dev.achmad.ledgerr.domain.category.interactor.SeedDefaultCategories
import dev.achmad.ledgerr.domain.category.interactor.UpsertCategory import dev.achmad.ledgerr.domain.category.interactor.UpsertCategory
import dev.achmad.ledgerr.domain.expense.interactor.ReassignExpenseCategory import dev.achmad.ledgerr.domain.expense.interactor.ReassignExpenseCategory
import dev.achmad.ledgerr.domain.recurring.interactor.DeleteRecurringExpense
import dev.achmad.ledgerr.domain.recurring.interactor.GetRecurringExpenses
import dev.achmad.ledgerr.domain.recurring.interactor.ProcessDueRecurringExpenses
import dev.achmad.ledgerr.domain.recurring.interactor.UpsertRecurringExpense
import org.koin.dsl.module import org.koin.dsl.module
val domainModule = module { val domainModule = module {
@@ -14,4 +18,9 @@ val domainModule = module {
factory { SeedDefaultCategories(get()) } factory { SeedDefaultCategories(get()) }
factory { ReassignExpenseCategory(get()) } factory { ReassignExpenseCategory(get()) }
factory { GetRecurringExpenses(get()) }
admin commented 2026-06-28 09:45:23 +00:00
Review
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.
factory { UpsertRecurringExpense(get()) }
factory { DeleteRecurringExpense(get()) }
factory { ProcessDueRecurringExpenses(get()) }
} }
app/src/main/java/dev/achmad/ledgerr/domain/recurring/interactor/DeleteRecurringExpense.kt
+11
View File
@@ -0,0 +1,11 @@
package dev.achmad.ledgerr.domain.recurring.interactor
import dev.achmad.ledgerr.data.local.dao.RecurringExpenseDao
class DeleteRecurringExpense(
private val dao: RecurringExpenseDao,
) {
suspend fun await(id: Long) {
dao.deleteById(id)
}
}
app/src/main/java/dev/achmad/ledgerr/domain/recurring/interactor/GetRecurringExpenses.kt
+25
View File
@@ -0,0 +1,25 @@
package dev.achmad.ledgerr.domain.recurring.interactor
import dev.achmad.ledgerr.data.local.dao.RecurringExpenseDao
import dev.achmad.ledgerr.data.local.mapper.toModel
import dev.achmad.ledgerr.domain.recurring.model.RecurringExpense
import dev.achmad.ledgerr.domain.recurring.model.RecurringExpenseWithCategory
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.map
class GetRecurringExpenses(
private val recurringDao: RecurringExpenseDao,
) {
fun subscribeAll(): Flow<List<RecurringExpenseWithCategory>> =
recurringDao.subscribeAll().map { rows ->
rows.map { row ->
RecurringExpenseWithCategory(
recurring = row.recurring.toModel(),
category = row.category.toModel(),
)
}
}
suspend fun awaitOne(id: Long): RecurringExpense? =
recurringDao.getById(id)?.toModel()
admin commented 2026-06-28 09:45:23 +00:00
Review
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
+38
View File
@@ -0,0 +1,38 @@
package dev.achmad.ledgerr.domain.recurring.interactor
import androidx.room.withTransaction
import dev.achmad.ledgerr.data.local.AppDatabase
import dev.achmad.ledgerr.data.local.mapper.toEntity
import dev.achmad.ledgerr.data.local.mapper.toModel
import dev.achmad.ledgerr.domain.expense.model.Expense
import java.time.LocalDate
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()
val dueTemplates = recurringDao.getDue(today.toEpochDay())
if (dueTemplates.isEmpty()) return@withTransaction emptyList()
val created = mutableListOf<Expense>()
for (templateEntity in dueTemplates) {
val template = templateEntity.toModel()
val expense = Expense(
amount = template.amount,
categoryId = template.categoryId,
date = template.nextDueDate,
note = template.note,
recurringExpenseId = template.id,
)
val newId = expenseDao.insert(expense.toEntity())
created += expense.copy(id = newId)
val advanced = template.copy(
nextDueDate = template.interval.advance(template.nextDueDate),
admin commented 2026-06-28 09:45:23 +00:00
Outdated
Review
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.
)
recurringDao.update(advanced.toEntity())
}
created
}
}
app/src/main/java/dev/achmad/ledgerr/domain/recurring/interactor/UpsertRecurringExpense.kt
+19
View File
@@ -0,0 +1,19 @@
package dev.achmad.ledgerr.domain.recurring.interactor
import dev.achmad.ledgerr.data.local.dao.RecurringExpenseDao
import dev.achmad.ledgerr.data.local.mapper.toEntity
import dev.achmad.ledgerr.domain.recurring.model.RecurringExpense
class UpsertRecurringExpense(
private val dao: RecurringExpenseDao,
) {
suspend fun await(recurring: RecurringExpense): Long {
val entity = recurring.toEntity()
return if (recurring.id == 0L) {
dao.insert(entity)
} else {
dao.update(entity)
recurring.id
}
}
admin commented 2026-06-28 09:45:23 +00:00
Review
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.
}