Conversation
Code Review SummaryThe PR adds the ability to transfer money between wallets. It includes database schema changes (v4), migration steps, DTOs, repository implementations, and a new UI for performing and viewing transfers. It also introduces paged/streaming sync capabilities to the synchronization core. 🚀 Key Improvements
💡 Minor Suggestions
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| if (selectedFromWallet != null && amount > selectedFromWallet!.balance) { |
There was a problem hiding this comment.
The error message 'amountMustNotBeZero' is used when the amount exceeds the balance. This is semantically incorrect and confusing for the user.
| if (selectedFromWallet != null && amount > selectedFromWallet!.balance) { | |
| if (selectedFromWallet != null && amount > selectedFromWallet!.balance) { | |
| return LocaleKeys.insufficientBalance.tr(); | |
| } |
| queryParams['no_client_id'] = noClientId; | ||
| } | ||
|
|
||
| queryParams['limit'] = limit; |
There was a problem hiding this comment.
Hardcoded 'limit' of 10 might be too small for initial syncs. Consider making this a configurable constant or using a higher default.
| queryParams['limit'] = limit; | |
| queryParams['limit'] = 50; |
| expenseDto = await transactionLocalDataSource.updateTransaction( | ||
| expenseDto.transaction.clientId, | ||
| transferClientId: transferClientId, | ||
| ); | ||
|
|
||
| incomeDto = await transactionLocalDataSource.updateTransaction( | ||
| incomeDto.transaction.clientId, | ||
| transferClientId: transferClientId, | ||
| ); | ||
|
|
||
|
|
||
| }); | ||
|
|
There was a problem hiding this comment.
The transactions are updated outside of the Drift transaction block. If an error occurs between the local update and the sync call, the data integrity between the transfer record and transactions might be compromised.
| expenseDto = await transactionLocalDataSource.updateTransaction( | |
| expenseDto.transaction.clientId, | |
| transferClientId: transferClientId, | |
| ); | |
| incomeDto = await transactionLocalDataSource.updateTransaction( | |
| incomeDto.transaction.clientId, | |
| transferClientId: transferClientId, | |
| ); | |
| }); | |
| expenseDto = await transactionLocalDataSource.updateTransaction( | |
| expenseDto.transaction.clientId, | |
| transferClientId: transferClientId, | |
| ); | |
| incomeDto = await transactionLocalDataSource.updateTransaction( | |
| incomeDto.transaction.clientId, | |
| transferClientId: transferClientId, | |
| ); | |
| }); |
| ); | ||
| } | ||
| if (selectedFromWallet != null && number > selectedFromWallet!.balance) { | ||
| return LocaleKeys.amountMustNotBeZero.tr(); // fallback until locale keys update |
There was a problem hiding this comment.
The error message 'amountMustNotBeZero' is being used for insufficient balance, which is confusing for the user.
| return LocaleKeys.amountMustNotBeZero.tr(); // fallback until locale keys update | |
| return LocaleKeys.insufficientBalance.tr(); |
| throw UnimplementedError(); | ||
| } | ||
|
|
||
| @override |
There was a problem hiding this comment.
The mock implementation of getAllTransactionsStream throws UnimplementedError. This will break unit tests or any functionality relying on this mock if the streaming sync is enabled.
| @override | |
| @override | |
| Stream<List<TransactionCompleteDto>> getAllTransactionsStream({DateTime? syncedSince, bool? noClientId}) async* { | |
| yield []; | |
| } |
| } | ||
| } | ||
|
|
||
| Map<String, dynamic> toServerJson(Transfer transfer) { |
There was a problem hiding this comment.
The toServerJson helper function should probably be part of the TransferDto or the Transfer data class to keep serialization logic centralized, similar to other entities in the project.
| Map<String, dynamic> toServerJson(Transfer transfer) { | |
| // Move this logic into a marshal/toJson method within the DTO or entity mapping layer |
|
|
||
| transactionsToSort.sort( | ||
| (a, b) => b.transaction.datetime.compareTo(a.transaction.datetime), | ||
| (a, b) => b.transaction.createdAt.compareTo(a.transaction.createdAt), |
There was a problem hiding this comment.
Sorting transactions by createdAt instead of datetime might lead to confusing UI if the user manually changes the transaction date to a past or future date. Users usually expect transactions to be sorted by the logical 'datetime' of the event.
| (a, b) => b.transaction.createdAt.compareTo(a.transaction.createdAt), | |
| (a, b) => b.transaction.datetime.compareTo(a.transaction.datetime), |
| @override | ||
| Future<Transfer> insertTransfer(TransfersCompanion companion) async { | ||
| final now = getNewFormattedUtcDateTime(); | ||
| final clientId = companion.clientId.present && companion.clientId.value.isNotEmpty |
There was a problem hiding this comment.
Using unawaited or just not awaiting generateDeviceScopedId() is risky if the ID generation has side effects or relies on async state. Ensure IDs are fully generated before insertion.
| final clientId = companion.clientId.present && companion.clientId.value.isNotEmpty | |
| final String clientId; | |
| if (companion.clientId.present && companion.clientId.value.isNotEmpty) { | |
| clientId = companion.clientId.value; | |
| } else { | |
| clientId = await generateDeviceScopedId(); | |
| } | |
| final toInsert = companion.copyWith( | |
| clientId: Value(clientId), | |
| createdAt: Value(now), | |
| updatedAt: Value(now), | |
| ); | |
| return database.into(database.transfers).insertReturning(toInsert); |
| _remoteConfig.getInt('updateReminderFrequency'); | ||
|
|
||
|
|
||
| double get minimumTransferAmount { |
There was a problem hiding this comment.
This parsing logic is repeated. Consider creating a private helper method _getDoubleWithFallback to reduce complexity and improve readability.
| double get minimumTransferAmount { | |
| double get minimumTransferAmount => _getDoubleWithFallback('minimumTransferAmount', defaultMinimumTransferAmount); | |
| double _getDoubleWithFallback(String key, double fallback) { | |
| try { | |
| final val = _remoteConfig.getDouble(key); | |
| if (val > 0) return val; | |
| } catch (_) {} | |
| try { | |
| final s = _remoteConfig.getString(key); | |
| if (s.isNotEmpty) { | |
| final parsed = double.tryParse(s.replaceAll(',', '.')); | |
| if (parsed != null && parsed > 0) return parsed; | |
| } | |
| } catch (_) {} | |
| return fallback; | |
| } |
- Transfers table and Drift schema v4 - transaction transferId, transferClientId columns - Transaction entity/DTO/mapper include transfer fields
- Transfer DTOs, local/remote datasources, mapper, repository - Transfer sync handler; sync module and dependency manager - Transaction local/remote datasource and repo updates for transfers - Drift sync core updates for transfer entity type
- On transaction upsert, keep existing transferId/transferClientId when server response has null (e.g. offline-created transfer not yet on backend)
- Replace AppUpdateConfig with RemoteFeatureConfig; GetRemoteFeatureConfigUseCase - Add RemoteConfigCubit; minimum transfer amount from remote config - Remove get_app_update_config_usecase; check_app_update uses shared config - Provide RemoteConfigCubit in app_widget
- Transfers list and wallet transfer screens; TransferCubit - Transfer tile, transaction tile transfer icon, wallet tile link - Drawer and home screen navigation to transfers; remove old wallet_transfer_screen
- Translation keys and locale files for transfer/min amount - Regenerate codegen_loader and locale_keys; README update - Amount parser; register transfer and remote config in injection
- Add PagedSyncTypeHandler; consume remote pages without buffering all items - TransactionRemoteDataSource: getAllTransactionsStream + delegate getAllTransactions - TransactionSyncHandler implements streaming; item-by-item upsert in synchronizer - Mock datasource: stub getAllTransactionsStream
- Add getAllTransfersStream (page + limit); getAllTransfers delegates to stream
- Wrap transfers list in SafeArea - Use insufficientBalance when amount exceeds wallet balance - Trim whitespace in transfer repository
austin047
force-pushed
the
feat/add-transfer
branch
from
c4dc1a5 to
74d2e20
Compare
2 participants
Description
Add transfer
Type of Change