Development: Combine post and answer post reactions bars in communication mode client code
#10224
Conversation
asliayk
added
tests
client
…omponents # Conflicts: # src/main/webapp/app/shared/metis/metis.module.ts
asliayk
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request involves a comprehensive refactoring of the reactions bar components in the Metis communication system. The changes primarily focus on consolidating multiple reaction bar components into a single, more generic Changes
Possibly related PRs
Suggested Labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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: 2
🔭 Outside diff range comments (3)
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (3)
Line range hint
1-400: Add tests for reactions bar integration.Given that this PR merges the reactions bar components, we should add tests to verify the integration with the new
PostingReactionsBarComponent.Consider adding these test cases:
it('should render PostingReactionsBarComponent', () => { component.posting = metisPostExerciseUser1; fixture.detectChanges(); const reactionsBar = getElement(debugElement, 'jhi-posting-reactions-bar'); expect(reactionsBar).not.toBeNull(); }); it('should pass correct props to PostingReactionsBarComponent', () => { component.posting = metisPostExerciseUser1; fixture.detectChanges(); const reactionsBar = debugElement.query(By.css('jhi-posting-reactions-bar')); expect(reactionsBar.properties['posting']).toBe(metisPostExerciseUser1); });
Line range hint
80-93: Add PostingReactionsBarComponent to test declarations.The component declarations array should include the mock for
PostingReactionsBarComponent.Add the following to the declarations array:
declarations: [ PostComponent, FaIconComponent, MockPipe(HtmlForMarkdownPipe), MockComponent(PostingHeaderComponent), MockComponent(PostingContentComponent), MockComponent(PostingFooterComponent), + MockComponent(PostingReactionsBarComponent), MockComponent(AnswerPostCreateEditModalComponent), MockRouterLinkDirective, MockQueryParamsDirective, TranslatePipeMock, ArtemisDatePipe, ArtemisTranslatePipe, MockDirective(TranslateDirective), ],
Line range hint
1-400: Follow jest expectation patterns from coding guidelines.Several test expectations don't follow the coding guidelines for specific types:
- Boolean expectations should use
toBeTrue()/toBeFalse()- Reference comparisons should use
toBe()- Existence checks should use
toBeNull()/toBeNotNull()Update these test cases to follow the guidelines:
-expect(contextLink).toBeNull(); +expect(contextLink).toBeNull(); // Already follows guidelines -expect(header).not.toBeNull(); +expect(header).toBeNotNull(); -expect(component.isPinned()).toBe(true); +expect(component.isPinned()).toBeTrue(); -expect(component.isPinned()).toBe(false); +expect(component.isPinned()).toBeFalse();
🧹 Nitpick comments (6)
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts (3)
1-1: Consider using separate imports for decorators like@Input,@Output, and@ViewChild.
While using theinput(),output(), andviewChild()signal-based approach is valid in Angular 16+, splitting out these decorators from the main Angular import can aid in clarity. For example:import { Component, OnChanges, OnInit, inject } from '@angular/core'; import { Input, Output, ViewChild } from '@angular/core';
223-223: Use@ts-expect-errorinstead of@ts-ignore.
Angular style guidelines discourage@ts-ignore; prefer@ts-expect-error, which will fail if the error is resolved.- // @ts-ignore + // @ts-expect-error🧰 Tools
🪛 GitHub Check: client-style
[warning] 223-223:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
303-305: Consider skipping the external click handling if the emoji selector is not visible.
When the user toggles the emoji selector rapidly, multiple reflows could occur. A defensive check could improve responsiveness.src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.html (3)
5-8: Add aria-label to improve accessibility.The reply button should have an aria-label for better accessibility.
<button class="reaction-button clickable reply-btn" + aria-label="{{ 'artemisApp.conversationsLayout.threadSideBar.reply' | artemisTranslate }}" (click)="isCommunicationPage() ? openThread.emit() : openAnswerView()">
74-80: Add aria-label to emoji picker button.The emoji picker button should have an aria-label for screen readers.
<button class="reaction-button clickable px-2 fs-small" + aria-label="{{ 'artemisApp.metis.chooseReaction' | artemisTranslate }}" (click)="showReactionSelector = !showReactionSelector" cdkOverlayOrigin #trigger="cdkOverlayOrigin">
149-158: Consider extracting pin button conditions to a method.The pin button has complex conditions that could be moved to a component method for better maintainability.
// In component class: isPinButtonEnabled(): boolean { return this.canPin && !this.isReadOnlyMode(); } isPinButtonActive(): boolean { return this.displayPriority === DisplayPriority.PINNED && this.canPin; }<button class="reaction-button pin clickable fs-small" [class.reaction-button--not-hoverable]="!canPin" - [class.reaction-button--reacted]="displayPriority === DisplayPriority.PINNED && canPin" + [class.reaction-button--reacted]="isPinButtonActive()" [disabled]="!canPin || isReadOnlyMode()" - (click)="canPin && togglePin()" + (click)="isPinButtonEnabled() && togglePin()" >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/main/webapp/app/overview/course-conversations/other/channel-icon/channel-icon.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/other/channel-icon/channel-icon.component.ts(2 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(2 hunks)src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts(3 hunks)src/main/webapp/app/shared/metis/post/post.component.html(2 hunks)src/main/webapp/app/shared/metis/post/post.component.ts(3 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html(0 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts(0 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html(0 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts(0 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts(1 hunks)src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.directive.ts(0 hunks)src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts(0 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts(0 hunks)src/test/javascript/spec/component/shared/metis/postings-reactions-bar/posting-reactions-bar.component.spec.ts(19 hunks)
💤 Files with no reviewable changes (7)
- src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.html
- src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.html
- src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
- src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts
- src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts
- src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts
- src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.directive.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
🧰 Additional context used
📓 Path-based instructions (10)
src/main/webapp/app/shared/metis/post/post.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/shared/metis/posting-reactions-bar/posting-reactions-bar.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/shared/metis/post/post.component.ts (1)
src/main/webapp/app/overview/course-conversations/other/channel-icon/channel-icon.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/shared/metis/answer-post/answer-post.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/shared/metis/answer-post/answer-post.component.ts (1)
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts (1)
src/test/javascript/spec/component/shared/metis/post/post.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/other/channel-icon/channel-icon.component.ts (1)
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/posting-reactions-bar.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}}
🪛 Biome (1.9.4)
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts
[error] 373-373: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
src/test/javascript/spec/component/shared/metis/postings-reactions-bar/posting-reactions-bar.component.spec.ts
[error] 361-361: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
🪛 GitHub Check: client-style
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts
[warning] 223-223:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (17)
src/main/webapp/app/overview/course-conversations/other/channel-icon/channel-icon.component.ts (3)
Line range hint
1-9: LGTM! Clean component setup with proper imports.The component follows Angular best practices with standalone imports and proper component configuration.
11-13: Well-structured input properties using signals!The migration from @input decorators to signal-based inputs is well implemented, maintaining proper typing and sensible default values.
16-19: Good use of readonly for immutable icon properties!The readonly modifier ensures icons cannot be accidentally modified after initialization.
src/main/webapp/app/overview/course-conversations/other/channel-icon/channel-icon.component.html (1)
1-14: Clean template with proper signal usage and modern Angular syntax!The template correctly:
- Uses the new @if syntax instead of *ngIf as per guidelines
- Properly accesses signal inputs using method call syntax
- Maintains consistent icon sizing
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (2)
7-8: LGTM! Import paths have been simplified.The import paths have been updated to use absolute paths instead of relative paths, which improves maintainability and readability.
Also applies to: 40-42
46-49: Verify AccountService and LocalStorageService mocking.The new services have been added with their mock implementations. However, let's verify their usage in the test suite.
Run this script to check if these services are actually used in the tests:
Also applies to: 77-78
✅ Verification successful
Services are correctly mocked for dependency injection and inheritance
The AccountService and LocalStorageService are correctly mocked as they are required by:
- The parent PostingDirective class that this component extends
- The dependency injection system for authorization and storage management
- The MetisService which this component uses for all authorization and state operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AccountService and LocalStorageService usage in the test file rg -A 5 "AccountService|LocalStorageService" "src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts" # Check if there are any assertions or spy setups for these services ast-grep --pattern 'expect($_).toHaveBeenCalled$$$' "src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts"Length of output: 1125
Script:
#!/bin/bash # Check the component implementation for service usage rg -A 5 "AccountService|LocalStorageService" "src/app/shared/metis/post/post.component.ts" # Look for any test cases that might be testing these services rg -B 2 -A 5 "describe|it\(" "src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts"Length of output: 9324
Script:
#!/bin/bash # Find the component file fd -e ts "post.component.ts" --exclude "*.spec.ts" # Search for service usage in related files rg -l "AccountService|LocalStorageService" --type ts --glob "!*.spec.ts" "src/"Length of output: 9849
Script:
#!/bin/bash # Check the actual component implementation cat "src/main/webapp/app/shared/metis/post/post.component.ts" # Check the component template if it exists cat "src/main/webapp/app/shared/metis/post/post.component.html"Length of output: 22461
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts (1)
91-122: Good use of Angular signals for binding inputs and outputs.
The approach of defining inputs/outputs as signals (e.g.input<T>,output<void>) follows modern Angular best practices and maintains consistent readability.src/test/javascript/spec/component/shared/metis/postings-reactions-bar/posting-reactions-bar.component.spec.ts (3)
393-393: Verify correct reaction creation.
The test logic for building a Reaction object is correct. It ensures the method constructs the object with the rightemojiId,post, oranswerPost.
546-555: Properly differentiates between anAnswerPostvs. aPost.
The tested logic cleanly validates that when thepostingis anAnswerPost, the reaction is attached accordingly, and vice versa.
568-577: Good check for insufficient permissions during pin toggling.
The test ensures that a user without the proper rights cannot pin or unpin. This aligns with the app's expected authorization flow.src/main/webapp/app/shared/metis/answer-post/answer-post.component.ts (3)
35-35: Successful import alignment with new component.
Replacing the oldAnswerPostReactionsBarComponentwithPostingReactionsBarComponentensures consistent usage of the unified reactions bar.
55-55: Compositional approach for Angular imports.
Using the newPostingReactionsBarComponentwithinimportsis coherent with Angular’s module-less style approach for standalone components.
80-80: Visibility changed fromprivatetoprotected.
This change allows derived classes to accessreactionsBarComponentif needed in an extended scenario. Confirm it aligns with your inheritance design.src/main/webapp/app/shared/metis/post/post.component.ts (1)
32-32: LGTM! Component consolidation changes look good.The import statement and ViewChild decorator have been correctly updated to use the new PostingReactionsBarComponent with proper generic type.
Also applies to: 97-97
src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts (2)
29-34: LGTM! Service dependencies properly configured.The addition of LocalStorageService and AccountService with their mock implementations improves the test setup.
124-125: LGTM! Component selector updated correctly.The test now correctly queries for 'jhi-posting-reactions-bar' instead of 'jhi-answer-post-reactions-bar', aligning with the component consolidation.
src/main/webapp/app/shared/metis/post/post.component.html (1)
95-97: LGTM! Component usage updated correctly.The changes correctly update both instances of the reactions bar:
- Component selector changed to posting-reactions-bar
- Input property renamed to isReadOnlyMode following boolean naming convention
Also applies to: 129-131
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.ts
Show resolved
Hide resolved
...t/spec/component/shared/metis/postings-reactions-bar/posting-reactions-bar.component.spec.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-reactions-bar/posting-reactions-bar.component.html
Outdated
Show resolved
Hide resolved
asliayk
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
Cathy0123456789
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
Cathy0123456789
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Communication: Combine Post and AnswerPost Reactions BarsDevelopment: Combine post and answer post reactions bars in communication mode client code
krusche
deleted the
chore/communication/combine-reactionbar-components
branch
Checklist
General
Client
Motivation and Context
PostReactionsBarComponentandAnswerPostReactionsBarComponentleads to duplicated logic, making updates and consistency harder to manage. Merging them into a singlePostingReactionsBarComponentimproves maintainability, ensures consistency, and simplifies future modifications.Description
PostReactionsBarComponentandAnswerPostReactionsBarComponentare merged into a singlePostingReactionsBarComponentto eliminate duplication and improve maintainability.PostingReactionsBarComponentis migrated to the client architecture using signals.ChannelIconComponentis also migrated to use signals as part of the client update.PostingReactionsBarComponentto ensure functionality remains intact after migration & combining components.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
Client
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
New Features
PostingReactionsBarComponentfor managing post reactions.Refactoring
input()function.Breaking Changes
PostReactionsBarComponentandAnswerPostReactionsBarComponent.@Input()decorators toinput()function.jhi-post-reactions-bartojhi-posting-reactions-bar.Improvements