Add AttachmentService test suite - #39
Conversation
…achmentServiceTest`
… for anonymous reporter token validation
…hmentServiceTest`
…serOrNull` method
|
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 46 minutes and 13 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a new comprehensive JUnit 5 test suite for AttachmentService and minor whitespace formatting changes in existing test classes and one blank-line removal in AttachmentService source. Tests cover upload behavior, authorization checks, error handling, filename defaults, and storage cleanup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (2)
src/test/java/org/example/alfs/services/AttachmentServiceTest.java (1)
243-251: Align the invalid-token test name with the asserted status.The method name says
throwsUnauthorized, but Line 251 assertsHttpStatus.NOT_FOUND. Rename the test ifNOT_FOUNDis intentional.🧹 Proposed naming cleanup
- `@DisplayName`("Anonymous reporter with invalid token denied") - void anonymousReporter_withWrongToken_throwsUnauthorized() { + `@DisplayName`("Anonymous reporter with invalid token should return Not Found") + void anonymousReporter_withWrongToken_throwsNotFound() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/alfs/services/AttachmentServiceTest.java` around lines 243 - 251, The test method anonymousReporter_withWrongToken_throwsUnauthorized has a name indicating UNAUTHORIZED but asserts HttpStatus.NOT_FOUND; make them consistent by either renaming the test method to reflect NOT_FOUND (e.g., anonymousReporter_withWrongToken_throwsNotFound) or changing the asserted status in the assertion to HttpStatus.UNAUTHORIZED depending on intended behavior of attachmentService.uploadToTicket; update the method name or the assertion in AttachmentServiceTest.uploadToTicket test accordingly.src/main/java/org/example/alfs/services/AttachmentService.java (1)
99-116: Resolve or track this TODO instead of leaving suspected dead code.If
getCurrentUserOrNull()has no usages, prefer removing the helper and the now-unusedSecurityUtilsdependency; otherwise, link this TODO to a tracked issue so it does not become stale.Do you want me to draft the cleanup diff for the method, field, constructor parameter, and test mock?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/services/AttachmentService.java` around lines 99 - 116, getCurrentUserOrNull() is likely dead code; search for usages of the method and the securityUtils dependency and either (A) remove the private method getCurrentUserOrNull(), delete the SecurityUtils field/constructor parameter and update any tests/mocks that reference it, or (B) if it is needed later, replace the inline TODO with a link to an open issue/track ID and add a short javadoc explaining intended ownership (e.g., "moved-to-service #<ISSUE_ID>"). Reference getCurrentUserOrNull, securityUtils, and SecurityUtils when making the change so related constructor params, fields, and test mocks are cleaned up or the TODO is tracked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/org/example/alfs/services/AttachmentServiceTest.java`:
- Around line 80-92: The test
authenticatedReporter_withValidTicket_shouldUploadSuccessfully currently passes
admin to attachmentService.uploadToTicket, so update the call to use the
reporter test fixture instead (replace admin with reporter) so the test
exercises the reporter scenario; keep the existing mocks
(ticketRepository.findById, storageService.upload, file.getOriginalFilename) and
assertions on result.getFileName(), result.getS3Key(), and
verify(attachmentRepository).save(...) intact.
---
Nitpick comments:
In `@src/main/java/org/example/alfs/services/AttachmentService.java`:
- Around line 99-116: getCurrentUserOrNull() is likely dead code; search for
usages of the method and the securityUtils dependency and either (A) remove the
private method getCurrentUserOrNull(), delete the SecurityUtils
field/constructor parameter and update any tests/mocks that reference it, or (B)
if it is needed later, replace the inline TODO with a link to an open
issue/track ID and add a short javadoc explaining intended ownership (e.g.,
"moved-to-service #<ISSUE_ID>"). Reference getCurrentUserOrNull, securityUtils,
and SecurityUtils when making the change so related constructor params, fields,
and test mocks are cleaned up or the TODO is tracked.
In `@src/test/java/org/example/alfs/services/AttachmentServiceTest.java`:
- Around line 243-251: The test method
anonymousReporter_withWrongToken_throwsUnauthorized has a name indicating
UNAUTHORIZED but asserts HttpStatus.NOT_FOUND; make them consistent by either
renaming the test method to reflect NOT_FOUND (e.g.,
anonymousReporter_withWrongToken_throwsNotFound) or changing the asserted status
in the assertion to HttpStatus.UNAUTHORIZED depending on intended behavior of
attachmentService.uploadToTicket; update the method name or the assertion in
AttachmentServiceTest.uploadToTicket test accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 718f7f6b-66a7-4c50-8bf9-b1227d166a62
📒 Files selected for processing (4)
src/main/java/org/example/alfs/services/AttachmentService.javasrc/test/java/org/example/alfs/services/AttachmentServiceTest.javasrc/test/java/org/example/alfs/services/AuthServiceTest.javasrc/test/java/org/example/alfs/services/TicketServiceTest.java
💤 Files with no reviewable changes (2)
- src/test/java/org/example/alfs/services/TicketServiceTest.java
- src/test/java/org/example/alfs/services/AuthServiceTest.java
…ust test for invalid reporter token to return Not Found
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/org/example/alfs/services/AttachmentServiceTest.java (3)
166-263: Access-control matrix is thorough — consider whether tests belong here or inUploadToTicketTest.All
CheckAccessTestcases exercisecheckAccessindirectly throughuploadToTicket, which is fine, but means failures here can be caused by unrelated changes to the upload pipeline. IfcheckAccessis (or can be made) package-private, calling it directly would give these tests tighter scope and avoid the need to stubstorageService.upload/file.getOriginalFilenamein the allowed-path cases. Not blocking — purely an optional structural cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/alfs/services/AttachmentServiceTest.java` around lines 166 - 263, These tests exercise access control indirectly via attachmentService.uploadToTicket which forces stubbing of storageService.upload and MultipartFile behavior; instead, extract or expose the checkAccess logic (the method checkAccess or equivalent inside AttachmentService) as package-private and write the access-focused assertions to call checkAccess directly from CheckAccessTest, so allowed-path tests no longer need to stub storageService.upload or file.getOriginalFilename — update CheckAccessTest to invoke AttachmentService.checkAccess(ticketId, user, reporterToken) (or the precise method name) and keep uploadToTicket tests (UploadToTicketTest) for end-to-end upload behavior.
139-149: Consider also verifying persistence on the filename-fallback path.The test asserts the fallback name but doesn't verify
attachmentRepository.save(...)was invoked. Addingverify(attachmentRepository).save(any(Attachment.class));(and optionallyassertThat(result.getS3Key()).isEqualTo("s3-key")) would make the happy-path expectations explicit and symmetric with the other upload tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/alfs/services/AttachmentServiceTest.java` around lines 139 - 149, Update the test nullFileName_fallsBackToDefaultName in AttachmentServiceTest to also verify persistence and S3 key: after calling attachmentService.uploadToTicket(10L, file, admin, null) add a verify(attachmentRepository).save(any(Attachment.class)) to ensure the Attachment was persisted, and add an assertion that result.getS3Key() isEqualTo("s3-key") to assert the upload path; this uses the existing mocks (ticketRepository, storageService) and the returned result from uploadToTicket.
255-262: Duplicate ofanonymousReporter_withMissingToken_throwsUnauthorizedinUploadToTicketTest(lines 108–116).This test in
CheckAccessTestcovers the exact same scenario (anonymous caller,token == null, expectsUNAUTHORIZED) as the one inUploadToTicketTest. The 401 is raised by the pre-access token validation inuploadToTicket, not bycheckAccess, so it arguably belongs only in the upload suite. Consider removing this one or reshaping it to actually exercisecheckAccess(e.g., with a non-empty token that doesn't match the ticket's reporter token — though that case is already covered byanonymousReporter_withWrongToken_throwsNotFound).✂️ Suggested removal
- `@Test` - `@DisplayName`("Anonymous reporter with no token denied") - void anonymousReporter_withMissingToken_throwsUnauthorized() { - ResponseStatusException ex = assertThrows(ResponseStatusException.class, - () -> attachmentService.uploadToTicket(10L, file, null, null)); - - assertThat(ex.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/alfs/services/AttachmentServiceTest.java` around lines 255 - 262, The test anonymousReporter_withMissingToken_throwsUnauthorized in AttachmentServiceTest duplicates the same scenario already covered in UploadToTicketTest (uploadToTicket with token == null), so either delete this test from AttachmentServiceTest or convert it to directly exercise the checkAccess method: replace the call to attachmentService.uploadToTicket(10L, file, null, null) with a call that invokes checkAccess (e.g., attachmentService.checkAccess(ticketId, token)) using a non-null token that should exercise the checkAccess logic (or assert the specific exception expected from checkAccess), and keep the test name in sync with the new behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/java/org/example/alfs/services/AttachmentServiceTest.java`:
- Around line 166-263: These tests exercise access control indirectly via
attachmentService.uploadToTicket which forces stubbing of storageService.upload
and MultipartFile behavior; instead, extract or expose the checkAccess logic
(the method checkAccess or equivalent inside AttachmentService) as
package-private and write the access-focused assertions to call checkAccess
directly from CheckAccessTest, so allowed-path tests no longer need to stub
storageService.upload or file.getOriginalFilename — update CheckAccessTest to
invoke AttachmentService.checkAccess(ticketId, user, reporterToken) (or the
precise method name) and keep uploadToTicket tests (UploadToTicketTest) for
end-to-end upload behavior.
- Around line 139-149: Update the test nullFileName_fallsBackToDefaultName in
AttachmentServiceTest to also verify persistence and S3 key: after calling
attachmentService.uploadToTicket(10L, file, admin, null) add a
verify(attachmentRepository).save(any(Attachment.class)) to ensure the
Attachment was persisted, and add an assertion that result.getS3Key()
isEqualTo("s3-key") to assert the upload path; this uses the existing mocks
(ticketRepository, storageService) and the returned result from uploadToTicket.
- Around line 255-262: The test
anonymousReporter_withMissingToken_throwsUnauthorized in AttachmentServiceTest
duplicates the same scenario already covered in UploadToTicketTest
(uploadToTicket with token == null), so either delete this test from
AttachmentServiceTest or convert it to directly exercise the checkAccess method:
replace the call to attachmentService.uploadToTicket(10L, file, null, null) with
a call that invokes checkAccess (e.g., attachmentService.checkAccess(ticketId,
token)) using a non-null token that should exercise the checkAccess logic (or
assert the specific exception expected from checkAccess), and keep the test name
in sync with the new behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae8010c2-835e-45f7-886f-b07da8ac6e74
📒 Files selected for processing (2)
src/main/java/org/example/alfs/services/AttachmentService.javasrc/test/java/org/example/alfs/services/AttachmentServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/alfs/services/AttachmentService.java
1 participant
@coderabbitai review
Summary by CodeRabbit
Tests
Chores