Feat/add academic debts feature #8
Conversation
WalkthroughAdds a new Debts feature: data models, repository, caching service, BLoC (events/states), UI pages/widgets, DI registration, router route, dashboard card, auth logout cache-clear, and English/Arabic localization entries. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DebtsPage
participant DebtsBloc
participant DebtsCacheService
participant DebtsRepositoryImpl
participant ApiClient
User->>DebtsPage: Open DebtsPage
DebtsPage->>DebtsBloc: Dispatch LoadDebts(forceRefresh: false)
DebtsBloc->>DebtsBloc: Emit DebtsLoading
alt Cache fresh and not forceRefresh
DebtsBloc->>DebtsCacheService: isDataStale()
DebtsCacheService-->>DebtsBloc: false
DebtsBloc->>DebtsCacheService: getCachedDebts()
DebtsCacheService-->>DebtsBloc: cached debts
DebtsBloc->>DebtsPage: Emit DebtsLoaded(fromCache: true)
else Fetch from API
DebtsBloc->>DebtsRepositoryImpl: getStudentDebts()
DebtsRepositoryImpl->>ApiClient: GET /infos/dettes/{uuid}
ApiClient-->>DebtsRepositoryImpl: response
DebtsRepositoryImpl-->>DebtsBloc: List<AcademicYearDebt>
DebtsBloc->>DebtsCacheService: cacheDebts(debts)
DebtsCacheService-->>DebtsBloc: cached
DebtsBloc->>DebtsPage: Emit DebtsLoaded(fromCache: false)
end
alt Fetch error
DebtsBloc->>DebtsCacheService: getCachedDebts()
DebtsCacheService-->>DebtsBloc: maybe cached debts / null
alt cached exists
DebtsBloc->>DebtsPage: Emit DebtsLoaded(fromCache: true)
else
DebtsBloc->>DebtsPage: Emit DebtsError(message)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
lib/l10n/app_localizations_ar.dart (1)
532-540: Consider enhanced Arabic plural forms for grammatical accuracy.Arabic has six plural categories (zero, one, two, few, many, other) according to CLDR. The current implementation only handles
oneandother, which works but may not be grammatically ideal for all count values (e.g., dual form for 2, or specific forms for 3-10).If more precise Arabic pluralization is desired, consider:
String debtCoursesCount(int count) { String _temp0 = intl.Intl.pluralLogic( count, locale: localeName, other: 'مقاييس راسبة', one: 'مقياس راسب', + two: 'مقياسان راسبان', + few: 'مقاييس راسبة', + many: 'مقياساً راسباً', ); return '$count $_temp0'; }This is optional and can be deferred if not critical for the target audience.
lib/features/dashboard/presentation/widgets/dashboard.dart (1)
143-149: New academic debts dashboard card is wired correctly; minor consistency nitThe card correctly uses the new localization key and navigates to the
debtsroute; behavior looks good. The only small nit is consistency: surrounding cards callgoNamedwith the*Pathconstants, while this one usesAppRouter.debts. Because both constants are the same string this is safe, but you may want to standardize on either the name or path constant for readability.lib/config/routes/app_router.dart (1)
6-6: Debts route integration under the dashboard shell looks correctThe new
debtsroute constants and nestedGoRouteunderdashboardPathare consistent with the existing routing structure and match the dashboard card’sgoNamed(AppRouter.debts)usage, so navigation to/dashboard/debtsshould work as expected. If you ever clean up this file, you might consider enforcing a single convention (always using either the*Pathor non‑Pathconstants for route names) to avoid confusion.Also applies to: 36-37, 53-54, 135-139
lib/features/debts/presentation/widgets/debt_course_card.dart (1)
1-220: DebtCourseCard is solid; consider locale‑aware course labelsThe widget structure and theming look good and should render debts cleanly. Two small improvements you might consider:
- Right now the title always uses
course.mcFr. If the model also exposesmcAr, you could pick between them based on the active locale so Arabic users see Arabic course names.- For the main title text you might want to start from
theme.textTheme.titleMedium(or similar) andcopyWithrather than constructing a freshTextStyle, to stay aligned with global typography defaults.These are purely refinements; the current implementation is functionally fine.
lib/features/debts/data/repositories/debts_repository_impl.dart (1)
1-28: Repository logic is fine; exception handling can be simplified
getStudentDebtscorrectly guards on missing UUID, hits the debts endpoint, and parses the result intoAcademicYearDebtobjects. Two small cleanups you might consider:
- The
try/catchthat just doesrethrowis redundant; you can drop thetryblock entirely unless you plan to map or log errors here.- Instead of throwing a generic
Exception('UUID not found, please login again')from the data layer, you could define a small domain/ auth‑related exception and let the bloc map that to a localized, user‑friendly message.Neither change is required for correctness, but they’d make error handling clearer.
lib/features/debts/presentation/pages/debts_page.dart (2)
21-25: Bloc usage is correct; consider standardizing oncontext.readDispatching
LoadDebtsininitStateand from the refresh button is the right behavior. For consistency with the rest of the file you might switch theinitStatecall fromBlocProvider.of<DebtsBloc>(context)tocontext.read<DebtsBloc>()so all dispatches use the same API.Also applies to: 44-46
48-53: Be mindful of how much error detail you surface to users
_buildErrorStateshows both a generic localized “something went wrong” title and the rawmessagefromDebtsError. If that message is derived from exceptions or backend responses, it could include technical details that you might not want in the UI. Consider mapping known failures to user‑friendly localized messages in the bloc and reserving the raw message for logs or debug builds.Also applies to: 106-125
lib/features/debts/presentation/bloc/debts_bloc.dart (1)
8-69: Robust cache‑first load flow; consider emitting a state on cache clearThe load handler nicely prioritizes fresh cache, falls back to remote, and uses cached data as a safety net on errors—good resilience and separation of concerns.
Optionally, you could have
_onClearCachealso emit something likeDebtsEmptyorDebtsInitialso any debts screen listening to the bloc immediately reflects that the cache was cleared (unless you intentionally want this event to be side‑effect only).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
lib/app.dart(2 hunks)lib/config/routes/app_router.dart(4 hunks)lib/core/di/injector.dart(3 hunks)lib/features/auth/presentation/bloc/auth_bloc.dart(2 hunks)lib/features/dashboard/presentation/widgets/dashboard.dart(1 hunks)lib/features/debts/data/models/academic_year_debt.dart(1 hunks)lib/features/debts/data/models/debt_course.dart(1 hunks)lib/features/debts/data/repositories/debts_repository_impl.dart(1 hunks)lib/features/debts/data/services/debts_cache_service.dart(1 hunks)lib/features/debts/presentation/bloc/debts_bloc.dart(1 hunks)lib/features/debts/presentation/bloc/debts_event.dart(1 hunks)lib/features/debts/presentation/bloc/debts_state.dart(1 hunks)lib/features/debts/presentation/pages/debts_page.dart(1 hunks)lib/features/debts/presentation/widgets/debt_course_card.dart(1 hunks)lib/features/debts/presentation/widgets/year_summary_card.dart(1 hunks)lib/l10n/app_ar.arb(1 hunks)lib/l10n/app_en.arb(1 hunks)lib/l10n/app_localizations.dart(1 hunks)lib/l10n/app_localizations_ar.dart(1 hunks)lib/l10n/app_localizations_en.dart(1 hunks)
🔇 Additional comments (15)
lib/l10n/app_localizations.dart (1)
1041-1101: LGTM! Localization entries for debts feature are well-structured.The new abstract getters and the
debtCoursesCountmethod with pluralization support follow the established patterns in the file. Documentation comments are properly formatted.lib/l10n/app_localizations_en.dart (1)
516-555: LGTM! English translations correctly implemented.The
debtCoursesCountmethod properly usesIntl.pluralLogicwith the correct locale and plural forms. All translations follow the established pattern.lib/l10n/app_en.arb (1)
673-720: LGTM! ARB entries properly defined with correct ICU message format.The pluralization syntax
{count, plural, =1{course} other{courses}}follows ICU MessageFormat correctly. Placeholder metadata is complete with type and example values.lib/features/auth/presentation/bloc/auth_bloc.dart (2)
5-6: LGTM! Imports added for debts feature integration.
116-120: LGTM! Debts cache clearing follows established pattern.The cache clearing for
DebtsBlocon logout is consistent with other bloc cache operations in the method. Error handling properly catches exceptions and logs them without interrupting the logout flow.lib/app.dart (1)
11-11: DebtsBloc registration matches existing DI patternAdding
DebtsBlocto theMultiBlocProviderviainjector<DebtsBloc>()is consistent with the other feature blocs and should make the debts feature available wherever needed without side effects.Also applies to: 38-38
lib/core/di/injector.dart (1)
28-30: Debts feature DI wiring is consistent with the rest of the appThe new registrations for
DebtsRepositoryImpl,DebtsCacheService, andDebtsBlocfollow the same singleton/factory pattern used by other features and respect dependency order (ApiClient → repositories → caches → blocs). This should integrate cleanly without lifecycle surprises.Also applies to: 35-35, 61-63, 69-69, 120-122
lib/features/debts/presentation/bloc/debts_event.dart (1)
1-21: DebtsEvent hierarchy is minimal and idiomaticThe event definitions (base
DebtsEvent,LoadDebtswithforceRefresh, andClearDebtsCache) follow the usual Equatable/Bloc patterns and give you the flexibility to distinguish cache‑busting loads without overcomplicating the API.lib/features/debts/presentation/pages/debts_page.dart (1)
131-238: Debts view composition and caching indicator look goodThe debts view correctly:
- Computes
totalDebtsfromyear.dette.length.- Shows a clear cached‑data indicator when
state.fromCacheis true.- Summarizes counts and then renders per‑year sections via
YearSummaryCardandDebtCourseCard.This structure should scale fine for typical datasets and keeps the UI logic nicely contained in helpers.
lib/features/debts/data/models/debt_course.dart (1)
1-62: DebtCourse DTO and JSON mapping look solidFields, constructor, and fromJson/toJson are symmetric and strongly typed; this is a clean, predictable model for the debts API.
lib/features/debts/data/models/academic_year_debt.dart (1)
1-35: AcademicYearDebt mapping is consistent and readableThe model cleanly represents the API shape, and the nested dette list mapping via DebtCourse.fromJson/toJson is straightforward and symmetric.
lib/features/debts/presentation/bloc/debts_state.dart (1)
1-34: Well-structured DebtsState hierarchyThe state set (initial/loading/loaded/empty/error) is clear, and Equatable props are correctly overridden so Bloc comparisons will behave as expected.
lib/features/debts/presentation/widgets/year_summary_card.dart (1)
5-93: YearSummaryCard is cleanly composed and localization-awareThe card layout, theming usage, and localized texts (year wrapper + pluralized debt count) are well put together and should be easy to reuse across the debts UI.
lib/l10n/app_ar.arb (1)
692-737: Debts-related Arabic localizations look consistent and well-scopedThe new debts strings (titles, empty-state description, summary labels, and pluralized debtCoursesCount) integrate cleanly with the existing ARB structure and match the fields used in the new UI.
lib/features/debts/data/services/debts_cache_service.dart (1)
12-23: Rewritten review comment should be removed — SharedPreferences API does not support the suggested fixThe suggestion to capture boolean returns from
setString()andremove()is based on outdated API behavior. In shared_preferences v2.5.3 (the version in pubspec.yaml), these methods returnFuture<void>, notFuture<bool>. The current implementation correctly handles errors via exception handling, which is the appropriate pattern for this API. The code as-is is correct and follows the intended design of the shared_preferences package.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/features/debts/data/repositories/debts_repository_impl.dart (1)
11-28: Remove the try-catch block or add meaningful error handling.The try-catch block that only rethrows adds no value. Either remove it entirely or add meaningful error handling (e.g., logging, exception transformation, or adding context).
🔎 Proposed fix
Future<List<AcademicYearDebt>> getStudentDebts() async { - try { - final uuid = await _apiClient.getUuid(); - if (uuid == null) { - throw Exception('UUID not found, please login again'); - } - final response = await _apiClient.get('/infos/dettes/$uuid'); - if (response.data == null) { - return []; - } - final List<dynamic> debtsJson = response.data; - final debts = debtsJson - .map((debtJson) => AcademicYearDebt.fromJson(debtJson)) - .toList(); - debts.sort((a, b) => b.idAnneeAcademique.compareTo(a.idAnneeAcademique)); - return debts; - } catch (e) { - rethrow; - } + final uuid = await _apiClient.getUuid(); + if (uuid == null) { + throw Exception('UUID not found, please login again'); + } + final response = await _apiClient.get('/infos/dettes/$uuid'); + if (response.data == null) { + return []; + } + final List<dynamic> debtsJson = response.data; + final debts = debtsJson + .map((debtJson) => AcademicYearDebt.fromJson(debtJson)) + .toList(); + debts.sort((a, b) => b.idAnneeAcademique.compareTo(a.idAnneeAcademique)); + return debts; }
| DebtsRepositoryImpl({ApiClient? apiClient}) | ||
| : _apiClient = apiClient ?? ApiClient(); |
There was a problem hiding this comment.
Remove the fallback ApiClient() instantiation.
Creating a new ApiClient() when none is provided defeats dependency injection and makes the class difficult to test. According to the DI configuration, apiClient is always injected when registering this repository. Make the parameter required to enforce proper DI usage.
🔎 Proposed fix
- DebtsRepositoryImpl({ApiClient? apiClient})
- : _apiClient = apiClient ?? ApiClient();
+ DebtsRepositoryImpl({required ApiClient apiClient})
+ : _apiClient = apiClient;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DebtsRepositoryImpl({ApiClient? apiClient}) | |
| : _apiClient = apiClient ?? ApiClient(); | |
| DebtsRepositoryImpl({required ApiClient apiClient}) | |
| : _apiClient = apiClient; |
🤖 Prompt for AI Agents
In lib/features/debts/data/repositories/debts_repository_impl.dart around lines
7-8, the constructor currently falls back to creating a new ApiClient() when
none is passed, which breaks DI and testing; change the constructor to require
an ApiClient parameter (remove the default null and the fallback instantiation)
and assign that required ApiClient to _apiClient so the repository always
receives the injected dependency.
Description
Types of changes
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.