Fix: Require Download started before Confirmation #559
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
CelineTrammi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/Altinn.Broker.Application/ConfirmDownload/ConfirmDownloadHandler.cs (1)
54-54: Track the TODO comment about DownloadFinished status.The comment indicates a planned enhancement to use
DownloadFinishedinstead ofDownloadStarted. This should be tracked to ensure it's not forgotten.Would you like me to create a GitHub issue to track this enhancement? The issue would cover:
- Adding a new
DownloadFinishedstatus- Updating the validation logic to use this status
- Ensuring backward compatibility during the transition
tests/Altinn.Broker.Tests/FileTransferControllerTests.cs (1)
238-255: LGTM! Consider adding a comment explaining the test scenario.The test implementation is well-structured and follows the AAA pattern. It effectively verifies that attempting to confirm a download before it starts fails with the correct error message.
Consider adding a brief XML comment above the test method to document the specific scenario being tested and its importance. For example:
+/// <summary> +/// Verifies that attempting to confirm a file transfer before initiating the download +/// fails with the appropriate error message. This prevents premature confirmations +/// that could lead to inconsistent state. +/// </summary> [Fact] public async Task ConfirmFileTransferBeforeDownloadStarted_Fails()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/Altinn.Broker.Application/ConfirmDownload/ConfirmDownloadHandler.cs (1 hunks)
- src/Altinn.Broker.Application/Errors.cs (1 hunks)
- tests/Altinn.Broker.Tests/FileTransferControllerTests.cs (1 hunks)
- tests/Altinn.Broker.Tests/LegacyFileControllerTests.cs (1 hunks)
🔇 Additional comments (2)
src/Altinn.Broker.Application/Errors.cs (1)
27-27: LGTM! New error definition aligns with PR objectives.The new
ConfirmDownloadBeforeDownloadStartederror is well-defined with a clear message and appropriate HTTP status code. It properly supports the validation requirement ensuring downloads are initiated before confirmation.tests/Altinn.Broker.Tests/LegacyFileControllerTests.cs (1)
435-436: LGTM! Good addition of download verification.The explicit check for download success before proceeding with confirmation aligns well with the PR objective of ensuring downloads are initiated before confirmation.
src/Altinn.Broker.Application/ConfirmDownload/ConfirmDownloadHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Broker.Application/ConfirmDownload/ConfirmDownloadHandler.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Altinn.Broker.Application/ConfirmDownload/ConfirmDownloadHandler.cs (1)
54-56: LGTM! Consider implementing the TODO comment in a separate PR.The validation logic correctly checks if the current recipient has started their download before allowing confirmation. This fixes the issue mentioned in the previous review where all recipients were being checked.
However, there's a TODO comment about replacing
DownloadStartedwithDownloadFinished. This would be a breaking change and should be tracked separately.Would you like me to create a GitHub issue to track the implementation of
DownloadFinishedstatus?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/Altinn.Broker.Application/ConfirmDownload/ConfirmDownloadHandler.cs (1 hunks)
🔇 Additional comments (1)
src/Altinn.Broker.Application/ConfirmDownload/ConfirmDownloadHandler.cs (1)
54-57: Well-integrated validation logic.The new validation check is cleanly integrated into the existing flow:
- It occurs at the right point - after basic checks but before the transaction
- Returns a specific error that clearly indicates the issue
- Maintains the integrity of the subsequent transaction and cleanup logic
Description
Add validation to ensure Download has been started before the user can Confirm a download.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation