-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adaptive learning
: Unlink edited competencies from standardised competencies
#9396
Adaptive learning
: Unlink edited competencies from standardised competencies
#9396
Conversation
WalkthroughThe pull request introduces modifications primarily to the competency management system within the application. Key changes include setting 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range comments (2)
src/main/webapp/app/course/competencies/forms/competency/competency-form.component.ts (1)
Line range hint
1-68
: Suggestions for improved adherence to coding guidelinesThe component structure generally adheres to the coding guidelines. However, consider the following suggestions:
- Verify that meaningful tests exist for this component, as per the guideline "tests:meaningful".
- Ensure that the component is using single quotes for strings in other parts of the file, as per "strings:single_quotes".
- Confirm that the component is part of a proper routing structure with a consistent naming schema, as per "routes:naming_schema".
- Verify that any subscriptions (if present in methods not shown here) are properly managed to prevent memory leaks.
To verify the use of single quotes for strings and proper subscription management, you can run the following script:
#!/bin/bash # Description: Check for double quotes and subscription management in CompetencyFormComponent # Check for double quotes echo "Occurrences of double quotes in CompetencyFormComponent:" rg --type typescript '"' src/main/webapp/app/course/competencies/forms/competency/competency-form.component.ts # Check for subscription management echo "\nSubscription management in CompetencyFormComponent:" rg --type typescript -i '(subscribe|subscription)' src/main/webapp/app/course/competencies/forms/competency/competency-form.component.tsPlease review the results and make necessary adjustments to ensure compliance with the coding guidelines.
src/main/webapp/i18n/de/competency.json (1)
Line range hint
265-265
: LGTM! New translation added for consistency.The new translation for "deleted" is accurate and maintains the informal tone (du-form). This addition improves consistency by providing messages for all CRUD operations on prerequisites.
For even better consistency, consider updating the message to match the style of the existing "created" and "updated" messages:
- "deleted": "Voraussetzung gelöscht", + "deleted": "Voraussetzungen gelöscht",This change would make all three messages use the plural form, maintaining a consistent style across CRUD operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (9)
- src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (1 hunks)
- src/main/webapp/app/course/competencies/edit/edit-competency.component.html (1 hunks)
- src/main/webapp/app/course/competencies/forms/competency/competency-form.component.html (1 hunks)
- src/main/webapp/app/course/competencies/forms/competency/competency-form.component.ts (1 hunks)
- src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts (3 hunks)
- src/main/webapp/app/course/competencies/forms/prerequisite/prerequisite-form.component.html (1 hunks)
- src/main/webapp/app/course/competencies/forms/prerequisite/prerequisite-form.component.ts (1 hunks)
- src/main/webapp/i18n/de/competency.json (1 hunks)
- src/main/webapp/i18n/en/competency.json (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/webapp/app/course/competencies/edit/edit-competency.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/course/competencies/forms/competency/competency-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/course/competencies/forms/competency/competency-form.component.ts (1)
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts (1)
src/main/webapp/app/course/competencies/forms/prerequisite/prerequisite-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/course/competencies/forms/prerequisite/prerequisite-form.component.ts (1)
src/main/webapp/i18n/de/competency.json (1)
Pattern
src/main/webapp/i18n/de/**/*.json
: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
🪛 Biome
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts
[error] 126-126: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (12)
src/main/webapp/app/course/competencies/edit/edit-competency.component.html (1)
Line range hint
1-7
: LGTM: Correct usage of @if directiveThe template correctly uses the new Angular syntax
@if
for conditional rendering, which aligns with the provided coding guidelines. This is a good practice and improves readability.src/main/webapp/app/course/competencies/forms/competency/competency-form.component.html (2)
23-23
: LGTM! Consistent use of method call and @if directive.The update to use
hasCancelButton()
is consistent with the earlier changes and follows the coding guidelines for using @if.
17-19
: LGTM! Verify translation key for the warning message.The new conditional block using @if aligns with the coding guidelines and implements the warning message as described in the PR objectives.
Please run the following script to verify the translation key:
src/main/webapp/app/course/competencies/forms/prerequisite/prerequisite-form.component.html (4)
23-23
: LGTM! Consistent with other changesThe update to use
hasCancelButton()
method call is consistent with the other changes in the file and aligns with the coding guidelines for using @if.
Line range hint
1-33
: Summary: Changes align well with PR objectivesThe modifications in this file effectively implement the PR's main objective of unlinking edited competencies from standardized competencies. Key points:
- Dynamic behavior: Property accesses changed to method calls for more flexibility.
- User feedback: New alert added to warn about unlinking standardized competencies.
- Consistent updates: All changes follow a similar pattern, including the cancel button condition.
- Coding guidelines: Use of @if aligns with the provided guidelines.
These changes enhance the user experience by providing clear warnings and should make the competency management more robust. However, it's important to verify that the increased use of method calls doesn't negatively impact performance.
17-19
: LGTM! Verify implementation of hasLinkedStandardizedCompetency()The new conditional alert using @if syntax aligns with both the coding guidelines and the PR objectives. It provides a clear warning to users about unlinking standardized competencies when editing.
To verify the implementation of the new method, run the following script:
#!/bin/bash # Description: Verify the implementation of hasLinkedStandardizedCompetency() # Test: Search for the method implementation rg --type typescript -g 'src/main/webapp/app/course/competencies/**/*.ts' 'hasLinkedStandardizedCompetency\(\s*\)\s*{' -A 10 # Analyze the results to ensure the method correctly determines # if there are linked standardized competencies.
7-11
: LGTM! Consider verifying performance impact.The changes from property access to method calls align with the PR objectives and provide more dynamic behavior. However, it's important to ensure that these method calls don't negatively impact performance, especially during change detection cycles.
To verify the performance impact, you can run the following script:
src/main/webapp/app/course/competencies/forms/prerequisite/prerequisite-form.component.ts (1)
Line range hint
1-68
: File review summary: Minor improvement, but relevance to PR objectives unclear.The change in this file improves code quality by updating a property access to a method call, which is more in line with Angular best practices. However, it's not immediately clear how this change contributes to the PR's main objective of unlinking edited competencies from standardized competencies.
Consider adding a comment explaining the rationale behind this change and its relation to the PR's goals. Additionally, ensure that this change is consistent across the entire codebase and doesn't introduce any performance issues.
To verify the overall impact of changes related to competency unlinking, please run the following script:
#!/bin/bash # Description: Search for code related to competency unlinking echo "Files potentially related to competency unlinking:" rg --type typescript -l '(competency|prerequisite).*\b(unlink|standardized|edit)' echo "\nRelevant function calls or property accesses:" rg --type typescript '(competency|prerequisite).*\.(unlink|isStandardized|edit)'This script will help identify other relevant parts of the codebase that might be involved in the competency unlinking process, which could provide context for the changes made in this file.
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts (1)
1-1
: LGTM: Updated import for new input syntax.The addition of
input
to the import statement from '@angular/core' is correct and aligns with the latest Angular practices for declaring input properties.src/main/webapp/i18n/de/competency.json (2)
209-210
: LGTM! New translations added correctly.The new translations for "noOfParticipants" and "participationRate" are accurate and maintain the informal tone (du-form) as required by the coding guidelines.
214-214
: LGTM! New warning message added as per PR objectives.The new translation for "unlinkStandardizedCompetenciesWarning" is accurate, maintains the informal tone (du-form), and directly addresses the main objective of the PR to warn users about unlinking edited competencies from standardized competencies.
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (1)
362-362
: Verify the impact of unlinking standardized competencies.While the change correctly implements the unlinking of edited competencies, it's important to ensure that this doesn't negatively impact other parts of the system that might rely on the
linkedStandardizedCompetency
being set.Please run the following script to check for any other usages of
linkedStandardizedCompetency
that might be affected by this change:This will help identify any areas of the codebase that might need adjustment due to this change.
src/main/webapp/app/course/competencies/edit/edit-competency.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/course/competencies/forms/competency/competency-form.component.html
Show resolved
Hide resolved
src/main/webapp/app/course/competencies/forms/competency/competency-form.component.ts
Show resolved
Hide resolved
src/main/webapp/app/course/competencies/forms/prerequisite/prerequisite-form.component.ts
Show resolved
Hide resolved
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall Code 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the failing client tests. You can set the inputs using
fixture.componentRef.setInput('attributeName', value);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range comments (3)
src/test/javascript/spec/component/competencies/competency-form.component.spec.ts (2)
Line range hint
108-128
: LGTM! Consider adding expectations for taxonomy.The changes look good. Using
setInput
is the correct way to set input properties in Angular tests. The test covers various form controls and ensures they are set correctly.Consider adding an expectation for the
taxonomy
form control to ensure it's set correctly:expect(competencyFormComponent.taxonomyControl?.value).toEqual(formData.taxonomy);
Line range hint
174-191
: LGTM! Consider adding a comment for the 250ms delay.The test cases for title validation look comprehensive and well-structured. They cover important scenarios including initial state, unique title, empty title, and non-unique title.
Consider adding a brief comment explaining the 250ms delay used in
tick(250)
. This would improve clarity for other developers:// Wait for 250ms to allow the asynchronous validator to complete tick(250);src/test/javascript/spec/component/competencies/prerequisite-form.component.spec.ts (1)
Line range hint
58-107
: LGTM: Improved form submission testThe test case for form submission has been significantly enhanced:
- It now includes mocking of the prerequisite service for asynchronous validation.
- The form submission scenario is more comprehensive, including setting form values and selecting lecture units.
- Expectations for form validity and service calls have been added.
These improvements align well with the PR objectives and provide more robust testing of the form submission process.
Consider adding an expectation to verify that the competency is unlinked after submission, as this is a key objective of the PR.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (6)
- src/main/java/de/tum/cit/aet/artemis/core/config/LoggingAspect.java (1 hunks)
- src/main/webapp/app/course/competencies/edit/edit-competency.component.html (1 hunks)
- src/main/webapp/app/course/competencies/edit/edit-prerequisite.component.html (1 hunks)
- src/main/webapp/i18n/de/competency.json (1 hunks)
- src/test/javascript/spec/component/competencies/competency-form.component.spec.ts (2 hunks)
- src/test/javascript/spec/component/competencies/prerequisite-form.component.spec.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/main/java/de/tum/cit/aet/artemis/core/config/LoggingAspect.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/webapp/app/course/competencies/edit/edit-competency.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/course/competencies/edit/edit-prerequisite.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/i18n/de/competency.json (1)
Pattern
src/main/webapp/i18n/de/**/*.json
: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".src/test/javascript/spec/component/competencies/competency-form.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/competencies/prerequisite-form.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 (11)
src/main/webapp/app/course/competencies/edit/edit-prerequisite.component.html (2)
Line range hint
1-7
: Correct usage of new Angular syntax.The @if and @else directives are correctly implemented for conditional rendering, replacing the older *ngIf syntax. This adheres to the coding guidelines and improves code readability.
Also applies to: 19-20
Line range hint
1-20
: Changes align well with PR objectives and coding guidelines.The modifications to this file effectively implement the requirement for handling linked standardized competencies. The use of new Angular syntax (@if/@else) and the addition of the
hasLinkedStandardizedCompetency
input property contribute to achieving the PR's goal of unlinking edited competencies from standardized competencies.The implementation appears to be correct and adheres to the project's coding guidelines. It sets the foundation for displaying warnings and handling the unlinking process as described in the PR objectives.
src/test/javascript/spec/component/competencies/competency-form.component.spec.ts (3)
172-173
: LGTM! Consistent use of setInput.The changes look good. Using
setInput
is consistent with the previous change and is the correct way to set input properties in Angular tests. This setup correctly initializes the component for the title validation test.
Line range hint
192-193
: LGTM! Proper test cleanup.The addition of
flush()
anddiscardPeriodicTasks()
at the end of the test is a good practice. This ensures that all pending asynchronous activities are completed and any remaining periodic tasks are cleaned up, preventing potential side effects in subsequent tests.
Line range hint
1-214
: Overall, the changes improve test reliability and coverage.The modifications to this test file enhance the reliability and coverage of the CompetencyFormComponent tests. Key improvements include:
- Consistent use of
setInput
for setting component input properties, which is the recommended approach in Angular testing.- Comprehensive coverage of title validation scenarios, including edge cases.
- Proper handling of asynchronous operations using
fakeAsync
,tick
, andflush
.These changes contribute to a more robust test suite, reducing the likelihood of false positives/negatives and improving the overall quality of the codebase.
src/test/javascript/spec/component/competencies/prerequisite-form.component.spec.ts (3)
32-32
: LGTM: Correct import for PrerequisiteFormComponentThe import statement has been updated to include PrerequisiteFormComponent, which aligns with the PR objectives of testing the prerequisite form component.
Line range hint
134-170
: LGTM: Retained taxonomy suggestion testsThe test cases for suggesting taxonomies based on title and description changes have been retained with minimal modifications. This ensures that the existing functionality for taxonomy suggestions remains intact and properly tested.
Line range hint
1-214
: Overall assessment: Well-implemented changes with room for minor enhancementsThe changes in this file align well with the PR objectives, particularly in updating the tests to reflect the new behavior of unlinking edited competencies. The test cases have been improved to be more comprehensive and accurate, especially in terms of simulating Angular's input property setting.
Key points:
- The import statements and component setup have been correctly updated.
- Form submission testing has been significantly enhanced.
- The method for setting isEditMode in tests has been consistently updated to use componentRef.setInput.
- Existing functionality tests (like taxonomy suggestions) have been retained.
Suggestions for further improvement:
- Add a specific test for the warning message displayed when editing a linked competency.
- Include a verification step in the form submission test to ensure the competency is unlinked after submission.
These additions would further strengthen the test coverage and ensure all aspects of the new functionality are properly verified.
src/main/webapp/i18n/de/competency.json (3)
212-212
: LGTM: New fields added to lectureUnitTableThe new fields "noOfParticipants" (Anzahl der Teilnehmenden) and "participationRate" (Teilnahmerate) have been appropriately added to the lectureUnitTable section. The translations are correct and use the informal style as required.
213-214
: LGTM: Improved warning message for unlinking standardized competenciesThe new warning message about unlinking from standardized competencies when editing has been added successfully. The translation uses the informal style as required and incorporates improvements suggested in past reviews:
- It uses "Bearbeiten" instead of "Editieren" as suggested.
- The message is clear and concise.
- The exclamation mark adds appropriate emphasis for a warning.
This addition aligns well with the PR objectives of ensuring that edited competencies are unlinked from standardized ones.
212-212
: LGTM: Added deletion confirmation for prerequisitesThe new "deleted" field with the value "Voraussetzung gelöscht" has been appropriately added to the "manage" section of prerequisites. This addition:
- Provides clear feedback when a prerequisite is deleted.
- Uses the correct informal language style.
- Is consistent with similar messages for other actions in the system.
This improvement enhances the user experience in managing prerequisites.
src/main/webapp/app/course/competencies/edit/edit-competency.component.html
Show resolved
Hide resolved
src/main/webapp/app/course/competencies/edit/edit-prerequisite.component.html
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/config/LoggingAspect.java
Outdated
Show resolved
Hide resolved
src/test/javascript/spec/component/competencies/prerequisite-form.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/competencies/prerequisite-form.component.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good just a small remark
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts (1)
58-65
:⚠️ Potential issueVerify the usage of 'input' function for defining input properties
The use of the
input
function to define input properties is unconventional in Angular. Typically, the@Input()
decorator is used for input properties. Please verify that this approach is intentional and aligns with the project's coding standards.If
input
is a custom utility function, ensure it is properly imported from its source and that its usage is consistent throughout the project.Consider reverting to the standard
@Input()
decorator if there is no specific advantage to using theinput
function.- isEditMode = input<boolean>(false); + @Input() + isEditMode: boolean = false;Likely invalid or redundant comment.
initialTitle = this.formData.title; | ||
} | ||
this.form = this.fb.nonNullable.group({ | ||
title: [ | ||
undefined as string | undefined, | ||
[Validators.required, Validators.maxLength(255)], | ||
[titleUniqueValidator(this.courseCompetencyService, this.courseId, initialTitle)], | ||
[titleUniqueValidator(this.courseCompetencyService, this.courseId(), initialTitle)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Confirm the necessity of calling courseId
as a function
In the validator, titleUniqueValidator(this.courseCompetencyService, this.courseId(), initialTitle)
, courseId
is called as a function. Ensure that courseId
is meant to be a function rather than a property.
If courseId
is intended to be a property, adjust the code accordingly:
- [titleUniqueValidator(this.courseCompetencyService, this.courseId(), initialTitle)],
+ [titleUniqueValidator(this.courseCompetencyService, this.courseId, initialTitle)],
If it must be a function, consider providing documentation or comments to explain this design choice.
Committable suggestion was skipped due to low confidence.
@@ -2,3 +1,4 @@ | |||
import { Component, EventEmitter, Output, input } from '@angular/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect import of 'input' from '@angular/core'
The input
function is being imported from @angular/core
, but it is not a standard export from that module. This could lead to import errors. Please ensure that input
is imported from the correct module.
Apply this diff to correct the import statement:
-import { Component, EventEmitter, Output, input } from '@angular/core';
+import { Component, EventEmitter, Output } from '@angular/core';
+import { input } from 'path/to/your/utility/module'; // Replace with the correct import path
Committable suggestion was skipped due to low confidence.
@@ -122,14 +116,14 @@ | |||
return; | |||
} | |||
let initialTitle: string | undefined = undefined; | |||
if (this.isEditMode && this.formData && this.formData.title) { | |||
if (this.isEditMode() && this.formData && this.formData.title) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Accessing input properties as functions may be confusing
In the condition if (this.isEditMode() && this.formData && this.formData.title)
, isEditMode
is accessed as a function (this.isEditMode()
). This may cause confusion, as input properties in Angular are typically accessed as variables, not functions.
Consider refactoring to access isEditMode
as a property:
- if (this.isEditMode() && this.formData && this.formData.title) {
+ if (this.isEditMode && this.formData && this.formData.title) {
If input
returns a function intentionally, consider renaming it to clarify its usage, or document this behavior to aid other developers.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. I left one question in the comments.
@@ -1,4 +1,4 @@ | |||
import { Component, EventEmitter, Input, Output } from '@angular/core'; | |||
import { Component, EventEmitter, Output, input } from '@angular/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for changing the order here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, input is now no longer capitalised (signal vs. @Input()
) and our linter wants all capitalised words before the others
Adaptive Learning
: Unlink edited competencies from standardised competenciesAdaptive learning
: Unlink edited competencies from standardised competencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and works as expected 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code lgtm
Checklist
General
Server
Client
Motivation and Context
Competencies and prerequisites can be created using standardised competencies. If these imported competencies or prerequisites are edited later on, they should loose their link to the standardised competency, as they do no longer resemble the standardised competency.
Description
When a competency is edited that is linked to a standardised competency, a warning is shown above the submit button that this change will unlink the competency. And on submit the competency is then actually unlinked.
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
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation