-
Notifications
You must be signed in to change notification settings - Fork 301
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
General
: Improve tab navigation on login page
#9373
Conversation
WalkthroughThe pull request introduces several changes across multiple files, including updates to the Jest configuration for coverage thresholds, modifications to routing and component structures in Angular, and refactoring of dependency injection methods. Key updates include the addition of a new route for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
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.
Actionable comments posted: 13
🧹 Outside diff range comments (6)
src/main/webapp/app/core/auth/state-storage.service.ts (1)
Line range hint
17-20
: Add JSDoc comment forgetUrl
method.To maintain consistency and improve code documentation, please add a JSDoc comment for the
getUrl
method, similar to the one provided forstoreUrl
.Here's a suggested JSDoc comment:
/** * Get the previousURL stored in the current session. * @returns The stored URL as a string. */ getUrl(): string { // ... existing implementation ... }src/main/webapp/app/core/auth/auth-jwt.service.ts (1)
Line range hint
1-50
: Overall structure aligns well with guidelines, minor improvements possibleThe file structure largely adheres to the provided coding guidelines:
- Single quotes are used for strings
- PascalCase is used for the class name
- camelCase is used for method names
- Curly braces and indentation are consistent
- JSDoc style comments are included
To fully align with the coding guidelines:
Consider refactoring class methods to use arrow functions. For example:
login = (credentials: Credentials): Observable<object> => { return this.http.post('api/public/authenticate', credentials); }; loginSAML2 = (rememberMe: boolean): Observable<object> => { return this.http.post('api/public/saml2', rememberMe.toString()); }; // Apply similar changes to other methodsThis change would make the code fully compliant with the arrow function preference specified in the coding guidelines.
src/main/webapp/app/home/saml2-login/saml2-login.component.ts (1)
Line range hint
25-62
: Overall structure looks good, with a minor suggestion for improvement.The unchanged parts of the component, including the
ngOnInit
andloginSAML2
methods, appear to be functioning correctly. The code adheres to the guidelines for string quotes and includes appropriate error handling and user feedback mechanisms.For consistency with modern JavaScript/TypeScript practices and to align with the arrow function style guideline, consider refactoring the methods to use arrow function syntax:
ngOnInit = (): void => { // ... (existing code) }; loginSAML2 = () => { // ... (existing code) };This change would make the code style more consistent throughout the component.
src/main/webapp/app/core/login/login.service.ts (1)
Line range hint
1-16
: Overall impact: Positive refactoring with no functional changes.The changes in this file represent a positive refactoring of the dependency injection mechanism without altering the core functionality of the
LoginService
. This update aligns the code with modern Angular practices and may offer slight performance improvements. The transition to using theinject
function simplifies the code structure and reduces boilerplate, making it easier to maintain in the future.These changes are low-risk as they don't introduce any new functionality or modify existing behavior. The service methods remain untouched, ensuring that the login, logout, and related processes continue to work as before.
Consider applying this dependency injection pattern consistently across other services in the application for uniformity and to leverage the benefits of this approach throughout the codebase.
src/main/webapp/app/core/auth/user-route-access-service.ts (1)
Line range hint
1-17
: Summary: Successful refactoring of dependency injection.The changes in this file successfully modernize the dependency injection approach by using the
inject
function from Angular's core library. This refactoring:
- Simplifies the code by removing the constructor.
- Aligns with current Angular best practices.
- Maintains the existing functionality of the
UserRouteAccessService
.These modifications contribute to the PR's objective of updating related services to utilize the new injection mechanism. The changes are minimal and focused, reducing the risk of introducing new issues while improving the code's maintainability.
As the team continues to adopt this new injection pattern, consider creating a brief documentation or style guide entry to ensure consistency across the codebase and to help onboard new team members to this approach.
src/test/javascript/spec/service/account.service.spec.ts (1)
Line range hint
1-54
: Excellent update to modern Angular testing practices!The changes to the import statements and TestBed configuration are well-implemented. Moving from HttpClientTestingModule to provideHttpClient and provideHttpClientTesting aligns with current best practices for Angular testing.
The addition of fakeAsync and tick for handling asynchronous operations is a great improvement. It will make the tests more reliable and easier to reason about.
Consider grouping related imports together for better readability. For example:
import { TestBed, fakeAsync, tick } from '@angular/core/testing'; import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing'; import { provideHttpClient } from '@angular/common/http'; import { of } from 'rxjs'; // ... other imports
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (15)
- jest.config.js (1 hunks)
- src/main/webapp/app/app-routing.module.ts (1 hunks)
- src/main/webapp/app/app.module.ts (0 hunks)
- src/main/webapp/app/core/auth/account.service.ts (2 hunks)
- src/main/webapp/app/core/auth/auth-jwt.service.ts (2 hunks)
- src/main/webapp/app/core/auth/state-storage.service.ts (1 hunks)
- src/main/webapp/app/core/auth/user-route-access-service.ts (2 hunks)
- src/main/webapp/app/core/login/login.service.ts (2 hunks)
- src/main/webapp/app/home/home.component.html (2 hunks)
- src/main/webapp/app/home/home.component.ts (4 hunks)
- src/main/webapp/app/home/home.module.ts (0 hunks)
- src/main/webapp/app/home/home.route.ts (0 hunks)
- src/main/webapp/app/home/saml2-login/saml2-login.component.ts (1 hunks)
- src/test/javascript/spec/service/account.service.spec.ts (3 hunks)
- src/test/javascript/spec/service/state-storage.service.spec.ts (0 hunks)
💤 Files not reviewed due to no reviewable changes (4)
- src/main/webapp/app/app.module.ts
- src/main/webapp/app/home/home.module.ts
- src/main/webapp/app/home/home.route.ts
- src/test/javascript/spec/service/state-storage.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (10)
src/main/webapp/app/app-routing.module.ts (1)
src/main/webapp/app/core/auth/account.service.ts (1)
src/main/webapp/app/core/auth/auth-jwt.service.ts (1)
src/main/webapp/app/core/auth/state-storage.service.ts (1)
src/main/webapp/app/core/auth/user-route-access-service.ts (1)
src/main/webapp/app/core/login/login.service.ts (1)
src/main/webapp/app/home/home.component.html (1)
Pattern
src/main/webapp/**/*.html
: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.src/main/webapp/app/home/home.component.ts (1)
src/main/webapp/app/home/saml2-login/saml2-login.component.ts (1)
src/test/javascript/spec/service/account.service.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts
: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🔇 Additional comments (25)
src/main/webapp/app/core/auth/state-storage.service.ts (4)
1-1
: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include the
inject
function from '@angular/core'. This change aligns with the new dependency injection approach used in the class.
5-6
: LGTM: Improved dependency injection approach.The changes to the class improve the dependency injection mechanism:
- Removal of the constructor simplifies the class structure.
- Use of the
inject
function forSessionStorageService
aligns with modern Angular practices.- The property name
sessionStorage
correctly follows the camelCase naming convention.These changes enhance code readability and maintainability.
Line range hint
8-15
: LGTM: Simplified class with clear URL storage functionality.The
StateStorageService
has been effectively simplified:
- Focus on storing and retrieving the 'previousUrl'.
- Proper use of camelCase for method names.
- Clear and concise JSDoc comment for
storeUrl
.- Correct use of single quotes for strings.
These changes align well with the PR objectives and coding guidelines.
Line range hint
1-22
: Summary: Effective simplification aligning with PR objectives.The changes to
StateStorageService
effectively contribute to the PR's goal of improving login page functionality:
- Simplified class structure focusing on URL storage.
- Improved dependency injection using the
inject
function.- Removal of unused state management methods.
- Adherence to coding guidelines (naming conventions, string quotes, etc.).
These changes should result in a more maintainable and focused component. The only suggested improvement is to add a JSDoc comment for the
getUrl
method to maintain consistent documentation.src/main/webapp/app/core/auth/auth-jwt.service.ts (1)
1-1
: LGTM: Import statement updated correctlyThe import statement has been appropriately updated to include the
inject
function from '@angular/core'. This change is necessary for the new dependency injection method and aligns with the Angular style guide.src/main/webapp/app/home/saml2-login/saml2-login.component.ts (3)
1-1
: Excellent migration to standalone component format!The changes in imports and component decorator effectively migrate the component to a standalone format, which aligns with the PR objectives. The use of the
inject
function and importingArtemisSharedModule
demonstrate good practices in modern Angular development and adherence to code reuse guidelines.Also applies to: 7-7, 13-14
21-23
: Clean and concise input property declarations!The direct declaration of input properties in the class body is a clean and modern approach. It simplifies the component by eliminating the need for constructor initialization. The naming conventions and type specifications are correctly followed, adhering to TypeScript best practices.
Line range hint
1-62
: Summary: Excellent migration to standalone component with modern practices!This review has covered all the changes made to the
Saml2LoginComponent
. The migration to a standalone component format, the use of modern dependency injection techniques, and the overall code structure are all commendable. The changes align well with the PR objectives and adhere to the provided coding guidelines.A few minor suggestions have been made for further improvements:
- Adding
readonly
to the injected services for added safety.- Considering the use of arrow function syntax for class methods for consistency.
These suggestions are optional and do not impact the overall quality of the changes. The PR appears ready for merging, pending any additional reviews or manual tests as specified in the PR objectives.
src/main/webapp/app/core/login/login.service.ts (1)
1-1
: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include the
inject
function from '@angular/core'. This change is necessary for the new dependency injection method and aligns with Angular's latest practices.jest.config.js (2)
105-108
: Summary of changes to jest.config.jsThe only changes in this file are slight decreases to the coverage thresholds for statements and lines. While these changes are minor, they don't align with the PR objectives of improving tab navigation or migrating components.
To ensure these changes don't negatively impact the project's overall quality:
- Could you clarify if these threshold adjustments are intentional or if they're a side effect of other changes?
- If intentional, what's the rationale behind lowering the thresholds?
- How do these changes relate to the PR's main objectives?
It's important to maintain high test coverage to ensure the reliability and maintainability of the codebase, especially when making UI and architectural changes as described in the PR objectives.
105-108
: Question: Why are we decreasing the coverage thresholds?I noticed that the coverage thresholds for
statements
andlines
have been slightly decreased:
statements
: 87.38 -> 87.35lines
: 87.44 -> 87.41This change seems to go against the TODO comment above, which suggests increasing these values to at least 90% in the future.
Could you please explain the reasoning behind this decrease? Given the project's goal to improve code quality, it might be beneficial to maintain or even increase these thresholds instead of lowering them. This would ensure that we're moving towards better test coverage over time.
To verify the impact of these changes, we can run the following script:
This script will help us understand if the current test coverage meets the new thresholds and if the decrease is necessary.
src/main/webapp/app/core/auth/user-route-access-service.ts (1)
1-1
: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
inject
andisDevMode
from '@angular/core'. This change aligns with the refactoring of dependency injection and maintains consistency with the existing code usage.src/main/webapp/app/app-routing.module.ts (1)
17-20
: LGTM! Verify impact on application routing.The addition of a lazy-loaded root route for the
HomeComponent
is a good practice and aligns with the project's coding guidelines. Here are a few points to consider:
- Ensure that any previous root route has been properly removed to avoid conflicts.
- Test the initial load behavior of the application to confirm it loads the
HomeComponent
as expected.- Verify that the
HomeComponent
exists at the path./home/home.component
.To verify the
HomeComponent
exists and there are no conflicting routes, run the following script:src/main/webapp/app/home/home.component.html (4)
48-50
: Improved error message display logic for username.The addition of the
usernameTouched
check prevents premature error messages, enhancing the user experience. This change aligns well with the PR objective of improving validation processes.
74-76
: Improved error message display logic for password.The addition of the
passwordTouched
check prevents premature error messages, enhancing the user experience. This change aligns well with the PR objective of improving validation processes.
96-101
: Improved login button validation and accessibility.The changes simplify the form validation logic and improve keyboard navigation. The use of
isFormValid
in the disabled condition provides a more robust validation check, and the addition oftabindex="3"
ensures a logical tab order in the form.
Line range hint
1-170
: Correct usage of new Angular syntax.The template consistently uses
@if
directives instead of*ngIf
, which aligns with the provided coding guidelines. This adoption of the new Angular syntax improves code readability and maintainability.src/main/webapp/app/core/auth/account.service.ts (3)
1-1
: LGTM: Import ofinject
functionThe addition of the
inject
import from '@angular/core' is appropriate for the transition to the new dependency injection method. This change aligns with modern Angular practices and simplifies the service's constructor.
Line range hint
1-42
: Summary: Successful transition to new dependency injection methodThe changes in this file successfully implement the transition from constructor-based dependency injection to using the
inject
function. This update aligns with the PR objectives of updating related services to utilize the new injection mechanism. Key points:
- The overall functionality of the
AccountService
remains unchanged.- The new approach simplifies the class structure and reduces boilerplate code.
- The change is consistent with modern Angular practices.
These modifications contribute to improved code maintainability without introducing breaking changes to the service's functionality.
38-42
: LGTM: Transition to new dependency injection methodThe change from constructor-based dependency injection to using the
inject
function is a positive improvement. This approach simplifies the class structure and aligns with modern Angular practices. The dependencies are correctly injected and assigned to private class properties.To ensure all previously injected dependencies are still included, let's run the following script:
✅ Verification successful
Dependency injection verification successful
All previously injected dependencies are correctly implemented using the
inject
function. No missing dependencies were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all previously injected dependencies are still included in the new injection method. # Test: Search for the old constructor and compare with new inject statements echo "Old constructor dependencies:" rg "constructor\(" src/main/webapp/app/core/auth/account.service.ts -A 10 echo "\nNew inject statements:" rg "private \w+ = inject\(" src/main/webapp/app/core/auth/account.service.tsLength of output: 563
src/test/javascript/spec/service/account.service.spec.ts (1)
54-63
: Well-structured test setup and teardown!The changes in the beforeEach and afterEach blocks are well-implemented:
- Using TestBed.inject for service injection is the correct approach in Angular tests.
- Initializing httpMock with TestBed.inject(HttpTestingController) is appropriate for HTTP request mocking.
- Adding httpMock.verify() in the afterEach block is an excellent practice to ensure all expected HTTP requests were made during each test.
These changes contribute to more robust and reliable tests.
src/main/webapp/app/home/home.component.ts (4)
1-1
: Imports updated appropriatelyThe import statements have been updated to include necessary modules and components. The code changes are appropriate.
Also applies to: 3-3, 14-14
47-47
: Assignment ofUSERNAME_MAX_LENGTH
Assigning imported constants to class properties for use within the component is appropriate.
74-74
: Initialization ofisFormValid
andisSubmittingLogin
The addition of these properties aids in form validation and submission handling. The code changes are appropriate.
81-82
: EnsureusernameTouched
andpasswordTouched
are utilizedYou have added the properties
usernameTouched
andpasswordTouched
, but it's unclear from the provided code how they are utilized. Ensure that these properties are being used appropriately to track the touched state of the input fields.Please verify that
usernameTouched
andpasswordTouched
are bound to the template and updated accordingly.
Checklist
General
Client
authorities
to all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Description
Steps for Testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores