feat(tests): add authenticated test attempt flow - #51
Merged
Merged
Conversation
…l + fixtures and edit guest ones
CowboyGH
added
area: network
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an authenticated test-attempt flow alongside the existing guest flow. It introduces new DTOs and API paths for authenticated start/save/complete operations, splits the repository contract into GuestTestAttemptRepository and AuthenticatedTestAttemptRepository with separate implementations, updates DI to register both implementations, adds router paths and a TestsAttemptPage/Builder for the authenticated UI, and changes tests catalog navigation to open the authenticated attempt route. The shared TestAttemptCubit and its domain contract remain unchanged. Sequence Diagram(s)sequenceDiagram
actor User
participant Router as GoRouter
participant UI as TestsAttemptPage
participant Cubit as TestAttemptCubit
participant Repo as AuthenticatedTestAttemptRepository
participant API as TestsApiClient
participant BE as Backend
User->>Router: Navigate to /tests/attempt/:testingId
Router->>UI: Build TestsAttemptPageBuilder (testingId)
UI->>Cubit: create(di<AuthenticatedTestAttemptRepository>) / startTest(testingId)
Cubit->>Repo: startTest(testingId)
Repo->>API: startTest(testingId)
API->>BE: GET /api/tests/:testingId/start
BE-->>API: StartTestResponseDto
API-->>Repo: StartTestDataDto
Repo-->>Cubit: Result<TestAttemptStart>
Cubit-->>UI: update state (show exercise)
User->>UI: choose result (1-4)
UI->>Cubit: saveResult(attemptId, exerciseId, value)
Cubit->>Repo: saveResult(...)
Repo->>API: saveTestResult(attemptId, SaveTestResultRequestDto)
API->>BE: POST /api/test-attempts/:attemptId/result
BE-->>API: SaveTestResultResponseDto
API-->>Repo: SaveTestResultDataDto
Repo-->>Cubit: Result<TestAttemptResult>
Cubit-->>UI: update state (next exercise or pulse)
User->>UI: submit pulse
UI->>Cubit: completeTest(attemptId, pulse)
Cubit->>Repo: completeTest(attemptId, pulse)
Repo->>API: completeTest(attemptId, CompleteTestRequestDto)
API->>BE: POST /api/test-attempts/:attemptId/complete
BE-->>API: 200 OK
API-->>Repo: success
Repo-->>Cubit: completion success
Cubit-->>UI: trigger navigation
UI->>Router: Navigate back to tests catalog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🧹 Nitpick comments (3)
lib/features/tests/attempt/presentation/pages/tests_attempt_page.dart (1)
101-103: Consider adding a brief delay before navigation for UX polish.Using
Duration.zeronavigates immediately on completion. If you want users to briefly see a success state before navigation, consider a small delay (e.g., 300-500ms).♻️ Optional: add brief delay
if (state.isCompleted) { - unawaited(Future<void>.delayed(Duration.zero, _returnToCatalog)); + unawaited(Future<void>.delayed(const Duration(milliseconds: 300), _returnToCatalog)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/tests/attempt/presentation/pages/tests_attempt_page.dart` around lines 101 - 103, The immediate navigation when state.isCompleted uses unawaited(Future<void>.delayed(Duration.zero, _returnToCatalog)); — change the delay to a short non-zero duration (e.g., Duration(milliseconds: 300) or 500) so the success state is visible briefly before calling _returnToCatalog; update the delayed call that wraps _returnToCatalog to use the chosen Duration.lib/features/tests/attempt/data/mappers/test_attempt_mapper.dart (1)
52-58: Consider renaming extension to match the DTO it operates on.The extension is named
SaveGuestTestResultMapperbut operates on the sharedSaveTestResultDataDto(not guest-specific). For consistency with the DTO renaming throughout this PR, consider renaming toSaveTestResultMapper.♻️ Suggested rename
-/// Extension that maps save-result DTOs to [TestAttemptResult]. -extension SaveGuestTestResultMapper on SaveTestResultDataDto { +/// Extension that maps [SaveTestResultDataDto] to [TestAttemptResult]. +extension SaveTestResultMapper on SaveTestResultDataDto {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/tests/attempt/data/mappers/test_attempt_mapper.dart` around lines 52 - 58, The extension SaveGuestTestResultMapper is misnamed for its target type SaveTestResultDataDto; rename the extension to SaveTestResultMapper so the extension name matches the DTO it operates on and any related refactors in this PR (update the declaration "extension SaveGuestTestResultMapper on SaveTestResultDataDto" to "extension SaveTestResultMapper on SaveTestResultDataDto" and update any references/imports if present).test/features/tests/attempt/support/test_attempt_dto_fixtures.dart (1)
48-59: Consider renaming fixture to match the returned DTO type.The function
createSaveGuestTestResultResponseDtonow returnsSaveTestResultResponseDto(not guest-specific), creating a naming mismatch with the PR's DTO renaming convention.♻️ Suggested rename
-/// Test fixture for save-result response DTO. -SaveTestResultResponseDto createSaveGuestTestResultResponseDto({ +/// Test fixture for save-result response DTO. +SaveTestResultResponseDto createSaveTestResultResponseDto({Note: This would require updating call sites in test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/tests/attempt/support/test_attempt_dto_fixtures.dart` around lines 48 - 59, Rename the fixture function createSaveGuestTestResultResponseDto to match the returned DTO name (e.g., createSaveTestResultResponseDto) because it returns SaveTestResultResponseDto (containing SaveTestResultDataDto); update the function declaration and all test call sites that reference createSaveGuestTestResultResponseDto to the new name so naming follows the DTO renaming convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 30-31: Move the two bullet points about DTO renames and
test-attempt DI wiring into the Unreleased section under a new or existing "###
Breaking" heading: cut the bullets "- Shared test-attempt transport DTOs..." and
"- Test-attempt DI wiring..." and paste them beneath "### Breaking" in the
Unreleased section (create the heading if missing), preserving markdown list
formatting and keeping the rest of the Unreleased content intact so
compatibility-impacting changes are clearly labeled.
In `@lib/features/tests/attempt/data/dto/save_test_result_response_dto.dart`:
- Line 10: Update the stale doc comment on the SaveTestResultResponseDto so it
no longer reads "Guest result payload" and instead describes that this DTO
represents a test result payload shared across multiple flows; locate the class
or typedef named SaveTestResultResponseDto in the file and replace the
single-line summary comment with a concise, accurate description such as "Test
result payload shared across flows" (or equivalent wording).
In
`@test/features/tests/attempt/data/repositories/authenticated_test_attempt_repository_impl_test.dart`:
- Around line 211-226: The test 'returns UnknownTestsFailure when unexpected
exception occurs' is missing verification that logger.e was called when
apiClient.completeTest throws; update the test for repository.completeTest to
verify logger.e was invoked (matching the pattern used in the startTest
unexpected-exception spec), i.e., after calling repository.completeTest and
asserting failure, add a verify(logger.e(any, any)) (or the exact logger.e
invocation used elsewhere) and verifyNoMoreInteractions(logger) as appropriate
to ensure the unexpected exception is logged.
- Around line 127-143: Test is missing verification that the repository logs the
malformed payload error; update the test for saveResult (the case where
apiClient.saveTestResult returns a malformed response) to also verify that
logger.e was called with the expected error context. Specifically, in the test
surrounding repository.saveResult
(AuthenticatedTestAttemptRepositoryImpl.saveResult scenario), add a verify call
for the mocked logger's e method (or logger.error equivalent) to assert it was
invoked once with an error containing the StateError (or at least any error),
and keep the existing verify(apiClient.saveTestResult(...)). Ensure you
reference the same mock logger instance used by the repository in the test setup
and use verifyNoMoreInteractions on the logger if desired.
---
Nitpick comments:
In `@lib/features/tests/attempt/data/mappers/test_attempt_mapper.dart`:
- Around line 52-58: The extension SaveGuestTestResultMapper is misnamed for its
target type SaveTestResultDataDto; rename the extension to SaveTestResultMapper
so the extension name matches the DTO it operates on and any related refactors
in this PR (update the declaration "extension SaveGuestTestResultMapper on
SaveTestResultDataDto" to "extension SaveTestResultMapper on
SaveTestResultDataDto" and update any references/imports if present).
In `@lib/features/tests/attempt/presentation/pages/tests_attempt_page.dart`:
- Around line 101-103: The immediate navigation when state.isCompleted uses
unawaited(Future<void>.delayed(Duration.zero, _returnToCatalog)); — change the
delay to a short non-zero duration (e.g., Duration(milliseconds: 300) or 500) so
the success state is visible briefly before calling _returnToCatalog; update the
delayed call that wraps _returnToCatalog to use the chosen Duration.
In `@test/features/tests/attempt/support/test_attempt_dto_fixtures.dart`:
- Around line 48-59: Rename the fixture function
createSaveGuestTestResultResponseDto to match the returned DTO name (e.g.,
createSaveTestResultResponseDto) because it returns SaveTestResultResponseDto
(containing SaveTestResultDataDto); update the function declaration and all test
call sites that reference createSaveGuestTestResultResponseDto to the new name
so naming follows the DTO renaming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f0ab3030-b683-4372-bac0-d76e58631b5f
📒 Files selected for processing (28)
CHANGELOG.mdlib/core/di/di.dartlib/core/network/api_paths.dartlib/core/router/router.dartlib/core/router/router_paths.dartlib/features/fitness_start/presentation/pages/fitness_start_test_attempt_page_builder.dartlib/features/tests/attempt/data/dto/complete_guest_test_request_dto.dartlib/features/tests/attempt/data/dto/complete_test_request_dto.dartlib/features/tests/attempt/data/dto/save_guest_test_result_request_dto.dartlib/features/tests/attempt/data/dto/save_guest_test_result_response_dto.dartlib/features/tests/attempt/data/dto/save_test_result_data_dto.dartlib/features/tests/attempt/data/dto/save_test_result_request_dto.dartlib/features/tests/attempt/data/dto/save_test_result_response_dto.dartlib/features/tests/attempt/data/dto/start_test_data_dto.dartlib/features/tests/attempt/data/dto/start_test_response_dto.dartlib/features/tests/attempt/data/mappers/test_attempt_mapper.dartlib/features/tests/attempt/data/repositories/authenticated_test_attempt_repository_impl.dartlib/features/tests/attempt/data/repositories/guest_test_attempt_repository_impl.dartlib/features/tests/attempt/data/repositories/test_attempt_result_payload_validator.dartlib/features/tests/attempt/domain/repositories/test_attempt_repository.dartlib/features/tests/attempt/presentation/cubits/test_attempt_cubit.dartlib/features/tests/attempt/presentation/pages/tests_attempt_page.dartlib/features/tests/attempt/presentation/pages/tests_attempt_page_builder.dartlib/features/tests/catalog/presentation/pages/tests_catalog_page.dartlib/features/tests/data/remote/tests_api_client.darttest/features/tests/attempt/data/repositories/authenticated_test_attempt_repository_impl_test.darttest/features/tests/attempt/data/repositories/guest_test_attempt_repository_impl_test.darttest/features/tests/attempt/support/test_attempt_dto_fixtures.dart
💤 Files with no reviewable changes (3)
- lib/features/tests/attempt/data/dto/complete_guest_test_request_dto.dart
- lib/features/tests/attempt/data/dto/save_guest_test_result_request_dto.dart
- lib/features/tests/attempt/data/dto/save_guest_test_result_response_dto.dart
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/features/tests/attempt/data/repositories/authenticated_test_attempt_repository_impl_test.dart (1)
82-166: Missing test case for general exception insaveResult.The
saveResultimplementation has a catch-allcatch (e, s)block (lines 62-64 in the implementation) that handles unexpected exceptions and logs vialogger.e, but there's no corresponding test. BothstartTest(line 66-79) andcompleteTest(line 211-227) have this test case for consistency.💚 Proposed test to add
test('returns UnknownTestsFailure when unexpected exception occurs', () async { final exception = Exception('unexpected_error'); when(apiClient.saveTestResult(any, any)).thenThrow(exception); final result = await repository.saveResult( attemptId: '11', testingExerciseId: 16, resultValue: 2, ); expect(result.isFailure, isTrue); expect(result.failure, isA<UnknownTestsFailure>()); expect(result.failure!.parentException, exception); verify(apiClient.saveTestResult(any, any)).called(1); verify(logger.e(any, exception, any)).called(1); verifyNoMoreInteractions(apiClient); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/tests/attempt/data/repositories/authenticated_test_attempt_repository_impl_test.dart` around lines 82 - 166, Add a test for saveResult to cover the catch-all exception path: when apiClient.saveTestResult throws a general Exception, assert the repository.saveResult returns a failure of type UnknownTestsFailure with parentException equal to that exception, verify apiClient.saveTestResult was called once, and verify logger.e was called once with the exception; reference the repository.saveResult method, the apiClient.saveTestResult stub, UnknownTestsFailure, and logger.e to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@test/features/tests/attempt/data/repositories/authenticated_test_attempt_repository_impl_test.dart`:
- Around line 82-166: Add a test for saveResult to cover the catch-all exception
path: when apiClient.saveTestResult throws a general Exception, assert the
repository.saveResult returns a failure of type UnknownTestsFailure with
parentException equal to that exception, verify apiClient.saveTestResult was
called once, and verify logger.e was called once with the exception; reference
the repository.saveResult method, the apiClient.saveTestResult stub,
UnknownTestsFailure, and logger.e to locate where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3590b1d9-b106-42b4-94c8-b28203db56b1
📒 Files selected for processing (3)
CHANGELOG.mdlib/features/tests/attempt/data/dto/save_test_result_response_dto.darttest/features/tests/attempt/data/repositories/authenticated_test_attempt_repository_impl_test.dart
✅ Files skipped from review due to trivial changes (2)
- lib/features/tests/attempt/data/dto/save_test_result_response_dto.dart
- CHANGELOG.md
…aveResult method.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 Summary
Implemented the authenticated test attempt flow for the
/testsroot tab by reusing the existingtests/attemptbusiness logic, adding auth API/repository support, normalizing shared attempt DTOs, and wiring a fullscreen attempt screen that mirrors the Fitness Start attempt UI.✨ Changes
TestsApiClient/api/tests/{testing}/startAuthenticatedTestAttemptRepositoryImplfor auth test attempts + unit testsFitnessStartTestAttemptPageBuilderto resolve the dedicated guest attempt repository explicitlyTestAttemptCubit/TestAttemptStatewithout changing their public contractGuestTestAttemptRepositoryandAuthenticatedTestAttemptRepositorybindings instead of relying on a single sharedTestAttemptRepositoryDI registration🧪 Verification
flutter analyzeflutter test test/features/tests/attempt/tests, starts a test, progresses through exercises, submits pulse, and returns to the tests catalog