Skip to content
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

Conversation

JohannesStoehr
Copy link
Contributor

@JohannesStoehr JohannesStoehr commented Oct 1, 2024

Checklist

General

Server

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the client coding and design guidelines.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

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:

  • 1 Instructor
  • Standardised competencies (e.g. on ts3)
  1. Log in to Artemis
  2. Import a competency and/or prerequisite based on a standardised competency
  3. Edit the imported competency/prerequisite
  4. Edit it again and check that there is no longer a warning as it is no longer linked

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

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Test Coverage

Screenshots

Bildschirmfoto 2024-10-01 um 14 03 05

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a warning message for users when editing competencies, indicating that it will unlink standardized competencies.
    • Introduced a new property to the competency form to indicate linked standardized competencies.
    • Enhanced user feedback with a conditional alert for linked standardized competencies during form submission.
  • Bug Fixes

    • Enhanced the handling of edit mode checks in various components for improved reliability.
  • Documentation

    • Updated localization files for both German and English to reflect new warning messages and additional fields in the competency management sections.

Copy link

coderabbitai bot commented Oct 1, 2024

Walkthrough

The pull request introduces modifications primarily to the competency management system within the application. Key changes include setting the linkedStandardizedCompetency property to null during competency updates, adjustments to how properties are accessed in various components, and the addition of new input properties and localization strings. These alterations enhance the handling of competencies and prerequisites, ensuring that the application accurately reflects the relationships and states of these entities.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java Updated updateCourseCompetency method to set linkedStandardizedCompetency to null during updates.
src/main/webapp/app/course/competencies/edit/edit-competency.component.html Added new input property [hasLinkedStandardizedCompetency] to <jhi-competency-form> component.
src/main/webapp/app/course/competencies/forms/competency/competency-form.component.html Changed property accesses to method calls for several properties and added a warning alert for linked standardized competencies.
src/main/webapp/app/course/competencies/forms/competency/competency-form.component.ts Updated logic for accessing properties as methods and replaced @Input() decorators with direct assignments using the input function.
src/main/webapp/app/course/competencies/forms/prerequisite/prerequisite-form.component.html Updated property accesses to method calls and added a conditional alert for linked standardized competencies.
src/main/webapp/app/course/competencies/forms/prerequisite/prerequisite-form.component.ts Modified ngOnChanges to use method calls for determining edit mode.
src/main/webapp/i18n/de/competency.json Added new localization strings for warnings and updated fields in the "create" section related to competencies.
src/main/webapp/i18n/en/competency.json Added a warning message in the edit section regarding unlinking standardized competencies during edits.
src/main/java/de/tum/cit/aet/artemis/core/config/LoggingAspect.java Commented out debug logging for method entry in logAround method, retaining exit logging and exception handling.
src/test/javascript/spec/component/competencies/competency-form.component.spec.ts Updated tests to use setInput for isEditMode and enhanced validation logic for form submission.
src/test/javascript/spec/component/competencies/prerequisite-form.component.spec.ts Adjusted setup for PrerequisiteFormComponent tests, focusing on validation and form submission logic.

Possibly related PRs

Suggested labels

bugfix, documentation, component:AdaptiveLearning

Suggested reviewers

  • JohannesWt
  • MaximilianAnzinger
  • Jan-Thurner
  • pzdr7
  • EneaGore
  • coolchock
  • sarpsahinalp

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) atlas Pull requests that affect the corresponding module labels Oct 1, 2024
Copy link

@coderabbitai coderabbitai bot left a 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 guidelines

The component structure generally adheres to the coding guidelines. However, consider the following suggestions:

  1. Verify that meaningful tests exist for this component, as per the guideline "tests:meaningful".
  2. Ensure that the component is using single quotes for strings in other parts of the file, as per "strings:single_quotes".
  3. Confirm that the component is part of a proper routing structure with a consistent naming schema, as per "routes:naming_schema".
  4. 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.ts

Please 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

📥 Commits

Files that changed from the base of the PR and between 4932dc9 and eccbeca.

📒 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_imports

src/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)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

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)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

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 directive

The 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 changes

The 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 objectives

The modifications in this file effectively implement the PR's main objective of unlinking edited competencies from standardized competencies. Key points:

  1. Dynamic behavior: Property accesses changed to method calls for more flexibility.
  2. User feedback: New alert added to warn about unlinking standardized competencies.
  3. Consistent updates: All changes follow a similar pattern, including the cancel button condition.
  4. 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.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 1, 2024
Copy link
Contributor

@az108 az108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall Code 👍

src/main/webapp/i18n/de/competency.json Outdated Show resolved Hide resolved
Copy link
Contributor

@pzdr7 pzdr7 left a 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);

src/main/webapp/i18n/de/competency.json Outdated Show resolved Hide resolved
@github-actions github-actions bot added tests core Pull requests that affect the corresponding module labels Oct 1, 2024
Copy link

@coderabbitai coderabbitai bot left a 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 test

The test case for form submission has been significantly enhanced:

  1. It now includes mocking of the prerequisite service for asynchronous validation.
  2. The form submission scenario is more comprehensive, including setting form values and selecting lecture units.
  3. 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

📥 Commits

Files that changed from the base of the PR and between eccbeca and a659639.

📒 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_imports

src/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() and discardPeriodicTasks() 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:

  1. Consistent use of setInput for setting component input properties, which is the recommended approach in Angular testing.
  2. Comprehensive coverage of title validation scenarios, including edge cases.
  3. Proper handling of asynchronous operations using fakeAsync, tick, and flush.

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 PrerequisiteFormComponent

The 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 tests

The 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 enhancements

The 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:

  1. The import statements and component setup have been correctly updated.
  2. Form submission testing has been significantly enhanced.
  3. The method for setting isEditMode in tests has been consistently updated to use componentRef.setInput.
  4. Existing functionality tests (like taxonomy suggestions) have been retained.

Suggestions for further improvement:

  1. Add a specific test for the warning message displayed when editing a linked competency.
  2. 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 lectureUnitTable

The 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 competencies

The 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:

  1. It uses "Bearbeiten" instead of "Editieren" as suggested.
  2. The message is clear and concise.
  3. 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 prerequisites

The new "deleted" field with the value "Voraussetzung gelöscht" has been appropriately added to the "manage" section of prerequisites. This addition:

  1. Provides clear feedback when a prerequisite is deleted.
  2. Uses the correct informal language style.
  3. Is consistent with similar messages for other actions in the system.

This improvement enhances the user experience in managing prerequisites.

@github-actions github-actions bot removed the core Pull requests that affect the corresponding module label Oct 1, 2024
coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 1, 2024
Copy link
Contributor

@JohannesWt JohannesWt left a 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

Copy link
Contributor

@pzdr7 pzdr7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Files that changed from the base of the PR and between a659639 and 527b05b.

📒 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)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

🔇 Additional comments (1)
src/main/webapp/app/course/competencies/forms/course-competency-form.component.ts (1)

58-65: ⚠️ Potential issue

Verify 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 the input 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)],
Copy link

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';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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) {
Copy link

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.

Copy link
Contributor

@Jan-Thurner Jan-Thurner left a 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';
Copy link
Contributor

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?

Copy link
Contributor Author

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

@MaximilianAnzinger MaximilianAnzinger changed the title Adaptive Learning: Unlink edited competencies from standardised competencies Adaptive learning: Unlink edited competencies from standardised competencies Oct 2, 2024
@az108 az108 temporarily deployed to artemis-test5.artemis.cit.tum.de October 2, 2024 11:55 — with GitHub Actions Inactive
@cremertim cremertim temporarily deployed to artemis-test2.artemis.cit.tum.de October 2, 2024 12:10 — with GitHub Actions Inactive
Copy link
Contributor

@az108 az108 left a 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 👍

Copy link
Contributor

@cremertim cremertim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
atlas Pull requests that affect the corresponding module client Pull requests that update TypeScript code. (Added Automatically!) ready for review server Pull requests that update Java code. (Added Automatically!) small tests
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants