Development: Use signals in lecture online and text unit
#9658
Conversation
github-actions
bot
added
the
client
WalkthroughThe pull request introduces several updates across multiple components in an Angular application, focusing on enhancing form validation and state management. Key changes include the renaming of validation properties, the introduction of new input properties with required decorators, and the adoption of Angular's reactive programming features. The HTML templates for form components have been adjusted to utilize a new method for determining form validity. Additionally, modifications to access levels of properties in various components have been made to improve encapsulation. 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
|
florian-glombik
changed the title
Development: Use signals in online unit form componentDevelopment: Use signals in lecture online unit form component
florian-glombik
changed the title
Development: Use signals in lecture online unit form componentDevelopment: Use signals in lecture online and text unit
.../lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.ts
Outdated
Show resolved
Hide resolved
.../app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.ts
Outdated
Show resolved
Hide resolved
florian-glombik
added
code quality
refactoring
lecture
github-actions
bot
removed
the
lecture
florian-glombik
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (11)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.ts (3)
50-59: Consider enhancing type safety for form controlsWhile the form initialization is clean, consider using typed form controls for better type safety and IDE support.
Here's how you could improve it:
interface OnlineUnitForm { name: FormControl<string | undefined>; description: FormControl<string | undefined>; releaseDate: FormControl<dayjs.Dayjs | undefined>; source: FormControl<string | undefined>; competencyLinks: FormControl<CompetencyLectureUnitLink[] | undefined>; } form = this.formBuilder.group<OnlineUnitForm>({ name: this.formBuilder.control(undefined, [Validators.required, Validators.maxLength(255)]), description: this.formBuilder.control(undefined, [Validators.maxLength(1000)]), releaseDate: this.formBuilder.control(undefined), source: this.formBuilder.control(undefined, [Validators.required, this.urlValidator]), competencyLinks: this.formBuilder.control(undefined), });
61-62: Consider adding initial value and error handling to signalsThe signal implementation could be more robust with explicit initial values and error handling.
Here's an improved version:
private readonly statusChanges = toSignal(this.form.statusChanges, { initialValue: 'INVALID' as const, requireSync: true }); protected readonly isFormValid = computed(() => { const status = this.statusChanges(); return status === 'VALID' && this.form.dirty; });This ensures:
- Type safety with
as const- Explicit initial value
- Form must be dirty to be considered valid
Line range hint
89-108: Prevent potential memory leaks in HTTP subscriptionThe HTTP subscription in
onLinkChangedshould be properly managed to prevent memory leaks.Consider using the
takeUntilDestroyedoperator or implementing proper cleanup:import { DestroyRef, inject } from '@angular/core'; import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; export class OnlineUnitFormComponent implements OnChanges { private readonly destroyRef = inject(DestroyRef); onLinkChanged(): void { // ... existing code ... if (this.sourceControl?.valid) { this.onlineUnitService .getOnlineResource(this.sourceControl.value) .pipe( takeUntilDestroyed(this.destroyRef), map((response: HttpResponse<OnlineResourceDTO>) => response.body!) ) .subscribe({ next: (onlineResource) => { const updateForm = { name: onlineResource.title || undefined, description: onlineResource.description || undefined, }; this.form.patchValue(updateForm); }, }); } } }src/main/webapp/app/shared/form/title-channel-name/title-channel-name.component.ts (1)
32-39: Consider adding migration guidance for deprecated properties.While the deprecation notices are helpful, consider adding migration examples in the JSDoc comments to guide developers on how to transition from the deprecated properties to the new signal-based approach.
Apply this enhancement to the deprecation notices:
/** - * @deprecated Use {@link isFormValidSignal} instead. + * @deprecated Use {@link isFormValidSignal} instead. + * @example + * // Before + * this.formValid + * // After + * this.isFormValidSignal() */ formValid: boolean;src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.html (1)
Signal implementation is partially complete but can be enhanced
The component already uses signals through:
toSignal()for form status changescomputed()for form validation stateHowever, the implementation could be improved by:
- Converting form control getters to signals using
toSignal()for better reactivity- Using
effect()to handle side effects inonLinkChanged()- Adding cleanup for the form status signal subscription
Example refactoring for the control getters:
nameControl = toSignal(this.form.get('name')!.valueChanges);🔗 Analysis chain
Line range hint
2-78: Verify signal implementation in the component.To ensure the PR's objective of introducing signals is fully met, please verify that:
- Form controls are exposed as signals in the component
- Form validation state is derived from these signals
- The component properly cleans up signal subscriptions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify signal implementation in the component # Test: Check if signals are properly implemented in the component ast-grep --pattern 'class OnlineUnitFormComponent { $$$ signal<$_>($_) { $$$ } $$$ }' # Test: Check for proper signal cleanup rg -A 5 'ngOnDestroy|effect' src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.tsLength of output: 366
Script:
#!/bin/bash # Let's try a different approach to check the component implementation # First, let's check the content of the component file cat src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.ts # As a backup, let's also search for signal-related imports and usage rg -l "@angular/core.*signal" src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/ # And check for FormControl-related code rg "FormControl|form\." src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.tsLength of output: 5045
src/main/webapp/app/lecture/lecture-update.component.ts (1)
30-33: LGTM! Well-structured file extension handling with clear separation of concerns.The implementation cleanly separates human-readable and technical formats. The comments are helpful but could be even more descriptive.
Consider enhancing the comments to be more specific:
- // A human-readable list of allowed file extensions + // Comma-separated list of allowed file extensions for display purposes (e.g., "pdf, txt, doc") - // The list of file extensions for the "accept" attribute of the file input field + // Formatted list of file extensions for HTML file input's "accept" attribute (e.g., ".pdf,.txt,.doc")src/main/webapp/app/exercises/programming/manage/update/update-components/information/programming-exercise-information.component.ts (2)
Line range hint
164-171: LGTM! Consider refactoring for improved readability.The validation logic is comprehensive and the rename to
isFormValidSignal()improves clarity. However, the boolean expression could be more readable.Consider breaking down the validation checks into a separate method:
- this.formValid = Boolean( - this.exerciseTitleChannelComponent()?.titleChannelNameComponent?.isFormValidSignal() && - this.getIsShortNameFieldValid() && - isCheckoutSolutionRepositoryValid && - isRecreateBuildPlansValid && - isUpdateTemplateFilesValid && - areAuxiliaryRepositoriesValid && - areCheckoutPathsValid, - ); + this.formValid = this.validateAllFormFields( + isCheckoutSolutionRepositoryValid, + isRecreateBuildPlansValid, + isUpdateTemplateFilesValid, + areAuxiliaryRepositoriesValid, + areCheckoutPathsValid, + ); + + private validateAllFormFields( + isCheckoutSolutionRepositoryValid: boolean, + isRecreateBuildPlansValid: boolean, + isUpdateTemplateFilesValid: boolean, + areAuxiliaryRepositoriesValid: boolean, + areCheckoutPathsValid: boolean, + ): boolean { + return Boolean( + this.exerciseTitleChannelComponent()?.titleChannelNameComponent?.isFormValidSignal() && + this.getIsShortNameFieldValid() && + isCheckoutSolutionRepositoryValid && + isRecreateBuildPlansValid && + isUpdateTemplateFilesValid && + areAuxiliaryRepositoriesValid && + areCheckoutPathsValid, + ); + }
Line range hint
89-98: Consider improving subscription management.While the current implementation handles cleanup, it could be enhanced using RxJS operators to prevent potential memory leaks.
Consider using the
takeUntilDestroyedoperator from@angular/core/rxjs-interop:+import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; - inputFieldSubscriptions: (Subscription | undefined)[] = []; + constructor() { + this.exerciseTitleChannelComponent()?.titleChannelNameComponent?.formValidChanges + .pipe(takeUntilDestroyed()) + .subscribe(() => this.calculateFormValid()); + + // Apply similar pattern to other subscriptions + } - ngOnDestroy(): void { - for (const subscription of this.inputFieldSubscriptions) { - subscription?.unsubscribe(); - } - }src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.ts (3)
27-33: Maintain consistent ordering and grouping of@Inputand@OutputpropertiesTo enhance code readability and maintainability, consider grouping all
@Inputproperties together, followed by@Outputproperties, and ordering them consistently.Apply this diff to reorder the properties:
@Input() formData: TextUnitFormData; @Input() isEditMode = false; +@Input() hasCancelButton: boolean; @Output() formSubmitted: EventEmitter<TextUnitFormData> = new EventEmitter<TextUnitFormData>(); +@Output() onCancel: EventEmitter<any> = new EventEmitter<any>();This aligns with the project's convention of grouping inputs and outputs, enhancing code consistency across the application.
40-41: Align dependency injection methods for consistencyCurrently,
FormBuilderis injected using theinject()function, whileRouterandTranslateServiceare injected via the constructor. For consistency, consider using the same injection method for all dependencies.Option 1: Use
inject()for all services:+private readonly router = inject(Router); +private readonly translateService = inject(TranslateService); +private readonly formBuilder = inject(FormBuilder); -constructor( - private router: Router, - private translateService: TranslateService, -) {}Option 2: Inject
FormBuildervia the constructor:-private readonly formBuilder = inject(FormBuilder); constructor( private router: Router, private translateService: TranslateService, + private formBuilder: FormBuilder, ) {}Consistent injection patterns improve code readability and reduce potential confusion.
42-46: Initialize form controls with default values appropriatelyInitializing form controls with
undefinedas the default value is acceptable. However, consider whether default values are necessary or if they could be omitted to simplify the code.For example:
form: FormGroup = this.formBuilder.group({ - name: [undefined as string | undefined, [Validators.required, Validators.maxLength(255)]], + name: ['', [Validators.required, Validators.maxLength(255)]], - releaseDate: [undefined as dayjs.Dayjs | undefined], + releaseDate: [null as dayjs.Dayjs | null], - competencyLinks: [undefined as CompetencyLectureUnitLink[] | undefined], + competencyLinks: [[] as CompetencyLectureUnitLink[]], });This can help avoid potential issues with form controls expecting certain types.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/main/webapp/app/exercises/programming/manage/update/update-components/information/programming-exercise-information.component.ts(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.html(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.ts(2 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.html(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.ts(3 hunks)src/main/webapp/app/lecture/lecture-update.component.ts(2 hunks)src/main/webapp/app/shared/form/title-channel-name/title-channel-name.component.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/main/webapp/app/exercises/programming/manage/update/update-components/information/programming-exercise-information.component.ts (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.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-unit/lecture-unit-management/online-unit-form/online-unit-form.component.ts (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.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-unit/lecture-unit-management/text-unit-form/text-unit-form.component.ts (1)
src/main/webapp/app/lecture/lecture-update.component.ts (1)
src/main/webapp/app/shared/form/title-channel-name/title-channel-name.component.ts (1)
📓 Learnings (3)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.html (1)
Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-02T22:57:57.717Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.html (1)
Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-02T22:57:57.717Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.ts (1)
Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2024-11-02T22:57:57.717Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.
🔇 Additional comments (10)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.html (1)
Line range hint 3-3: LGTM: Proper usage of new Angular control flow syntax.
The template correctly implements the new @if syntax instead of the older *ngIf, which aligns with the coding guidelines.
Also applies to: 8-15
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.ts (1)
2-2: LGTM: Modern Angular features adoption
The addition of computed, inject, and toSignal imports aligns with modern Angular best practices and the PR's objective to introduce signals.
Also applies to: 10-10
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.html (1)
Line range hint 2-2: Excellent use of new Angular syntax!
The template consistently uses the new @if syntax instead of *ngIf throughout the file, which aligns perfectly with the coding guidelines.
Also applies to: 10-10, 13-13, 16-16, 26-26, 33-33, 36-36, 46-46, 49-49, 79-79
src/main/webapp/app/lecture/lecture-update.component.ts (3)
24-29: LGTM! Property access modifiers enhance encapsulation.
The changes to make icon properties and documentationType protected readonly improve encapsulation while maintaining inheritance capability. This aligns with Angular's style guide and supports the PR's goal of template optimization.
89-90: LGTM! Improved documentation clarity.
The updated comments clearly explain the navigation behavior based on the lecture's state, which helps maintainability.
24-33: Verify template usage of the protected properties.
The properties have been made protected. Let's verify that all template references have been updated accordingly.
✅ Verification successful
Template references to protected properties are correctly used
The verification shows that all template references to the protected properties are valid and working as expected. The properties are accessed in the component templates where they are defined:
lecture-update.component.htmlcorrectly uses all the protected properties defined in its component- Other components (
lecture-attachments.component,lecture-unit-management, etc.) that show similar property usage have their own component-level declarations
This confirms that making these properties protected in lecture-update.component.ts doesn't break any template references, as they are properly scoped and accessed within their respective components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check template usage of the protected properties
# Expected: No direct references to these properties in templates
# Search for template references to the protected properties
echo "Checking template references..."
rg -t html "documentationType|faQuestionCircle|faSave|faPuzzleProcess|faBan|faHandShakeAngle|allowedFileExtensions|acceptedFileExtensionsFileBrowser" "src/main/webapp/app/lecture"
Length of output: 3814
src/main/webapp/app/exercises/programming/manage/update/update-components/information/programming-exercise-information.component.ts (2)
Line range hint 39-42: Well-implemented reactive state management using signals!
The implementation follows Angular's best practices for reactive programming with signals. The state management is clean and efficient.
Also applies to: 52-55
Line range hint 45-52: Excellent use of required input decorators!
The implementation follows Angular's latest best practices for input properties, improving type safety and runtime validation.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.ts (2)
1-1: Good adoption of Angular's inject and computed functions
The inclusion of inject and computed from @angular/core aligns with Angular's move towards more streamlined dependency injection and reactive programming patterns.
25-25: Consistent visibility modifier for template-bound properties
Defining faTimes as protected ensures it is accessible in the template while encapsulating it from external classes. This aligns with Angular's best practices for component property visibility.
...pp/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.html
Show resolved
Hide resolved
.../lecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/shared/form/title-channel-name/title-channel-name.component.ts
Show resolved
Hide resolved
src/main/webapp/app/shared/form/title-channel-name/title-channel-name.component.ts
Show resolved
Hide resolved
...ecture/lecture-unit/lecture-unit-management/online-unit-form/online-unit-form.component.html
Show resolved
Hide resolved
.../app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.ts
Show resolved
Hide resolved
.../app/lecture/lecture-unit/lecture-unit-management/text-unit-form/text-unit-form.component.ts
Show resolved
Hide resolved
JanaNF
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
|
Thanks for testing and the detailed information! @BBesrour #9658 (review) As the issues do not seem to be introduced with this PR, I will not address them. I also do not think that creating issues makes much sense as #9655 aims at removing the guided mode in the near future anyways. I will keep them in mind and make sure the same issues do not occur for the new "status bar" edit mode :) |
florian-glombik
dismissed
BBesrour’s stale review
Out of scope, will not fix found issues in this PR
Haoyuli2002
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS3 as Instructor 17.
I have create a text lecture unit and an online lecture unit. Then I editted these units and they are working as expected.
I also created a Lecture in the guided mode, add the dates, an Online Lecture Unit and a Text Lecture Unit. This function works fine as well.
The only small issue I noticed is that:
- If I used the Markdown Language in the Text Unit, it will be displayed properly. But if I then clicked the "View Isolated", it will redirect me to a page where the Markdown Language got some problems.
- You may check the screenshots below.
Other than that, I think everything is working well!
|
#9658 (review) As far as I am aware this is the expected behavior for the isolated view |
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
florian-glombik
added
the
lecture
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 Lectures: Add status bar to lecture creation and edit mode #9655 and aims to keep #9655 at a manageable size of changes.
Description
online-unit-form.component(got rid of a method call in the template)text-unit-form.component(got rid of a method call in the template)Steps for Testing
Prerequisites:
Units>Textto create a text lecture unit, verify the behavior is the same as on developUnits>Onlineto create an online lecture unit, verify the behavior is the same as on developThere are known issues with the guided mode that will not be addressed in this PR #9658 (review), see reasoning #9658 (comment)
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
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
isFormValid()for determining form submission eligibility.Bug Fixes
Refactor
Documentation
previousStatemethod.