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: Improve import of competencies when some already exist #9774

Conversation

JohannesStoehr
Copy link
Contributor

@JohannesStoehr JohannesStoehr commented Nov 13, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all 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 (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.

Motivation and Context

There is a bug/unhandled situation when an instructor imports some competencies and the target course already contains some competencies with the same title. Currently this creates multiple competencies with the same title in the course.

Description

The import now handles existing competencies with the same title by not importing the competencies again. In case relations, exercises or lecture units are imported alongside, they are still linked to the existing competencies as excepted.
This also applies to prerequisites.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 2 Courses
  1. Log in to Artemis
  2. Create some competencies or prerequisites in one course
  3. Create at least one competency or prerequisite with the same title in the other course
  4. Import all competencies/prerequisites/both into the second course
  5. Verify that there are no duplicates in the target course of the import

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.







Review Progress

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced competency import functionality to prevent duplicate entries.
    • Streamlined methods for importing competencies and prerequisites, improving performance and readability.
  • Bug Fixes

    • Resolved issues with duplicate competencies during import operations.
  • Tests

    • Added new test cases to verify competency import scenarios, ensuring robustness in handling existing competencies.

@JohannesStoehr JohannesStoehr added this to the 7.7.1 milestone Nov 13, 2024
@JohannesStoehr JohannesStoehr self-assigned this Nov 13, 2024
@JohannesStoehr JohannesStoehr requested a review from a team as a code owner November 13, 2024 17:43
@github-actions github-actions bot added tests 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 Nov 13, 2024
Copy link

coderabbitai bot commented Nov 13, 2024

Walkthrough

The pull request introduces significant modifications across multiple services and components related to competency management. Key changes include refactoring methods in CompetencyService, CourseCompetencyService, and PrerequisiteService to utilize method references for object creation, enhancing code clarity and reducing boilerplate. Additionally, updates to the front-end components ensure that duplicate competencies are not added during imports. New test cases are introduced to validate the import functionality when existing competencies are present, ensuring comprehensive coverage of the changes.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java Refactored importCompetencies, importStandardizedCompetencies, and createCompetencies methods to use Competency::new, simplifying logic and improving readability.
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java Updated importCourseCompetencies method to use a functional parameter for creating new competencies, streamlining the import process and removing unnecessary parameters.
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/PrerequisiteService.java Streamlined importPrerequisites method by directly calling importCourseCompetencies with Prerequisite::new, eliminating intermediate mapping steps.
src/main/webapp/app/course/competencies/competency-management/competency-management-table.component.ts Modified updateDataAfterImportAll to filter out existing competencies before updating courseCompetencies, preventing duplicates.
src/main/webapp/app/course/competencies/competency-management/competency-management.component.ts Updated updateDataAfterImportAll to ensure only new competencies are added to courseCompetencies, enhancing data integrity.
src/test/java/de/tum/cit/aet/artemis/atlas/competency/AbstractCompetencyPrerequisiteIntegrationTest.java Added a new test method shouldImportAllCompetenciesWithSomeExisting to verify import functionality with existing competencies.
src/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyIntegrationTest.java Introduced a new test method shouldImportAllCompetenciesWithSomeExisting to test importing competencies while some already exist.
src/test/java/de/tum/cit/aet/artemis/atlas/competency/CourseCompetencyIntegrationTest.java Added a new test method shouldImportAllCompetenciesWithSomeExisting to validate import functionality with existing competencies.
src/test/java/de/tum/cit/aet/artemis/atlas/competency/PrerequisiteIntegrationTest.java Introduced a new test method shouldImportAllCompetenciesWithSomeExisting to check import functionality for existing competencies.

Possibly related PRs

Suggested labels

tests, server, client, bugfix, ready to merge, component:AdaptiveLearning, atlas

Suggested reviewers

  • JohannesWt
  • MaximilianAnzinger
  • florian-glombik
  • az108
  • Jan-Thurner

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 pmd
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java

The following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.

src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java

The following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.

src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/PrerequisiteService.java

The following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.

  • 4 others

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

🧹 Outside diff range and nitpick comments (9)
src/main/webapp/app/course/competencies/competency-management/competency-management-table.component.ts (1)

98-99: Consider optimizing for larger datasets.

For better performance with larger datasets, consider using a Set to check for duplicates:

-        const newCourseCompetencies = importedCompetencies.filter((competency) => !this.courseCompetencies.some((existingCompetency) => existingCompetency.id === competency.id));
+        const existingIds = new Set(this.courseCompetencies.map(comp => comp.id));
+        const newCourseCompetencies = importedCompetencies.filter((competency) => !existingIds.has(competency.id));

This approach reduces the time complexity from O(n*m) to O(n+m) where:

  • n = number of imported competencies
  • m = number of existing competencies
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java (2)

69-69: LGTM! Clean refactoring to prevent duplicate competencies.

The change to use Competency::new as a method reference in importCourseCompetencies aligns with the PR's objective of preventing duplicate competencies. This approach delegates the duplicate checking logic to the parent class while maintaining a clean and functional programming style.

Consider adding logging statements around the import operation to help track potential issues in production, especially for cases where duplicates are detected.


Line range hint 1-150: Consider enhancing documentation for import behavior.

While the code implementation looks good, the class-level documentation could be enhanced to explicitly mention the duplicate handling behavior, as this is a key feature that other developers should be aware of.

Add a note to the class documentation:

 /**
  * Service for managing competencies.
+ * 
+ * This service handles the creation and import of competencies into courses,
+ * ensuring that duplicate competencies are not created when importing competencies
+ * that already exist in the target course.
  */
src/main/webapp/app/course/competencies/competency-management/competency-management.component.ts (2)

158-161: LGTM! Consider enhancing readability with Set

The implementation correctly prevents duplicate competencies during import by filtering based on ID comparison. This aligns with the PR objectives and follows Angular's immutability principles.

Consider using Set for improved readability and potentially better performance:

-        const newCourseCompetencies = importedCourseCompetencies.filter(
-            (competency) => !this.courseCompetencies().some((existingCompetency) => existingCompetency.id === competency.id),
-        );
+        const existingIds = new Set(this.courseCompetencies().map(c => c.id));
+        const newCourseCompetencies = importedCourseCompetencies.filter(
+            competency => !existingIds.has(competency.id)
+        );

Line range hint 92-112: Enhance error handling for duplicate competencies

Consider adding a specific message when all imported competencies are duplicates to provide clearer feedback to users.

Here's a suggested implementation:

         if (importedCompetencies.length) {
+            const newCompetencyCount = importedCompetencies.filter(
+                dto => !this.courseCompetencies().some(existing => existing.id === dto.competency?.id)
+            ).length;
+            if (newCompetencyCount === 0) {
+                this.alertService.warning('artemisApp.courseCompetency.importAll.allDuplicates', {
+                    courseTitle: courseTitle,
+                });
+            } else {
                 this.alertService.success(`artemisApp.courseCompetency.importAll.success`, {
-                    noOfCompetencies: importedCompetencies.length,
+                    noOfCompetencies: newCompetencyCount,
                     courseTitle: courseTitle,
                 });
+            }
         } else {
             this.alertService.warning(`artemisApp.courseCompetency.importAll.warning`, { courseTitle: courseTitle });
         }
src/test/java/de/tum/cit/aet/artemis/atlas/competency/PrerequisiteIntegrationTest.java (1)

281-285: LGTM! Consider adding Javadoc.

The test method is well-structured and aligns with the PR objectives to handle duplicate competencies during import. The name is descriptive and follows the test naming convention.

Consider adding a brief Javadoc to document the test scenario:

+    /**
+     * Test that verifies the import behavior when some competencies with matching titles already exist in the target course.
+     * This ensures no duplicates are created and existing competencies are properly linked.
+     */
     @Test
     @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void shouldImportAllCompetenciesWithSomeExisting() throws Exception {
         shouldImportAllCompetenciesWithSomeExisting(Prerequisite::new, 3);
     }
src/test/java/de/tum/cit/aet/artemis/atlas/competency/AbstractCompetencyPrerequisiteIntegrationTest.java (1)

517-536: Test method needs additional assertions and documentation.

While the test correctly verifies the basic functionality, it could be improved in several ways:

  1. The test method lacks JavaDoc documentation explaining the test scenario and expectations.
  2. The assertions could be more comprehensive:
    • Verify that the imported competencies maintain their properties (title, description, etc.)
    • Check that no duplicate competencies are created
    • Validate that relationships between competencies are preserved

Consider adding these improvements:

+    /**
+     * Tests the import of competencies when some competencies already exist in the target course.
+     * Verifies that:
+     * 1. Existing competencies are not duplicated
+     * 2. The correct number of competencies are imported
+     * 3. The competency properties are preserved
+     * 4. The competency type (Competency/Prerequisite) is maintained
+     *
+     * @param copyCourseCompetency Function to copy a course competency
+     * @param expectedCourseCompetencies Expected total number of competencies after import
+     */
     @Test
     void shouldImportAllCompetenciesWithSomeExisting(Function<CourseCompetency, CourseCompetency> copyCourseCompetency, int expectedCourseCompetencies) throws Exception {
         competencyUtilService.createCompetencies(course, 2);
         prerequisiteUtilService.createPrerequisites(course, 2);

         CourseCompetency newCompetency = copyCourseCompetency.apply(courseCompetency);
         newCompetency.setCourse(course2);
         newCompetency = courseCompetencyRepository.save(newCompetency);

         CompetencyImportOptionsDTO importOptions = new CompetencyImportOptionsDTO(Set.of(), Optional.of(course.getId()), false, false, false, Optional.empty(), false);
         importAllCall(course2.getId(), importOptions, HttpStatus.CREATED);

         course2 = courseRepository.findByIdWithExercisesAndLecturesAndLectureUnitsAndCompetenciesElseThrow(course2.getId());
         assertThat(course2.getPrerequisites().size() + course2.getCompetencies().size()).isEqualTo(expectedCourseCompetencies);
+        // Verify no duplicates were created
+        assertThat(course2.getCompetencies().stream().map(CourseCompetency::getTitle).distinct().count())
+            .isEqualTo(course2.getCompetencies().size());
+        // Verify properties are preserved
+        var importedCompetency = course2.getCompetencies().stream()
+            .filter(c -> c.getTitle().equals(newCompetency.getTitle()))
+            .findFirst()
+            .orElseThrow();
+        assertThat(importedCompetency)
+            .usingRecursiveComparison()
+            .ignoringFields("id", "course")
+            .isEqualTo(newCompetency);
         switch (newCompetency) {
             case Competency competency -> assertThat(course2.getCompetencies().stream().map(DomainObject::getId)).contains(competency.getId());
             case Prerequisite prerequisite -> assertThat(course2.getPrerequisites().stream().map(DomainObject::getId)).contains(prerequisite.getId());
             default -> fail("Unexpected CourseCompetency subclass");
         }
     }
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (2)

215-219: Consider using polymorphism to replace type-based switch

The switch expression on courseCompetency checks for specific subtypes, which can make the code less extensible if new subtypes are added in the future. To enhance maintainability, consider adding a copy or clone method to the CourseCompetency class hierarchy. This leverages polymorphism to handle object creation, eliminating the need for explicit type checks.


234-247: Optimize existing competency lookup for improved performance

Currently, for each competency to import, the code streams and filters competenciesInCourse to find matches by title and type. This approach may become inefficient with a large number of competencies. To enhance performance, consider constructing a Map of existing competencies keyed by title and type before the loop. This change reduces the time complexity of lookups from linear to constant time.

Example implementation:

// Create a map of existing competencies keyed by title and type
Map<Pair<String, CourseCompetency.CourseCompetencyType>, CourseCompetency> existingCompetenciesMap = competenciesInCourse.stream()
    .collect(Collectors.toMap(
        c -> Pair.of(c.getTitle(), c.getType()),
        Function.identity()
    ));

for (var courseCompetency : competenciesToImport) {
    CourseCompetency existingCompetency = existingCompetenciesMap.get(Pair.of(courseCompetency.getTitle(), courseCompetency.getType()));
    CourseCompetency importedCompetency = existingCompetency != null ? existingCompetency : createNewCourseCompetency.apply(courseCompetency);
    importedCompetency.setCourse(course);
    idToImportedCompetency.put(courseCompetency.getId(), new CompetencyWithTailRelationDTO(importedCompetency, new ArrayList<>()));
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 383945a and dd00233.

📒 Files selected for processing (9)
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/PrerequisiteService.java (1 hunks)
  • src/main/webapp/app/course/competencies/competency-management/competency-management-table.component.ts (1 hunks)
  • src/main/webapp/app/course/competencies/competency-management/competency-management.component.ts (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/competency/AbstractCompetencyPrerequisiteIntegrationTest.java (4 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyIntegrationTest.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/competency/CourseCompetencyIntegrationTest.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/competency/PrerequisiteIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.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/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/java/de/tum/cit/aet/artemis/atlas/service/competency/PrerequisiteService.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/competency-management/competency-management-table.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/competency-management/competency-management.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/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

📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/atlas/competency/CompetencyIntegrationTest.java (2)
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-11-12T12:51:58.050Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-11-12T12:52:03.805Z
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 (6)
src/main/webapp/app/course/competencies/competency-management/competency-management-table.component.ts (1)

98-99: LGTM! The changes correctly prevent duplicate competencies.

The implementation successfully addresses the bug by filtering out existing competencies before adding new ones, ensuring no duplicates are created during import.

src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/PrerequisiteService.java (1)

60-60: LGTM! Verify behavior with existing prerequisites.

The refactoring improves code clarity by using method references and reduces duplication by delegating to the parent class. This change aligns well with the PR objective of handling existing competencies during import.

Let's verify the behavior when prerequisites already exist:

✅ Verification successful

Based on the extensive search results, I can now generate a final response. The integration tests and repository implementations show that the prerequisite handling is well-tested and properly implemented in the parent class.

Verified: Prerequisite import behavior is safe and well-tested

The codebase shows:

  • Comprehensive integration tests in PrerequisiteIntegrationTest that verify the import behavior
  • Proper repository implementation with findAllByCourseId to handle existing prerequisites
  • The parent class CourseCompetencyService provides the base implementation that safely handles existing competencies
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for duplicate prerequisite handling in tests
# Expected: Should find test cases that verify the behavior with existing prerequisites

# Search for test methods that verify existing prerequisite handling
rg -A 5 "test.*[Ee]xisting.*[Pp]rerequisite" --type java

Length of output: 58


Script:

#!/bin/bash
# Let's broaden our search to find relevant test cases for prerequisites

# Search for prerequisite-related test files
fd "PrerequisiteTest" --type f

# Search for test methods related to prerequisites and imports
rg -A 5 "test.*[Pp]rerequisite.*[Ii]mport" --type java

# Check the parent class's implementation of importCourseCompetencies
ast-grep --pattern 'protected <$_> importCourseCompetencies($$$) {
    $$$
}'

Length of output: 382


Script:

#!/bin/bash
# Let's examine the parent class and test repository implementation

# Check the parent class CourseCompetencyService
rg -A 10 "class CourseCompetencyService" --type java

# Look at the test repository implementation
cat src/test/java/de/tum/cit/aet/artemis/atlas/test_repository/PrerequisiteTestRepository.java

# Search for any prerequisite-related tests in the entire codebase
rg -A 5 "@Test.*[Pp]rerequisite" --type java

Length of output: 2182


Script:

#!/bin/bash
# Let's check the implementation of importCourseCompetencies in CourseCompetencyService

# Get the implementation of importCourseCompetencies
rg -A 20 "protected.*importCourseCompetencies" --type java

# Check for any existing prerequisite handling logic
rg -B 2 -A 10 "findAllByCourseIdOrderById|findAllByCourseId" --type java

# Look for any integration tests that might cover this functionality
rg -A 10 "class.*PrerequisiteIT|class.*PrerequisiteIntegrationTest" --type java

Length of output: 82922

src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyService.java (1)

Line range hint 64-70: Verify error handling for large imports.

The method handles importing multiple competencies at once. Let's verify the behavior with large datasets to ensure proper error handling and performance.

✅ Verification successful

Let me generate additional scripts to check for error handling and transaction management in the competency import code:


Based on the analysis of the code, I can now provide a final response:

Error handling and transaction management are properly implemented for competency imports

The code shows appropriate error handling and transaction management for large imports:

  • Input validation is performed before processing (BadRequestAlertException for empty imports)
  • Uses @Transactional service layer methods for atomic operations
  • Implements batch saving through courseCompetencyRepository.saveAll()
  • Handles duplicate detection through existing competency checks
  • Proper error propagation with specific exceptions
  • Transaction boundaries ensure consistency when linking competencies to courses and learning paths
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential error handling and transaction management in the codebase
# Look for similar bulk import operations and their error handling patterns

# Search for bulk import methods and their error handling
rg -A 10 "import.*Competenc.*Collection|import.*Competenc.*List" 

# Look for transaction handling patterns
ast-grep --pattern 'class $_ {
  $$$
  @Transactional
  $$$
  import$_($$$) {
    $$$
  }
  $$$
}'

Length of output: 304441


Script:

#!/bin/bash
# Search for error handling patterns in the competency import code
rg -A 10 "importCompetencies|importCourseCompetencies" --type java

# Look for transaction management and batch processing patterns
ast-grep --pattern 'class $_ {
  $$$
  @Transactional
  $$$
  importCourseCompetencies($$$) {
    $$$
    saveAll($$$)
    $$$
  }
  $$$
}'

# Check for any batch size configurations
rg -i "batch.*size|chunk.*size" --type java

Length of output: 22370

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

287-291: LGTM! Consider adding edge cases.

The test method is well-structured and aligns with the PR objective of handling duplicate competencies during import. The name is descriptive, and it follows the test naming conventions.

To ensure comprehensive test coverage, let's verify if we have tests for edge cases:

Consider adding test cases for:

  • Importing with all existing competencies (100% overlap)
  • Importing with partial title matches
  • Importing with case-insensitive title matches
src/test/java/de/tum/cit/aet/artemis/atlas/competency/CourseCompetencyIntegrationTest.java (1)

197-201: LGTM! Test method aligns with PR objectives.

The new test method correctly validates the import functionality when some competencies already exist, which is the core objective of this PR.

Let's verify if we have coverage for all edge cases:

✅ Verification successful

Let me gather more specific information about the test coverage for competency import cases.


Let me check the implementation of the parent class method to understand how the test is implemented.


Test method correctly verifies competency import with existing competencies

The test method is correctly implemented and aligns with the parent class's implementation. It verifies:

  1. The import of competencies when some already exist in the target course
  2. The correct handling of both regular competencies and prerequisites
  3. The expected total number of competencies (5) after import
  4. The preservation of existing competencies during import
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other test methods related to competency import
rg -l 'import.*[Cc]ompetenc' --type java

# Search for test methods that might handle error cases
ast-grep --pattern 'class $_ {
  $$$
  @Test
  $$$
  void $_($$$) {
    $$$
    import$$$Competenc$$$
    $$$
  }
  $$$
}'

Length of output: 12218


Script:

#!/bin/bash
# Search for test methods in AbstractCompetencyPrerequisiteIntegrationTest and CourseCompetencyIntegrationTest
rg -A 10 "void shouldImport.*Competenc" --type java src/test/java/de/tum/cit/aet/artemis/atlas/competency/

# Search for error handling test cases
rg -A 10 "@Test.*import.*[Cc]ompetenc.*[Ff]ail|[Ee]rror|[Ee]xception" --type java src/test/java/de/tum/cit/aet/artemis/atlas/competency/

Length of output: 305808


Script:

#!/bin/bash
# Search for the implementation of shouldImportAllCompetenciesWithSomeExisting in the abstract class
rg -A 20 "void shouldImportAllCompetenciesWithSomeExisting.*Function.*CourseCompetency" --type java src/test/java/de/tum/cit/aet/artemis/atlas/competency/

Length of output: 3783

src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (1)

227-230: JavaDoc comments are updated appropriately

The method documentation accurately reflects the new parameter createNewCourseCompetency, ensuring consistency between the code and its documentation.

Copy link

⚠️ Unable to deploy to test servers ⚠️

Testserver "artemis-test5.artemis.cit.tum.de" is already in use by PR #9715.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Nov 13, 2024
@github-actions github-actions bot removed the deployment-error Added by deployment workflows if an error occured label Nov 13, 2024
Copy link

⚠️ Unable to deploy to test servers ⚠️

Testserver "artemis-test5.artemis.cit.tum.de" is already in use by PR #9715.

@github-actions github-actions bot added the deployment-error Added by deployment workflows if an error occured label Nov 13, 2024
@github-actions github-actions bot removed the deployment-error Added by deployment workflows if an error occured label Nov 13, 2024
@vinceclifford vinceclifford temporarily deployed to artemis-test5.artemis.cit.tum.de November 13, 2024 19:48 — with GitHub Actions Inactive
Copy link

@vinceclifford vinceclifford left a comment

Choose a reason for hiding this comment

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

Tested on TS5, works as expected

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

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

Tested on TS5. No duplicate was imported

image

Copy link

@sachmii sachmii left a comment

Choose a reason for hiding this comment

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

Tested on TS4, works as described yet the duplicates disappear just after reloading the page.

Copy link

@sawys777 sawys777 left a comment

Choose a reason for hiding this comment

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

Duplicate Tested on TS1, everything almost worked as described, except that after importing the competencies, I need to refresh to make one of it to disappear.

@krusche krusche merged commit 7bf8bcd into develop Nov 16, 2024
128 of 134 checks passed
@krusche krusche deleted the feature/adaptive-learning/handle-existing-competencies-during-import branch November 16, 2024 09:13
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!) tests
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants