Development: Migrate client code for edit modals of posts
#10198
Conversation
github-actions
bot
added
tests
client
WalkthroughThe pull request involves modifications to several Angular components within the Metis shared module, focusing on updating property decorators and method calls. The changes primarily transition from using Changes
Suggested labels
Suggested reviewers
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
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
|
WalkthroughThe pull request involves modifications to several Angular components within the Metis application, focusing on updating input and output property declarations and method calls. The changes primarily transition from using traditional Angular decorators like Changes
Sequence DiagramsequenceDiagram
participant Parent as Parent Component
participant Modal as Create/Edit Modal
participant Container as View Container
Parent ->> Modal: Set input properties
Modal ->> Container: Initialize container reference
Modal ->> Container: Create/Clear embedded views
Modal -->> Parent: Emit updates via output events
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts (1)
19-20: Adopt Angular Signals naming conventions for clarity
Great job replacing@Input()and@Output()withinput<>()andoutput<>(). To further align with Angular's recommended best practices when using Signals, consider adopting naming patterns (such as$suffix) to help future maintainers easily distinguish signals from standard properties or methods.src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts (1)
67-67: Validate container reference before clearing on destroy
Callingthis.answerPostCreateEditModal?.createEditAnswerPostContainerRef()?.clear();is consistent with the new approach. Make sure that other code paths cannot hold references to the container afterngOnDestroyto prevent unexpected lifecycle issues.src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (1)
84-87: Reduce code duplication for mock container references
These lines repeat the samemockContainerRefsetup. Consider introducing a helper function or fixture setup method to improve maintainability and reduce code repetition.Example:
- const mockContainerRef = { clear: jest.fn() } as any; - component.answerPostCreateEditModal = { - createEditAnswerPostContainerRef: signal(mockContainerRef), - } as unknown as AnswerPostCreateEditModalComponent; + const mockContainerRef = createMockContainerRef(); + component.answerPostCreateEditModal = { + createEditAnswerPostContainerRef: signal(mockContainerRef), + } as unknown as AnswerPostCreateEditModalComponent;Then define a reusable function:
function createMockContainerRef() { return { clear: jest.fn() } as any; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts(4 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.html(2 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts(1 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts (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}}
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.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-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (1)
src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
🔇 Additional comments (18)
src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts (4)
28-28: Clarify optional chaining for createEmbeddedView
Usingthis.createEditAnswerPostContainerRef()?.createEmbeddedView()is correct and prevents runtime errors if the reference is undefined. Ensure all calling sites handle the potentialundefinedscenario.
36-36: Check if container is initialized before clearing
Again, optional chaining is a good safety measure here. Confirm that no code path ever needs to handle the scenario where clearing is attempted butcreateEditAnswerPostContainerRef()remains uninitialized.
63-63: Reinforce consistency with container clearing
Repeated usage ofthis.createEditAnswerPostContainerRef()?.clear();maintains consistency in approach. No issues here.
82-82: Ensure consistent cleanup flow
Clearing the container inupdatePosting()is a good practice to visibly finalize the edit process. Just verify the user experience remains smooth ifcreateEditAnswerPostContainerRef()is unexpectedly nonexistent.src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts (4)
11-11: Leverage structured imports for clarity
Importinginput,runInInjectionContext, andViewContainerRefbehind a single destructuring is concise. Ensure that future contributors are aware of the significance of each import to avoid confusion.
40-40: Mock usage of component methods
jest.spyOn(component, 'updatePosting')is a clean way to verify side effects. Keep an eye on potential performance overhead if many methods are spied on.
65-77: Improve mocking logic for container references
You are correctly isolating thecreateEditAnswerPostContainerReffunctionality withrunInInjectionContext. This approach ensures minimal coupling toViewContainerRefinternals and leads to more robust tests.
82-94: Assert container clearing is called once
VerifyingmockClearis called confirms the embedded view is cleaned up properly. This is an excellent example of a targeted test.src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (1)
41-41: Use of functional input property
Replacing@Input() isCommunicationPagewithisCommunicationPage = input<boolean>(false)showcases the new Signals-based approach. Confirm all template references invokeisCommunicationPage()where needed.src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (7)
20-20: Check Angular version for signals
This import ofsignalandinputfrom@angular/corerequires Angular v16+ or higher. Ensure that your environment is using a compatible Angular version to leverage these features properly.
98-101: Repeated code usage
110-111: Repeated code usage
176-179: Repeated code usage
187-190: Repeated code usage
195-197: Repeated code usage
216-219: Repeated code usagesrc/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.html (2)
9-9: Ensure method purity
Switching from a property to a method call (isCommunicationPage()) can be beneficial if the method is pure. Verify that no unexpected side effects or performance bottlenecks might occur from repeated invocations.
23-23: Logic appears consistent
Updating!isCommunicationPageto!isCommunicationPage()aligns with your functional approach. The existing condition remains logically sound as long as the method returns the same boolean values as the former property.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts (1)
Line range hint
19-19: Remove unused variable to fix pipeline failure.The
viewContainerRefvariable is declared but never used, causing a pipeline failure.- let viewContainerRef: ViewContainerRef;
🧹 Nitpick comments (8)
src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts (3)
1-1: LGTM! Consider adding explicit signal types.The transition to the new input/output syntax is correct. For better type safety, consider explicitly typing the signals:
- createEditAnswerPostContainerRef = input<ViewContainerRef>(); - postingUpdated = output<Posting>(); + createEditAnswerPostContainerRef = input.required<ViewContainerRef>(); + postingUpdated = output<Posting>({ alias: 'postingUpdated' });Also applies to: 19-20
28-28: Consider adding error handling for container operations.The changes to method call syntax are correct. However, consider adding error handling for cases where the container reference is undefined.
- this.createEditAnswerPostContainerRef()?.createEmbeddedView(this.postingEditor); + const containerRef = this.createEditAnswerPostContainerRef(); + if (!containerRef) { + console.warn('Container reference is undefined'); + return; + } + containerRef.createEmbeddedView(this.postingEditor);Also applies to: 36-36, 63-63, 82-82
Line range hint
52-68: Enhance error handling in async operations.While basic error handling exists, consider adding more detailed error handling and logging.
error: () => { + console.error('Failed to create/update answer post'); this.isLoading = false; + // Consider showing a user-friendly error message + this.showErrorMessage('artemisApp.metis.error.saveFailed'); },Also applies to: 73-87
src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts (1)
67-67: Consider implementing a complete cleanup pattern.While the change to method call syntax is correct, consider implementing a more comprehensive cleanup pattern.
+ private readonly destroy$ = new Subject<void>(); ngOnDestroy(): void { this.answerPostCreateEditModal?.createEditAnswerPostContainerRef()?.clear(); + this.destroy$.next(); + this.destroy$.complete(); }src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts (1)
65-94: Consider extracting mock setup to a helper function.The mock setup is repeated across tests. Consider extracting it to a reusable helper function.
+ function setupMockContainerRef(component: AnswerPostCreateEditModalComponent, fixture: ComponentFixture<AnswerPostCreateEditModalComponent>) { + const mockClear = jest.fn(); + const mockCreateEmbeddedView = jest.fn(); + + runInInjectionContext(fixture.debugElement.injector, () => { + component.createEditAnswerPostContainerRef = input<ViewContainerRef>({ + clear: mockClear, + createEmbeddedView: mockCreateEmbeddedView, + } as unknown as ViewContainerRef); + }); + + return { mockClear, mockCreateEmbeddedView }; + }src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (1)
41-41: Add JSDoc and explicit typing for the signal.While the change to signal syntax is correct, consider adding documentation and explicit typing.
- isCommunicationPage = input<boolean>(false); + /** + * Indicates whether the modal is being used in the communication page context. + * @default false + */ + isCommunicationPage = input<boolean, false>(false);src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (1)
84-87: Refactor duplicate test setup code.The initialization of
answerPostCreateEditModalwithmockContainerRefis duplicated across multiple test cases. Consider extracting this setup into a beforeEach block or a helper function to improve maintainability.describe('PostingFooterComponent', () => { let component: PostingFooterComponent; let fixture: ComponentFixture<PostingFooterComponent>; let metisService: MetisService; let metisServiceUserAuthorityStub: jest.SpyInstance; let injector: Injector; + let mockContainerRef: { clear: jest.Mock }; beforeEach(() => { return TestBed.configureTestingModule({ // ... existing setup ... }) .compileComponents() .then(() => { fixture = TestBed.createComponent(PostingFooterComponent); component = fixture.componentInstance; metisService = TestBed.inject(MetisService); metisServiceUserAuthorityStub = jest.spyOn(metisService, 'metisUserIsAtLeastTutorInCourse'); injector = fixture.debugElement.injector; + mockContainerRef = { clear: jest.fn() } as any; + component.answerPostCreateEditModal = { + createEditAnswerPostContainerRef: signal(mockContainerRef), + } as unknown as AnswerPostCreateEditModalComponent; }); });Also applies to: 98-101, 110-111, 176-179, 187-190, 195-197, 216-219
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.html (1)
9-11: LGTM! Modern Angular syntax usage.The template correctly uses the new
@ifsyntax instead of*ngIf, following Angular's latest best practices. The conditions are properly structured as function calls.Consider extracting complex condition into a component method.
The condition on line 23 is complex and could be moved to the component class for better maintainability:
+// In component class +shouldShowTagSelector(): boolean { + return this.pageType !== PageType.PLAGIARISM_CASE_INSTRUCTOR && + this.pageType !== PageType.PLAGIARISM_CASE_STUDENT && + !this.isCommunicationPage() && + !this.posting.conversation; +} -@if (pageType !== PageType.PLAGIARISM_CASE_INSTRUCTOR && pageType !== PageType.PLAGIARISM_CASE_STUDENT && !isCommunicationPage() && !posting.conversation) { +@if (shouldShowTagSelector()) {Also applies to: 23-32
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/shared/metis/posting-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts(4 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.html(2 hunks)src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts(2 hunks)src/main/webapp/app/shared/metis/posting-footer/posting-footer.component.ts(1 hunks)src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts(3 hunks)src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.component.ts (1)
src/main/webapp/app/shared/metis/posting-create-edit-modal/post-create-edit-modal/post-create-edit-modal.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/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.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/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-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.ts (1)
🪛 GitHub Actions: Test
src/test/javascript/spec/component/shared/metis/postings-create-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts
[error] 19-19: 'viewContainerRef' is declared but its value is never read.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
src/test/javascript/spec/component/shared/metis/postings-footer/posting-footer.component.spec.ts (1)
Line range hint
110-116: LGTM! Test coverage for signal behavior.The test case properly verifies that the container is cleared on component destruction, which is crucial for preventing memory leaks when using signals.
asliayk
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Cathy0123456789
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
flbrgit
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Feras797
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
...ate-edit-modal/answer-post-create-edit-modal/answer-post-create-edit-modal.component.spec.ts
Show resolved
Hide resolved
krusche
changed the title
Communicaition: Migrate client code for edit modals of PostsDevelopment: Migrate client code for edit modals of posts
Checklist
General
Client
Motivation and Context
This PR migrates existing code to the new Client guidelines using signals
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
Summary by CodeRabbit
Refactor
@Input()and@Output()decorators withinput()andoutput()functions.Tests