Development: Use signals in lecture unit wizard
#9708
Conversation
github-actions
bot
added
the
client
This reverts commit c638460
WalkthroughThe changes in this pull request primarily involve modifications to 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: 3
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/shared/date-time-picker/date-time-picker.component.html (1)
22-23: Consider consolidating validation class bindings.The validation state is currently set through both
ngClassandclass.ng-invalid. Consider consolidating these to reduce redundancy:-[ngClass]="{ 'is-invalid': isValid(), 'border-warning': warning() }" -[class.ng-invalid]="!isValid()" +[ngClass]="{ 'is-invalid': isValid(), 'border-warning': warning(), 'ng-invalid': !isValid() }"src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.html (1)
Line range hint
22-71: Consider refactoring repeated form patterns for better maintainability.The code shows similar patterns repeated across different unit types (text, video, online, attachment, exercise). Consider refactoring this to reduce duplication and improve maintainability.
You could create a mapping of unit types to their respective components and use a more dynamic approach:
interface UnitFormConfig { component: Type<any>; isOpen: () => boolean; formData?: any; } const unitForms: Record<string, UnitFormConfig> = { text: { component: TextUnitFormComponent, isOpen: isTextUnitFormOpen, formData: textUnitFormData }, // ... other unit types };Then in the template:
@for (const [type, config] of Object.entries(unitForms); track type) { @if (config.isOpen()) { <ng-container [ngComponentOutlet]="config.component" [ngComponentOutletInputs]="{ isEditMode: isEditingLectureUnit, hasCancelButton: true, formData: config.formData }" /> } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.html(6 hunks)src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts(4 hunks)src/main/webapp/app/shared/date-time-picker/date-time-picker.component.html(3 hunks)src/main/webapp/app/shared/date-time-picker/date-time-picker.component.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.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/lecture/wizard-mode/lecture-wizard-units.component.ts (1)
src/main/webapp/app/shared/date-time-picker/date-time-picker.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/date-time-picker/date-time-picker.component.ts (1)
📓 Learnings (1)
src/main/webapp/app/shared/date-time-picker/date-time-picker.component.ts (1)
Learnt from: florian-glombik
PR: ls1intum/Artemis#9694
File: src/main/webapp/app/shared/date-time-picker/date-time-picker.component.ts:29-29
Timestamp: 2024-11-07T00:36:51.500Z
Learning: In the Angular `FormDateTimePickerComponent` in `src/main/webapp/app/shared/date-time-picker/date-time-picker.component.ts`, the `value` model property may sometimes be a `string` because the component is used in multiple places where a `string` is passed. Therefore, changing its type from `any` to a more specific type could cause issues.
🪛 Biome
src/main/webapp/app/shared/date-time-picker/date-time-picker.component.ts
[error] 49-49: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (9)
src/main/webapp/app/shared/date-time-picker/date-time-picker.component.html (4)
1-10: LGTM! Proper usage of new Angular control flow syntax.
The changes correctly implement the new @if syntax and properly convert properties to function calls for reactive updates.
Line range hint 11-16: LGTM! Correct implementation of timezone warning display.
The timezone warning section properly implements the new control flow syntax and maintains good accessibility with semantic icon usage.
Line range hint 43-51: LGTM! Well-structured validation feedback.
The validation messages section properly implements the new control flow syntax and provides clear user feedback with appropriate translations and visual indicators.
Line range hint 1-51: Verify translation key availability.
Please ensure all translation keys are defined in the translation files:
- 'entity.timeZoneWarning'
- 'entity.dateMissingOrNotValid'
- 'entity.warningError'
- 'entity.warningToolTip'
✅ Verification successful
Let me check the actual content of these files to verify if all the required translation keys are present.
All required translation keys are present and properly defined
All translation keys used in the date-time-picker component are correctly defined in both English and German translation files:
- 'entity.timeZoneWarning' with timezone placeholder
- 'entity.dateMissingOrNotValid' with labelName placeholder
- 'entity.warningError'
- 'entity.warningToolTip'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for translation keys in i18n files
echo "Checking for translation keys..."
rg -l "timeZoneWarning|dateMissingOrNotValid|warningError|warningToolTip" src/main/webapp/i18n/
Length of output: 244
Script:
#!/bin/bash
# Check the content of translation files for specific keys
echo "=== English translations ==="
rg "timeZoneWarning|dateMissingOrNotValid|warningError|warningToolTip" src/main/webapp/i18n/en/global.json -A 1
echo -e "\n=== German translations ==="
rg "timeZoneWarning|dateMissingOrNotValid|warningError|warningToolTip" src/main/webapp/i18n/de/global.json -A 1
Length of output: 1646
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.html (3)
Line range hint 4-11: LGTM! Component bindings are well-structured.
The lecture unit management component implementation follows Angular best practices with clear input/output bindings.
12-15: LGTM! Proper usage of new Angular control flow syntax.
The code correctly uses the new @if syntax as recommended in the coding guidelines, and the condition check aligns with the signal-based approach.
2-2: Verify header hierarchy in the page structure.
The change from h1 to h2 affects the document outline. Ensure this aligns with the page's semantic structure and there's a parent h1 element.
✅ Verification successful
Header hierarchy is correctly structured
The change from h1 to h2 in the units component is appropriate as it follows the correct semantic hierarchy. The main title component (lecture-wizard-title) contains the page's h1 heading, while other wizard steps, including the units component, properly use h2 for their section headings. This structure maintains a clear and semantically correct document outline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for h1 elements in parent components
# Expected: Should find h1 elements in parent layout/components
# Search for h1 elements in parent components
rg -l '<h1' src/main/webapp/app/lecture/
Length of output: 277
Script:
#!/bin/bash
# Let's check the content of these files to understand the header structure
# and their relationship with the units component
# Check lecture-wizard-title.component.html which likely contains the main title
cat src/main/webapp/app/lecture/wizard-mode/lecture-wizard-title.component.html
# Also check if there's a parent component that might organize these wizard components
rg -l "lecture-wizard" src/main/webapp/app/lecture/
Length of output: 1206
Script:
#!/bin/bash
# Let's check the parent wizard component to understand the overall structure
cat src/main/webapp/app/lecture/wizard-mode/lecture-update-wizard.component.html
Length of output: 3588
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts (2)
86-98: Consistent State Management with Signals
The use of signals to manage the visibility states of lecture unit forms enhances reactivity and maintainability. Setting the form visibility with .set() improves clarity and aligns with modern Angular practices.
Also applies to: 273-277
103-105: 🛠️ Refactor suggestion
Avoid Using Methods in Templates
According to the coding guidelines (methods_in_html:false), methods should not be used directly in templates because they can degrade performance due to repeated invocations during change detection. The isAnyUnitFormOpen is a computed function and would be invoked as a method in the template.
Consider converting isAnyUnitFormOpen into a signal or using a getter property instead. This allows it to be used in the template without invoking it as a function:
- isAnyUnitFormOpen = computed(() => {
- return this.isTextUnitFormOpen() || this.isVideoUnitFormOpen() || this.isOnlineUnitFormOpen() || this.isAttachmentUnitFormOpen() || this.isExerciseUnitFormOpen();
- });
+ get isAnyUnitFormOpen() {
+ return this.isTextUnitFormOpen() || this.isVideoUnitFormOpen() || this.isOnlineUnitFormOpen() || this.isAttachmentUnitFormOpen() || this.isExerciseUnitFormOpen();
+ }Likely invalid or redundant comment.
src/main/webapp/app/shared/date-time-picker/date-time-picker.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts
Outdated
Show resolved
Hide resolved
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts (2)
36-40: Consider grouping related signals using a state objectWhile the migration to signals is correct, consider grouping these related form visibility signals into a single state object for better maintainability.
- isTextUnitFormOpen = signal<boolean>(false); - isExerciseUnitFormOpen = signal<boolean>(false); - isVideoUnitFormOpen = signal<boolean>(false); - isOnlineUnitFormOpen = signal<boolean>(false); - isAttachmentUnitFormOpen = signal<boolean>(false); + formVisibility = signal<Record<LectureUnitType, boolean>>({ + [LectureUnitType.TEXT]: false, + [LectureUnitType.EXERCISE]: false, + [LectureUnitType.VIDEO]: false, + [LectureUnitType.ONLINE]: false, + [LectureUnitType.ATTACHMENT]: false, + });
91-93: Simplify computed property with Object methodsThe computed property can be simplified by leveraging the suggested state object:
- isAnyUnitFormOpen = computed(() => { - return this.isTextUnitFormOpen() || this.isVideoUnitFormOpen() || this.isOnlineUnitFormOpen() || this.isAttachmentUnitFormOpen() || this.isExerciseUnitFormOpen(); - }); + isAnyUnitFormOpen = computed(() => + Object.values(this.formVisibility()).some(isOpen => isOpen) + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-period.component.ts(0 hunks)src/main/webapp/app/lecture/wizard-mode/lecture-wizard-title.component.ts(0 hunks)src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts(4 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/lecture/wizard-mode/lecture-wizard-period.component.ts
- src/main/webapp/app/lecture/wizard-mode/lecture-wizard-title.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts (1)
1-1: LGTM: Clean import of signal primitives
The imports have been correctly updated to include Angular's reactive primitives (computed and signal).
src/main/webapp/app/lecture/wizard-mode/lecture-wizard-units.component.ts
Show resolved
Hide resolved
florian-glombik
added
code quality
refactoring
lecture
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/lecture/lecture-attachments.component.html (2)
Line range hint
102-113: Consider adding loading state for delete action.The delete button's disabled state only considers the edit operation (
attachmentToBeCreated?.id === attachment?.id). Consider adding a loading state during deletion to prevent multiple delete attempts on the same attachment.@if (lecture().isAtLeastInstructor) { <button type="button" - [disabled]="attachmentToBeCreated?.id === attachment?.id" + [disabled]="attachmentToBeCreated?.id === attachment?.id || isDeletingAttachment === attachment.id" jhiDeleteButton [entityTitle]="attachment?.name || ''" deleteQuestion="artemisApp.attachment.deleteQuestion" (delete)="deleteAttachment(attachment)" [dialogError]="dialogError$" > - <fa-icon [icon]="faTrash" /> + <fa-icon [icon]="isDeletingAttachment === attachment.id ? faSpinner : faTrash" [spin]="isDeletingAttachment === attachment.id" /> </button> }
201-207: Consider enhancing accessibility for form actions.The save and cancel buttons could benefit from ARIA labels to improve screen reader support, especially when icons are used.
-<button id="upload-attachment" type="button" [disabled]="!isSubmitPossible" (click)="saveAttachment()" class="btn btn-primary me-2"> +<button id="upload-attachment" type="button" [disabled]="!isSubmitPossible" (click)="saveAttachment()" class="btn btn-primary me-2" [attr.aria-label]="'entity.action.saveAttachment' | artemisTranslate"> <fa-icon [icon]="faPaperclip" /> <span jhiTranslate="entity.action.saveAttachment"></span> </button> -<button type="button" (click)="cancel()" class="btn btn-default"> +<button type="button" (click)="cancel()" class="btn btn-default" [attr.aria-label]="'entity.action.cancel' | artemisTranslate"> <fa-icon [icon]="faTimes" /> <span jhiTranslate="entity.action.cancel"></span> </button>src/main/webapp/app/lecture/lecture-attachments.component.ts (1)
31-34: Consider using constructor injection for dependenciesCurrently, dependencies are injected using the
inject()function. While this is acceptable in Angular 14 and above, the Angular Style Guide recommends using constructor injection for better readability and testability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/lecture/lecture-attachments.component.html(6 hunks)src/main/webapp/app/lecture/lecture-attachments.component.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/lecture/lecture-attachments.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/lecture/lecture-attachments.component.ts (1)
🔇 Additional comments (6)
src/main/webapp/app/lecture/lecture-attachments.component.html (2)
Line range hint 3-37: LGTM! Header section follows new Angular syntax and best practices.
The code correctly uses the new @if syntax and properly accesses lecture data through function calls. The semantic structure and data presentation are well-organized.
95-95: LGTM! Button type changes improve form handling.
The change from type="submit" to type="button" prevents unintended form submissions, which is a good practice.
Also applies to: 104-104
src/main/webapp/app/lecture/lecture-attachments.component.ts (4)
37-38: Verify the use of input for defining component inputs
The use of input.required<number>() and input<boolean>(true) is a new pattern introduced in Angular 16 for reactive inputs. Ensure that this aligns with the project's coding guidelines and that all team members are familiar with this syntax.
97-99: Ensure correct usage of signals when accessing lecture in addAttachment
In the addAttachment method, you're accessing the lecture using this.lecture(). Verify that this returns the expected lecture object when creating a new attachment.
70-70: Confirm necessity of allowSignalWrites: true in effect options
The allowSignalWrites: true option allows writing to signals within the effect callback. Ensure that this is necessary and that unintended side effects are avoided.
1-1: Verify project compatibility with Angular 16 features
The imports include signal, effect, inject, and input, which are features introduced in Angular 16. Confirm that the project is using Angular 16 or later, and that all team members are aware of the update.
github-actions
bot
added
tests
and removed
lecture
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/lecture/wizard-mode/lecture-wizard-units.component.spec.ts (2)
842-842: Verify signal-based state management in initialization testsThe test correctly verifies the exercise form state using the function call pattern. However, we should also verify that the state transitions correctly after initialization.
Consider adding a test to verify the state transition:
it('should correctly transition exercise form state after initialization', fakeAsync(() => { const route = TestBed.inject(ActivatedRoute); route.queryParams = of({ shouldOpenCreateExercise: true }); wizardUnitComponentFixture.detectChanges(); tick(); expect(wizardUnitComponent.isExerciseUnitFormOpen()).toBeTrue(); wizardUnitComponent.onCloseLectureUnitForms(); tick(); expect(wizardUnitComponent.isExerciseUnitFormOpen()).toBeFalse(); }));
Line range hint
1-842: Enhance test coverage for signal-based state managementWhile the tests cover the basic functionality, they could be enhanced to better test the reactive nature of signals.
Consider adding tests for:
- Signal effect triggers when form states change
- Computed property updates when dependent signals change
- Signal-based error state management
- Race conditions in async operations with signals
Example test:
it('should trigger effects when form states change', fakeAsync(() => { const effectSpy = jest.fn(); const cleanup = effect(() => { effectSpy(wizardUnitComponent.isVideoUnitFormOpen()); }); wizardUnitComponentFixture.detectChanges(); tick(); wizardUnitComponent.isVideoUnitFormOpen.set(true); tick(); expect(effectSpy).toHaveBeenCalledWith(true); cleanup(); // Clean up the effect }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/lecture/wizard-mode/lecture-wizard-units.component.spec.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/lecture/wizard-mode/lecture-wizard-units.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}}
🔇 Additional comments (1)
src/test/javascript/spec/component/lecture/wizard-mode/lecture-wizard-units.component.spec.ts (1)
197-197: Consistent use of signal's set method for form state management
The test cases correctly use the signal's set method to update form states. This aligns with Angular's new signal-based reactive programming model.
Also applies to: 280-280, 437-437, 541-541, 610-610
src/test/javascript/spec/component/lecture/wizard-mode/lecture-wizard-units.component.spec.ts
Show resolved
Hide resolved
florian-glombik
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
…ard-units-component
github-actions
bot
removed
the
lecture
krusche
deleted the
chore/lectures/use-signals-in-lecture-wizard-units-component
branch
Checklist
General
Client
Motivation and Context
This PR introduces signals, and fixes code smells in components used for editing and updating lectures. This is a preparation for #9655 and aims to keep #9655 at a manageable size of changes.
Description
lecture-attachments.component.ts(also refined the usage of lifecycle hooks there)lecture-wizard-units.component.tsSteps 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
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores