Pasternak/study subjects #2752
Pasternak/study subjects #2752
Conversation
WalkthroughThe pull request enhances the provider functionality by introducing study subject management. New navigation buttons, enum entries, interfaces, services, and components have been added to support creating, editing, listing, and deleting study subjects. Additionally, state management has been updated through new NGXS actions and handlers to integrate these subject-related features. New routing configurations and UI elements ensure that provider users can seamlessly access and manage study subjects within their personal cabinet. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as CreateStudySubjectComponent
participant Store as ProviderState
participant Service as StudySubjectService
participant API as Backend API
UI->>Store: Dispatch CreateStudySubject action with form data
Store->>Service: Invoke createStudySubject(studySubject)
Service->>API: POST /study-subject with payload
API-->>Service: Return created study subject
Service-->>Store: Forward response
Store->>UI: Dispatch OnCreateStudySubjectSuccess action
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
|
unit tests in progress |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (21)
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.scss (5)
1-9: SCSS Imports Consistency.
The file imports multiple shared style sheets needed for layout, buttons, tables, etc. If you are using Dart Sass and aiming for improved module scoping, consider migrating to the newer@usesyntax in future refactors.
107-112: Overriding Datepicker Actions with ::ng-deep.
Using::ng-deepto center and style the Material datepicker actions is effective but note that::ng-deepis deprecated. Consider exploring alternative encapsulation strategies in future updates.
129-135: Foundational .date Container Styles.
This block sets up the base styling for date elements (min-width, background, box-shadow, border-radius). Consider reviewing if these rules could be merged with subsequent.datedeclarations to reduce redundancy.
137-146: Scoped Date Input Adjustments.
The:host ::ng-deep .dateselector further refines date component layout by enforcing width, max-width, and padding adjustments. If possible, consolidating these rules with the earlier.dateblock might improve maintainability.
148-154: Typography and Sizing for .date Elements.
This.dateblock sets typography (font-family, font-style, font-size) and dimensions for date elements. Given the existence of a similar.dateblock earlier, double-check that the separation is intentional. Merging similar selectors could simplify style management.src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.ts (5)
28-33: Avoid code duplication with shared validator sets
These default validators are duplicated across multiple fields. While this is a good approach, consider centralizing the validator arrays in a dedicated utility or constant that can be imported and reused anywhere in the codebase to increase maintainability.
41-80: Consider using synchronous form initialization in the parent
Since the constructor is performing form initialization andsubscribeOnDirtyFormcalls, it might be cleaner to initialize the form inngOnInit(or a dedicated setup method in the parent) to keep constructor logic lightweight. This improves readability and consistency with Angular best practices.
97-130: Clarify edit-mode fetching logic
BothngOnInitandsetEditModefetch the same provider data. Combining them or ensuring a single source of truth can enhance maintainability and reduce complexity.
154-179: Prevent repeated modal confirmation
When the form is dirty and a user triggers multiple submissions, theisDispatchingguard helps, but consider also disabling the submit button or unsubscribing from subsequent dialog events until the dispatch is complete to prevent repeated modal invocations.
181-184: Navigation path usage
Navigating to a hardcoded path ('/personal-cabinet/provider/study-subjects') is fine for now, but extracting it into a routes constant or using an Angular routing approach that references a named route can help avoid typos and future changes.src/app/shared/store/provider.state.ts (3)
71-74: Null as initial defaults
If you never rely on strictnullchecks and prefer arrays, consider initializing them with empty arrays instead ofnull. This can simplify your code by preventing potentialnullchecks down the line.- languageList: null, - studySubject: null, - selectedSubject: null + languageList: [], + studySubject: { totalAmount: 0, entities: [] }, + selectedSubject: {} as SubjectModel
161-164: Selector for selectedSubject
Selecting a single subject from the state is a logical design choice. Keep consistent naming across selectors for clarity (e.g.,getSelectedSubject).
86-88: Constructor injection
Good call injectingpositionService,studySubjectService, andlanguageListService. Consider maintaining consistent alphabetical or grouped ordering of injections for readability.src/app/shared/services/language-list/language-list.service.ts (1)
18-18: Use environment configuration for API endpoint.The API endpoint is hardcoded. Consider moving it to environment configuration for better maintainability across different environments.
src/app/shared/models/study-subject.model.ts (1)
23-25: Consider using null instead of empty strings.Empty strings are used as fallbacks for optional fields. Consider using
nullto better represent the absence of values.Apply this diff:
- this.activeFrom = subjectModel.activeFrom || ''; - this.activeTo = subjectModel.activeTo || ''; - this.workshopId = subjectModel.workshopId || ''; + this.activeFrom = subjectModel.activeFrom || null; + this.activeTo = subjectModel.activeTo || null; + this.workshopId = subjectModel.workshopId || null;src/app/shared/services/study-subjects/study-subjects.service.ts (2)
13-13: Consider extracting the base URL to environment configuration.The hardcoded base URL should be moved to the environment configuration for better maintainability and environment-specific settings.
- private readonly baseUrl: string = '/api/v1/providers'; + private readonly baseUrl: string = environment.apiUrl + '/providers';
19-21: Consider using URL builder utility for consistent URL construction.The URL construction is done inline and repeated across methods. Consider creating a utility method for consistent URL building.
+ private buildUrl(providerId: string, endpoint: string): string { + return `${this.baseUrl}/${providerId}/studysubjects/${endpoint}`; + } public createStudySubject(studySubject: SubjectModel): Observable<SubjectModel> { - return this.http.post<SubjectModel>(`${this.baseUrl}/${studySubject.providerId}/studysubjects/Create`, studySubject); + return this.http.post<SubjectModel>(this.buildUrl(studySubject.providerId, 'Create'), studySubject); }src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.ts (2)
64-78: Consider extracting filter logic to a separate method.The filter logic in ngOnInit is complex and could be more maintainable as a separate method.
+ private initializeFilter(): void { + this.filterFormControl.valueChanges + .pipe( + distinctUntilChanged(), + startWith(''), + skip(1), + debounceTime(1000), + takeUntil(this.destroy$), + map((searchedText: string) => searchedText.trim()) + ) + .subscribe((searchedText: string) => { + this.subjectParameters.searchString = searchedText; + this.currentPage = PaginationConstants.firstPage; + this.getStudySubjects(); + }); + } public ngOnInit(): void { super.ngOnInit(); - this.filterFormControl.valueChanges... + this.initializeFilter(); }
108-117: Consider adding loading state during delete operation.The delete operation should show a loading state to prevent multiple delete attempts.
+ public isDeleting: boolean = false; public onDelete(subject: SubjectModel): void { + if (this.isDeleting) { + return; + } + this.isDeleting = true; const dialogRef = this.matDialog.open(ConfirmationModalWindowComponent, { width: Constants.MODAL_SMALL, data: { type: ModalConfirmationType.deleteSubject, property: subject.nameInUkrainian } }); dialogRef .afterClosed() .pipe( filter(Boolean), + finalize(() => this.isDeleting = false) ) .subscribe(() => this.store.dispatch(new DeleteStudySubjectById(this.subjectParameters, subject.id))); }src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html (1)
49-130: Study Subjects Table and Pagination
The table is comprehensively built using Angular Material’smat-table, with well-defined columns for subject details, creation and last referred dates, and action buttons for edit/delete functionalities. The conditional display of a dash ('-') when the active-to date is set to'9999-12-31'is a smart way to handle placeholder dates. The paginator component integration further enhances the component’s usability.Note: The “Used In” column currently displays a hardcoded
"0". When dynamic data becomes available, consider replacing this value with a dynamic binding to reflect the actual number of workshop connections.src/app/header/header.component.html (1)
143-147: New Subjects Navigation Button in Header
The new button for navigating to the study subjects page is well-implemented—it is conditionally rendered withisRoleProvider(user.role)and correctly points to the new route. For consistency in role-check logic, consider verifying that the same or equivalent condition is applied uniformly across all components that handle provider-specific UI elements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/i18n/en.jsonis excluded by!src/assets/**and included bysrc/**src/assets/i18n/uk.jsonis excluded by!src/assets/**and included bysrc/**
📒 Files selected for processing (23)
src/app/header/header.component.html(1 hunks)src/app/shared/enum/enumUA/message-bar.ts(1 hunks)src/app/shared/enum/enumUA/navigation-bar.ts(1 hunks)src/app/shared/enum/enumUA/no-results.ts(1 hunks)src/app/shared/enum/modal-confirmation.ts(3 hunks)src/app/shared/models/language-list.model.ts(1 hunks)src/app/shared/models/study-subject.model.ts(1 hunks)src/app/shared/services/language-list/language-list.service.ts(1 hunks)src/app/shared/services/study-subjects/study-subjects.service.ts(1 hunks)src/app/shared/store/provider.actions.ts(2 hunks)src/app/shared/store/provider.state.ts(7 hunks)src/app/shell/personal-cabinet/personal-cabinet.component.html(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.html(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.scss(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.spec.ts(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-routing.module.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.scss(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.spec.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider.module.ts(2 hunks)src/app/shell/shell-routing.module.ts(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.spec.ts
- src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.spec.ts
- src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.scss
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/store/provider.actions.ts
[error] 479-479: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/app/shared/store/provider.state.ts
[error] 972-972: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 983-983: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 1038-1038: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 1069-1069: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (38)
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.scss (18)
10-20: Table Styling Verification.
The table selector sets a full-width layout with a fixed height of 20px and styles child anchors and spans appropriately. Verify that a fixed height of 20px is intentional and works well with dynamic content, as it may constrain the visual appearance when content grows.
22-43: Component Button and Layout Styling.
The styles for.btn,.actions-wrapper,.search, and.search-areaare straightforward and help establish a clean layout. Ensure that the fixed height (40px for buttons) and spacing values are consistent with the overall design system.
45-55: Responsive Adjustments for 1200px Viewports.
The media query adjusts.search-areaand.actions-wrapperwith updated position and margin settings. Confirm that these modifications enhance usability on devices with widths below 1200px.
57-62: Responsive Alignment for 1070px Viewports.
The media query targeting a max-width of 1070px ensures that the.search-areacontent is aligned to the start. This adjustment improves layout consistency on devices in this viewport range.
64-68: Flex Direction Adjustment for Narrow Screens.
Changing the flex direction of.actions-wrapperwhen the viewport is below 1000px helps stack elements vertically, which is beneficial for smaller screens. Verify that the stacking order meets design expectations.
70-74: Header Column Layout.
The.header-columnclass leverages flexbox with a specified gap to ensure proper spacing between header elements. This concise configuration should maintain alignment effectively.
76-80: Filters Wrapper Layout.
The.filters-wrapperclass utilizes flex display, aligning items center with a small column gap for compactness. Make sure the gap value (5px) maintains visual clarity across different resolutions.
82-105: Filters Period Styling and Material Overrides.
This block styles the filters period area with customized typography for internal spans and sets pointer-events on Angular Material elements (disabling on form fields and re-enabling for toggles). Confirm that disabling pointer events does not impede accessibility or user interaction.
114-120: Custom Datepicker Content Styling.
The styles for::ng-deep .mat-datepicker-contentadd a border-radius and adjust padding inside the calendar component. These rules help maintain a uniform visual identity with the rest of the component.
121-123: Date Range Picker Actions.
The centering of date-buttons within the date range picker is clearly defined, ensuring consistent alignment of interactive elements.
125-127: Simple Flex Layout for Date Buttons.
The.date-buttonsselector applies a simple flex display, which is adequate for laying out grouped button elements.
156-159: Angular Material Form Field Customization.
The removal of borders for.mat-form-field.mat-focused.mat-primaryand.mat-form-field-infixhelps achieve a cleaner design. Ensure this aligns with the desired visual emphasis during focus states.
161-163: Legacy Material Field Adjustments.
The style adjustment for.mat-form-field-appearance-legacy .mat-form-field-infixeffectively removes extra padding and supports design consistency.
165-167: Narrowing the Material Form Field Wrapper.
The:host ::ng-deep .mat-form-field-wrapperrule removes padding to maintain a compact field design. This is a straightforward and fitting adjustment.
169-188: Reset Button Styling and Interaction.
The.resetclass defines a flex layout with centered alignment and a hover effect that reduces opacity. The nested&__iconstyling adds visual flair with a rotated icon. This implementation supports intuitive user interaction.
190-196: Responsive Filters Wrapper Adjustments at 1300px.
The media query for max-width of 1300px adjusts.filters-wrapperto flex-wrap and sets a row gap for better layout on wider screens with reduced real estate. This adaptive approach enhances responsiveness.
198-202: Centering Filters on Medium-Sized Screens.
The media query at max-width 1000px re-centers.filters-wrapper, which assists in maintaining a balanced alignment on moderately sized devices.
204-212: Mobile Responsive Adjustments for Filters Period.
For very narrow viewports (max-width 440px), the nested span within.filters-periodis adjusted to block display with modified margins. This ensures that the filter labels remain legible on small devices.src/app/shared/store/provider.state.ts (5)
27-28: Imports look fine
The new services (StudySubjectServiceandLanguageListService) are appropriately injected later. No issues here.
31-33: New type imports
Using dedicated models (LanguageListItem,SubjectModel) for typed clarity is good. This helps highlight domain-specific objects and improves code clarity.
51-53: State model extensions
AddinglanguageList,studySubject, andselectedSubjectto the state model is a good approach for storing relevant data. Ensure thorough null checks before usage, or initialize them as empty arrays to avoid runtime issues.
131-134: Selector usage
Having a dedicated selector forlanguageListfosters more consistent usage across components. Nicely done.
156-159: Selector for studySubject
This is a well-structured approach for retrieving a list of study subjects. No issues spotted.src/app/shared/models/language-list.model.ts (1)
1-5: Interface definition
This new interface covers core properties for language items succinctly. Consider adding JSDoc or comments for each property if you want to clarify usage within the codebase.src/app/shared/enum/enumUA/no-results.ts (1)
13-14: AI summary is inconsistent with the code.The AI summary indicates that
noNotificationswas removed, but it's still present in the code. The changes only show the addition ofnoStudySubjects.Likely an incorrect or invalid review comment.
src/app/shell/personal-cabinet/provider/provider-routing.module.ts (1)
18-18:❓ Verification inconclusive
Verify if NotEmployeeGuard is needed for study-subjects route.
Other routes like 'administration' use the
NotEmployeeGuard. Verify if the same access control is needed for the study-subjects route.
🏁 Script executed:
#!/bin/bash # Description: Check if other provider routes use NotEmployeeGuard # Search for routes with NotEmployeeGuard rg "canActivate.*NotEmployeeGuard" -A 2 -B 2Length of output: 691
Action: Confirm Access Control for the Study-Subjects Route
- The
/administrationroute is guarded byNotEmployeeGuard:{ path: 'administration', component: EmployeesComponent, canActivate: [NotEmployeeGuard] }- In contrast, the
/study-subjectsroute currently does not include this guard:{ path: 'study-subjects', component: ProviderStudySubjectsComponent }Please verify whether this omission is intentional or if adding
canActivate: [NotEmployeeGuard]to thestudy-subjectsroute is required for consistent access control.src/app/shared/enum/enumUA/navigation-bar.ts (1)
32-34: LGTM! New navigation entries follow existing patterns.The new enum entries for subjects follow the established naming convention and translation key pattern.
src/app/shell/personal-cabinet/provider/provider.module.ts (1)
31-32: LGTM! Components are properly declared in the module.The new components are correctly imported and declared in the module following Angular best practices.
Also applies to: 58-59
src/app/shared/enum/enumUA/message-bar.ts (1)
63-65: LGTM!The new snackbar text entries for study subject operations follow the existing naming pattern and are properly grouped with similar operations.
src/app/shell/shell-routing.module.ts (1)
38-38: LGTM!The new route for creating study subjects follows the same pattern as other similar routes, with proper guards and lazy loading.
Also applies to: 97-103
src/app/shared/enum/modal-confirmation.ts (1)
47-49: LGTM!The new modal confirmation entries for study subject operations follow the existing naming pattern and are properly grouped with similar operations.
Also applies to: 102-104, 151-153
src/app/shared/store/provider.actions.ts (1)
482-541: LGTM!The new action classes for study subject operations follow the same pattern as other similar actions, with proper typing and error handling.
src/app/shell/personal-cabinet/personal-cabinet.component.html (1)
34-38: Provider-specific Navigation for Study Subjects
The new<ng-container>block correctly renders the Study Subjects link whenuserRole === Role.provider. This addition is clear and consistent with the design intent to offer extra navigation options for provider users. Ensure that the condition used here aligns with similar role-checks in other parts of the application.src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.html (3)
1-7: Component Header and Conditional Display
The container’s conditional display using*ngIf="(editMode && studySubject) || !editMode"effectively differentiates between edit and add modes. The dynamic header text toggles appropriately based on the value ofeditMode, which makes the intent clear.
8-43: Reactive Form Implementation for Study Subject Details
The form is well-structured with clear bindings usingformControlNamefor both subject names and the teaching language field. The integration of theapp-validation-hintcomponent for validating input length adds important usability feedback. Please verify that the component’s TypeScript declares and configures the corresponding form controls and validators consistently.
45-57: Footer Actions and Button Enablement
The cancel and save buttons are correctly set up. The save button’s[disabled]binding ensures it remains inactive unless the form has been modified and is valid. Optionally, consider handling form submission via the form’s submit event for enhanced semantic behavior, though the current approach is acceptable.src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html (2)
1-15: Search Bar for Study Subjects
The search area implementation leverages a reactive form control (bound viafilterFormControl) and Angular Material’s input component to facilitate subject filtering. The straightforward layout enhances usability.
16-47: Date Range Filter and Add Subject Button
The date range filter utilizes Angular Material’s date range components effectively, providing users with an intuitive way to filter subjects by creation period. The adjacent “Add Subject” button that routes to the creation page (usingModeConstants.NEW) is clearly defined. Double-check thatModeConstants.NEWis set up as expected to avoid routing issues.
| public ngOnInit(): void { | ||
| this.provider$ | ||
| .pipe( | ||
| filter((provider: Provider) => Boolean(provider)), | ||
| tap((provider: Provider) => { | ||
| this.provider = provider; | ||
| }), | ||
| takeUntil(this.destroy$) | ||
| ) | ||
| .subscribe(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leverage existing provider subscription
The provider is also subscribed to again in setEditMode. You could reuse a single subscription (or a single shared data structure) to store the provider in ngOnInit, eliminating multiple identical subscriptions.
- this.provider$
- .pipe(
- filter((provider: Provider) => Boolean(provider)),
- tap((provider: Provider) => {
- this.provider = provider;
- }),
- takeUntil(this.destroy$)
- )
- .subscribe();
+ // Possibly store the provider in a local variable in ngOnInit and reuse it in setEditModeCommittable suggestion skipped: line range outside the PR's diff.
| createStudySubject( | ||
| { dispatch, patchState }: StateContext<ProviderStateModel>, | ||
| { payload }: providerActions.CreateStudySubject | ||
| ): Observable<SubjectModel | void> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use undefined instead of void in union types
Declaring return types as Observable<SubjectModel | void> can be confusing. Replace void with undefined.
- createStudySubject(...): Observable<SubjectModel | void> {
+ createStudySubject(...): Observable<SubjectModel | undefined> {
- UpdateStudySubject(...): Observable<SubjectModel | void> {
+ UpdateStudySubject(...): Observable<SubjectModel | undefined> {
- DeleteStudySubjectById(...): Observable<SubjectModel[] | void> {
+ DeleteStudySubjectById(...): Observable<SubjectModel[] | undefined> {Also applies to: 1038-1038, 1069-1069
🧰 Tools
🪛 Biome (1.9.4)
[error] 983-983: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| private http: HttpClient, | ||
| private store: Store | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused Store dependency.
The Store is injected but not utilized in this service. Consider removing it to maintain clean dependency injection.
Apply this diff to remove the unused dependency:
constructor(
- private http: HttpClient,
- private store: Store
+ private http: HttpClient
) {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private http: HttpClient, | |
| private store: Store | |
| ) {} | |
| constructor( | |
| private http: HttpClient | |
| ) {} |
| this.nameInInstructionLanguage = subjectModel.nameInInstructionLanguage; | ||
| this.language = subjectModel.language; | ||
| this.languageId = subjectModel.language.id; | ||
| this.isLanguageUkrainian = subjectModel.language.id === 2; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding language IDs.
The Ukrainian language check uses a hardcoded ID (2). Consider using an enum or constant to improve maintainability and readability.
Create a new enum for language IDs and use it instead:
export enum LanguageId {
Ukrainian = 2
// Add other languages as needed
}Then update the check:
- this.isLanguageUkrainian = subjectModel.language.id === 2;
+ this.isLanguageUkrainian = subjectModel.language.id === LanguageId.Ukrainian;| public getStudySubjects(subjectParameters: SubjectParameters): Observable<SearchResponse<SubjectModel[]>> { | ||
| const params = new HttpParams() | ||
| .set('SearchString', subjectParameters.searchString || '') | ||
| .set('From', subjectParameters.from || '0') | ||
| .set('Size', subjectParameters.size || '10'); | ||
| return this.http.get<SearchResponse<SubjectModel[]>>(`${this.baseUrl}/${subjectParameters.providerId}/studysubjects/Get`, { | ||
| params | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid parameters.
The getStudySubjects method should validate input parameters before making the HTTP request.
public getStudySubjects(subjectParameters: SubjectParameters): Observable<SearchResponse<SubjectModel[]>> {
+ if (!subjectParameters.providerId) {
+ return throwError(() => new Error('Provider ID is required'));
+ }
const params = new HttpParams()
.set('SearchString', subjectParameters.searchString || '')
.set('From', subjectParameters.from || '0')
.set('Size', subjectParameters.size || '10');
return this.http.get<SearchResponse<SubjectModel[]>>(`${this.baseUrl}/${subjectParameters.providerId}/studysubjects/Get`, {
params
});
}| private getStudySubjects(): void { | ||
| this.subjectParameters.providerId = this.provider.id; | ||
| Util.setFromPaginationParam(this.subjectParameters, this.currentPage, this.totalElements); | ||
| this.store.dispatch(new GetStudySubjects(this.subjectParameters)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to getStudySubjects.
The private method should handle potential errors when dispatching the action.
private getStudySubjects(): void {
+ if (!this.provider?.id) {
+ console.error('Provider ID is not available');
+ return;
+ }
this.subjectParameters.providerId = this.provider.id;
Util.setFromPaginationParam(this.subjectParameters, this.currentPage, this.totalElements);
this.store.dispatch(new GetStudySubjects(this.subjectParameters));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private getStudySubjects(): void { | |
| this.subjectParameters.providerId = this.provider.id; | |
| Util.setFromPaginationParam(this.subjectParameters, this.currentPage, this.totalElements); | |
| this.store.dispatch(new GetStudySubjects(this.subjectParameters)); | |
| } | |
| private getStudySubjects(): void { | |
| if (!this.provider?.id) { | |
| console.error('Provider ID is not available'); | |
| return; | |
| } | |
| this.subjectParameters.providerId = this.provider.id; | |
| Util.setFromPaginationParam(this.subjectParameters, this.currentPage, this.totalElements); | |
| this.store.dispatch(new GetStudySubjects(this.subjectParameters)); | |
| } |
| public ngOnDestroy(): void { | ||
| this.destroy$.next(true); | ||
| this.destroy$.unsubscribe(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cleanup in ngOnDestroy.
The destroy$ Subject should be completed before unsubscribing.
public ngOnDestroy(): void {
this.destroy$.next(true);
+ this.destroy$.complete();
this.destroy$.unsubscribe();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ngOnDestroy(): void { | |
| this.destroy$.next(true); | |
| this.destroy$.unsubscribe(); | |
| } | |
| public ngOnDestroy(): void { | |
| this.destroy$.next(true); | |
| this.destroy$.complete(); | |
| this.destroy$.unsubscribe(); | |
| } |
| export class GetLanguageList { | ||
| static readonly type = '[provider] get language list'; | ||
| constructor() {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary constructor.
The empty constructor in the GetLanguageList class is unnecessary and can be removed.
Apply this diff to remove the unnecessary constructor:
export class GetLanguageList {
static readonly type = '[provider] get language list';
- constructor() {}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class GetLanguageList { | |
| static readonly type = '[provider] get language list'; | |
| constructor() {} | |
| } | |
| export class GetLanguageList { | |
| static readonly type = '[provider] get language list'; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 479-479: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/app/shared/store/provider.actions.ts (1)
477-480: Remove unnecessary constructor.The empty constructor in the
GetLanguageListclass is unnecessary and can be removed.Apply this diff to remove the unnecessary constructor:
export class GetLanguageList { static readonly type = '[provider] get language list'; - constructor() {} }🧰 Tools
🪛 Biome (1.9.4)
[error] 479-479: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/app/shared/store/provider.state.ts (2)
976-982: Avoid empty destructuring pattern.Using an empty pattern
{}for the second argument can trigger lint warnings. You can omit it or rename it to_actionto avoid confusion.Apply this diff:
- getLanguageList({ patchState }: StateContext<ProviderStateModel>, {}: providerActions.GetLanguageList): Observable<LanguageListItem[]> { + getLanguageList({ patchState }: StateContext<ProviderStateModel>, _action: providerActions.GetLanguageList): Observable<LanguageListItem[]> {🧰 Tools
🪛 Biome (1.9.4)
[error] 977-977: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
984-1013: Useundefinedinstead ofvoidin union types.Declaring return types as
Observable<SubjectModel | void>can be confusing. Replacevoidwithundefined.Apply these diffs:
- createStudySubject(...): Observable<SubjectModel | void> { + createStudySubject(...): Observable<SubjectModel | undefined> { - UpdateStudySubject(...): Observable<SubjectModel | void> { + UpdateStudySubject(...): Observable<SubjectModel | undefined> {Also applies to: 1039-1068
🧰 Tools
🪛 Biome (1.9.4)
[error] 988-988: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/i18n/en.jsonis excluded by!src/assets/**and included bysrc/**src/assets/i18n/uk.jsonis excluded by!src/assets/**and included bysrc/**
📒 Files selected for processing (7)
src/app/header/header.component.html(1 hunks)src/app/shared/store/provider.actions.ts(2 hunks)src/app/shared/store/provider.state.ts(7 hunks)src/app/shell/personal-cabinet/personal-cabinet.component.html(1 hunks)src/app/shell/personal-cabinet/provider/provider-routing.module.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider.module.ts(2 hunks)src/app/shell/shell-routing.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/shell/personal-cabinet/provider/provider-routing.module.ts
- src/app/shell/personal-cabinet/provider/provider.module.ts
- src/app/header/header.component.html
- src/app/shell/personal-cabinet/personal-cabinet.component.html
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/store/provider.actions.ts
[error] 479-479: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/app/shared/store/provider.state.ts
[error] 977-977: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 988-988: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 1043-1043: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 1074-1074: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (5)
src/app/shell/shell-routing.module.ts (2)
39-39: LGTM!The import statement follows Angular's convention and maintains consistency with other provider component imports.
98-104: LGTM!The route configuration is well-structured and follows the established patterns:
- Uses consistent path parameter naming (
:param)- Implements proper lazy loading of the provider module
- Protected by appropriate guards (
ProviderGuardfor authorization andCreateGuardfor form handling)src/app/shared/store/provider.actions.ts (1)
482-541: Well-structured NGXS actions for study subject management.The implementation follows NGXS best practices with:
- Clear action types and naming conventions
- Proper payload typing
- Comprehensive error handling with success/fail states
src/app/shared/store/provider.state.ts (2)
28-33: Well-structured state model extensions.The state model is properly extended with new properties and imports for study subject management.
Also applies to: 52-54
1014-1037: Well-implemented study subject state management.The implementation follows NGXS patterns with:
- Proper loading state management
- Comprehensive error handling
- Clear success/failure flows
- Proper state updates
Also applies to: 1070-1097
wovkun
force-pushed
the
Pasternak/study-subjects
branch
2 times, most recently
from
8c9ec61 to
0598c06
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/app/shared/store/provider.actions.ts (1)
477-480: 🛠️ Refactor suggestionRemove unnecessary constructor.
The empty constructor serves no purpose and can be removed.
export class GetLanguageList { static readonly type = '[provider] get language list'; - constructor() {} }🧰 Tools
🪛 Biome (1.9.4)
[error] 479-479: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🧹 Nitpick comments (4)
src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.ts (1)
72-76: Extract form control names as constants.To prevent typos and improve maintainability, consider extracting form control names as constants:
+const FORM_CONTROLS = { + NAME_IN_UKRAINIAN: 'nameInUkrainian', + NAME_IN_INSTRUCTION_LANGUAGE: 'nameInInstructionLanguage', + LANGUAGE: 'language' +} as const; this.studySubjectFormGroup = this.formBuilder.group({ - nameInUkrainian: new FormControl('', defaultValidators), - nameInInstructionLanguage: new FormControl('', defaultValidators), - language: new FormControl('', Validators.required) + [FORM_CONTROLS.NAME_IN_UKRAINIAN]: new FormControl('', defaultValidators), + [FORM_CONTROLS.NAME_IN_INSTRUCTION_LANGUAGE]: new FormControl('', defaultValidators), + [FORM_CONTROLS.LANGUAGE]: new FormControl('', Validators.required) });src/app/shared/store/provider.state.ts (1)
81-82: TODO comments need to be addressed.There are two TODO comments that need to be addressed:
- Show date of last connection with workshop
- Study subject connection with workshop (number of connections)
Would you like me to help implement these features or create issues to track them?
Also applies to: 89-90
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html (2)
20-35: Consider adding form validation for date range.The date range picker lacks validation to ensure:
- The end date is not before the start date
- The selected range is not too wide
Consider adding form validation and error messages for better user experience.
86-86: Consider using a constant for the magic date.The hardcoded date
'9999-12-31'should be moved to a constant.Consider creating a constant like
MAX_DATEin your constants file:export const MAX_DATE = '9999-12-31';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/i18n/en.jsonis excluded by!src/assets/**and included bysrc/**src/assets/i18n/uk.jsonis excluded by!src/assets/**and included bysrc/**
📒 Files selected for processing (23)
src/app/header/header.component.html(1 hunks)src/app/shared/enum/enumUA/message-bar.ts(1 hunks)src/app/shared/enum/enumUA/navigation-bar.ts(1 hunks)src/app/shared/enum/enumUA/no-results.ts(1 hunks)src/app/shared/enum/modal-confirmation.ts(3 hunks)src/app/shared/models/language-list.model.ts(1 hunks)src/app/shared/models/study-subject.model.ts(1 hunks)src/app/shared/services/language-list/language-list.service.ts(1 hunks)src/app/shared/services/study-subjects/study-subjects.service.ts(1 hunks)src/app/shared/store/provider.actions.ts(2 hunks)src/app/shared/store/provider.state.ts(7 hunks)src/app/shell/personal-cabinet/personal-cabinet.component.html(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.html(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.scss(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.spec.ts(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-routing.module.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.scss(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.spec.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider.module.ts(2 hunks)src/app/shell/shell-routing.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- src/app/shared/models/language-list.model.ts
- src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.spec.ts
- src/app/shared/services/language-list/language-list.service.ts
- src/app/shell/personal-cabinet/personal-cabinet.component.html
- src/app/shell/personal-cabinet/provider/provider.module.ts
- src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.scss
- src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.scss
- src/app/shell/personal-cabinet/provider/provider-routing.module.ts
- src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.spec.ts
- src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.html
- src/app/shared/enum/enumUA/message-bar.ts
- src/app/shared/enum/enumUA/navigation-bar.ts
- src/app/header/header.component.html
- src/app/shell/shell-routing.module.ts
- src/app/shared/services/study-subjects/study-subjects.service.ts
- src/app/shared/enum/enumUA/no-results.ts
- src/app/shared/models/study-subject.model.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/store/provider.actions.ts
[error] 479-479: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/app/shared/store/provider.state.ts
[error] 977-977: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 988-988: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 1043-1043: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 1074-1074: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (21)
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.ts (5)
81-89: LGTM!The navigation path is correctly configured and dispatched.
119-122: LGTM!The pagination size change is handled correctly.
124-127: LGTM!The page change is handled correctly.
129-132: Improve cleanup in ngOnDestroy.The destroy$ Subject should be completed before unsubscribing.
public ngOnDestroy(): void { this.destroy$.next(true); + this.destroy$.complete(); this.destroy$.unsubscribe(); }
134-138: Add error handling to getStudySubjects.The private method should handle potential errors when dispatching the action.
private getStudySubjects(): void { + if (!this.provider?.id) { + console.error('Provider ID is not available'); + return; + } this.subjectParameters.providerId = this.provider.id; Util.setFromPaginationParam(this.subjectParameters, this.currentPage, this.totalElements); this.store.dispatch(new GetStudySubjects(this.subjectParameters)); }src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.ts (3)
81-90: Leverage existing provider subscription.The provider is also subscribed to again in
setEditMode. You could reuse a single subscription to store the provider inngOnInit, eliminating multiple identical subscriptions.- this.provider$ - .pipe( - filter((provider: Provider) => Boolean(provider)), - tap((provider: Provider) => { - this.provider = provider; - }), - takeUntil(this.destroy$) - ) - .subscribe(); + // Possibly store the provider in a local variable in ngOnInit and reuse it in setEditMode
132-152: LGTM!The navigation paths are correctly configured based on the user role and edit mode.
181-183: LGTM!The cancel navigation is handled correctly.
src/app/shared/enum/modal-confirmation.ts (3)
46-49: LGTM!The new enum members for subject management follow the existing naming convention and are consistent with the enum's purpose.
101-104: LGTM!The new title enum members are correctly mapped to their respective translation keys.
150-153: LGTM!The new text enum members are correctly mapped to their respective translation keys.
src/app/shared/store/provider.actions.ts (1)
482-541: LGTM!The study subject action classes are well-structured and follow consistent patterns:
- Clear and consistent type identifiers
- Properly typed payloads
- Corresponding success and fail actions for each operation
src/app/shared/store/provider.state.ts (5)
28-33: LGTM! New imports and state model properties are well-organized.The new imports and state model properties for language list and study subjects are logically grouped and follow the existing pattern.
Also applies to: 52-54
88-89: LGTM! Services are properly injected.The new services for study subjects and language list are correctly injected into the constructor.
132-135: LGTM! Selectors are well-implemented.The new selectors for
languageList,studySubject, andselectedSubjectfollow the established pattern and are properly typed.Also applies to: 157-165
976-977: Avoid empty destructuring pattern.Using an empty pattern
{}for the second argument can trigger lint warnings.Apply this diff to fix the issue:
- getLanguageList({ patchState }: StateContext<ProviderStateModel>, {}: providerActions.GetLanguageList): Observable<LanguageListItem[]> { + getLanguageList({ patchState }: StateContext<ProviderStateModel>, _action: providerActions.GetLanguageList): Observable<LanguageListItem[]> {🧰 Tools
🪛 Biome (1.9.4)
[error] 977-977: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
988-988: Useundefinedinstead ofvoidin union types.Declaring return types as
Observable<SubjectModel | void>can be confusing.Apply this diff to fix the issue:
- createStudySubject(...): Observable<SubjectModel | void> { + createStudySubject(...): Observable<SubjectModel | undefined> { - UpdateStudySubject(...): Observable<SubjectModel | void> { + UpdateStudySubject(...): Observable<SubjectModel | undefined> { - DeleteStudySubjectById(...): Observable<SubjectModel[] | void> { + DeleteStudySubjectById(...): Observable<SubjectModel[] | undefined> {Also applies to: 1043-1043, 1074-1074
🧰 Tools
🪛 Biome (1.9.4)
[error] 988-988: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html (4)
1-47: LGTM! Search and filter area is well-structured.The search and filter area is well-organized with proper flex layout and clear visual hierarchy.
49-122: LGTM! Table implementation follows best practices.The table implementation:
- Uses proper ARIA attributes for accessibility
- Implements sorting functionality
- Handles conditional rendering
- Includes pagination
124-128: LGTM! Empty state is properly handled.The empty state is handled gracefully using a reusable no-result card component.
94-94:❓ Verification inconclusive
Avoid hardcoding values.
The hardcoded value
'0'for workshop connections should be replaced with actual data once the TODO is implemented.
🏁 Script executed:
#!/bin/bash # Description: Check if workshop connections are implemented in the service. ast-grep --pattern $'class StudySubjectService { $$$ getWorkshopConnections($_, $_) { $$$ } $$$ }'Length of output: 111
Manual Verification Required: Replace the Hardcoded Value Once Data Integration Is Confirmed
The automated search did not detect any implementation of the
getWorkshopConnectionsmethod in theStudySubjectService. Please manually verify whether the workshop connections logic exists or is planned to be implemented. Once confirmed, update the hardcoded value in the template accordingly.
- Location:
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.htmlat line 94.- Action: Replace the literal
'0'with the dynamic data obtained from the service call once the implementation is confirmed.
| this.store | ||
| .select(ProviderState.studySubject) | ||
| .pipe( | ||
| filter((subject: SearchResponse<SubjectModel[]>) => Boolean(subject)), | ||
| takeUntil(this.destroy$) | ||
| ) | ||
| .subscribe((subjects: SearchResponse<SubjectModel[]>) => { | ||
| this.dataSource.data = subjects.entities; | ||
| this.totalElements = subjects.totalAmount; | ||
| this.isLoaded = true; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the subscription.
Consider adding error handling to the subscription to gracefully handle potential errors:
this.store
.select(ProviderState.studySubject)
.pipe(
filter((subject: SearchResponse<SubjectModel[]>) => Boolean(subject)),
takeUntil(this.destroy$)
)
.subscribe(
+ {
+ next: (subjects: SearchResponse<SubjectModel[]>) => {
this.dataSource.data = subjects.entities;
this.totalElements = subjects.totalAmount;
this.isLoaded = true;
+ },
+ error: (error) => {
+ console.error('Error loading study subjects:', error);
+ this.isLoaded = true;
+ }
+ }
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.store | |
| .select(ProviderState.studySubject) | |
| .pipe( | |
| filter((subject: SearchResponse<SubjectModel[]>) => Boolean(subject)), | |
| takeUntil(this.destroy$) | |
| ) | |
| .subscribe((subjects: SearchResponse<SubjectModel[]>) => { | |
| this.dataSource.data = subjects.entities; | |
| this.totalElements = subjects.totalAmount; | |
| this.isLoaded = true; | |
| }); | |
| this.store | |
| .select(ProviderState.studySubject) | |
| .pipe( | |
| filter((subject: SearchResponse<SubjectModel[]>) => Boolean(subject)), | |
| takeUntil(this.destroy$) | |
| ) | |
| .subscribe({ | |
| next: (subjects: SearchResponse<SubjectModel[]>) => { | |
| this.dataSource.data = subjects.entities; | |
| this.totalElements = subjects.totalAmount; | |
| this.isLoaded = true; | |
| }, | |
| error: (error) => { | |
| console.error('Error loading study subjects:', error); | |
| this.isLoaded = true; | |
| } | |
| }); |
| public onDelete(subject: SubjectModel): void { | ||
| const dialogRef = this.matDialog.open(ConfirmationModalWindowComponent, { | ||
| width: Constants.MODAL_SMALL, | ||
| data: { type: ModalConfirmationType.deleteSubject, property: subject.nameInUkrainian } | ||
| }); | ||
| dialogRef | ||
| .afterClosed() | ||
| .pipe(filter(Boolean)) | ||
| .subscribe(() => this.store.dispatch(new DeleteStudySubjectById(this.subjectParameters, subject.id))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add subscription cleanup to prevent memory leaks.
The subscription to afterClosed() should be cleaned up to prevent memory leaks:
dialogRef
.afterClosed()
.pipe(
filter(Boolean),
+ takeUntil(this.destroy$)
)
.subscribe(() => this.store.dispatch(new DeleteStudySubjectById(this.subjectParameters, subject.id)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public onDelete(subject: SubjectModel): void { | |
| const dialogRef = this.matDialog.open(ConfirmationModalWindowComponent, { | |
| width: Constants.MODAL_SMALL, | |
| data: { type: ModalConfirmationType.deleteSubject, property: subject.nameInUkrainian } | |
| }); | |
| dialogRef | |
| .afterClosed() | |
| .pipe(filter(Boolean)) | |
| .subscribe(() => this.store.dispatch(new DeleteStudySubjectById(this.subjectParameters, subject.id))); | |
| } | |
| public onDelete(subject: SubjectModel): void { | |
| const dialogRef = this.matDialog.open(ConfirmationModalWindowComponent, { | |
| width: Constants.MODAL_SMALL, | |
| data: { type: ModalConfirmationType.deleteSubject, property: subject.nameInUkrainian } | |
| }); | |
| dialogRef | |
| .afterClosed() | |
| .pipe( | |
| filter(Boolean), | |
| takeUntil(this.destroy$) | |
| ) | |
| .subscribe(() => this.store.dispatch(new DeleteStudySubjectById(this.subjectParameters, subject.id))); | |
| } |
| this.filterFormControl.valueChanges | ||
| .pipe( | ||
| distinctUntilChanged(), | ||
| startWith(''), | ||
| skip(1), | ||
| debounceTime(1000), | ||
| takeUntil(this.destroy$), | ||
| map((searchedText: string) => searchedText.trim()) | ||
| ) | ||
| .subscribe((searchedText: string) => { | ||
| this.subjectParameters.searchString = searchedText; | ||
|
|
||
| this.currentPage = PaginationConstants.firstPage; | ||
| this.getStudySubjects(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove conflicting RxJS operators.
The skip(1) operator conflicts with startWith('') as it negates the initial emission. Consider removing one of these operators based on your requirements:
- Keep
startWith('')if you want to trigger an initial search - Keep
skip(1)if you want to avoid the initial search
Apply one of these diffs based on your needs:
this.filterFormControl.valueChanges
.pipe(
distinctUntilChanged(),
- startWith(''),
skip(1),
debounceTime(1000),
takeUntil(this.destroy$),
map((searchedText: string) => searchedText.trim())
)OR
this.filterFormControl.valueChanges
.pipe(
distinctUntilChanged(),
startWith(''),
- skip(1),
debounceTime(1000),
takeUntil(this.destroy$),
map((searchedText: string) => searchedText.trim())
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.filterFormControl.valueChanges | |
| .pipe( | |
| distinctUntilChanged(), | |
| startWith(''), | |
| skip(1), | |
| debounceTime(1000), | |
| takeUntil(this.destroy$), | |
| map((searchedText: string) => searchedText.trim()) | |
| ) | |
| .subscribe((searchedText: string) => { | |
| this.subjectParameters.searchString = searchedText; | |
| this.currentPage = PaginationConstants.firstPage; | |
| this.getStudySubjects(); | |
| }); | |
| this.filterFormControl.valueChanges | |
| .pipe( | |
| distinctUntilChanged(), | |
| startWith(''), | |
| debounceTime(1000), | |
| takeUntil(this.destroy$), | |
| map((searchedText: string) => searchedText.trim()) | |
| ) | |
| .subscribe((searchedText: string) => { | |
| this.subjectParameters.searchString = searchedText; | |
| this.currentPage = PaginationConstants.firstPage; | |
| this.getStudySubjects(); | |
| }); |
| combineLatest([ | ||
| this.selectedSubject$.pipe(filter((subject: SubjectModel) => !!subject && subject.id === subjectId)), | ||
| this.languageList$.pipe(filter((languages: LanguageListItem[]) => !!languages && languages.length > 0)) | ||
| ]) | ||
| .pipe(takeUntil(this.destroy$)) | ||
| .subscribe(([subject, languages]) => { | ||
| this.studySubject = new SubjectModel(subject, this.provider, subjectId); | ||
| const selectedLanguage = languages.find((lang) => lang.id === subject.language.id); | ||
| this.studySubjectFormGroup.patchValue( | ||
| { | ||
| nameInUkrainian: subject.nameInUkrainian, | ||
| nameInInstructionLanguage: subject.nameInInstructionLanguage, | ||
| language: selectedLanguage | ||
| }, | ||
| { emitEvent: false } | ||
| ); | ||
|
|
||
| this.cdr.markForCheck(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the combineLatest subscription.
Consider adding error handling to handle potential errors gracefully:
combineLatest([
this.selectedSubject$.pipe(filter((subject: SubjectModel) => !!subject && subject.id === subjectId)),
this.languageList$.pipe(filter((languages: LanguageListItem[]) => !!languages && languages.length > 0))
])
.pipe(takeUntil(this.destroy$))
- .subscribe(([subject, languages]) => {
- this.studySubject = new SubjectModel(subject, this.provider, subjectId);
- const selectedLanguage = languages.find((lang) => lang.id === subject.language.id);
- this.studySubjectFormGroup.patchValue(
- {
- nameInUkrainian: subject.nameInUkrainian,
- nameInInstructionLanguage: subject.nameInInstructionLanguage,
- language: selectedLanguage
- },
- { emitEvent: false }
- );
-
- this.cdr.markForCheck();
- });
+ .subscribe({
+ next: ([subject, languages]) => {
+ this.studySubject = new SubjectModel(subject, this.provider, subjectId);
+ const selectedLanguage = languages.find((lang) => lang.id === subject.language.id);
+ this.studySubjectFormGroup.patchValue(
+ {
+ nameInUkrainian: subject.nameInUkrainian,
+ nameInInstructionLanguage: subject.nameInInstructionLanguage,
+ language: selectedLanguage
+ },
+ { emitEvent: false }
+ );
+
+ this.cdr.markForCheck();
+ },
+ error: (error) => {
+ console.error('Error setting edit mode:', error);
+ // Handle the error appropriately
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| combineLatest([ | |
| this.selectedSubject$.pipe(filter((subject: SubjectModel) => !!subject && subject.id === subjectId)), | |
| this.languageList$.pipe(filter((languages: LanguageListItem[]) => !!languages && languages.length > 0)) | |
| ]) | |
| .pipe(takeUntil(this.destroy$)) | |
| .subscribe(([subject, languages]) => { | |
| this.studySubject = new SubjectModel(subject, this.provider, subjectId); | |
| const selectedLanguage = languages.find((lang) => lang.id === subject.language.id); | |
| this.studySubjectFormGroup.patchValue( | |
| { | |
| nameInUkrainian: subject.nameInUkrainian, | |
| nameInInstructionLanguage: subject.nameInInstructionLanguage, | |
| language: selectedLanguage | |
| }, | |
| { emitEvent: false } | |
| ); | |
| this.cdr.markForCheck(); | |
| }); | |
| combineLatest([ | |
| this.selectedSubject$.pipe(filter((subject: SubjectModel) => !!subject && subject.id === subjectId)), | |
| this.languageList$.pipe(filter((languages: LanguageListItem[]) => !!languages && languages.length > 0)) | |
| ]) | |
| .pipe(takeUntil(this.destroy$)) | |
| .subscribe({ | |
| next: ([subject, languages]) => { | |
| this.studySubject = new SubjectModel(subject, this.provider, subjectId); | |
| const selectedLanguage = languages.find((lang) => lang.id === subject.language.id); | |
| this.studySubjectFormGroup.patchValue( | |
| { | |
| nameInUkrainian: subject.nameInUkrainian, | |
| nameInInstructionLanguage: subject.nameInInstructionLanguage, | |
| language: selectedLanguage | |
| }, | |
| { emitEvent: false } | |
| ); | |
| this.cdr.markForCheck(); | |
| }, | |
| error: (error) => { | |
| console.error('Error setting edit mode:', error); | |
| // Handle the error appropriately | |
| } | |
| }); |
| public onSubmit(): void { | ||
| if (this.studySubjectFormGroup.dirty && !this.isDispatching) { | ||
| this.matDialog | ||
| .open(ConfirmationModalWindowComponent, { | ||
| width: Constants.MODAL_SMALL, | ||
| data: { | ||
| type: this.editMode ? ModalConfirmationType.editSubject : ModalConfirmationType.createSubject | ||
| } | ||
| }) | ||
| .afterClosed() | ||
| .pipe(filter(Boolean)) | ||
| .subscribe(() => { | ||
| this.isDispatching = true; | ||
|
|
||
| const subjectInfo = this.studySubjectFormGroup.value; | ||
| let subject: SubjectModel; | ||
| if (this.editMode) { | ||
| subject = new SubjectModel(subjectInfo, this.provider, this.studySubject.id); | ||
| this.store.dispatch(new UpdateStudySubject(subject)); | ||
| } else { | ||
| subject = new SubjectModel(subjectInfo, this.provider); | ||
| this.store.dispatch(new CreateStudySubject(subject)); | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve subscription management and error handling.
The function needs subscription cleanup and better error handling:
if (this.studySubjectFormGroup.dirty && !this.isDispatching) {
this.matDialog
.open(ConfirmationModalWindowComponent, {
width: Constants.MODAL_SMALL,
data: {
type: this.editMode ? ModalConfirmationType.editSubject : ModalConfirmationType.createSubject
}
})
.afterClosed()
.pipe(
filter(Boolean),
+ takeUntil(this.destroy$)
)
.subscribe(() => {
this.isDispatching = true;
const subjectInfo = this.studySubjectFormGroup.value;
let subject: SubjectModel;
if (this.editMode) {
subject = new SubjectModel(subjectInfo, this.provider, this.studySubject.id);
- this.store.dispatch(new UpdateStudySubject(subject));
+ this.store.dispatch(new UpdateStudySubject(subject))
+ .pipe(
+ finalize(() => this.isDispatching = false)
+ )
+ .subscribe({
+ error: (error) => console.error('Error updating subject:', error)
+ });
} else {
subject = new SubjectModel(subjectInfo, this.provider);
- this.store.dispatch(new CreateStudySubject(subject));
+ this.store.dispatch(new CreateStudySubject(subject))
+ .pipe(
+ finalize(() => this.isDispatching = false)
+ )
+ .subscribe({
+ error: (error) => console.error('Error creating subject:', error)
+ });
}
});
}Committable suggestion skipped: line range outside the PR's diff.
| @Action(providerActions.UpdateStudySubject) | ||
| UpdateStudySubject( | ||
| { dispatch }: StateContext<ProviderStateModel>, | ||
| { payload }: providerActions.UpdateStudySubject | ||
| ): Observable<SubjectModel | void> { | ||
| return this.studySubjectService.updateStudySubject(payload).pipe( | ||
| tap((res: SubjectModel) => dispatch(new providerActions.OnUpdateStudySubjectSuccess(res))), | ||
| catchError((error: HttpErrorResponse) => dispatch(new providerActions.OnUpdateStudySubjectFail(error))) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent method naming convention.
The methods UpdateStudySubject, DeleteStudySubjectById, and their corresponding success/fail handlers use PascalCase, while other methods use camelCase.
Apply this diff to maintain consistency:
- UpdateStudySubject(
+ updateStudySubject(
- DeleteStudySubjectById(
+ deleteStudySubjectById(Also applies to: 1070-1081
🧰 Tools
🪛 Biome (1.9.4)
[error] 1043-1043: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
wovkun
force-pushed
the
Pasternak/study-subjects
branch
from
3af2b4d to
21d375b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.ts (1)
154-179: 🛠️ Refactor suggestionReset isDispatching flag after dispatch completes.
The
isDispatchingflag is set to true but never reset to false, which could prevent future submissions if an error occurs.public onSubmit(): void { if (this.studySubjectFormGroup.dirty && !this.isDispatching) { this.matDialog .open(ConfirmationModalWindowComponent, { width: Constants.MODAL_SMALL, data: { type: this.editMode ? ModalConfirmationType.editSubject : ModalConfirmationType.createSubject } }) .afterClosed() - .pipe(filter(Boolean)) + .pipe( + filter(Boolean), + takeUntil(this.destroy$) + ) .subscribe(() => { this.isDispatching = true; const subjectInfo = this.studySubjectFormGroup.value; let subject: SubjectModel; if (this.editMode) { subject = new SubjectModel(subjectInfo, this.provider, this.studySubject.id); - this.store.dispatch(new UpdateStudySubject(subject)); + this.store.dispatch(new UpdateStudySubject(subject)) + .pipe( + finalize(() => this.isDispatching = false) + ) + .subscribe({ + error: (error) => console.error('Error updating subject:', error) + }); } else { subject = new SubjectModel(subjectInfo, this.provider); - this.store.dispatch(new CreateStudySubject(subject)); + this.store.dispatch(new CreateStudySubject(subject)) + .pipe( + finalize(() => this.isDispatching = false) + ) + .subscribe({ + error: (error) => console.error('Error creating subject:', error) + }); } }); } }src/app/shared/store/provider.state.ts (4)
1122-1128: 🛠️ Refactor suggestionFix empty object pattern in action parameter.
The destructuring of an empty object
{}in the action parameter can cause lint warnings. Replace it with a named parameter.- getLanguageList({ patchState }: StateContext<ProviderStateModel>, {}: providerActions.GetLanguageList): Observable<LanguageListItem[]> { + getLanguageList({ patchState }: StateContext<ProviderStateModel>, _action: providerActions.GetLanguageList): Observable<LanguageListItem[]> {🧰 Tools
🪛 Biome (1.9.4)
[error] 1123-1123: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
1130-1140: 🛠️ Refactor suggestionReplace
voidwithundefinedin union type.Using
voidin a union type is confusing and not semantically correct. Replace it withundefinedfor better type safety.- createStudySubject(...): Observable<SubjectModel | void> { + createStudySubject(...): Observable<SubjectModel | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 1134-1134: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
1185-1194: 🛠️ Refactor suggestionUse consistent method naming conventions.
The methods
UpdateStudySubject,OnUpdateStudySubjectSuccessuse PascalCase while most other methods in the class use camelCase. Maintain consistency across the codebase.- @Action(providerActions.UpdateStudySubject) - UpdateStudySubject( + @Action(providerActions.UpdateStudySubject) + updateStudySubject(Additionally, fix the return type by replacing
voidwithundefined.🧰 Tools
🪛 Biome (1.9.4)
[error] 1189-1189: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
1216-1227: 🛠️ Refactor suggestionUse consistent method naming and fix return type.
The method
DeleteStudySubjectByIduses PascalCase while most other methods in the class use camelCase. Also, the return type union should useundefinedinstead ofvoid.- @Action(providerActions.DeleteStudySubjectById) - DeleteStudySubjectById( + @Action(providerActions.DeleteStudySubjectById) + deleteStudySubjectById( ... - ): Observable<SubjectModel[] | void> { + ): Observable<SubjectModel[] | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 1220-1220: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🧹 Nitpick comments (9)
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html (1)
19-39: Remove or uncomment the filters-wrapper section.There's a large commented-out section containing date filters functionality. Either remove it completely if it's not needed or uncomment and implement it if it's required for the feature.
src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.spec.ts (1)
92-108: Enhance onSubmit test coverage.The test only covers the case where the form is pristine. Add tests for successful form submission in both create and edit modes.
it('should dispatch CreateStudySubject when form is valid and in create mode', () => { component.editMode = false; component.studySubjectFormGroup.markAsDirty(); component.provider = { id: '123' } as any; // Mock the dialog open to return observable that emits true mockMatDialog.open.mockReturnValue({ afterClosed: jest.fn().mockReturnValue(of(true)) } as any); component.onSubmit(); // Dialog should be opened expect(mockMatDialog.open).toHaveBeenCalled(); // When dialog closes with true, dispatch should be called expect(mockStore.dispatch).toHaveBeenCalledWith(expect.any(CreateStudySubject)); }); it('should dispatch UpdateStudySubject when form is valid and in edit mode', () => { component.editMode = true; component.studySubjectFormGroup.markAsDirty(); component.provider = { id: '123' } as any; component.studySubject = { id: '456' } as any; // Mock the dialog open to return observable that emits true mockMatDialog.open.mockReturnValue({ afterClosed: jest.fn().mockReturnValue(of(true)) } as any); component.onSubmit(); // Dialog should be opened expect(mockMatDialog.open).toHaveBeenCalled(); // When dialog closes with true, dispatch should be called expect(mockStore.dispatch).toHaveBeenCalledWith(expect.any(UpdateStudySubject)); });src/app/shared/services/language-list/language-list.service.spec.ts (1)
35-43: Add error handling test for HTTP requests.The current test only verifies successful HTTP responses. Add a test to handle error scenarios as well.
it('should handle errors when the request fails', () => { const errorResponse = new ErrorEvent('Network error', { message: 'simulated network error', }); service.getLanguageList().subscribe({ next: () => fail('should have failed with an error'), error: (error) => { expect(error.message).toContain('simulated network error'); } }); const req = httpTestingController.expectOne(mockApiUrl); req.error(errorResponse); });src/app/shared/services/study-subjects/study-subjects.spec.ts (3)
51-61: Consider adding negative tests forcreateStudySubject.Currently, the test for
createStudySubjectonly covers successful creation with a validmockSubject. It would be beneficial to add a test to validate error handling when required fields are missing or invalid (e.g., noproviderId). This helps ensure the service gracefully manages bad request scenarios.
63-79: Verify handling of special characters in the search string.The test confirms a straightforward search flow, but it's good to ensure that special or escaped characters (e.g.,
%,#, or whitespace) are handled properly. If such cases are common, consider adding a test to ensure encoding and server acceptance are correct.
135-146: Enhance error assertion for improved clarity.When testing HTTP errors (lines 135–146), consider verifying not just the status code but also the response message or error object structure, if applicable. Doing so ensures you catch unexpected changes in server error responses early.
src/app/shared/services/study-subjects/study-subjects.service.ts (1)
19-21: Validate the provider ID increateStudySubject.Similar to
getStudySubjectById, consider checkingstudySubject.providerIdduring creation. An early guard clause would prevent unintended API calls if the provider ID is missing.public createStudySubject(studySubject: SubjectModel): Observable<SubjectModel> { + if (!studySubject.providerId) { + return throwError(() => new Error('Provider ID is required for creating a study subject')); + } return this.http.post<SubjectModel>(`${this.baseUrl}/${studySubject.providerId}/studysubjects/Create`, studySubject); }src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.spec.ts (2)
61-69: Consider merging beforeEach blocks.There are two separate
beforeEachblocks that could be combined into a single block to improve readability and maintainability. While multiplebeforeEachblocks are functionally correct, consolidating them follows better testing practices.- beforeEach(async () => { - await TestBed.configureTestingModule({ - imports: [ - NgxsModule.forRoot([]), - MatDialogModule, - MatTableModule, - TranslateModule.forRoot(), - MatSelectModule, - RouterTestingModule, - ReactiveFormsModule, - MatTooltipModule, - MatIconModule, - BrowserAnimationsModule, - MatDialog - ], - declarations: [ProviderStudySubjectsComponent, PaginatorComponent] - }).compileComponents(); - }); - - beforeEach(() => { - fixture = TestBed.createComponent(ProviderStudySubjectsComponent); - component = fixture.componentInstance; - store = TestBed.inject(Store); - - // Mock provider object here - component.provider = { id: '1' } as any; // Ensure provider has id property - fixture.detectChanges(); - }); + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [ + NgxsModule.forRoot([]), + MatDialogModule, + MatTableModule, + TranslateModule.forRoot(), + MatSelectModule, + RouterTestingModule, + ReactiveFormsModule, + MatTooltipModule, + MatIconModule, + BrowserAnimationsModule, + MatDialog + ], + declarations: [ProviderStudySubjectsComponent, PaginatorComponent] + }).compileComponents(); + + fixture = TestBed.createComponent(ProviderStudySubjectsComponent); + component = fixture.componentInstance; + store = TestBed.inject(Store); + + // Mock provider object here + component.provider = { id: '1' } as any; // Ensure provider has id property + fixture.detectChanges(); + });🧰 Tools
🪛 Biome (1.9.4)
[error] 61-69: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
89-95: Improve type safety in test.Line 89 casts
componenttoanybefore assigning aSubjectModelto the provider property, which bypasses type checking. Consider creating a more accurate mock that matches the expected provider type.- (component as any).provider = { id: '1', providerId: '123' } as SubjectModel; + // Create a properly typed mock that matches the Provider interface + component.provider = { id: '1', providerId: '123' } as unknown as Provider;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/i18n/en.jsonis excluded by!src/assets/**and included bysrc/**src/assets/i18n/uk.jsonis excluded by!src/assets/**and included bysrc/**
📒 Files selected for processing (25)
src/app/header/header.component.html(1 hunks)src/app/shared/enum/enumUA/message-bar.ts(1 hunks)src/app/shared/enum/enumUA/navigation-bar.ts(1 hunks)src/app/shared/enum/enumUA/no-results.ts(1 hunks)src/app/shared/enum/modal-confirmation.ts(3 hunks)src/app/shared/models/language-list.model.ts(1 hunks)src/app/shared/models/study-subject.model.ts(1 hunks)src/app/shared/services/language-list/language-list.service.spec.ts(1 hunks)src/app/shared/services/language-list/language-list.service.ts(1 hunks)src/app/shared/services/study-subjects/study-subjects.service.ts(1 hunks)src/app/shared/services/study-subjects/study-subjects.spec.ts(1 hunks)src/app/shared/store/provider.actions.ts(2 hunks)src/app/shared/store/provider.state.ts(7 hunks)src/app/shell/personal-cabinet/personal-cabinet.component.html(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.html(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.scss(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.spec.ts(1 hunks)src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-routing.module.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.scss(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.spec.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.ts(1 hunks)src/app/shell/personal-cabinet/provider/provider.module.ts(2 hunks)src/app/shell/shell-routing.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- src/app/shared/models/language-list.model.ts
- src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.scss
- src/app/shell/personal-cabinet/personal-cabinet.component.html
- src/app/shell/personal-cabinet/provider/provider-routing.module.ts
- src/app/header/header.component.html
- src/app/shared/services/language-list/language-list.service.ts
- src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.scss
- src/app/shared/enum/enumUA/navigation-bar.ts
- src/app/shell/personal-cabinet/provider/provider.module.ts
- src/app/shared/models/study-subject.model.ts
- src/app/shared/enum/enumUA/no-results.ts
- src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.html
- src/app/shared/enum/enumUA/message-bar.ts
- src/app/shared/enum/modal-confirmation.ts
- src/app/shell/shell-routing.module.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.spec.ts
[error] 61-69: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/app/shared/store/provider.actions.ts
[error] 551-551: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/app/shared/store/provider.state.ts
[error] 1123-1123: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 1134-1134: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 1189-1189: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 1220-1220: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.spec.ts
[error] 67-73: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_test
- GitHub Check: SonarCloud
🔇 Additional comments (19)
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.html (1)
83-89: Address the TODO comment about showing the last connection date.The TODO comment indicates that this column should show the date of the last connection with a workshop, but currently it's displaying the
activeToproperty with a date fallback.Do you need assistance implementing this feature?
src/app/shell/personal-cabinet/provider/create-study-subject/create-study-subject.component.ts (2)
100-110: Leverage existing provider subscription.The provider is also subscribed to again in
setEditMode. You could reuse a single subscription (or a single shared data structure) to store the provider inngOnInit, eliminating multiple identical subscriptions.
111-129: Add error handling to the combineLatest subscription.Consider adding error handling to handle potential errors gracefully:
src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.ts (5)
76-79: Remove conflicting RxJS operators.
You are currently using bothstartWith('')andskip(1), which can produce unexpected behavior when combined. Either removeskip(1)if the initial empty emission is necessary or removestartWith('')if you want to avoid the initial emission.
104-114: Add error callback to subscription.
The observable pipeline forProviderState.studySubjecthas a success callback only. Consider adding anerrorcallback to gracefully handle any errors that occur during state retrieval.
124-127: Add subscription cleanup to prevent potential memory leaks.
UsetakeUntil(this.destroy$)or a similar mechanism for theafterClosed()subscription, ensuring it’s also terminated when the component is destroyed.
139-142: Complete thedestroy$subject before unsubscribing.
Callingthis.destroy$.complete()helps avoid edge cases where subsequent emissions might occur after unsubscribing, ensuring proper cleanup.
144-148: Add error handling for missing provider ID.
Ifthis.provider.idis unavailable ingetStudySubjects(), consider exiting early or logging an error. This matches the approach shown in other methods and avoids dispatching invalid requests.src/app/shared/services/study-subjects/study-subjects.service.ts (3)
23-31: Add error handling for invalid parameters ingetStudySubjects.
Consider validatingsubjectParameters.providerIdbefore constructing the request. If it’s empty, throw an error to prevent invalid API calls.
32-38: The error handling for missing provider ID is a best practice.
This guard clause ensures you don’t attempt to call an endpoint with an invalid URL or break the flow. Great job including it here.
40-42: Add error handling for thedeleteStudySubjectmethod.
Similar to the other methods, it’s beneficial to throw an error ifsubjectIdorsubjectParameters.providerIdis missing, preventing invalid or accidental API calls.src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.spec.ts (2)
42-59: Configure TestBed with appropriate imports and declarations.The test module setup looks good with all necessary imports for testing the Angular Material components, NGXS store, and other required modules.
99-114: Well-structured test for provider data initialization.This test effectively verifies that the component properly initializes provider data and subscribes to study subjects in the store. It mocks the store response appropriately and checks that component properties are updated correctly.
src/app/shared/store/provider.actions.ts (4)
549-552: Remove unnecessary constructor.The empty constructor in the
GetLanguageListclass is unnecessary and can be removed.export class GetLanguageList { static readonly type = '[provider] get language list'; - constructor() {} }🧰 Tools
🪛 Biome (1.9.4)
[error] 551-551: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
554-567: Well-structured action classes for study subject creation.The action classes for creating study subjects follow the pattern established elsewhere in the codebase with success and failure handlers.
569-572: Good implementation of GetStudySubjects action.The
GetStudySubjectsaction properly usesSubjectParametersfor filtering and pagination, following the same pattern as other resource fetching actions.
597-613: Well-implemented deletion actions.The deletion actions follow the established pattern in the codebase with proper parameters for both targeting the specific subject and handling the search parameters for refreshing the list.
src/app/shared/store/provider.state.ts (2)
63-65: Well-structured state model extensions.The new properties added to the ProviderStateModel interface are properly typed and follow the established pattern in the codebase.
1229-1238: Good implementation of delete success handler.The
onDeleteStudySubjectSuccessmethod correctly dispatches both a success message and an action to refresh the list with the provided parameters.
| <!-- TODO: Study subject connection with workshop(number of connections) --> | ||
| <ng-container matColumnDef="workshopId"> | ||
| <th mat-header-cell *matHeaderCellDef mat-sort-header> | ||
| {{ 'FORMS.LABELS.USED_IN' | translate }} | ||
| </th> | ||
| <td mat-cell *matCellDef="let element">{{ '0' }}</td> | ||
| </ng-container> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded workshop count with actual data.
The table column for "Used In" shows a hardcoded value of "0" instead of displaying the actual number of workshops connected to each study subject.
- <td mat-cell *matCellDef="let element">{{ '0' }}</td>
+ <td mat-cell *matCellDef="let element">{{ element.workshopCount || '0' }}</td>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <!-- TODO: Study subject connection with workshop(number of connections) --> | |
| <ng-container matColumnDef="workshopId"> | |
| <th mat-header-cell *matHeaderCellDef mat-sort-header> | |
| {{ 'FORMS.LABELS.USED_IN' | translate }} | |
| </th> | |
| <td mat-cell *matCellDef="let element">{{ '0' }}</td> | |
| </ng-container> | |
| <!-- TODO: Study subject connection with workshop(number of connections) --> | |
| <ng-container matColumnDef="workshopId"> | |
| <th mat-header-cell *matHeaderCellDef mat-sort-header> | |
| {{ 'FORMS.LABELS.USED_IN' | translate }} | |
| </th> | |
| <td mat-cell *matCellDef="let element">{{ element.workshopCount || '0' }}</td> | |
| </ng-container> |
| <div class="reset"> | ||
| <mat-icon class="reset__icon">refresh</mat-icon> | ||
| </div> |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add click handler for the reset button.
The reset button has UI elements but lacks a click handler to actually reset the search filters.
🏁 Script executed:
#!/bin/bash
# Check if there's a reset function in the component
rg -A 3 "reset.*\(" src/app/shell/personal-cabinet/provider/provider-study-subjects/provider-study-subjects.component.tsLength of output: 122
Action Required: Attach Click Handler for Reset Button
It appears that the TS component file currently does not define any reset (or onReset) function corresponding to the reset button in the HTML. Please add a click binding in the template and implement the corresponding handler in the TypeScript file. For example:
-
In the Template (
provider-study-subjects.component.html):<div class="reset" (click)="onResetFilters()"> <mat-icon class="reset__icon">refresh</mat-icon> </div>
-
In the Component (
provider-study-subjects.component.ts):export class ProviderStudySubjectsComponent { // ... other component properties and methods public onResetFilters(): void { // TODO: Reset the search filters here, e.g.: // this.searchForm.reset(); or update component filtering logic appropriately } }
This will ensure that when the reset button is clicked, the corresponding handler is invoked to clear the search filters.
|
|
||
| describe('onCancel', () => { | ||
| it('should navigate to the study subjects page', () => { | ||
| component.onCancel(); | ||
| expect(mockRouter.navigate).toHaveBeenCalledWith(['/personal-cabinet/provider/study-subjects']); | ||
| }); | ||
| }); | ||
|
|
||
| describe('addNavPath', () => { | ||
| it('should dispatch AddNavPath with correct arguments', () => { | ||
| component.addNavPath(); | ||
| expect(mockStore.dispatch).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing test for setEditMode method.
The setEditMode method is not tested despite being a critical part of the component's functionality.
describe('setEditMode', () => {
it('should dispatch GetStudySubjectById and patch form values', () => {
// Mock route snapshot
mockRoute.snapshot.paramMap.get.mockReturnValue('123');
// Mock provider observable
const mockProvider = { id: 'provider1' };
mockStore.select.mockImplementation((selector) => {
if (selector === RegistrationState.provider) {
return of(mockProvider);
}
return of(null);
});
// Mock subject and languages
const mockSubject = {
id: '123',
nameInUkrainian: 'Test',
nameInInstructionLanguage: 'Test Eng',
language: { id: 1 }
};
const mockLanguages = [{ id: 1, name: 'English' }];
mockStore.select.mockImplementation((selector) => {
if (selector === ProviderState.selectedSubject) {
return of(mockSubject);
} else if (selector === ProviderState.languageList) {
return of(mockLanguages);
} else if (selector === RegistrationState.provider) {
return of(mockProvider);
}
return of(null);
});
component.setEditMode();
expect(mockStore.dispatch).toHaveBeenCalledWith(new GetStudySubjectById('123', 'provider1'));
// Form should be patched with subject values
expect(component.studySubjectFormGroup.value).toEqual({
nameInUkrainian: 'Test',
nameInInstructionLanguage: 'Test Eng',
language: { id: 1, name: 'English' }
});
});
});| beforeEach(() => { | ||
| fixture = TestBed.createComponent(CreateStudySubjectComponent); | ||
| component = fixture.componentInstance; | ||
|
|
||
| mockStore.select.mockReturnValue(of(null)); | ||
| mockStore.selectSnapshot.mockReturnValue(null); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Merge duplicate beforeEach hooks.
There are two separate beforeEach blocks that should be combined to avoid duplication and potential issues with test setup.
- beforeEach(() => {
- fixture = TestBed.createComponent(CreateStudySubjectComponent);
- component = fixture.componentInstance;
-
- mockStore.select.mockReturnValue(of(null));
- mockStore.selectSnapshot.mockReturnValue(null);
- });
beforeEach(async () => {
mockStore = {
dispatch: jest.fn(),
select: jest.fn().mockReturnValue(of(null)),
selectSnapshot: jest.fn().mockReturnValue(null)
} as any;
// Rest of the setup code...
await TestBed.configureTestingModule({
// Component configuration...
}).compileComponents();
+ fixture = TestBed.createComponent(CreateStudySubjectComponent);
+ component = fixture.componentInstance;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeEach(() => { | |
| fixture = TestBed.createComponent(CreateStudySubjectComponent); | |
| component = fixture.componentInstance; | |
| mockStore.select.mockReturnValue(of(null)); | |
| mockStore.selectSnapshot.mockReturnValue(null); | |
| }); | |
| beforeEach(async () => { | |
| mockStore = { | |
| dispatch: jest.fn(), | |
| select: jest.fn().mockReturnValue(of(null)), | |
| selectSnapshot: jest.fn().mockReturnValue(null) | |
| } as any; | |
| // Rest of the setup code... | |
| await TestBed.configureTestingModule({ | |
| // Component configuration... | |
| }).compileComponents(); | |
| fixture = TestBed.createComponent(CreateStudySubjectComponent); | |
| component = fixture.componentInstance; | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-73: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
| import { Component, OnDestroy, OnInit, ChangeDetectionStrategy, ChangeDetectorRef } from '@angular/core'; | ||
| import { FormBuilder, FormControl, FormGroup, ValidatorFn, Validators } from '@angular/forms'; | ||
| import { MatDialog } from '@angular/material/dialog'; | ||
| import { ActivatedRoute, Router } from '@angular/router'; | ||
| import { Select, Store } from '@ngxs/store'; | ||
| import { Observable, tap, combineLatest } from 'rxjs'; | ||
| import { filter, takeUntil } from 'rxjs/operators'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing import for finalize operator.
The finalize operator is used in the updated onSubmit method but isn't imported.
import { Component, OnDestroy, OnInit, ChangeDetectionStrategy, ChangeDetectorRef } from '@angular/core';
import { FormBuilder, FormControl, FormGroup, ValidatorFn, Validators } from '@angular/forms';
import { MatDialog } from '@angular/material/dialog';
import { ActivatedRoute, Router } from '@angular/router';
import { Select, Store } from '@ngxs/store';
import { Observable, tap, combineLatest } from 'rxjs';
-import { filter, takeUntil } from 'rxjs/operators';
+import { filter, takeUntil, finalize } from 'rxjs/operators';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Component, OnDestroy, OnInit, ChangeDetectionStrategy, ChangeDetectorRef } from '@angular/core'; | |
| import { FormBuilder, FormControl, FormGroup, ValidatorFn, Validators } from '@angular/forms'; | |
| import { MatDialog } from '@angular/material/dialog'; | |
| import { ActivatedRoute, Router } from '@angular/router'; | |
| import { Select, Store } from '@ngxs/store'; | |
| import { Observable, tap, combineLatest } from 'rxjs'; | |
| import { filter, takeUntil } from 'rxjs/operators'; | |
| import { Component, OnDestroy, OnInit, ChangeDetectionStrategy, ChangeDetectorRef } from '@angular/core'; | |
| import { FormBuilder, FormControl, FormGroup, ValidatorFn, Validators } from '@angular/forms'; | |
| import { MatDialog } from '@angular/material/dialog'; | |
| import { ActivatedRoute, Router } from '@angular/router'; | |
| import { Select, Store } from '@ngxs/store'; | |
| import { Observable, tap, combineLatest } from 'rxjs'; | |
| import { filter, takeUntil, finalize } from 'rxjs/operators'; |
-Added multiple new components for handling study subjects view and creation.
-Added new services for language list and study subjects:
-Created model files for study-subjects and language list
-Updated message-bar.ts, navigation-bar.ts, no-results.ts, and modal-confirmation.ts to add necessary enum values.
Summary by CodeRabbit
New Features
Bug Fixes
Tests