Development: Migrate tutorial groups service folder to new angular guidelines
#9941
Conversation
github-actions
bot
added
the
client
az108
changed the title
Tutorial Groups: Migrate tutorial groups service folder to new angular guidelinesTutorial groups: Migrate tutorial groups service folder to new angular guidelines
WalkthroughThe pull request introduces modifications to several service classes within the tutorial groups module, specifically transitioning from constructor-based dependency injection to using 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/main/webapp/app/course/tutorial-groups/services/tutorial-group-free-period.service.ts (2)
18-20: Consider reordering class membersAccording to Angular style guide, consider organizing class members in the following order:
- Public properties
- Private properties
- Constructor/lifecycle hooks
- Public methods
- Private methods
@Injectable({ providedIn: 'root' }) export class TutorialGroupFreePeriodService { - private httpClient = inject(HttpClient); - - private resourceURL = 'api'; + // Private properties + private readonly httpClient = inject(HttpClient); + private readonly resourceURL = 'api';
Line range hint
71-89: Consider simplifying date conversion methodsThe date conversion methods could be simplified using optional chaining and nullish coalescing operators.
private convertTutorialGroupFreePeriodDatesFromClient(tutorialGroupFreePeriodDTO: TutorialGroupFreePeriodDTO): TutorialGroupFreePeriodDTO { - if (tutorialGroupFreePeriodDTO) { - return Object.assign({}, tutorialGroupFreePeriodDTO, { - startDate: toISO8601DateTimeString(tutorialGroupFreePeriodDTO.startDate), - endDate: toISO8601DateTimeString(tutorialGroupFreePeriodDTO.endDate), - }); - } else { - return tutorialGroupFreePeriodDTO; - } + return tutorialGroupFreePeriodDTO && { + ...tutorialGroupFreePeriodDTO, + startDate: toISO8601DateTimeString(tutorialGroupFreePeriodDTO.startDate), + endDate: toISO8601DateTimeString(tutorialGroupFreePeriodDTO.endDate), + }; }src/main/webapp/app/course/tutorial-groups/services/tutorial-groups-configuration.service.ts (2)
12-14: Consider reordering class members and adding readonly modifierFor consistency with other services and immutability best practices.
@Injectable({ providedIn: 'root' }) export class TutorialGroupsConfigurationService { - private httpClient = inject(HttpClient); - - private resourceURL = 'api'; + // Private properties + private readonly httpClient = inject(HttpClient); + private readonly resourceURL = 'api';
Line range hint
47-61: Improve null safety in date conversion methodsThe method uses excessive non-null assertions (!). Consider using optional chaining and type guards.
private convertTutorialGroupsConfigurationResponseDatesFromServer(res: HttpResponse<TutorialGroupsConfiguration>): HttpResponse<TutorialGroupsConfiguration> { - if (res.body) { - res.body.tutorialPeriodStartInclusive = convertDateFromServer(res.body!.tutorialPeriodStartInclusive); - res.body!.tutorialPeriodEndInclusive = convertDateFromServer(res.body!.tutorialPeriodEndInclusive); - if (res.body!.tutorialGroupFreePeriods) { - res.body!.tutorialGroupFreePeriods.forEach((tutorialGroupFreePeriod) => { - tutorialGroupFreePeriod.start = convertDateFromServer(tutorialGroupFreePeriod.start); - tutorialGroupFreePeriod.end = convertDateFromServer(tutorialGroupFreePeriod.end); - }); - } - } + const body = res.body; + if (body) { + body.tutorialPeriodStartInclusive = convertDateFromServer(body.tutorialPeriodStartInclusive); + body.tutorialPeriodEndInclusive = convertDateFromServer(body.tutorialPeriodEndInclusive); + body.tutorialGroupFreePeriods?.forEach((period) => { + period.start = convertDateFromServer(period.start); + period.end = convertDateFromServer(period.end); + }); + } return res; }src/main/webapp/app/course/tutorial-groups/services/tutorial-group-session.service.ts (3)
20-23: Consider reordering class members and adding readonly modifierFor consistency with other services and immutability best practices.
@Injectable({ providedIn: 'root' }) export class TutorialGroupSessionService { - private httpClient = inject(HttpClient); - private tutorialGroupFreePeriodService = inject(TutorialGroupFreePeriodService); - - private resourceURL = 'api'; + // Private properties + private readonly httpClient = inject(HttpClient); + private readonly tutorialGroupFreePeriodService = inject(TutorialGroupFreePeriodService); + private readonly resourceURL = 'api';
Line range hint
40-50: Simplify HTTP params constructionThe current implementation has redundant null checks. Consider using nullish coalescing.
updateAttendanceCount(courseId: number, tutorialGroupId: number, sessionId: number, attendanceCount: number | undefined): Observable<EntityResponseType> { - let params = new HttpParams(); - if (attendanceCount !== undefined && attendanceCount !== null) { - params = params.append('attendanceCount', attendanceCount.toString()); - } + const params = attendanceCount != null ? new HttpParams().append('attendanceCount', attendanceCount.toString()) : new HttpParams(); return this.httpClient .patch<TutorialGroupSession>(`${this.resourceURL}/courses/${courseId}/tutorial-groups/${tutorialGroupId}/sessions/${sessionId}/attendance-count`, null, { observe: 'response', params, }) .pipe(map((res: EntityResponseType) => this.convertTutorialGroupSessionResponseDatesFromServer(res))); }
Based on the search results, I can now provide a final response. The codebase shows consistent error handling patterns across various services using RxJS's
catchErroroperator, often combined withAlertServicefor user notifications. However, the tutorial group session service lacks this pattern.Add consistent error handling to align with codebase patterns
The service methods should include error handling using RxJS operators, following the established pattern in the codebase. Example implementation:
import { catchError } from 'rxjs/operators'; import { HttpErrorResponse } from '@angular/common/http'; import { AlertService } from 'app/core/util/alert.service'; // Add to constructor: constructor( private alertService: AlertService, // ... other dependencies ) {} // Modify each HTTP method to include: .pipe( catchError((error: HttpErrorResponse) => { this.alertService.error('artemisApp.tutorialGroup.error.operation'); return throwError(() => error); }) )🔗 Analysis chain
Line range hint
26-91: Consider adding consistent error handlingThe service methods handle HTTP responses differently. Consider adding consistent error handling across all methods.
Consider adding error handling to all HTTP methods:
import { catchError } from 'rxjs/operators'; import { throwError } from 'rxjs'; // Add to each HTTP method: .pipe( catchError(error => { console.error('API Error:', error); return throwError(() => error); }) )🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for error handling patterns in other services rg -A 2 "catchError|throwError" src/main/webapp/app/course/tutorial-groups/services/Length of output: 86
Script:
#!/bin/bash # Let's broaden the search to check error handling patterns across the application rg -A 2 "catchError|throwError" "src/main/webapp/app/" # Also check for any HTTP error handling patterns that might use different approaches rg -A 2 "error.*handle|handle.*error" "src/main/webapp/app/"Length of output: 69016
src/main/webapp/app/course/tutorial-groups/services/tutorial-groups.service.ts (1)
18-22: Consider enhancing immutabilityConsider the following improvements:
- Mark injected services as
readonlysince they are immutable dependencies- Convert
resourceURLto a readonly constant using uppercase naming- private httpClient = inject(HttpClient); - private tutorialGroupSessionService = inject(TutorialGroupSessionService); - private tutorialGroupsConfigurationService = inject(TutorialGroupsConfigurationService); - - private resourceURL = 'api'; + private readonly httpClient = inject(HttpClient); + private readonly tutorialGroupSessionService = inject(TutorialGroupSessionService); + private readonly tutorialGroupsConfigurationService = inject(TutorialGroupsConfigurationService); + + private readonly RESOURCE_URL = 'api';Remember to update all references of
resourceURLtoRESOURCE_URLthroughout the file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/main/webapp/app/course/tutorial-groups/services/tutorial-group-free-period.service.ts(2 hunks)src/main/webapp/app/course/tutorial-groups/services/tutorial-group-session.service.ts(2 hunks)src/main/webapp/app/course/tutorial-groups/services/tutorial-groups-configuration.service.ts(2 hunks)src/main/webapp/app/course/tutorial-groups/services/tutorial-groups.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/course/tutorial-groups/services/tutorial-groups.service.ts (1)
src/main/webapp/app/course/tutorial-groups/services/tutorial-groups-configuration.service.ts (1)
src/main/webapp/app/course/tutorial-groups/services/tutorial-group-free-period.service.ts (1)
src/main/webapp/app/course/tutorial-groups/services/tutorial-group-session.service.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/course/tutorial-groups/services/tutorial-groups.service.ts (2)
2-2: LGTM: Import statement correctly updated
The inject import from @angular/core is properly added and grouped with other Angular core imports.
18-20: LGTM: Proper migration to new dependency injection pattern
The service correctly implements the new Angular dependency injection pattern using inject(). All dependencies are properly initialized and marked as private.
|
github-actions
bot
added
the
deployment-error
sarpsahinalp
added
deploy:artemis-test1
and removed
deployment-error
sarpsahinalp
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Tutorial groups: Migrate tutorial groups service folder to new angular guidelinesDevelopment: Migrate tutorial groups service folder to new angular guidelines
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Keeping Code new and up to date should be high priority for projects, therefore we are moving to the new Angular Signals,Inject and Standalone.
Description
Migrated old Code to new Client Code guidelines
Review Progress
Code Review
Summary by CodeRabbit
New Features
injectfunction for various services, enhancing code readability and maintainability.Bug Fixes
Refactor