-
Notifications
You must be signed in to change notification settings - Fork 39
Login and client command line refactoring #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Login and client command line refactoring #175
Conversation
|
@mgaffigan Is it possible to break this up into a few commits to make it easier to review? There is enough change that it's a bit difficult to mentally keep track of which changes are related to the same refactor and which aren't with all of the scrolling. |
mgaffigan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible, yes, but it would result in ~4 PR's with 1 dependent upon the other three. I've added some comments in hopes those are sufficient to avoid splitting with dependencies. Vanilla git really sucks at stacked PR's.
|
@mgaffigan I'm fine with them all being part of this single PR. I was just asking if the logical chunks can be separated into multiple commits. |
|
Ah. Good call. Yes. I'll break to commits. |
4229704 to
f9aad48
Compare
|
@tonygermano, not sure each commit builds in isolation, but I rewrote the history by topic. Let me know if you need anything further. |
4710eae to
37fe874
Compare
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
…ying on global state Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
37fe874 to
b258969
Compare
…tations Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
b258969 to
e738ced
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the login system to support alternative login panel implementations in preparation for OIDC authentication. The refactoring extracts the handleLoginSuccess method from LoginPanel to the Mirth class (where it logically belongs since it performs application initialization) and introduces a factory pattern for obtaining LoginPanel instances.
Key changes:
- Introduced
AbstractLoginPanelbase class andLoginPanelFactoryfor pluggable login implementations - Moved
handleLoginSuccesslogic fromLoginPaneltoMirth.handleLoginSuccess()to separate login UI concerns from application setup - Updated all references from
LoginPanel.getInstance()toLoginPanelFactory.getInstance()
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
AbstractLoginPanel.java |
New abstract base class defining the contract for login panel implementations with initialize() and setStatus() methods |
LoginPanelFactory.java |
New factory class providing singleton access to login panel instances with support for runtime provider replacement |
LoginPanel.java |
Refactored to extend AbstractLoginPanel; removed handleLoginSuccess() method (moved to Mirth), changed from singleton to factory-managed instance, added setError() helper method |
Mirth.java |
Added public static handleLoginSuccess() method containing the application setup logic previously in LoginPanel; updated to use LoginPanelFactory |
Frame.java |
Updated all references from LoginPanel.getInstance() to LoginPanelFactory.getInstance() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }); | ||
| } | ||
|
|
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new public method handleLoginSuccess lacks javadoc documentation. Add a javadoc comment explaining the method's purpose, parameters (@param client, @param loginStatus, @param userName), return value (@return), and the exception it throws (@throws ClientException).
| /** | |
| * Handles post-login success actions such as displaying login notifications, | |
| * setting environment and server names, and updating UI settings. | |
| * | |
| * @param client The client instance used to retrieve server settings and acknowledge notifications. | |
| * @param loginStatus The status of the login attempt. | |
| * @param userName The username of the logged-in user. | |
| * @return {@code true} if login success actions are completed and the user accepted any required notifications; {@code false} otherwise. | |
| * @throws ClientException If an error occurs while retrieving server settings or acknowledging notifications. | |
| */ |
| try { | ||
| PublicServerSettings publicServerSettings = client.getPublicServerSettings(); | ||
|
|
||
| if (publicServerSettings.getLoginNotificationEnabled() == true) { |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid explicit boolean comparisons. Instead of if (publicServerSettings.getLoginNotificationEnabled() == true), use if (publicServerSettings.getLoginNotificationEnabled()).
| if (publicServerSettings.getLoginNotificationEnabled() == true) { | |
| if (publicServerSettings.getLoginNotificationEnabled()) { |
| CustomBannerPanelDialog customBannerPanelDialog = new CustomBannerPanelDialog(loginPanel, "Login Notification", publicServerSettings.getLoginNotificationMessage()); | ||
| boolean isAccepted = customBannerPanelDialog.isAccepted(); | ||
|
|
||
| if (isAccepted == true) { |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid explicit boolean comparisons. Instead of if (isAccepted == true), use if (isAccepted).
| if (isAccepted == true) { | |
| if (isAccepted) { |
In support for future merge of mgaffigan:feature/add-oidc-auth