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: Add option to import exercises and lecture units when importing competencies #9205

Merged

Conversation

JohannesStoehr
Copy link
Contributor

@JohannesStoehr JohannesStoehr commented Aug 11, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I documented the Java code using JavaDoc style.

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.
  • I strictly followed the client coding and design guidelines.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

Setting up new courses that use a lot of material from last year's course is quite a lot of work. Especially linking the learning objects to their respective competencies.
This should be automated to simplify the setup of a new course.

Description

When importing all competencies and prerequisites from another course, the instructor can choose to include the exercises and/or lecture units linked to these competencies. Additionally a date can be set that is used as reference for all exercise and lecture unit dates, so they don't have to be manually adapted from the last year.
In case exercises with the same title already exist in the course, the exercise with that title is not imported but the existing exercise is linked to the imported competency instead.

This PR does not change the import validation for competencies. So it is still possible to create multiple competencies in a course with the same title, by importing competencies with the same name.

The import functionality for all linked learning objects is already implemented for all import options on the server side but only available for importing all competencies and prerequisites on the client side. @JohannesWt will implement this in a follow up, where he'll refactor the whole page.

Steps for Testing

Prerequisites:

  • 1 Instructor
  1. Log in to Artemis
  2. Create a few exercises and lecture units and link them to competencies/prerequisites
  3. Go to another course and import all competencies and prerequisites from the other course with different configurations:
    • Only import exercises
    • Only import lecture units
    • Have some lecture units/exercises already existing in the course with the same title
    • Set a reference date and check that all dates of the learning objects are set relative to that
  4. Delete all competencies before importing them again, since they would get duplicated otherwise!

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
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
competency-management.component.ts 94.39%
competency.service.ts 100%
import-all-course-competencies-modal.component.ts 100%
import-course-competencies-settings.component.ts 100%
prerequisite.service.ts 100%
course-competency-api.service.ts 100%
learning-path-student-page.component.ts 97.29%
base-api-http.service.ts 94.73%
competency.model.ts 90.78%
import-table.component.ts 100%

Server

Class/File Line Coverage Confirmation (assert/expect)
LectureImportService.java 100%
LectureUnitImportService.java 88%
CompetencyService.java 90%
CourseCompetencyService.java 91%
LearningObjectImportService.java 83%
PrerequisiteService.java 90%
LectureResource.java 90%
CompetencyResource.java 92%
CourseCompetencyResource.java 97%
PrerequisiteResource.java 92%
CompetencyImportOptionsDTO.java 100%

Screenshots

Bildschirmfoto 2024-08-29 um 22 15 26

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced course competency retrieval methods for better data-fetching capabilities, including fetching associated exercises, lecture units, and lectures in single queries.
    • Introduced new import options for competencies and their relations, improving flexibility during the import process.
    • Added methods to query various entities (e.g., exercises, lectures) by title and course ID for more precise searches.
    • Introduced CompetencyImportOptionsDTO to encapsulate various import options, streamlining the import process.
    • Enhanced lecture import functionality with additional configuration options.
    • Improved data retrieval for quiz exercises with additional related information.
    • Added new methods allowing for comprehensive import of learning objects associated with competencies.
    • Implemented a dynamic import table interface for managing import objects with search, sort, and pagination capabilities.
    • Added a debouncing method to optimize function calls in the API service.
    • Introduced a new delete method to the API service for better HTTP operation support.
    • Added unit tests for the JudgementOfLearningRatingComponent to validate rating functionality and error handling.
    • Enhanced methods for retrieving prerequisites and competencies, including associated exercises and lectures.
    • Improved error handling for course competency retrieval methods.
    • Added new methods for handling course competencies and their associated data, improving overall functionality.
  • Bug Fixes

    • Improved error handling during competency imports.
  • Documentation

    • Updated method signatures in several resource classes to reflect new DTO implementations and functionality.
    • Enhanced localization files to provide clearer guidance on import functionalities.
    • Refined documentation comments in the API service for consistency and clarity.

@JohannesStoehr JohannesStoehr requested a review from a team as a code owner August 11, 2024 13:57
@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Aug 11, 2024
Copy link

coderabbitai bot commented Aug 11, 2024

Walkthrough

The recent changes enhance the functionality of the competency management system by introducing new methods in various repository interfaces for retrieving CourseCompetency, Competency, and Prerequisite entities along with their associated data. Key improvements include the renaming of existing methods for clarity and the addition of methods that utilize LEFT JOIN FETCH to optimize data retrieval in single queries. Additionally, updates to the test classes improve the robustness and clarity of testing related functionalities.

Changes

Files Change Summary
src/main/java/de/tum/in/www1/artemis/repository/CourseCompetencyRepository.java, src/main/java/de/tum/in/www1/artemis/repository/CompetencyRepository.java, src/main/java/de/tum/in/www1/artemis/repository/PrerequisiteRepository.java, src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRepository.java, src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java, src/main/java/de/tum/cit/aet/artemis/atlas/repository/PrerequisiteRepository.java Renamed methods and added new methods for improved data retrieval, allowing fetching of related entities in a single query.
src/test/java/de/tum/cit/aet/artemis/competency/AbstractCompetencyPrerequisiteIntegrationTest.java Updated method signatures and introduced new test cases for validating the import functionality and associated entities.

Possibly related PRs

Suggested labels

feature, documentation, component:AdaptiveLearning

Suggested reviewers

  • MaximilianAnzinger
  • Jan-Thurner
  • SimonEntholzer
  • florian-glombik
  • egekurt123
  • BBesrour

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.

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

Outside diff range comments (1)
src/main/java/de/tum/in/www1/artemis/service/competency/CompetencyService.java (1)

[!TIP]
Codebase Verification

Ensure importOptions is utilized in importCompetencies method.

The importOptions parameter is not currently used within the importCompetencies method in CompetencyService.java. This suggests that the integration of importOptions is incomplete or missing. Please review the intended functionality and ensure importOptions is utilized appropriately.

  • File: src/main/java/de/tum/in/www1/artemis/service/competency/CompetencyService.java
Analysis chain

Line range hint 57-68: Verify integration of importOptions.

The importCompetencies method now includes CompetencyImportOptionsDTO importOptions. Ensure that all usages of this method are updated to pass the correct options and that the options are utilized correctly within the method.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify all usages of importCompetencies are updated to pass the correct importOptions.
rg --type java -A 5 'importCompetencies\('

Length of output: 4396


Script:

#!/bin/bash
# Search for the implementation of the importCompetencies method to verify the usage of importOptions.
ast-grep --lang java --pattern $'public Set<CompetencyWithTailRelationDTO> importCompetencies($_, $_, $_) { $$$ }'

Length of output: 239


Script:

#!/bin/bash
# Use rg to search for the implementation of the importCompetencies method in CompetencyService.java.
rg --type java -A 10 'public Set<CompetencyWithTailRelationDTO> importCompetencies' src/main/java/de/tum/in/www1/artemis/service/competency/CompetencyService.java

Length of output: 825

coderabbitai[bot]
coderabbitai bot previously approved these changes Aug 12, 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: 1

@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Aug 15, 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: 7

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

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

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

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

az108
az108 previously approved these changes Sep 30, 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.

reapprove after merge conflict

pzdr7
pzdr7 previously approved these changes Sep 30, 2024
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.

Reapprove after merge conflicts

JohannesWt
JohannesWt previously approved these changes Sep 30, 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.

Reapprove after merge conflicts

Copy link
Contributor

@MaximilianAnzinger MaximilianAnzinger left a comment

Choose a reason for hiding this comment

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

Maintainer approved

@iyannsch iyannsch removed their request for review October 3, 2024 10:01
@iyannsch iyannsch added the maintainer-approved The feature maintainer has approved the PR label Oct 3, 2024
@krusche krusche changed the title Adaptive Learning: Add option to import exercises and lecture units when importing competencies Adaptive learning: Add option to import exercises and lecture units when importing competencies Oct 3, 2024
…s-lecture-units

# Conflicts:
#	src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts
#	src/test/java/de/tum/cit/aet/artemis/atlas/competency/AbstractCompetencyPrerequisiteIntegrationTest.java
#	src/test/java/de/tum/cit/aet/artemis/atlas/competency/CourseCompetencyIntegrationTest.java
#	src/test/javascript/spec/component/learning-paths/pages/learning-path-student-page.component.spec.ts
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: 17

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 92a1f3e and a29c52c.

📒 Files selected for processing (8)
  • src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts (2 hunks)
  • src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (7 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/competency/AbstractCompetencyPrerequisiteIntegrationTest.java (17 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyIntegrationTest.java (8 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/competency/CourseCompetencyIntegrationTest.java (14 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/competency/PrerequisiteIntegrationTest.java (8 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (3 hunks)
  • src/test/javascript/spec/component/learning-paths/pages/learning-path-student-page.component.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.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/learning-paths/services/base-api-http.service.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/test/java/de/tum/cit/aet/artemis/atlas/competency/AbstractCompetencyPrerequisiteIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/test/java/de/tum/cit/aet/artemis/atlas/competency/CourseCompetencyIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/test/java/de/tum/cit/aet/artemis/atlas/competency/PrerequisiteIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/test/javascript/spec/component/learning-paths/pages/learning-path-student-page.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}}

📓 Learnings (3)
src/test/java/de/tum/cit/aet/artemis/atlas/competency/AbstractCompetencyPrerequisiteIntegrationTest.java (1)
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-02-23T00:03:06.365Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
src/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyIntegrationTest.java (1)
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-02-23T00:03:06.365Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-02-23T00:03:06.365Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
🔇 Additional comments (50)
src/main/webapp/app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component.ts (2)

7-7: LGTM: Correct import for ActivatedRoute.

The import of ActivatedRoute from '@angular/router' is correct and follows Angular's best practices.


12-12: Excellent: Centralized error handling with onError utility.

The import of onError from a shared utility file promotes code reuse and consistent error handling across the application. This aligns well with our coding guidelines and best practices.

src/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyIntegrationTest.java (5)

16-16: Import statement is appropriate.

The import of CompetencyImportOptionsDTO is necessary for the updated methods and is correctly placed.


215-216: Method signature updated correctly.

The importCall method signature has been updated to accept CompetencyImportOptionsDTO, which improves the encapsulation of import options.


277-278: Method signature updated appropriately.

The importAllCall method now uses CompetencyImportOptionsDTO, which aligns with the new import options structure.


327-328: Method signature updated as per new structure.

The importBulkCall method correctly incorporates CompetencyImportOptionsDTO, enhancing consistency across import methods.


343-353: Consider simplifying test method names for clarity.

The test method names are quite long and could be simplified to improve readability. For instance:

  • Rename shouldImportCompetenciesExerciseAndLectureWithCompetency to shouldImportCompetenciesWithExercisesAndLectures.
  • Rename shouldImportCompetenciesExerciseAndLectureWithCompetencyAndChangeDates to shouldImportCompetenciesWithExercisesLecturesAndChangeDates.

This follows the guideline test_naming: descriptive while keeping the names concise.

src/test/java/de/tum/cit/aet/artemis/atlas/competency/CourseCompetencyIntegrationTest.java (5)

245-246: No action needed: Floating-point assertion

The assertion on averageStudentScore() uses isEqualTo(30). Given that this value is an exact representation of a floating-point number without decimal precision issues, the use of isEqualTo is acceptable here.


266-266: No action needed: Floating-point assertion

Similarly, the assertion isEqualTo(53.3) is acceptable if the method averageStudentScore() reliably returns the exact value. If there's a risk of floating-point precision errors, consider using isCloseTo with a tolerance. However, since this was addressed in a previous review comment and is still valid, no further action is required.


297-297: No action needed: Floating-point assertion

The assertion on studentCompetencyProgress1.getConfidence() uses isEqualTo(0.75). If the confidence value is calculated in a way that ensures exact representation, isEqualTo is suitable. Otherwise, consider using isCloseTo with an appropriate tolerance. As per previous review comments, this has already been noted.


514-515: Avoid self-referential competency relations

In the PreAuthorize nested class, the relation is created with both headCompetency and tailCompetency referencing courseCompetency:

relation.setHeadCompetency(courseCompetency);
relation.setTailCompetency(courseCompetency);

Creating a competency relation where a competency relates to itself might not be meaningful and could lead to confusion or errors in the relation logic. Verify if this is intentional for testing purposes. If so, consider adding comments to clarify the intent.


375-376: Ensure correct endpoint usage for fetching competency title

The code fetches the competency title with:

String title = request.get("/api/course-competencies/" + courseCompetency.getId() + "/title", HttpStatus.OK, String.class);

Confirm that the endpoint /api/course-competencies/{competencyId}/title is designed to return a plain String. If the API convention is to return a DTO or a specific response object, adjust the request accordingly.

src/test/java/de/tum/cit/aet/artemis/atlas/competency/AbstractCompetencyPrerequisiteIntegrationTest.java (34)

3-3: Appropriate Import of HalfSecond for Time Assertions

The static import of HalfSecond is necessary for precise time-based assertions in the tests, ensuring consistency in time tolerances.


26-28: Necessary Imports for Competency DTOs

The addition of imports for CompetencyImportOptionsDTO, CompetencyImportResponseDTO, and CompetencyWithTailRelationDTO is appropriate as these classes are utilized in the updated methods.


52-52: Renaming Variable for Clarity

Renaming the variable from competency to courseCompetency improves semantic clarity and enhances code readability by explicitly indicating that the competency is associated with a course.


77-77: Initializing courseCompetency Appropriately

Assigning courseCompetency using the provided function aligns with the test setup requirements, ensuring that the competency is properly initialized for the tests.


80-81: Creating Text Exercises with Associated Competency

The creation of textExercise and teamTextExercise with the associated courseCompetency ensures that the tests accurately reflect scenarios where exercises are linked to competencies.


100-100: Storing textUnitOfLectureOne for Test Consistency

Assigning the saved TextUnit to textUnitOfLectureOne allows for consistent reference in subsequent test methods, enhancing test reliability.


102-106: Storing attachmentUnitOfLectureOne and Ensuring Proper Association

Initializing attachmentUnitOfLectureOne similarly ensures that attachment units are correctly associated with the competency in tests.


138-141: Configuring Team Assignment Parameters

Setting the minimum and maximum team sizes for teamTextExercise ensures that team-based exercises are properly configured, which is crucial for accurate testing of team assignments.


151-156: Implementation of createProgrammingExercise Method

The new method createProgrammingExercise facilitates the creation of programming exercises with competencies, enhancing test versatility. The implementation correctly initializes the exercise with the appropriate settings.


162-163: Retrieving Competency for Verification

Using getCall to retrieve courseCompetency allows the test to verify that competencies are correctly fetched, which is essential for validating the GET endpoint.


169-172: Creating Competency Progress for Multiple Users

Setting up competency progress for different users aids in testing user-specific data retrieval and ensures that progress tracking is functioning as expected.


177-178: Validating Competency Response

Asserting that the retrieved competency matches the expected courseCompetency ensures that the API returns correct data.


189-189: Correctly Verifying Access Control

The test correctly verifies that a user not enrolled in the course receives a FORBIDDEN response, ensuring that access control is enforced.


216-216: Ensuring Unreleased Lecture Units Are Not Exposed

Asserting that unreleased lecture units are not included in the response protects against unintended data exposure to students.


229-230: Deleting Competency and Validating Removal

The test appropriately checks that a competency can be deleted and that subsequent retrieval results in a NOT_FOUND status, confirming the deletion was successful.


235-237: Deleting Competency Also Removes Relations

Ensuring that associated CompetencyRelation entities are deleted when a competency is removed maintains data integrity and prevents orphaned relations.


245-245: Enforcing Course Ownership on Deletion

Verifying that an instructor not associated with the course cannot delete competencies enforces proper permissions and course ownership.


250-255: Cascade Deletion Validated

The test confirms that deleting a course cascades deletions to competencies, relations, and prerequisites, which is essential for database consistency.


265-265: Updating Competency After Lecture Deletion

The competency's lecture units are correctly updated when a lecture is deleted, ensuring that no references to non-existent lectures remain.


271-273: Removing Lecture Unit from Competency

Deleting a lecture unit and verifying that it is removed from the competency's lecture units maintains accurate associations.


280-285: Updating Competency Details Successfully

The test effectively updates the competency's title, description, and lecture units, verifying that the update functionality works as intended.


359-359: Method Signature Updated for Flexibility

Changing the method signature of importCall to accept CompetencyImportOptionsDTO allows for more flexible import options and aligns with best practices for API design.


377-390: Testing Import of Exercises and Lectures with Competency

The test method shouldImportExerciseAndLectureWithCompetency effectively verifies that exercises and lectures are imported alongside competencies. The approach is comprehensive and the assertions are appropriate.


Line range hint 484-506: Validating Import of Competencies and Relations

Tests for importing competencies with and without relations ensure that the import functionality handles different scenarios correctly. The assertions confirm that relations are imported when specified.


515-524: Testing Import All Competencies with Exercises and Lectures

The method shouldImportAllExerciseAndLectureWithCompetency verifies that all competencies, along with their associated exercises and lectures, are imported correctly, which is crucial for comprehensive import operations.


528-559: Handling Date Adjustments During Bulk Import

The test shouldImportAllExerciseAndLectureWithCompetencyAndChangeDates ensures that the release and visible dates are adjusted relative to a new reference date during import. The calculations and assertions accurately reflect the intended functionality.


606-606: Method Signature Consistency in importBulkCall

Updating the method signature to accept CompetencyImportOptionsDTO in importBulkCall ensures consistency across import methods and enhances flexibility.


642-656: Testing Bulk Import of Competencies with Exercises and Lectures

The test shouldImportCompetenciesExerciseAndLectureWithCompetency effectively checks that competencies are imported along with their associated exercises and lectures in bulk operations.


174-174: ⚠️ Potential issue

Handle Optional Safely When Retrieving Entities

Directly calling .get() on an Optional without checking if a value is present can lead to a NoSuchElementException. Consider using .orElseThrow() with a meaningful exception or verifying that the Optional has a value before accessing it.


294-295: 🧹 Nitpick (assertive)

Avoid Setting null ID When Updating Competency

Setting courseCompetency.setId(null); may not accurately simulate an update without an ID. Instead, consider creating a new CourseCompetency instance without setting an ID to reflect the scenario more precisely.


363-364: 🛠️ Refactor suggestion

Consider Using a Builder Pattern for CompetencyImportOptionsDTO

The constructor is called with several parameters, including multiple booleans and Optional values, which may reduce readability and increase the risk of errors. Implementing a builder pattern would improve clarity and maintainability.


393-426: 🛠️ Refactor suggestion

Refactor Tests to Reduce Code Duplication

The test shouldImportExerciseAndLectureWithCompetencyAndChangeDates contains duplicated setup code similar to other tests. Extracting common setup steps into helper methods or using parameterized tests would enhance maintainability.


621-622: 🛠️ Refactor suggestion

Consider Using a Builder Pattern for CompetencyImportOptionsDTO

Similar to earlier instances, using a builder pattern here would improve code readability when constructing CompetencyImportOptionsDTO with multiple parameters.


658-692: 🛠️ Refactor suggestion

Refactor Bulk Import Tests to Eliminate Redundancy

The test shouldImportCompetenciesExerciseAndLectureWithCompetencyAndChangeDates repeats code found in other tests. Refactoring common code into helper methods would reduce redundancy and improve test maintainability.

src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (4)

11-11: LGTM!

The import of java.util.Optional is necessary for the updated code.


34-34: LGTM!

The import of CompetencyImportOptionsDTO is required for the new import options functionality.


151-153: Consider using a builder pattern for CompetencyImportOptionsDTO

As previously suggested, using a builder pattern can improve readability when creating CompetencyImportOptionsDTO with multiple parameters.


161-162: Consider using a builder pattern for CompetencyImportOptionsDTO

As previously suggested, using a builder pattern can enhance readability when constructing CompetencyImportOptionsDTO with multiple parameters.

@iyannsch iyannsch removed their request for review October 4, 2024 08:48
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!) core Pull requests that affect the corresponding module exam Pull requests that affect the corresponding module fileupload Pull requests that affect the corresponding module lecture Pull requests that affect the corresponding module maintainer-approved The feature maintainer has approved the PR modeling Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module quiz Pull requests that affect the corresponding module ready to merge server Pull requests that update Java code. (Added Automatically!) tests text Pull requests that affect the corresponding module
Projects
Status: Merged
Status: Done
Development

Successfully merging this pull request may close these issues.