Development: Migrate client code for posting header and footer components
#9932
Conversation
github-actions
bot
added
tests
client
WalkthroughThe changes in this pull request involve significant renaming and restructuring of components within the Metis application. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.component.html (2)
3-14: Consider optimizing profile picture bindings with a local variable.While the implementation is correct, the multiple
posting()function calls could be optimized by storing the author in a local variable.- @if (posting()?.author) { + @if (posting()?.author; as author) { <span class="d-inline-flex align-items-start gap-2 flex-wrap"> <jhi-profile-picture imageSizeInRem="2.15" fontSizeInRem="0.9" imageId="post-profile-picture" defaultPictureId="post-default-profile-picture" [isGray]="isDeleted()" - [authorId]="posting()?.author?.id" - [authorName]="posting()?.author?.name" - [imageUrl]="posting()?.author?.imageUrl" - [isEditable]="currentUser !== undefined && posting()?.author?.id === currentUser.id" + [authorId]="author.id" + [authorName]="author.name" + [imageUrl]="author.imageUrl" + [isEditable]="currentUser !== undefined && author.id === currentUser.id" >
33-34: Consider extracting creation date to improve readability.While the implementation is correct, readability could be improved by storing the creation date in a local variable.
+ @if (posting()?.creationDate; as creationDate) { <span class="fs-small" [disableTooltip]="postingIsOfToday" - ngbTooltip="{{ posting()?.creationDate | artemisDate: 'time' }}"> + ngbTooltip="{{ creationDate | artemisDate: 'time' }}"> - {{ postingIsOfToday ? (posting()?.creationDate | artemisDate: 'time') : (posting()?.creationDate | artemisDate: 'short-date') }} + {{ postingIsOfToday ? (creationDate | artemisDate: 'time') : (creationDate | artemisDate: 'short-date') }} </span> + }src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (1)
89-94: Avoid mutating input properties directlyThe method should avoid mutating properties of
sortedAnswerPosts()directly, as it may lead to unexpected side effects. Instead, create a copy before performing operations.Apply this diff to prevent direct mutation:
groupAnswerPosts(): void { if (!this.sortedAnswerPosts() || this.sortedAnswerPosts().length === 0) { this.groupedAnswerPosts = []; return; } - const sortedAnswerPosts = this.sortedAnswerPosts() + const sortedAnswerPosts = [...this.sortedAnswerPosts()] .reverse() .map((post) => { (post as any).creationDateDayjs = post.creationDate ? dayjs(post.creationDate) : undefined; return post; });src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.html (1)
Line range hint
1-28: Methods used in template may cause performance issuesSeveral properties in the template are now invoked as functions (e.g.,
showAnswers(),lastReadDate()). Calling methods or getters in the template can lead to performance degradation due to frequent change detection. Consider using variables instead.Apply this diff to store function results in variables:
export class PostFooterComponent implements OnInit, OnDestroy, AfterContentChecked, OnChanges { + showAnswersValue = this.showAnswers(); + lastReadDateValue = this.lastReadDate(); + readOnlyModeValue = this.readOnlyMode(); + isCommunicationPageValue = this.isCommunicationPage(); + isThreadSidebarValue = this.isThreadSidebar(); + hasChannelModerationRightsValue = this.hasChannelModerationRights(); // ... } // In the template, use the variables instead of function calls @if (showAnswersValue) { <!-- ... --> <jhi-answer-post - [lastReadDate]="lastReadDate()" - [readOnlyMode]="readOnlyMode()" <!-- ... --> - [isCommunicationPage]="isCommunicationPage()" - [isThreadSidebar]="isThreadSidebar()" <!-- ... --> - [hasChannelModerationRights]="hasChannelModerationRights()" /> }src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (2)
8-8: Consider using path aliases instead of relative pathsThe import path with multiple parent directory references (
../../../../../../../) is fragile and hard to maintain. Consider using TypeScript path aliases to make imports more readable and maintainable.Example configuration in
tsconfig.json:{ "compilerOptions": { "paths": { "@shared/*": ["src/main/webapp/app/shared/*"] } } }Then the import could be simplified to:
import { PostingHeaderComponent } from '@shared/metis/posting-header/post-header/posting-header.component';
Line range hint
84-359: Consider organizing related test cases using describe blocksWhile the test cases are comprehensive and follow best practices, the readability could be improved by grouping related tests. Consider organizing the tests into logical describe blocks.
Example organization:
describe('PostComponent', () => { describe('Answer Management', () => { it('should sort answers', () => {/*...*/}); it('should not sort empty array of answers', () => {/*...*/}); }); describe('Navigation', () => { it('should set router link and query params', () => {/*...*/}); it('should navigate to channel when not on messaging page', () => {/*...*/}); // ... other navigation tests }); describe('User Interactions', () => { it('should open create answer post modal', () => {/*...*/}); it('should close create answer post modal', () => {/*...*/}); // ... other interaction tests }); // ... other test groups });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(1 hunks)src/main/webapp/app/shared/metis/metis.module.ts(3 hunks)src/main/webapp/app/shared/metis/post/post.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.html(3 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts(4 hunks)src/main/webapp/app/shared/metis/posting-footer/posting-footer.directive.ts(0 hunks)src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html(0 hunks)src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts(0 hunks)src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts(0 hunks)src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.component.html(2 hunks)src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.component.ts(5 hunks)src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts(5 hunks)src/test/javascript/spec/component/shared/metis/postings-header/answer-post-header/answer-post-header.component.spec.ts(0 hunks)src/test/javascript/spec/component/shared/metis/postings-header/post-header/post-header.component.spec.ts(0 hunks)src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (6)
- src/main/webapp/app/shared/metis/posting-footer/posting-footer.directive.ts
- src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html
- src/test/javascript/spec/component/shared/metis/postings-header/post-header/post-header.component.spec.ts
- src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.ts
- src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.ts
- src/test/javascript/spec/component/shared/metis/postings-header/answer-post-header/answer-post-header.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (10)
src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.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/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/shared/metis/metis.module.ts (1)
src/test/javascript/spec/component/shared/metis/postings-header/posting-header.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.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.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/postings-footer/post-footer/post-footer.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/posting-footer/post-footer/post-footer.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-header/post-header/posting-header.component.ts (1)
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (1)
📓 Learnings (1)
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
Learnt from: rabeatwork
PR: ls1intum/Artemis#9103
File: src/test/javascript/spec/component/shared/metis/postings-header/answer-post-header/answer-post-header.component.spec.ts:94-94
Timestamp: 2024-11-12T12:52:03.805Z
Learning: The `#today-flag` ID is only present in the test files and not in the actual component's HTML or TypeScript files.
🪛 Biome (1.9.4)
src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts
[error] 53-58: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🪛 GitHub Check: client-tests-selected
src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts
[failure] 150-150:
Property 'answers' does not exist on type 'InputSignal<Posting | undefined>'.
🪛 GitHub Check: client-tests
src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts
[failure] 150-150:
Property 'answers' does not exist on type 'InputSignal<Posting | undefined>'.
🔇 Additional comments (18)
src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.component.html (2)
Line range hint 18-24: LGTM! Well-structured role badge implementation.
The implementation correctly handles translations, icons, and null safety.
37-43: LGTM! Clean implementation of resolved post indicator.
The code correctly uses the new @if syntax and properly implements the icon with tooltip.
src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.component.ts (5)
1-31: Use of Signals and Dependency Injection aligns with Angular 16 standards
The implementation properly utilizes Angular 16 features such as input, output, and inject functions for signals and dependency injection. The input properties are correctly declared using input<T>(), and services are injected using inject(), promoting a modern and clean codebase.
70-73: Type guard isPost is correctly implemented
The isPost function accurately acts as a type guard to differentiate between Post and AnswerPost instances, ensuring type safety when accessing properties specific to Post.
74-77: Getter isPostResolved appropriately uses the type guard
The isPostResolved getter method effectively utilizes the isPost type guard to safely check the resolved property of a Post instance.
79-85: Lifecycle hook ngOnChanges correctly re-evaluates properties
The ngOnChanges method properly calls setUserProperties() and setUserAuthorityIconAndTooltip() to update relevant properties when input bindings change, ensuring the component stays in sync with its inputs.
90-91: Ensure all resources are cleaned up in ngOnDestroy
The ngOnDestroy method correctly closes the modal if it is open. After addressing the subscription unsubscription issue, the component will properly clean up its resources upon destruction.
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (5)
1-9: Update to Angular 16 injectables and input/output signals
The component correctly adopts Angular 16 standards by using inject, input, and output functions for dependency injection and property bindings. This modernizes the component and aligns it with the latest Angular practices.
22-32: Proper declaration of input and output properties using signals
The input and output properties are appropriately defined using input<T>() and output<T>(), enhancing reactivity and interoperability within the component.
47-48: Services are correctly injected using inject function
The MetisService and ChangeDetectorRef are injected using inject(), aligning with Angular 16's improved dependency injection patterns.
83-85: Ensure createEmptyAnswerPost correctly initializes answerPost
The answerPost.post assignment uses this.posting(), which ensures the new answer post is associated with the correct post. This maintains the integrity of the data relationships.
89-102: Efficient grouping and sorting of answer posts
The groupAnswerPosts method efficiently reverses, maps, sorts, and groups the answer posts to optimize display logic. This implementation enhances performance and user experience.
src/main/webapp/app/shared/metis/metis.module.ts (2)
70-70: Component declarations updated appropriately
The PostingHeaderComponent is correctly declared and exported, replacing the AnswerPostHeaderComponent and PostHeaderComponent. This consolidation simplifies the module and reduces redundancy.
94-94: Exports updated to include PostingHeaderComponent
The module exports are updated to include PostingHeaderComponent, ensuring that it can be used across the application where needed.
src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts (1)
Line range hint 64-75: LGTM: Well-structured test with proper initialization
The test case properly initializes the component within the Angular injection context and includes clear expectations.
src/main/webapp/app/shared/metis/post/post.component.html (1)
Line range hint 12-20: LGTM: Clean component renaming and modern Angular syntax
The component has been correctly renamed from jhi-post-header to jhi-posting-header and uses proper input bindings. The template also effectively uses Angular 17's new control flow syntax.
src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts (1)
110-158: LGTM: Well-structured parameterized tests
Excellent use of parameterized tests with it.each to test different user roles and their corresponding icons and tooltips. This approach reduces code duplication and makes the test cases more maintainable.
src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts (1)
72-74: LGTM! Test setup follows best practices
The test module configuration correctly uses MockComponent for dependencies and follows the testing guidelines.
src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.html
Show resolved
Hide resolved
...script/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts
Outdated
Show resolved
Hide resolved
...test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (2)
22-31: Consider adding default values for all input signals.Some input signals like
lastReadDateandpostinglack default values, which could lead to undefined behavior. Consider providing default values for all inputs.- lastReadDate = input<dayjs.Dayjs | undefined>(); + lastReadDate = input<dayjs.Dayjs | undefined>(undefined); - posting = input<Posting>(); + posting = input<Posting | undefined>(undefined);
Line range hint
94-107: Fix type safety issues with creationDateDayjs property.Using
(post as any).creationDateDayjsis unsafe. Consider creating an interface for the extended post type.+interface PostWithDayjs extends AnswerPost { + creationDateDayjs?: dayjs.Dayjs; +} - const sortedAnswerPosts = this.sortedAnswerPosts() + const sortedAnswerPosts: PostWithDayjs[] = this.sortedAnswerPosts() .slice() .reverse() .map((post) => { - (post as any).creationDateDayjs = post.creationDate ? dayjs(post.creationDate) : undefined; + const postWithDayjs: PostWithDayjs = { ...post }; + postWithDayjs.creationDateDayjs = post.creationDate ? dayjs(post.creationDate) : undefined; - return post; + return postWithDayjs; }); - const sortedPosts = sortedAnswerPosts.sort((a, b) => { - const aDate = (a as any).creationDateDayjs; - const bDate = (b as any).creationDateDayjs; + const sortedPosts = sortedAnswerPosts.sort((a, b) => { + const aDate = a.creationDateDayjs; + const bDate = b.creationDateDayjs; return aDate?.valueOf() - bDate?.valueOf(); });src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts (1)
79-84: Add edge case tests for groupAnswerPosts.The current test only verifies that groups are created. Consider adding tests for:
- Empty array
- Single post
- Posts with same author but different timestamps
- Posts with missing creation dates
src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts (1)
59-68: Enhance header visibility test assertions.The test could be more specific about why the header should be visible when isConsecutive is false.
it('should contain the posting header when isConsecutive is false', () => { runInInjectionContext(fixture.debugElement.injector, () => { component.isConsecutive = input<boolean>(false); component.posting = metisResolvingAnswerPostUser1; }); fixture.detectChanges(); const header = debugElement.query(By.css('jhi-posting-header')); expect(header).not.toBeNull(); + // Verify header properties + const headerComponent = ngMocks.findInstance(header, PostingHeaderComponent); + expect(headerComponent.posting).toBe(metisResolvingAnswerPostUser1); });src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts (2)
169-178: Enhance undefined posting test coverageWhile the test handles the basic undefined case, consider adding more assertions to verify the complete component state:
it('should handle undefined posting gracefully', () => { runInInjectionContext(injector, () => { component.posting = input<Posting>(); component.ngOnInit(); fixture.detectChanges(); expect(component.isPostResolved).toBeFalse(); expect(getElement(debugElement, '.resolved')).toBeNull(); + expect(component.userAuthorityIcon).toBeDefined(); + expect(component.userAuthorityTooltip).toBeDefined(); + expect(component.isAuthorOfPosting).toBeFalse(); + expect(getElement(debugElement, '#today-flag')).toBeNull(); + expect(getElement(debugElement, '#header-author-date')).not.toBeNull(); }); });
62-70: Improve date-related test assertionsThe date-related tests could be more specific in their assertions to ensure the complete date handling logic:
it('should set date information correctly for post of today', () => { runInInjectionContext(injector, () => { component.posting = input<Posting>(metisPostLectureUser1); component.ngOnInit(); fixture.detectChanges(); expect(getElement(debugElement, '#today-flag')).toBeDefined(); + const headerDate = getElement(debugElement, '#header-author-date'); + expect(headerDate.textContent).toContain('artemisApp.metis.today'); }); }); it('should not set today flag for posts not created today', () => { runInInjectionContext(injector, () => { const pastDatePost = { ...metisPostLectureUser1, creationDate: dayjs().subtract(1, 'day').toDate(), } as unknown as Post; component.posting = input<Posting>(pastDatePost); component.ngOnInit(); fixture.detectChanges(); expect(getElement(debugElement, '#today-flag')).toBeNull(); + const headerDate = getElement(debugElement, '#header-author-date'); + expect(headerDate.textContent).not.toContain('artemisApp.metis.today'); + expect(headerDate.textContent).toContain(pastDatePost.creationDate.toLocaleDateString()); }); });Also applies to: 72-84
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts(5 hunks)src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts(5 hunks)src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/test/javascript/spec/component/shared/metis/postings-header/posting-header.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/shared/metis/postings-footer/post-footer/post-footer.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/posting-footer/post-footer/post-footer.component.ts (1)
📓 Learnings (1)
src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts (1)
Learnt from: rabeatwork
PR: ls1intum/Artemis#9103
File: src/test/javascript/spec/component/shared/metis/postings-header/answer-post-header/answer-post-header.component.spec.ts:94-94
Timestamp: 2024-11-12T12:52:03.805Z
Learning: The `#today-flag` ID is only present in the test files and not in the actual component's HTML or TypeScript files.
🔇 Additional comments (3)
src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts (3)
1-28: LGTM: Import statements are well-organized and complete
The imports cover all necessary testing utilities, mock components/services, and models required for the test suite.
35-56: Consolidate duplicate beforeEach blocks
The beforeEach blocks could be combined to improve readability and maintenance.
- beforeEach(async () => {
- await TestBed.configureTestingModule({
- imports: [MockModule(FormsModule), MockModule(ReactiveFormsModule), MockDirective(NgbTooltip), MockModule(MetisModule)],
- providers: [FormBuilder, { provide: MetisService, useClass: MockMetisService }, { provide: AccountService, useClass: MockAccountService }],
- declarations: [
- PostingHeaderComponent,
- FaIconComponent,
- MockComponent(PostCreateEditModalComponent),
- MockPipe(ArtemisTranslatePipe),
- MockPipe(ArtemisDatePipe),
- MockComponent(PostingMarkdownEditorComponent),
- MockComponent(PostingButtonComponent),
- MockComponent(ConfirmIconComponent),
- MockComponent(ProfilePictureComponent),
- ],
- }).compileComponents();
- });
-
- beforeEach(() => {
- fixture = TestBed.createComponent(PostingHeaderComponent);
- component = fixture.componentInstance;
- debugElement = fixture.debugElement;
- injector = fixture.debugElement.injector;
- });
+ beforeEach(async () => {
+ await TestBed.configureTestingModule({
+ imports: [MockModule(FormsModule), MockModule(ReactiveFormsModule), MockDirective(NgbTooltip), MockModule(MetisModule)],
+ providers: [FormBuilder, { provide: MetisService, useClass: MockMetisService }, { provide: AccountService, useClass: MockAccountService }],
+ declarations: [
+ PostingHeaderComponent,
+ FaIconComponent,
+ MockComponent(PostCreateEditModalComponent),
+ MockPipe(ArtemisTranslatePipe),
+ MockPipe(ArtemisDatePipe),
+ MockComponent(PostingMarkdownEditorComponent),
+ MockComponent(PostingButtonComponent),
+ MockComponent(ConfirmIconComponent),
+ MockComponent(ProfilePictureComponent),
+ ],
+ }).compileComponents();
+
+ fixture = TestBed.createComponent(PostingHeaderComponent);
+ component = fixture.componentInstance;
+ debugElement = fixture.debugElement;
+ injector = fixture.debugElement.injector;
+ });108-156: LGTM: Comprehensive user role testing
The test cases for user roles are well-structured using Jest's parameterized testing feature. They thoroughly verify both the visual elements and component properties for different user roles.
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts
Show resolved
Hide resolved
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
# Conflicts: # src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html # src/main/webapp/app/shared/metis/posting-header/post-header/posting-header.component.html # src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts # src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (2)
Line range hint
11-16: Consider moving the PostGroup interface to a separate file.The
PostGroupinterface could be moved to a shared types file to improve code organization and reusability.
22-36: Add JSDoc comments for input and output signals.Adding documentation for these signals would improve code maintainability and help other developers understand their purpose and expected values.
Example:
/** The last read date for the post. Used to highlight new content. */ lastReadDate = input<dayjs.Dayjs | undefined>(); /** Emitted when a thread should be opened. */ openThread = output<void>();src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts (1)
Line range hint
69-178: Improve test organization with describe blocks.Consider grouping related tests using describe blocks for better organization and readability:
describe('PostFooterComponent', () => { // ... setup code ... describe('Initialization', () => { it('should be initialized correctly for users that are at least tutors in course', () => { // ... existing test ... }); it('should be initialized correctly for users that are not at least tutors in course', () => { // ... existing test ... }); }); describe('Answer Posts Grouping', () => { it('should group answer posts correctly', () => { // ... existing test ... }); it('should group answer posts and detect changes on changes to sortedAnswerPosts input', () => { // ... existing test ... }); }); describe('Modal Operations', () => { it('should open create answer post modal', () => { // ... existing test ... }); it('should close create answer post modal', () => { // ... existing test ... }); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/main/webapp/app/shared/metis/answer-post/answer-post.component.html(1 hunks)src/main/webapp/app/shared/metis/post/post.component.html(1 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts(5 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(2 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.component.spec.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/webapp/app/shared/metis/post/post.component.html
- src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts
- src/main/webapp/app/shared/metis/answer-post/answer-post.component.html
- src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/test/javascript/spec/component/shared/metis/postings-footer/post-footer/post-footer.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/posting-footer/post-footer/post-footer.component.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts (1)
82-85:
Add null check for posting signal value.
The posting() signal value could be undefined. Consider adding a null check.
createEmptyAnswerPost(): AnswerPost {
const answerPost = new AnswerPost();
answerPost.content = '';
- answerPost.post = this.posting();
+ const post = this.posting();
+ if (!post) {
+ throw new Error('Cannot create answer post: posting is undefined');
+ }
+ answerPost.post = post;
answerPost.resolvesPost = this.isAtLeastTutorInCourse;
return answerPost;
}
src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.ts
Show resolved
Hide resolved
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (13)
src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.html (3)
1-8: Consider memoizing the sortedAnswerPosts() resultWhile the transition to function calls provides better reactivity, frequent calls to
sortedAnswerPosts()within template expressions might impact performance, especially with large datasets.Consider memoizing the result:
// In the component class private readonly memoizedSortedAnswerPosts = memoize(() => this.sortedAnswerPosts());
17-29: Consider extracting the answer post templateThe answer post template contains multiple bindings and could benefit from being extracted into a separate component for better maintainability.
Consider creating a dedicated component:
@Component({ selector: 'jhi-answer-post-item', template: ` <jhi-answer-post [lastReadDate]="lastReadDate()" [isReadOnlyMode]="readOnlyMode()" ... /> ` }) export class AnswerPostItemComponent { @Input() post: AnswerPost; @Input() group: PostGroup; // ... other inputs }
41-41: Improve event handler clarityThe current event handler combines multiple operations in a single line, which could make debugging and maintenance more challenging.
Consider breaking this into a method:
// In the component class handleAnswerPostCreation(): void { this.createdAnswerPost = this.createEmptyAnswerPost(); this.showAnswers(); }Then in the template:
-(onCreate)="createdAnswerPost = createEmptyAnswerPost(); showAnswers()" +(onCreate)="handleAnswerPostCreation()"src/main/webapp/app/shared/metis/posting-header/posting-header.component.html (1)
33-34: Consider extracting the date format logic for better readability.The date display logic works correctly, but the inline ternary expression is quite long.
Consider extracting the date format logic to a component method:
-{{ postingIsOfToday ? (creationDate | artemisDate: 'time') : (creationDate | artemisDate: 'short-date') + ' - ' + (creationDate | artemisDate: 'time') }} +{{ formatPostDate(creationDate, postingIsOfToday) }}src/main/webapp/app/shared/metis/post/post.component.ts (3)
Line range hint
171-207: Consider implementing proper cleanup for document click listener.The
@HostListener('document:click')should be cleaned up when the component is destroyed to prevent memory leaks.Add the following cleanup code:
+ import { OnDestroy } from '@angular/core'; - export class PostComponent extends PostingDirective<Post> implements OnInit, OnChanges, AfterContentChecked { + export class PostComponent extends PostingDirective<Post> implements OnInit, OnChanges, AfterContentChecked, OnDestroy { + private documentClickListener: () => void; + + constructor(...) { + super(); + this.documentClickListener = this.onClickOutside.bind(this); + document.addEventListener('click', this.documentClickListener); + } + + ngOnDestroy() { + document.removeEventListener('click', this.documentClickListener); + } - @HostListener('document:click', ['$event']) - onClickOutside() { + onClickOutside() { this.showDropdown = false; this.enableBodyScroll(); }
Line range hint
249-271: Consider splitting complex method into smaller, focused functions.The
sortAnswerPostsmethod handles both null checking and complex sorting logic. Consider splitting this for better maintainability.Consider refactoring to:
- sortAnswerPosts(): void { - if (!this.posting.answers) { - this.sortedAnswerPosts = []; - return; - } - this.sortedAnswerPosts = this.posting.answers.sort( - (answerPostA, answerPostB) => - Number(answerPostB.resolvesPost) - Number(answerPostA.resolvesPost) || - answerPostA.creationDate!.valueOf() - answerPostB.creationDate!.valueOf(), - ); - } + private compareAnswerPosts(answerPostA: AnswerPost, answerPostB: AnswerPost): number { + const resolveComparison = Number(answerPostB.resolvesPost) - Number(answerPostA.resolvesPost); + if (resolveComparison !== 0) { + return resolveComparison; + } + return answerPostA.creationDate!.valueOf() - answerPostB.creationDate!.valueOf(); + } + + sortAnswerPosts(): void { + this.sortedAnswerPosts = this.posting.answers?.sort(this.compareAnswerPosts.bind(this)) ?? []; + }
Line range hint
273-297: Add type safety for method parameters and return values.The
onUserReferenceClickedmethod could benefit from stronger typing and return type declaration.Consider adding types:
- onUserReferenceClicked(referencedUserLogin: string) { + onUserReferenceClicked(referencedUserLogin: string): void { const course = this.metisService.getCourse(); if (isMessagingEnabled(course)) { if (this.isCommunicationPage) { - this.metisConversationService.createOneToOneChat(referencedUserLogin).subscribe(); + this.metisConversationService.createOneToOneChat(referencedUserLogin).subscribe({ + error: (error: Error) => { + console.error('Failed to create one-to-one chat:', error); + } + });src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts (2)
17-17: Component Selector Should Use 'app' PrefixPer the Angular style guide, component selectors should start with an
appprefix to avoid naming collisions and maintain consistency.Apply this diff to rename the selector:
-selector: 'jhi-posting-footer', +selector: 'app-posting-footer',
88-93: Simplify Conditional Checks ingroupAnswerPostsMethodThe condition in the
groupAnswerPostsmethod can be simplified for better readability and efficiency.Apply this diff:
- if (!this.sortedAnswerPosts() || this.sortedAnswerPosts().length === 0) { + if (!this.sortedAnswerPosts || this.sortedAnswerPosts.length === 0) {src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (3)
71-73: Assign Input Properties Directly in TestsIn the test, assign input properties directly without using
input<>(), which is intended for use within components, not in tests.Apply this diff:
- component.posting = input<Posting>(post); + component.posting = post;
93-95: Avoid Unnecessary Spying on Internal MethodsSpying on internal methods like
changeDetector.detectChanges()in tests is generally unnecessary and can make tests brittle. Focus on testing public APIs and observable behaviors.
160-179: Optimize Change Detection in TestsEnsure that calls to
fixture.detectChanges()are necessary. Unnecessary calls may slow down tests without adding value.src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts (1)
47-48: Consider using constructor injection for services in componentsIn Angular components, it's recommended to inject services via the constructor rather than using the
injectfunction. Constructor injection promotes better testability and aligns with Angular's style guidelines.Apply this diff to refactor the service injections:
export class PostingHeaderComponent implements OnInit, OnDestroy, OnChanges { + constructor( + protected metisService: MetisService, + protected accountService: AccountService, + ) {} - protected metisService: MetisService = inject(MetisService); - protected accountService: AccountService = inject(AccountService);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
src/main/webapp/app/shared/metis/metis.module.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(2 hunks)src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.scss(0 hunks)src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.html(3 hunks)src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts(4 hunks)src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html(0 hunks)src/main/webapp/app/shared/metis/posting-header/posting-header.component.html(2 hunks)src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts(5 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(1 hunks)src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/shared/metis/posting-footer/post-footer/post-footer.component.scss
- src/main/webapp/app/shared/metis/posting-header/answer-post-header/answer-post-header.component.html
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/javascript/spec/component/shared/metis/answer-post/answer-post.component.spec.ts
- src/test/javascript/spec/component/shared/metis/postings-header/posting-header.component.spec.ts
- src/main/webapp/app/shared/metis/metis.module.ts
- src/main/webapp/app/shared/metis/post/post.component.html
- src/test/javascript/spec/component/shared/metis/post/post.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/main/webapp/app/shared/metis/post/post.component.ts (1)
src/main/webapp/app/shared/metis/posting-header/posting-header.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/postings-footer/posting-footer.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/posting-footer/posting-footer.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-footer/posting-footer.component.ts (1)
src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts (1)
🔇 Additional comments (8)
src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.html (1)
15-16: LGTM! Correct usage of new @for syntax
The migration to the new Angular control flow syntax with proper tracking functions is well implemented.
src/main/webapp/app/shared/metis/posting-header/posting-header.component.html (4)
3-14: LGTM! Proper use of new Angular syntax and null checks.
The author information display follows the new Angular syntax guidelines and implements proper null checking for authorOfPosting. The profile picture component bindings and editability checks are well-structured.
37-43: LGTM! Clean implementation of resolution status.
The resolution status display is well-implemented with proper accessibility considerations through tooltips.
45-47: 🛠️ Refactor suggestion
Simplify complex condition and verify animation behavior.
The condition for displaying the new badge is complex and could be more readable. Also, the hideAfter5Seconds class suggests animation behavior.
Consider extracting the condition to a method:
-@if ((!!isCommunicationPage() && !lastReadDate()) || (lastReadDate() && creationDate && isAfter && !isAuthorOfPosting)) {
+@if (shouldShowNewBadge()) {✅ Verification successful
Let me gather more information about the animation implementation.
Simplify condition and consider animation implementation improvements
The condition should be extracted into a method for better readability. The animation implementation is found and works as follows:
- CSS class
hideAfter5Secondsuses a 5-second animation - Animation fades out the badge (opacity 1 to 0) after 90% of the duration
- No JavaScript manipulation is involved, purely CSS-based
Suggested improvements:
- Extract complex condition into a method for better maintainability
- Consider using Angular animations instead of raw CSS for better control and testability
- The same animation is used in multiple components, suggesting it could be moved to a shared animation definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of hideAfter5Seconds animation
# Search for hideAfter5Seconds CSS/animation implementation
rg -A 5 "hideAfter5Seconds"
# Search for any related animation timing logic in component
ast-grep --pattern 'class PostingHeaderComponent {
$$$
hideAfter$_
$$$
}'
Length of output: 1711
Script:
#!/bin/bash
# Get the complete CSS animation definition
rg -A 10 "@keyframes cssAnimation"
# Search for any JavaScript/TypeScript code that might be manipulating the animation
rg "hideAfter5Seconds" --type ts
Length of output: 793
Line range hint 18-24: Verify role-related variable implementations.
The role badge implementation looks good, but let's verify the role-related variables are properly defined in the component.
✅ Verification successful
Let me gather more specific information about the component implementation.
Role-related variables are properly implemented and working as expected.
The implementation shows that:
- All role-related variables (
userAuthority,userAuthorityIcon,userAuthorityTooltip,userRoleBadge) are properly defined in the component - The component handles different user roles (USER, INSTRUCTOR, TUTOR) with appropriate icons and translations
- Proper fallback handling for deleted users
- Comprehensive test coverage verifying the role-based behavior
- Translations are available in both English and German
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of role-related variables in the component.
# Search for userAuthority, userAuthorityIcon, and userAuthorityTooltip definitions
ast-grep --pattern 'class PostingHeaderComponent {
$$$
userAuthority$_
$$$
}'
# Also check for any role-related enums or types
rg -A 5 "enum.*Authority|type.*Authority"
Length of output: 2041
Script:
#!/bin/bash
# Search for the PostingHeaderComponent implementation
rg -l "PostingHeaderComponent"
# Then get its content
ast-grep --pattern 'class PostingHeaderComponent {
$$$
}'
# Also check for any icon-related imports and usage
rg "userAuthorityIcon|userAuthorityTooltip|userRoleBadge" -A 3
Length of output: 10174
src/main/webapp/app/shared/metis/post/post.component.ts (2)
25-25: LGTM: Import statement aligned with component renaming.
The import statement correctly reflects the renamed component, following Angular naming conventions.
63-63: LGTM: Property type updated correctly.
The ViewChild property type is correctly updated to PostingFooterComponent, maintaining consistency with the component renaming.
Let's verify the template usage matches this change:
✅ Verification successful
Let's check the actual content of the template file to verify how postFooter is being used.
Let's verify if the selector for PostingFooterComponent matches with what's used in the template.
Let's try a more specific search to find the PostingFooterComponent's selector:
Let's check the component decorator for PostingFooterComponent:
The verification is complete. I can now generate the final response:
Property type and template usage are correctly aligned
The ViewChild property postFooterComponent is correctly typed as PostingFooterComponent, and the template uses the matching selector jhi-posting-footer as confirmed in the component's decorator. The change is consistent throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify template references to postFooter
# Check if the template uses the correct selector for the new PostingFooterComponent
rg -l "postFooter" --type html
Length of output: 88
Script:
#!/bin/bash
# Get the content of the template file to see how postFooter is referenced
cat src/main/webapp/app/shared/metis/post/post.component.html
Length of output: 10944
Script:
#!/bin/bash
# Find the component decorator for PostingFooterComponent to check its selector
ast-grep --pattern 'Component({
$$$
selector: $_,
$$$
})'
Length of output: 66252
Script:
#!/bin/bash
# Search for the PostingFooterComponent definition
rg -A 5 "export class PostingFooterComponent"
Length of output: 843
Script:
#!/bin/bash
# Get the content of the posting-footer component file
cat src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts
Length of output: 6579
src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts (1)
23-31: Verify the usage of input and output functions for Inputs and Outputs
The use of input and output functions for defining input and output properties is part of Angular's experimental Signals API. Please ensure that this approach aligns with your project's Angular version and stability requirements. If your project is not leveraging the experimental API, consider using the standard @Input and @Output decorators.
Run the following script to check the Angular version in use:
✅ Verification successful
I'll analyze the usage patterns across the codebase and the compiler configuration to determine if this is a consistent approach being adopted project-wide.
Based on the gathered information, I can now provide the final response:
The usage of input() and output() functions is appropriate for your Angular version
The codebase is using Angular v18.2.13, which fully supports the new input/output signals syntax. The pattern is being consistently used across multiple components in the codebase, showing it's an intentional architectural choice. This modern approach:
- Improves performance through fine-grained reactivity
- Provides better type safety
- Reduces the need for NgOnChanges lifecycle hooks
- Is the recommended way for handling inputs/outputs in Angular 16+
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the Angular Core version used in the project.
grep '"@angular/core"' package.json
Length of output: 73
Script:
#!/bin/bash
# Check if other components are using the new input/output syntax
rg "input<|output<" "src/main/webapp/app" -A 2 -B 2
Length of output: 67755
Script:
#!/bin/bash
# Check for traditional @Input/@Output decorators usage
rg "@Input\(\)|@Output\(\)" "src/main/webapp/app" -A 2 -B 2
Length of output: 66841
Script:
#!/bin/bash
# Check package.json for any experimental flags or configurations
grep -A 5 -B 5 '"compilerOptions"' tsconfig.json
Length of output: 228
src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts
Show resolved
Hide resolved
...test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/metis/posting-header/posting-header.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (2)
63-72: Improve test description and assertionsThe test description "should be initialized correctly for users that are at least tutors in course" could be more specific about what "correctly" means.
Consider renaming to:
-it('should be initialized correctly for users that are at least tutors in course', () => { +it('should set isAtLeastTutorInCourse to true and enable post resolution for tutors', () => {
176-210: Split complex grouping test into smaller focused testsThe current test case tests multiple aspects of the grouping logic in a single test. This makes it harder to identify what exactly failed when the test fails.
Consider splitting into multiple focused tests:
- Test grouping by author
- Test grouping by time difference
- Test sorting within groups
Example:
it('should group posts by the same author', () => { const authorA: User = { id: 1, login: 'authorA' } as User; const post1 = createPost(1, authorA, baseTime); const post2 = createPost(2, authorA, baseTime.add(3, 'minute')); runInInjectionContext(injector, () => { component.sortedAnswerPosts = input<AnswerPost[]>([post1, post2]); component.groupAnswerPosts(); expect(component.groupedAnswerPosts).toHaveLength(1); expect(component.groupedAnswerPosts[0].posts).toHaveLength(2); }); }); it('should create new group when time difference exceeds threshold', () => { const authorA: User = { id: 1, login: 'authorA' } as User; const post1 = createPost(1, authorA, baseTime); const post2 = createPost(2, authorA, baseTime.add(10, 'minute')); runInInjectionContext(injector, () => { component.sortedAnswerPosts = input<AnswerPost[]>([post1, post2]); component.groupAnswerPosts(); expect(component.groupedAnswerPosts).toHaveLength(2); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.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}}
🪛 GitHub Check: client-tests-selected
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
[failure] 112-112:
Cannot find name 'PostGroup'.
[failure] 123-123:
Cannot find name 'PostGroup'.
[failure] 134-134:
Cannot find name 'PostGroup'.
🪛 GitHub Check: client-tests
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
[failure] 112-112:
Cannot find name 'PostGroup'.
[failure] 123-123:
Cannot find name 'PostGroup'.
[failure] 134-134:
Cannot find name 'PostGroup'.
🔇 Additional comments (2)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (2)
24-61: Well-structured test setup with proper mocking
The test setup follows best practices with proper dependency mocking, spy setup, and cleanup.
1-23:
Import PostGroup interface from the component file
The PostGroup interface is used in the tests but not imported. This matches the existing static analysis error.
Add this import at the top of the file:
import { PostingFooterComponent } from '../../../../../../../main/webapp/app/shared/metis/posting-footer/posting-footer.component';
+import { PostGroup } from '../../../../../../../main/webapp/app/shared/metis/posting-footer/posting-footer.component';
...test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (2)
63-219: Add test cases for additional edge scenarios.While the test coverage is good, consider adding these test cases:
- Posts with undefined/null creation dates
- Posts with same creation time but different authors
- Error handling in modal operations (e.g., when modal fails to open)
- Boundary conditions for time differences in grouping
Example test case:
it('should handle posts with undefined creation date', () => { const post1: AnswerPost = { id: 1, author: { id: 1 } as User, creationDate: undefined } as AnswerPost; runInInjectionContext(injector, () => { component.sortedAnswerPosts = input<AnswerPost[]>([post1]); component.groupAnswerPosts(); expect(component.groupedAnswerPosts).toHaveLength(1); }); });🧰 Tools
🪛 GitHub Check: client-tests
[failure] 112-112:
Cannot find name 'PostGroup'.
[failure] 123-123:
Cannot find name 'PostGroup'.
[failure] 134-134:
Cannot find name 'PostGroup'.🪛 GitHub Check: client-tests-selected
[failure] 112-112:
Cannot find name 'PostGroup'.
[failure] 123-123:
Cannot find name 'PostGroup'.
[failure] 134-134:
Cannot find name 'PostGroup'.
176-210: Extract test data setup into helper functions.The test data setup for grouped posts testing is quite lengthy and contains magic numbers. Consider extracting it into helper functions for better maintainability and reusability.
Example refactor:
function createTestPost(id: number, author: User, minutesOffset: number): AnswerPost { const baseTime = dayjs(); return { id, author, creationDate: baseTime.add(minutesOffset, 'minute').toDate() } as unknown as AnswerPost; } function createTestData(): { posts: AnswerPost[], authorA: User, authorB: User } { const authorA = { id: 1, login: 'authorA' } as User; const authorB = { id: 2, login: 'authorB' } as User; return { authorA, authorB, posts: [ createTestPost(1, authorA, 0), createTestPost(2, authorA, 3), createTestPost(3, authorA, 10), createTestPost(4, authorB, 12), createTestPost(5, authorB, 14) ] }; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.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}}
🪛 GitHub Check: client-tests
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
[failure] 112-112:
Cannot find name 'PostGroup'.
[failure] 123-123:
Cannot find name 'PostGroup'.
[failure] 134-134:
Cannot find name 'PostGroup'.
🪛 GitHub Check: client-tests-selected
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts
[failure] 112-112:
Cannot find name 'PostGroup'.
[failure] 123-123:
Cannot find name 'PostGroup'.
[failure] 134-134:
Cannot find name 'PostGroup'.
🔇 Additional comments (1)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (1)
1-22:
Import PostGroup interface from the component.
The PostGroup interface is used in the tests but not imported, causing TypeScript errors. This is a duplicate of a previous comment.
Add this import:
import { Posting } from '../../../../../../../main/webapp/app/entities/metis/posting.model';
import { PostingFooterComponent } from '../../../../../../../main/webapp/app/shared/metis/posting-footer/posting-footer.component';
+import { PostGroup } from '../../../../../../../main/webapp/app/shared/metis/posting-footer/posting-footer.component';
Checklist
General
Client
Motivation and Context
The
AnswerPostHeaderandPostHeadercomponents are almost identical, causing code duplication, and one of them needs to be removed.In this case, the
PostingHeaderDirectivewould also be used for only a single component, making its existence redundant, and it should be removed as well.Similarly, the
AnswerPostFootercomponent was previously removed due to duplication with thePostFootercomponent. This means thePostingFooterDirectiveis now being used for only one component and should also be removed to avoid unnecessary abstraction.Description
The
PostHeaderandAnswerPostHeadercomponents have been merged into a single component namedPostingHeaderComponent.The
PostingHeaderDirectiveandPostingFooterDirectivehave been removed.PostFooterComponenthas been renamed toPostingFooterComponent.The
PostingHeaderComponentandPostingFooterComponenthave been updated to use inject and signals.Tests have been updated accordingly.
Known issue: All signals, except those related to
@ViewChild, have been updated. Updating viewChild requires making the components standalone, which I will address in a follow-up PR as it will impact other 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
Summary by CodeRabbit
New Features
PostingHeaderComponentandPostingFooterComponent.Bug Fixes
Documentation
Chores