Feature: Adjust Hub workflow, if device is already registerd #3210
Conversation
Co-authored-by: Tobias Hagemann <tobias.hagemann@skymatic.de>
| /** | ||
| * Thrown, when Hub registerDevice-Request returns with 409 | ||
| */ | ||
| public class DeviceAlreadyExistsException extends RuntimeException {} |
There was a problem hiding this comment.
We have a checked exception type for errors during unlock. I believe its name is sth like MasterkeyLoadingFailed
| if (cause instanceof DeviceAlreadyExistsException) { | ||
| LOG.debug("Device already registered in hub instance {} for different user", hubConfig.authSuccessUrl); | ||
| } else { |
There was a problem hiding this comment.
Can be a separate case in the switch block
| @Provides | ||
| @KeyLoadingScoped | ||
| @Named("registerException") | ||
| static AtomicReference<Throwable> provideRegisterException() { | ||
| return new AtomicReference<>(); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
I reused the registrationFailed dialog. To differentiate between "Device already exists" and any other generic error, i have to inject the Throwable into the dialog controller. This is the injection object.
There was a problem hiding this comment.
The point is: According to its name, this exception is specific to "registration", however I'm pretty certain that we already have some kind of mechanism to reference exceptional outcomes during this unlock workflow.
|
Warning Rate Limit Exceeded@overheadhunter has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 20 seconds before requesting another review. 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. WalkthroughThe updates primarily address error handling and user feedback for device registration with Cryptomator Hub. A new exception class is introduced for a specific error case, and related UI components are updated to reflect more accurate error messages. The changes ensure that the error presented to the user when attempting to register a device already in use is clear and accurate. Additionally, there's a minor update to handle a window close event. Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (10)
- src/main/java/org/cryptomator/ui/common/FxmlFile.java (1 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/DeviceAlreadyExistsException.java (1 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/HubKeyLoadingModule.java (2 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/HubKeyLoadingStrategy.java (2 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java (5 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java (6 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterFailedController.java (1 hunks)
- src/main/resources/fxml/hub_register_device.fxml (1 hunks)
- src/main/resources/fxml/hub_register_failed.fxml (1 hunks)
- src/main/resources/i18n/strings.properties (1 hunks)
Additional comments: 18
src/main/java/org/cryptomator/ui/keyloading/hub/DeviceAlreadyExistsException.java (1)
- 3-6: The
DeviceAlreadyExistsExceptionclass is well-defined and extendsRuntimeException. The comment above the class provides clear context for when this exception should be thrown.src/main/java/org/cryptomator/ui/keyloading/hub/RegisterFailedController.java (1)
- 7-37: The updates to
RegisterFailedControllerinclude handling a new type of exception (DeviceAlreadyExistsException) and differentiating between a generic error and a device already existing error. These changes are consistent with the PR objectives.src/main/resources/fxml/hub_register_failed.fxml (1)
- 41-42: The FXML changes include adding two new
Labelelements with conditional visibility based on controller properties, providing different error messages for generic registration failure and device already existing failure. These changes are consistent with the PR objectives.src/main/java/org/cryptomator/ui/common/FxmlFile.java (1)
- 26-26: The
FxmlFileenum has been updated to rename theHUB_SETUP_DEVICEconstant toHUB_REGISTER_DEVICE. This change is consistent with the PR objectives.src/main/java/org/cryptomator/ui/keyloading/hub/HubKeyLoadingStrategy.java (1)
- 46-46: The addition of a window close event handler in
HubKeyLoadingStrategyto cancel the registration process if the window is closed is a good control flow improvement and is consistent with the PR objectives.src/main/java/org/cryptomator/ui/keyloading/hub/HubKeyLoadingModule.java (2)
75-80: The addition of the
provideRegisterExceptionmethod inHubKeyLoadingModuleis a new feature that supports the differentiation of errors in the registration process, aligning with the PR objectives.146-149: The update to
provideHubRegisterDeviceScenemethod to use the renamed FXML fileHUB_REGISTER_DEVICEis consistent with the PR objectives and the renaming done in theFxmlFileenum.src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java (3)
58-80: The updates to
RegisterDeviceControllerinclude the addition of theregisterExceptionfield and modifications to the constructor to handle the new exception type. These changes are consistent with the PR objectives.173-178: The
handleRegisterDeviceResponsemethod has been updated to handle theDeviceAlreadyExistsExceptionspecifically, which aligns with the PR objectives to improve the user experience when a device is already registered with another user.188-193: The
setupFailedmethod has been refined to log different messages based on whether the cause is aDeviceAlreadyExistsExceptionor another type of exception. This is a good approach to provide more specific feedback to the user.src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java (3)
- 58-70: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [52-67]
The updates to
ReceiveKeyControllerinclude renaming thesetupDeviceScenefield toregisterDeviceScreenand modifying theneedsDeviceSetupmethod toneedsDeviceRegistration. These changes are consistent with the PR objectives.
142-142: The
needsDeviceRegistrationmethod has been correctly updated to reflect the new registration flow, which is part of the PR objectives to improve the device registration process.150-151: The
needsDeviceRegistrationmethod correctly sets the scene toregisterDeviceScreen, which is the updated flow for registering a device that is not yet associated with the user.src/main/resources/i18n/strings.properties (5)
164-164: The label for the registration button has been changed from "Confirm" to "Authorize" to better reflect the action being taken by the user.
166-166: The success message for device registration has been updated to "Device registered" for clarity and to confirm the successful action.
169-169: The failure message for device registration has been changed to "Device registration failed" which is more specific and informative.
170-170: The generic error description for a failed registration has been updated to reflect the registration context rather than the previous "naming" context.
171-171: A new error description has been added for the
DeviceAlreadyExistsException, providing clear guidance for the user on how to proceed if the device is already registered under a different user.
use separate views, no need to pass state between views
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- src/main/java/org/cryptomator/ui/common/FxmlFile.java (1 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/HubKeyLoadingModule.java (1 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java (6 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterFailedController.java (2 hunks)
- src/main/resources/fxml/hub_register_device_already_exists.fxml (1 hunks)
- src/main/resources/fxml/hub_register_failed.fxml (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/cryptomator/ui/keyloading/hub/HubKeyLoadingModule.java
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterFailedController.java
- src/main/resources/fxml/hub_register_failed.fxml
Additional comments: 2
src/main/resources/fxml/hub_register_device_already_exists.fxml (1)
- 1-51: The FXML structure for the
hub_register_device_already_exists.fxmlappears to be well-formed and correctly defines the UI elements for the new error screen. The use ofFontAwesome5IconViewfor the icon,Labelfor messages, andButtonfor the close action are appropriate. TheRegisterFailedControlleris correctly set as the controller for this view.Ensure that the keys
%hub.registerFailed.messageand%hub.registerFailed.description.deviceAlreadyExistsare properly defined in thestrings.propertiesfile and that they provide clear and user-friendly messages as intended by the PR objectives.src/main/java/org/cryptomator/ui/common/FxmlFile.java (1)
- 25-27: The changes to the
FxmlFileenum include the addition of theHUB_REGISTER_DEVICE_ALREADY_EXISTSconstant and the update to theHUB_REGISTER_DEVICEconstant. This aligns with the PR objectives to introduce a new error screen and update the device registration flow.Ensure that the new FXML file paths provided as constants are correct and that the corresponding FXML files exist at those locations in the project. Also, verify that all references to the old
HUB_SETUP_DEVICEconstant have been updated to the newHUB_REGISTER_DEVICEconstant throughout the codebase.Verification successful
The verification scripts have successfully located the
hub_register_device_already_exists.fxmlandhub_register_device.fxmlfiles in thesrc/main/resources/fxmldirectory, confirming their existence in the codebase. Additionally, thergcommand did not produce any output, which indicates that there are no remaining references to theHUB_SETUP_DEVICEconstant in the Java files. This aligns with the PR objectives and the changes mentioned in the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new FXML file exists. fd 'hub_register_device_already_exists.fxml' --exec test -f # Verify that the renamed FXML file exists. fd 'hub_register_device.fxml' --exec test -f # Search for any remaining references to the old constant name. rg 'HUB_SETUP_DEVICE' --type javaLength of output: 210
Script:
#!/bin/bash # Correct the verification of the existence of the new FXML file. fd 'hub_register_device_already_exists.fxml' --exec test -f {} # Correct the verification of the existence of the renamed FXML file. fd 'hub_register_device.fxml' --exec test -f {} # Re-run the search for any remaining references to the old constant name with a corrected command. rg 'HUB_SETUP_DEVICE' --type javaLength of output: 220
Script:
#!/bin/bash # Simplify the verification of the existence of the new FXML file. fd 'hub_register_device_already_exists.fxml' # Simplify the verification of the existence of the renamed FXML file. fd 'hub_register_device.fxml' # Confirm that the rg command is executed correctly and there are no references to HUB_SETUP_DEVICE. rg 'HUB_SETUP_DEVICE' --type javaLength of output: 221
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Closes #3203
Removes the "device already exists label" in register device screen and shows setupFailed screen, when the device already exists. This changes it now to a unlock terminating action.
The setupFailed screen shows a custom message, if it is shown with a DeviceAlreadyExistsException:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Documentation