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

Development: Remove unused code related to ngx graph #9628

Merged
merged 9 commits into from
Nov 2, 2024

Conversation

JohannesStoehr
Copy link
Contributor

@JohannesStoehr JohannesStoehr commented Oct 29, 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 removed multiple integration tests (Spring) related to the features (with a high test coverage).

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 removed multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.

Motivation and Context

The nix graph representing competencies and their learning objects was replaced in multiple previous PRs and is unused now.

Description

This PR removes all legacy learning path code for the server and client.
The removed client code was covered better than the average, so the coverage thresholds have to be lowered a bit.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Student
  • 1 Course with competencies and exercises
  1. Log in to Artemis
  2. Check that the learning paths page for instructors or any clickable element in it does not look broken or completely empty
  3. Check that the learning path view for a students or any clickable element in it does not look broken or completely empty

Testserver States

Note

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







Review Progress

Performance Review

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Summary by CodeRabbit

  • New Features
    • Removed support for Ngx representations of learning paths, including related components and services.
  • Bug Fixes
    • Adjusted code coverage thresholds for various metrics.
  • Documentation
    • Updated routing configuration for learning paths to load components directly instead of modules.
  • Chores
    • Deleted multiple components, services, and associated tests related to the Ngx learning path functionality.
    • Streamlined the LearningPathStudentPageComponent by simplifying route parameter extraction.

@JohannesStoehr JohannesStoehr added this to the 7.6.5 milestone Oct 29, 2024
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) atlas Pull requests that affect the corresponding module labels Oct 29, 2024
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Oct 29, 2024
@JohannesStoehr JohannesStoehr marked this pull request as ready for review October 30, 2024 11:48
@JohannesStoehr JohannesStoehr requested a review from a team as a code owner October 30, 2024 11:48
@iyannsch iyannsch self-requested a review October 30, 2024 11:59
@iyannsch iyannsch temporarily deployed to artemis-test6.artemis.cit.tum.de October 30, 2024 12:00 — with GitHub Actions Inactive
Copy link

coderabbitai bot commented Oct 30, 2024

Walkthrough

The changes in this pull request involve the removal of various components, services, and tests related to the learning path functionality in an Angular application. Key modifications include the deletion of the NgxLearningPathDTO, LearningPathNgxService, and associated UI components for rendering learning paths. Additionally, adjustments were made to code coverage thresholds in configuration files, reflecting a slight decrease in required coverage metrics. The integration tests and unit tests for the removed components and services were also eliminated, indicating a significant reduction in the testing scope for learning paths.

Changes

File Path Change Summary
build.gradle Adjusted JaCoCo plugin coverage threshold for "INSTRUCTION" from 0.894 to 0.892.
jest.config.js Modified coverage thresholds for statements, branches, functions, and lines.
src/main/java/de/tum/cit/aet/artemis/atlas/dto/NgxLearningPathDTO.java Deleted file defining DTO for learning paths, including records and enum.
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNgxService.java Deleted service responsible for generating Ngx representations of learning paths.
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java Removed dependency on LearningPathNgxService and related methods for Ngx representations.
src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java Deleted methods related to fetching Ngx representations of learning paths.
src/main/webapp/app/course/learning-paths/learning-path-graph/*.html Deleted various HTML templates for rendering learning path graphs and nodes.
src/main/webapp/app/course/learning-paths/learning-path-graph/*.scss Deleted associated styles for learning path graph components.
src/main/webapp/app/course/learning-paths/learning-path-graph/*.ts Deleted TypeScript components for rendering learning path graphs and nodes.
src/main/webapp/app/course/learning-paths/learning-path-management/*.html Deleted HTML templates for managing learning paths.
src/main/webapp/app/course/learning-paths/learning-path-management/*.ts Deleted TypeScript components for managing learning paths.
src/main/webapp/app/course/learning-paths/participate/*.ts Deleted components related to participating in learning paths.
src/main/webapp/app/course/learning-paths/progress-modal/*.html Deleted modal and navigation components for learning path progress.
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java Removed multiple integration tests related to learning paths.
src/test/java/de/tum/cit/aet/artemis/atlas/service/LearningPathServiceTest.java Deleted nested test classes and methods focused on Ngx graph representation.
src/test/javascript/spec/component/learning-paths/graph/*.spec.ts Deleted unit tests for various learning path components and services.
src/test/javascript/spec/service/learning-path-paging.service.spec.ts Deleted unit tests for the LearningPathPagingService.
src/test/javascript/spec/service/learning-path-storage.service.spec.ts Deleted unit tests for the LearningPathStorageService.
src/test/javascript/spec/service/learning-path.service.spec.ts Deleted unit tests for the LearningPathService.
src/test/javascript/spec/component/learning-paths/management/*.spec.ts Deleted unit tests for management components related to learning paths.

Possibly related PRs

Suggested labels

ready to merge, small, bugfix

Suggested reviewers

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

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 (4)
jest.config.js (1)

104-108: Consider addressing the TODO comment in a future PR.

There's a TODO comment indicating these coverage thresholds should increase to at least 90% in the future. Given that this PR removes well-tested code (as mentioned in the PR description), it might be worth creating a tracking issue to gradually improve these metrics.

Would you like me to create a GitHub issue to track the coverage improvements?

src/main/webapp/app/overview/courses-routing.module.ts (1)

192-194: Remove commented out code and approve architectural change.

The switch from loadChildren (module loading) to loadComponent (standalone component loading) is a good architectural decision that aligns with modern Angular practices and helps reduce bundle size. However, the commented-out code should be removed as it's no longer needed.

Apply this diff to clean up the code:

  loadComponent: () =>
      import('app/course/learning-paths/pages/learning-path-student-page/learning-path-student-page.component').then((c) => c.LearningPathStudentPageComponent),
-  // loadChildren: () => import('app/course/learning-paths/learning-paths.module').then((m) => m.ArtemisLearningPathsModule),
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (2)

Line range hint 48-56: Update class documentation to remove ngx graph reference

The class-level documentation still mentions "retrieval of ngx graph representations" which is no longer a valid feature after the removal of the ngx graph functionality.

Apply this diff to update the documentation:

 * This includes
 * <ul>
 * <li>the generation of learning paths in courses,</li>
 * <li>performing pageable searches for learning paths,</li>
- * <li>performing health status checks,</li>
- * <li>and retrieval of ngx graph representations.</li>
+ * <li>and performing health status checks.</li>
 * </ul>

Line range hint 419-484: Consider optimizing database queries in findWithCompetenciesAndReleasedLearningObjectsAndCompletedUsersById

The method performs multiple separate database queries and in-memory operations. Consider optimizing by:

  1. Using a single JOIN query to fetch competency progresses, lecture unit completions, and student participations
  2. Implementing batch fetching using @batchsize annotation
  3. Adding appropriate database indexes for frequently queried columns

This would reduce the number of database roundtrips and improve performance, especially with large datasets.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ac4c2c1 and 85f7705.

📒 Files selected for processing (65)
  • build.gradle (1 hunks)
  • jest.config.js (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/atlas/dto/NgxLearningPathDTO.java (0 hunks)
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNgxService.java (0 hunks)
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-graph.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-graph.component.scss (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-graph.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-graph.module.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-legend.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-legend.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-node.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-node.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path.component.scss (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/competency-node-details.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/competency-node-details.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/exercise-node-details.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/exercise-node-details.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/lecture-unit-node-details.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/lecture-unit-node-details.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-health-status-warning.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-health-status-warning.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-management.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-management.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-management.module.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path-paging.service.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-path.service.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/learning-paths.module.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/participate/learning-path-container.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/participate/learning-path-container.component.scss (0 hunks)
  • src/main/webapp/app/course/learning-paths/participate/learning-path-container.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/participate/learning-path-storage.service.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/participate/lecture-unit/learning-path-lecture-unit-view.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/participate/lecture-unit/learning-path-lecture-unit-view.component.scss (0 hunks)
  • src/main/webapp/app/course/learning-paths/participate/lecture-unit/learning-path-lecture-unit-view.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/participate/lecture-unit/learning-path-lecture-unit-view.module.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-modal.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-modal.component.scss (0 hunks)
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-modal.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-nav.component.html (0 hunks)
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-nav.component.ts (0 hunks)
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress.module.ts (0 hunks)
  • src/main/webapp/app/course/manage/course-management.module.ts (0 hunks)
  • src/main/webapp/app/overview/courses-routing.module.ts (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (0 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/service/LearningPathServiceTest.java (0 hunks)
  • src/test/javascript/spec/component/learning-paths/graph/learning-path-graph-node.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/graph/learning-path-graph.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/graph/learning-path-legend.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/graph/learning-path.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/graph/node-details/competency-node-details.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/graph/node-details/exercise-node-details.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/graph/node-details/lecture-unit-node-details.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/management/learning-path-health-status-warning.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/management/learning-path-management.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/management/learning-path-progress-modal.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/management/learning-path-progress-nav.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/participate/learning-path-container.component.spec.ts (0 hunks)
  • src/test/javascript/spec/component/learning-paths/participate/learning-path-lecture-unit-view.component.spec.ts (0 hunks)
  • src/test/javascript/spec/service/learning-path-paging.service.spec.ts (0 hunks)
  • src/test/javascript/spec/service/learning-path-storage.service.spec.ts (0 hunks)
  • src/test/javascript/spec/service/learning-path/learning-path.service.spec.ts (0 hunks)
💤 Files with no reviewable changes (61)
  • src/main/java/de/tum/cit/aet/artemis/atlas/dto/NgxLearningPathDTO.java
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNgxService.java
  • src/main/java/de/tum/cit/aet/artemis/atlas/web/LearningPathResource.java
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-graph.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-graph.component.scss
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-graph.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-graph.module.ts
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-legend.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-legend.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-node.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path-node.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path.component.scss
  • src/main/webapp/app/course/learning-paths/learning-path-graph/learning-path.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/competency-node-details.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/competency-node-details.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/exercise-node-details.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/exercise-node-details.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/lecture-unit-node-details.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-graph/node-details/lecture-unit-node-details.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-health-status-warning.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-health-status-warning.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-management.component.html
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-management.component.ts
  • src/main/webapp/app/course/learning-paths/learning-path-management/learning-path-management.module.ts
  • src/main/webapp/app/course/learning-paths/learning-path-paging.service.ts
  • src/main/webapp/app/course/learning-paths/learning-path.service.ts
  • src/main/webapp/app/course/learning-paths/learning-paths.module.ts
  • src/main/webapp/app/course/learning-paths/participate/learning-path-container.component.html
  • src/main/webapp/app/course/learning-paths/participate/learning-path-container.component.scss
  • src/main/webapp/app/course/learning-paths/participate/learning-path-container.component.ts
  • src/main/webapp/app/course/learning-paths/participate/learning-path-storage.service.ts
  • src/main/webapp/app/course/learning-paths/participate/lecture-unit/learning-path-lecture-unit-view.component.html
  • src/main/webapp/app/course/learning-paths/participate/lecture-unit/learning-path-lecture-unit-view.component.scss
  • src/main/webapp/app/course/learning-paths/participate/lecture-unit/learning-path-lecture-unit-view.component.ts
  • src/main/webapp/app/course/learning-paths/participate/lecture-unit/learning-path-lecture-unit-view.module.ts
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-modal.component.html
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-modal.component.scss
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-modal.component.ts
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-nav.component.html
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress-nav.component.ts
  • src/main/webapp/app/course/learning-paths/progress-modal/learning-path-progress.module.ts
  • src/main/webapp/app/course/manage/course-management.module.ts
  • src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java
  • src/test/java/de/tum/cit/aet/artemis/atlas/service/LearningPathServiceTest.java
  • src/test/javascript/spec/component/learning-paths/graph/learning-path-graph-node.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/graph/learning-path-graph.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/graph/learning-path-legend.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/graph/learning-path.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/graph/node-details/competency-node-details.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/graph/node-details/exercise-node-details.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/graph/node-details/lecture-unit-node-details.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/management/learning-path-health-status-warning.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/management/learning-path-management.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/management/learning-path-progress-modal.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/management/learning-path-progress-nav.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/participate/learning-path-container.component.spec.ts
  • src/test/javascript/spec/component/learning-paths/participate/learning-path-lecture-unit-view.component.spec.ts
  • src/test/javascript/spec/service/learning-path-paging.service.spec.ts
  • src/test/javascript/spec/service/learning-path-storage.service.spec.ts
  • src/test/javascript/spec/service/learning-path/learning-path.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.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/overview/courses-routing.module.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 (4)
jest.config.js (1)

105-108: LGTM! Coverage threshold adjustments are reasonable.

The minor decreases in coverage thresholds (≤0.08%) align well with the PR's objective of removing the unused ngx graph components. The changes are minimal and proportional to the removed code.

src/main/webapp/app/overview/courses-routing.module.ts (1)

192-194: Verify module-level dependencies.

Since we're transitioning from a module-based to a component-based approach, please verify that any module-level providers (services, guards, etc.) previously provided by ArtemisLearningPathsModule are now properly handled.

Let's check for any module-level providers that might need migration:

src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (1)

94-95: LGTM: Constructor changes align with ngx graph removal

The removal of the NgxService dependency from the constructor is consistent with the PR's objective of removing unused ngx graph functionality.

build.gradle (1)

180-180: LGTM! Verify the coverage threshold adjustment.

The decrease in coverage threshold from 0.894 to 0.892 (0.2%) is reasonable given the removal of well-tested ngx graph components.

Let's verify the current coverage metrics to ensure the new threshold is appropriate:

✅ Verification successful

Coverage threshold adjustment is appropriate

The code context shows:

  • A TODO comment indicating a future goal to increase coverage above 0.92
  • The current adjustment to 0.892 is a temporary measure
  • The change is part of a well-configured JaCoCo setup with proper tooling and reporting

The threshold decrease is minimal and justified as an intermediate step, with clear documentation of plans to increase it in the future.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the current test coverage to validate the threshold adjustment

# Run tests and generate coverage report
./gradlew test jacocoTestReport

# Extract the current instruction coverage from the XML report
rg -A 1 'counter type="INSTRUCTION"' build/reports/jacoco/test/jacocoTestReport.xml

Length of output: 391


Script:

#!/bin/bash
# Let's try a different approach to analyze the coverage threshold context
# First, check if there are any other coverage thresholds in the build file
rg -B2 -A2 "minimum = 0\.89" build.gradle

# Also look for any coverage-related configuration or tasks
rg -B2 -A2 "jacoco" build.gradle

# Check for any test-related configuration
rg -B2 -A2 "test[[:space:]]*{" build.gradle

Length of output: 1817

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 30, 2024
cremertim
cremertim previously approved these changes Oct 30, 2024
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.

Clean 👍

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 (4)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (4)

Line range hint 52-59: Update Documentation to Reflect Removal of Ngx Graph Representations

The class-level Javadoc still mentions the retrieval of Ngx graph representations, which is no longer applicable after removing the Ngx-related methods and services.

Please update the documentation to remove references to Ngx graph representations. Here's the suggested change:

 /**
  * Service Implementation for managing Learning Paths.
  * <p>
  * This includes
  * <ul>
  * <li>the generation of learning paths in courses,</li>
  * <li>performing pageable searches for learning paths,</li>
  * <li>performing health status checks,</li>
- * <li>and retrieval of ngx graph representations.</li>
  * </ul>
  */

Line range hint 162-164: Consider Optimizing Progress Calculation Logic

In the updateLearningPathProgress method, the calculation of progress could be optimized for clarity and efficiency.

Here's a suggested refactor:

  • Use learningPath.getCompetencies().size() only once.
  • Avoid setting progress to 0 explicitly when there are no competencies, as the division handles this case.
 private void updateLearningPathProgress(@NotNull LearningPath learningPath) {
     final var userId = learningPath.getUser().getId();
     final var competencyIds = learningPath.getCompetencies().stream().map(CourseCompetency::getId).collect(Collectors.toSet());
     final var competencyProgresses = competencyProgressRepository.findAllByCompetencyIdsAndUserId(competencyIds, userId);

-    final float completed = competencyProgresses.stream().filter(CompetencyProgressService::isMastered).count();
-    final var numberOfCompetencies = learningPath.getCompetencies().size();
-    if (numberOfCompetencies == 0) {
-        learningPath.setProgress(0);
-    }
-    else {
-        learningPath.setProgress(Math.round(completed * 100 / numberOfCompetencies));
-    }
+    final int completed = (int) competencyProgresses.stream().filter(CompetencyProgressService::isMastered).count();
+    final int totalCompetencies = learningPath.getCompetencies().size();
+    final int progress = totalCompetencies == 0 ? 0 : Math.round((float) completed * 100 / totalCompetencies);
+    learningPath.setProgress(progress);
     learningPathRepository.save(learningPath);
     log.debug("Updated LearningPath (id={}) for user (id={})", learningPath.getId(), userId);
 }

This refactor improves readability by simplifying the logic.


Line range hint 52-59: Adhere to the Single Responsibility Principle in Documentation

The documentation lists multiple responsibilities, which might indicate that the class is handling more than one responsibility, potentially violating the single responsibility principle.

Consider reviewing the class's responsibilities and refactoring if necessary to ensure that it adheres to best practices.


Line range hint 52-59: Remove Duplicate or Unnecessary Imports

Review the import statements and remove any unused or duplicate imports to clean up the code:

-import de.tum.cit.aet.artemis.atlas.dto.LearningPathNgxDTO;
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 25713ac and 9cf1242.

📒 Files selected for processing (2)
  • src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (0 hunks)
💤 Files with no reviewable changes (1)
  • src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.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

🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java (3)

Line range hint 259-261: Ensure Proper Null Handling for User in Learning Path

In the findWithCompetenciesAndReleasedLearningObjectsAndCompletedUsersById method, when checking if learningPath.getUser() is null, consider handling potential NullPointerException and ensure that the application logic accounts for this scenario appropriately.

Please verify that learningPath.getUser() cannot be null at this point, or add necessary null checks to prevent runtime exceptions.


95-96: Inject Dependencies via Constructor for Better Testability

The current constructor is injecting several repositories and services. Ensure that all dependencies are essential and consider using constructor injection for better testability and adherence to the dependency inversion principle.

[best_practice]

If any of these repositories are optional, consider injecting them via setters or using optional dependencies.


95-96: Verify Dependency Usage in Constructor Parameters

After removing the LearningPathNgxService, ensure that all remaining injected dependencies in the constructor are still necessary and used within the class.

Run the following script to check for unused constructor parameters:

This script will list any constructor parameters that appear to be unused or used less than twice (once in the constructor and once elsewhere). Please review and remove any unnecessary dependencies to adhere to the single responsibility principle and improve maintainability.

✅ Verification successful

All Constructor Dependencies Are Actively Used

Based on the analysis, all constructor parameters are properly utilized within the class:

  • CompetencyRelationRepository: Used for querying relations and counts (6 occurrences)
  • LectureUnitCompletionRepository: Used for finding lecture unit completions (4 occurrences)
  • StudentParticipationRepository: Used for finding student participations (4 occurrences)
  • CourseCompetencyRepository: Used for finding course competencies (4 occurrences)

Each dependency is properly initialized in the constructor and used in meaningful operations throughout the class.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if all constructor parameters are used in the class methods.

# Extract constructor parameter names
params=$(rg 'public LearningPathService\((.*?)\)' -or '$1' -r | sed 's/, /\n/g' | awk '{print $2}')

# Check usage of each parameter in the class file
for param in $params; do
  usage_count=$(rg -c "\b$param\b" src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java)
  if [[ $usage_count -le 2 ]]; then
    echo "Parameter '$param' might be unused or underutilized."
  fi
done

Length of output: 542


Script:

#!/bin/bash
# Let's fix the command and check for constructor parameters and their usage

# First, let's get the full content of the constructor and class usage
echo "=== Constructor and Parameter Usage Analysis ==="
rg -A 2 "public LearningPathService\(" src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java

echo -e "\n=== Usage of CompetencyRelationRepository ==="
rg "competencyRelationRepository" src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java

echo -e "\n=== Usage of LectureUnitCompletionRepository ==="
rg "lectureUnitCompletionRepository" src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java

echo -e "\n=== Usage of StudentParticipationRepository ==="
rg "studentParticipationRepository" src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java

echo -e "\n=== Usage of CourseCompetencyRepository ==="
rg "courseCompetencyRepository" src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathService.java

Length of output: 3532

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 TS1. Everything was great!

Copy link

@SindiBuklaji SindiBuklaji 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 Ts1. Everything works as expected👍

Copy link

@alekspetrov9e alekspetrov9e 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 TS1. No issues were found. Could generate learning paths and also view learning paths.

Copy link
Contributor

@coolchock coolchock 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

Copy link
Contributor

@florian-glombik florian-glombik left a comment

Choose a reason for hiding this comment

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

Re-approve code after merge

@krusche krusche changed the title Adaptive learning: Remove unused ngx graph Development: Remove unused code related to ngx graph Nov 2, 2024
@krusche krusche merged commit f30a41c into develop Nov 2, 2024
62 of 65 checks passed
@krusche krusche deleted the chore/adaptive-learning/remove-ngx-graph branch November 2, 2024 06:46
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 to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Merged
Status: Done
Development

Successfully merging this pull request may close these issues.