Development: Migrate client code for conversation detail tabs
#9973
Conversation
asliayk
added
tests
client
WalkthroughThe pull request introduces several modifications across multiple components in the Angular application. Key changes include renaming input properties to align with new naming conventions, transitioning from traditional decorators to function-based input and output handling, and adopting a reactive programming model using signals. The updates affect how properties are accessed and initialized, ensuring that components use function calls instead of direct property access. The overall structure and logic of the components remain intact, focusing on enhancing code maintainability and consistency. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (15)
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts (1)
125-129: Improve type safety in conversation manipulation.The current implementation uses type casting which could be made more type-safe using type guards.
runInInjectionContext(fixture.debugElement.injector, () => { const activeConversation = component.activeConversation(); - (activeConversation as ChannelDTO).isArchived = true; - component.activeConversation = input<ConversationDTO>(activeConversation as ConversationDTO); + if (isChannelDTO(activeConversation)) { + activeConversation.isArchived = true; + component.activeConversation = input<ConversationDTO>(activeConversation); + } });src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts (4)
44-47: Make injected services readonly.To prevent accidental reassignment of injected services, consider making them readonly.
- private modalService = inject(NgbModal); - private channelService = inject(ChannelService); - private groupChatService = inject(GroupChatService); - private alertService = inject(AlertService); + private readonly modalService = inject(NgbModal); + private readonly channelService = inject(ChannelService); + private readonly groupChatService = inject(GroupChatService); + private readonly alertService = inject(AlertService);
Line range hint
58-74: Improve error handling and null checks in conversation management.The current implementation has several potential issues:
- Unsafe null assertions
- No error handling for the group chat service call
- Duplicate subscription logic
leaveConversation($event: MouseEvent) { $event.stopPropagation(); - if (isGroupChatDTO(this.activeConversation()!)) { + const conversation = this.activeConversation(); + const courseId = this.course()?.id; + + if (!conversation || !courseId) { + this.alertService.error('artemisApp.conversation.errors.invalidState'); + return; + } + + if (isGroupChatDTO(conversation)) { this.groupChatService - .removeUsersFromGroupChat(this.course()!.id!, this.activeConversation()?.id!) - .pipe(takeUntil(this.ngUnsubscribe)) + .removeUsersFromGroupChat(courseId, conversation.id) + .pipe( + takeUntil(this.ngUnsubscribe), + catchError((error: HttpErrorResponse) => { + onError(this.alertService, error); + return EMPTY; + }) + ) .subscribe(() => { this.conversationLeave.emit(); }); return; - } else if (isChannelDTO(this.activeConversation()!)) { + } else if (isChannelDTO(conversation)) { this.channelService - .deregisterUsersFromChannel(this.course()!.id!, this.activeConversation()?.id!) - .pipe(takeUntil(this.ngUnsubscribe)) + .deregisterUsersFromChannel(courseId, conversation.id) + .pipe( + takeUntil(this.ngUnsubscribe), + catchError((error: HttpErrorResponse) => { + onError(this.alertService, error); + return EMPTY; + }) + ) .subscribe(() => { this.conversationLeave.emit(); }); return; } - throw new Error('The conversation type is not supported'); + this.alertService.error('artemisApp.conversation.errors.unsupportedType'); }
Line range hint
84-161: Extract duplicate modal logic into a reusable method.The modal setup logic is duplicated between
openArchivalModalandopenUnArchivalModal.+ private openConfirmationModal( + channel: ChannelDTO, + keys: { + titleKey: string; + questionKey: string; + descriptionKey: string; + confirmButtonKey: string; + }, + onConfirm: () => void + ) { + const translationParams = { channelName: channel.name }; + const modalRef: NgbModalRef = this.modalService.open( + GenericConfirmationDialogComponent, + defaultSecondLayerDialogOptions + ); + + modalRef.componentInstance.translationParameters = translationParams; + modalRef.componentInstance.translationKeys = keys; + modalRef.componentInstance.canBeUndone = true; + modalRef.componentInstance.initialize(); + + from(modalRef.result) + .pipe( + catchError(() => EMPTY), + takeUntil(this.ngUnsubscribe) + ) + .subscribe(() => onConfirm()); + } openArchivalModal(event: MouseEvent) { - const channel = getAsChannelDTO(this.activeConversation()!); + const channel = getAsChannelDTO(this.activeConversation()); if (!channel) { return; } + event.stopPropagation(); - const keys = { + this.openConfirmationModal( + channel, + { titleKey: 'artemisApp.dialogs.archiveChannel.title', questionKey: 'artemisApp.dialogs.archiveChannel.question', descriptionKey: 'artemisApp.dialogs.archiveChannel.description', confirmButtonKey: 'artemisApp.dialogs.archiveChannel.confirmButton', - }; - - const translationParams = { - channelName: channel.name, - }; - - event.stopPropagation(); - const modalRef: NgbModalRef = this.modalService.open(GenericConfirmationDialogComponent, defaultSecondLayerDialogOptions); - modalRef.componentInstance.translationParameters = translationParams; - modalRef.componentInstance.translationKeys = keys; - modalRef.componentInstance.canBeUndone = true; - modalRef.componentInstance.initialize(); - - from(modalRef.result) - .pipe( - catchError(() => EMPTY), - takeUntil(this.ngUnsubscribe), - ) - .subscribe(() => { + }, + () => { this.channelService.archive(this.course()?.id!, channel.id!).subscribe({ next: () => { this.channelArchivalChange.emit(); }, error: (errorResponse: HttpErrorResponse) => onError(this.alertService, errorResponse), }); - }); + } + ); }
Line range hint
165-179: Improve error handling in channel deletion.The current implementation could benefit from more specific error handling and user feedback.
deleteChannel() { - const channel = getAsChannelDTO(this.activeConversation()!); + const channel = getAsChannelDTO(this.activeConversation()); + const courseId = this.course()?.id; + - if (!channel) { + if (!channel || !courseId) { + this.alertService.error('artemisApp.channel.errors.invalidState'); return; } + this.channelService - .delete(this.course()?.id!, channel.id!) - .pipe(takeUntil(this.ngUnsubscribe)) + .delete(courseId, channel.id) + .pipe( + takeUntil(this.ngUnsubscribe), + catchError((error: HttpErrorResponse) => { + const errorMessage = error.status === 403 + ? 'artemisApp.channel.errors.notAuthorized' + : 'artemisApp.channel.errors.deleteFailed'; + this.dialogErrorSource.next(errorMessage); + return EMPTY; + }) + ) .subscribe({ next: () => { this.dialogErrorSource.next(''); this.channelDeleted.emit(); - }, - error: (errorResponse: HttpErrorResponse) => this.dialogErrorSource.next(errorResponse.message), + } }); }src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts (2)
33-34: Consider renamingactiveConversationInputfor consistency.The input property
activeConversationInputcould be renamed toactiveConversationto maintain consistency and improve readability, as theinputfunction already indicates that it's an input property.Apply this diff to rename the input property:
course = input<Course>(); -activeConversationInput = input<ConversationDTO>(); +activeConversation = input<ConversationDTO>(); activeConversation = signal<ConversationDTO | null>(null); // In ngOnInit -const inputValue = this.activeConversationInput(); +const inputValue = this.activeConversation();
62-62: Remove redundantpublicmodifier.In TypeScript classes, properties are public by default. The
publickeyword onconversationServiceis redundant and can be omitted for brevity.Apply this diff to remove the redundant
publickeyword:-public conversationService = inject(ConversationService); +conversationService = inject(ConversationService);src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html (1)
65-65: Consider standardizing input property names for consistency.The input property for
jhi-conversation-membershas been renamed toactiveConversationInput. To maintain consistency across components, consider usingactiveConversationas the input property name unless there is a specific reason for the distinction.Apply this diff to rename the input property:
-<jhi-conversation-members [course]="course" [activeConversationInput]="activeConversation" (changesPerformed)="changesWerePerformed = true" /> +<jhi-conversation-members [course]="course" [activeConversation]="activeConversation" (changesPerformed)="changesWerePerformed = true" />Ensure that the
ConversationMembersComponentis updated accordingly to acceptactiveConversationas the input.src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.html (1)
58-59: Consider performance optimization for frequent function calls.The template bindings
activeConversation()andcourse()will be called during every change detection cycle. Consider caching these values in local variables if they don't change frequently within the component.- [activeConversation]="activeConversation()!" - [course]="course()" + [activeConversation]="cachedActiveConversation" + [course]="cachedCourse"Add to the component class:
private cachedActiveConversation = computed(() => this.activeConversation()); private cachedCourse = computed(() => this.course());src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.html (2)
85-93: Simplify nested conditionals and null checks.The current implementation has multiple nested null checks and function calls which could be simplified.
- @if (activeConversation()?.creator) { - <li> - {{ 'artemisApp.dialogs.conversationDetail.infoTab.createdBy' | artemisTranslate }}: - {{ getCreator() ? getUserLabel(getCreator()!) : '' }} - </li> - } + @if (getCreator(); as creator) { + <li> + {{ 'artemisApp.dialogs.conversationDetail.infoTab.createdBy' | artemisTranslate }}: + {{ getUserLabel(creator) }} + </li> + }
93-93: Simplify date display logic.The date display logic could be simplified by using the non-null assertion more effectively.
- {{ activeConversation()!.creationDate ? (activeConversation()!.creationDate | artemisDate) : '' }} + {{ activeConversation()!.creationDate | artemisDate }}src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (1)
Line range hint
109-127: Enhance test specificity per coding guidelines.The test expectations could be more specific according to the coding guidelines.
- expect(component.changesWerePerformed).toBeFalse(); + expect(component.changesWerePerformed).toBeExactlyFalse(); - expect(closeSpy).toHaveBeenCalledOnce(); + expect(closeSpy).toHaveBeenCalledExactlyOnceWith();src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (2)
Line range hint
230-236: Consider using a type discriminator for conversation types.The conversation type checking could be improved using a type discriminator pattern.
Consider this pattern:
type ConversationType = 'channel' | 'groupChat'; interface BaseConversation { type: ConversationType; } // Then use type predicates function isChannel(conversation: BaseConversation): conversation is ChannelDTO { return conversation.type === 'channel'; }
Line range hint
242-251: Consider using an enum for user roles.The user authority logic could be more maintainable using an enum for roles.
Consider this pattern:
enum UserRole { INSTRUCTOR = 'instructor', TUTOR = 'tutor', STUDENT = 'student' } interface UserAuthority { role: UserRole; icon: IconProp; tooltipKey: string; }src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts (1)
162-166: Consider extracting test setup logic.The test setup logic for modifying component properties could be extracted into a helper method for better reusability.
Consider this pattern:
function updateComponentMember(fixture: ComponentFixture<ConversationMemberRowComponent>, updates: Partial<ConversationUserDTO>) { runInInjectionContext(fixture.debugElement.injector, () => { const member = { ...fixture.componentInstance.conversationMember(), ...updates }; fixture.componentInstance.conversationMember = input<ConversationUserDTO>(member); }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.html(4 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts(9 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html(2 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts(11 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.html(3 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts(7 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts(8 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.spec.ts(2 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts(3 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.spec.ts(4 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts (1)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts (1)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts (1)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (1)
🪛 GitHub Check: client-style
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts
[warning] 176-176:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 194-194:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
[warning] 126-126:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 126-126:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 146-146:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 146-146:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 177-177:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 177-177:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 177-177:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 196-196:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
🔇 Additional comments (15)
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts (1)
Line range hint 55-174: Test suite structure and coverage look good!
The test suite:
- Properly tests permissions and conditional rendering
- Correctly mocks services and handles async operations
- Uses specific expectations and proper test isolation
- Follows jest and NgMocks best practices
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts (1)
1-1: LGTM! Component follows Angular style guidelines.
The migration to the new input/output syntax is implemented correctly.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts (2)
31-34: Consistent use of input and signal functions.
Great job transitioning to the new input and signal APIs for defining inputs and reactive properties. This enhances readability and aligns with Angular's recommendations.
182-190: Avoid mutating state directly within signals.
While updating the activeConversation signal, ensure that you are not mutating the existing state. Returning a new object is good practice. Verify that current is not mutated elsewhere in the code.
Run the following script to check for any direct mutations of current:
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html (2)
1-1: Proper use of the new @if syntax with function calls.
Good job updating the conditional to use @if (activeConversation() && course()), aligning with the new Angular syntax and reactive programming model.
18-18: Consistent null checking with optional chaining.
Using conversationMember()?.isStudent ensures that you safely access isStudent even if conversationMember() returns null or undefined. Good practice!
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.html (1)
1-1: Updated conditional check with function calls.
The conditional now correctly uses @if (activeConversation() && course()), reflecting the reactive approach and aligning with the new Angular syntax.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.html (1)
1-1: LGTM! Proper usage of new Angular control flow syntax.
The conversion from *ngIf to @if follows the new Angular syntax guidelines.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/conversation-detail-dialog.component.spec.ts (1)
28-28: LGTM! Property rename aligns with component changes.
The input property rename from activeConversation to activeConversationInput is consistent with the component changes.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.spec.ts (1)
78-82: LGTM: Proper initialization of signals and inputs
The component initialization correctly uses TestBed.runInInjectionContext to set up the signals and inputs, following Angular's latest practices.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts (2)
47-49: LGTM: Clean implementation of inputs and outputs
The component properly implements required and optional inputs using the new Angular syntax.
51-54: LGTM: Modern dependency injection
Clean implementation of dependency injection using the new inject function.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.spec.ts (1)
52-56: LGTM: Proper test setup with new input system
The component initialization correctly uses TestBed.runInInjectionContext to set up the inputs, following Angular's latest practices.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts (1)
33-36: LGTM: Input/Output declarations follow the new Angular syntax.
The component correctly uses the new input/output syntax with signals, which is the recommended approach in modern Angular.
src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.spec.ts (1)
89-94: LGTM: Proper setup of input signals in tests.
The test correctly uses runInInjectionContext to set up input signals, following Angular's best practices.
...onversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts
Outdated
Show resolved
Hide resolved
...ogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts
Show resolved
Hide resolved
...ogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts
Outdated
Show resolved
Hide resolved
...alogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts
Outdated
Show resolved
Hide resolved
...log/tabs/conversation-members/conversation-member-row/conversation-member-row.component.html
Show resolved
Hide resolved
.../conversation-detail-dialog/tabs/conversation-members/conversation-members.component.spec.ts
Show resolved
Hide resolved
...ons/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts
Outdated
Show resolved
Hide resolved
...ialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
Outdated
Show resolved
Hide resolved
...ialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
Show resolved
Hide resolved
…igration' into chore/conversation-detail-tabs-migration
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts (1)
43-45: Consider using type guard instead of type castingThe type casting in
getCreatormethod could be made safer by using a type guard function.-return this.activeConversation()?.creator as ConversationUserDTO | null; +return this.activeConversation()?.creator && isConversationUserDTO(this.activeConversation()?.creator) + ? this.activeConversation()?.creator + : null;src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts (1)
Line range hint
62-174: Add comprehensive null checking for service callsMultiple service calls use unsafe non-null assertions with optional chaining, which could lead to runtime errors.
Apply this pattern across all service methods:
leaveConversation($event: MouseEvent) { $event.stopPropagation(); - if (isGroupChatDTO(this.activeConversation()!)) { + const conversation = this.activeConversation(); + const courseId = this.course()?.id; + if (!conversation || !courseId) { + return; + } + if (isGroupChatDTO(conversation)) { this.groupChatService - .removeUsersFromGroupChat(this.course()!.id!, this.activeConversation()?.id!) + .removeUsersFromGroupChat(courseId, conversation.id) .pipe(takeUntil(this.ngUnsubscribe)) .subscribe(() => { this.conversationLeave.emit(); }); return; }Similar pattern should be applied to:
- openArchivalModal
- openUnArchivalModal
- deleteChannel
🧰 Tools
🪛 GitHub Check: client-style
[warning] 64-64:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 72-72:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts(9 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts(11 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts(7 hunks)src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts(8 hunks)src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/javascript/spec/component/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.spec.ts
- src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-member-row/conversation-member-row.component.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts (1)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts (1)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts (1)
🪛 GitHub Check: client-style
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts
[warning] 118-118:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 119-119:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts
[warning] 64-64:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 72-72:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 117-117:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 157-157:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 174-174:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
🔇 Additional comments (14)
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-info/conversation-info.component.ts (6)
1-1: LGTM! Imports are properly updated for signals migration
The imports have been correctly updated to include the necessary Angular features (inject, input, output) for the migration to signals, and the ConversationUserDTO model has been properly imported.
Also applies to: 22-22
47-49: LGTM! Properties correctly migrated to new input/output API
The migration from decorators to the new input/output API is correctly implemented, maintaining the required/optional status of inputs.
51-54: LGTM! Services properly injected using modern DI pattern
The migration from constructor injection to the inject() function follows Angular's modern dependency injection patterns.
172-175: LGTM! Proper null checks added for courseId
The addition of null checks for courseId improves safety by preventing undefined behavior.
Also applies to: 195-198
220-221: LGTM! Type references properly added for template type checking
The addition of protected readonly type references enables proper type checking in templates when using strict mode.
180-180:
Address remaining non-null assertions
There are still unsafe non-null assertions on channel.id! and groupChat.id! that should be handled similarly to the courseId check.
Apply this pattern to both update methods:
private updateChannel<K extends keyof ChannelDTO>(channel: ChannelDTO, propertyName: K, updateValue: ChannelDTO[K]) {
const courseId = this.course()?.id;
if (!courseId) {
return;
}
+ if (!channel.id) {
+ this.alertService.error('artemisApp.conversation.error.channelNotFound');
+ return;
+ }
const updateDTO = new ChannelDTO();
updateDTO[propertyName] = updateValue;
- this.channelService.update(courseId, channel.id!, updateDTO)
+ this.channelService.update(courseId, channel.id, updateDTO)Also applies to: 202-202
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-settings/conversation-settings.component.ts (4)
1-1: LGTM: Imports updated correctly for new Angular syntax
The imports have been properly updated to include the new input/output and inject functions from @angular/core.
27-32: Consider implementing cleanup for outputs
While the migration to function-based inputs/outputs is correct, ensure proper cleanup of output subscriptions to prevent memory leaks.
44-47: LGTM: Clean dependency injection implementation
The migration to the new inject() syntax is well-implemented, making the code more maintainable and cleaner than constructor-based DI.
50-57: 🛠️ Refactor suggestion
Replace non-null assertion with safe navigation
While the early return for conversation is good, there's still an unsafe non-null assertion when checking canDeleteChannel.
- this.canDeleteChannel = this.conversationAsChannel ? canDeleteChannel(this.course()!, this.conversationAsChannel) : false;
+ const course = this.course();
+ this.canDeleteChannel = course && this.conversationAsChannel ? canDeleteChannel(course, this.conversationAsChannel) : false;Likely invalid or redundant comment.
src/main/webapp/app/overview/course-conversations/dialogs/conversation-detail-dialog/tabs/conversation-members/conversation-members.component.ts (4)
62-65: LGTM!
The usage of the inject function for dependency injection aligns with the updated Angular patterns and enhances code readability.
146-149: LGTM!
Initializing the activeConversation signal with the input value ensures reactive updates and state consistency.
186-194: LGTM!
Updating the activeConversation signal to reflect the new numberOfMembers maintains consistency across the component when members change.
118-119:
Avoid using non-null assertions after optional chaining
Using non-null assertions ! immediately after optional chaining ?. can lead to runtime errors if the value is null or undefined. Since the if condition already ensures that this.course()?.id and this.activeConversation()?.id are truthy, you can safely access id without optional chaining or non-null assertions.
Apply this diff to correct the issue:
return this.conversationService.searchMembersOfConversation(
- this.course()?.id!,
- this.activeConversation()?.id!,
+ this.course().id,
+ this.activeConversation().id,
this.searchTerm,
this.page - 1,
this.itemsPerPage,
Number(this.selectedFilter),
);🧰 Tools
🪛 GitHub Check: client-style
[warning] 118-118:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
[warning] 119-119:
Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS2. I tested the followings:
✅ Edit the name of a channel
✅ Edit the topic of a channel
✅ Edit the description of a channel (although I'm not really sure how the change is reflected)
✅ Searching for members
✅ Archiving channels
Everything works fine. One thing I noticed was the change for the name of the channel was only updated after I close the edit popup, even after clicking on save, the change is not reflected imediately.
After closing the pop up for editing, the change is updated
asliayk
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
For the client migration, following files need to be updated:
Description
ConversationSettingsComponent,ConversationMembersComponent,ConversationMemberRowComponent, andConversationInfoComponenthave been updated to use inject and signals.Known Issue: These components have been migrated to use inject and signals, but they haven't been converted to standalone yet, as it would impact other components. This will be addressed in a follow-up PR.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
New Features
Bug Fixes
Tests