Adaptive learning: Fix multiple small bugs
#9756
Conversation
github-actions
bot
added
server
github-actions
bot
added
the
client
JohannesWt
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
JohannesWt
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
# Conflicts: # src/main/webapp/app/course/competencies/competency-management/competency-management.component.ts
WalkthroughThis pull request introduces several significant changes across multiple files related to competency management in the application. A new record, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (7.7.0)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.javaThe 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.javaThe 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/web/CourseCompetencyResource.javaThe 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. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (14)
src/main/webapp/app/course/competencies/components/course-competencies-management-table/course-competencies-management-table.component.ts (3)
67-69: Enhance error handling in the catch blockDirectly accessing
error.messagemay lead to issues iferroris not an instance ofErroror doesn't have amessageproperty. Consider using a more robust method to extract the error message.Apply this diff for improved error handling:
- this.dialogErrorSource.emit(error.message); + const message = error instanceof Error ? error.message : 'An unexpected error occurred'; + this.dialogErrorSource.emit(message);
86-102: Extract mapping logic into a private method for reusabilityThe mapping logic converting
CompetencyWithTailRelationDTOtoCourseCompetencyStudentProgressDTOis complex and could be reused elsewhere. Extracting it into a private method improves readability and promotes code reuse.Apply this diff to refactor the mapping logic:
+ private mapToCourseCompetencyStudentProgressDTO(courseCompetency: CourseCompetency): CourseCompetencyStudentProgressDTO { + return { + id: courseCompetency.id, + title: courseCompetency.title, + description: courseCompetency.description, + numberOfMasteredStudents: 0, + numberOfStudents: 0, + optional: courseCompetency.optional, + softDueDate: courseCompetency.softDueDate, + taxonomy: courseCompetency.taxonomy, + type: courseCompetency.type, + }; + } // Within importAllCourseCompetencies method const importedCompetencies = importedCompetenciesWithTailRelation .map((dto) => dto.competency) .filter((element): element is CourseCompetency => !!element) - .map( - (courseCompetency) => - <CourseCompetencyStudentProgressDTO>{ - id: courseCompetency.id, - title: courseCompetency.title, - description: courseCompetency.description, - numberOfMasteredStudents: 0, - numberOfStudents: 0, - optional: courseCompetency.optional, - softDueDate: courseCompetency.softDueDate, - taxonomy: courseCompetency.taxonomy, - type: courseCompetency.type, - }, - ); + .map((courseCompetency) => this.mapToCourseCompetencyStudentProgressDTO(courseCompetency));
113-114: Improve error reporting in the catch blockPassing the entire
errorobject toalertService.error()may not display meaningful information. Extract a user-friendly message to inform the user appropriately.Apply this diff for better error reporting:
- this.alertService.error(error); + const message = error instanceof Error ? error.message : 'An unexpected error occurred'; + this.alertService.error(message);src/main/webapp/app/course/competencies/competency-management/competency-management.component.ts (1)
31-31: Remove unused importCompetencyManagementTableComponentThe
CompetencyManagementTableComponentimported at line 31 is not used within this component. Removing it cleans up the code and eliminates unnecessary imports.Apply this diff to remove the unused import:
- imports: [CompetencyManagementTableComponent, TranslateDirective, FontAwesomeModule, RouterModule, ArtemisSharedComponentModule, CourseCompetenciesManagementTableComponent], + imports: [TranslateDirective, FontAwesomeModule, RouterModule, ArtemisSharedComponentModule, CourseCompetenciesManagementTableComponent],src/main/webapp/app/course/competencies/services/course-competency-api.service.ts (1)
44-44: Consider using a more RESTful API endpointAppending
-student-progressto the endpoint URL may not align with RESTful principles. Consider using a query parameter or a sub-resource for fetching competencies with student progress.For example, the endpoint could be:
return await this.get<CourseCompetencyStudentProgressDTO[]>(`${this.getPath(courseId)}/student-progress`);src/main/webapp/app/course/competencies/competency-management/competency-management.component.html (1)
42-48: Ensure consistent event handling between tablesThe duplicate table component with identical event bindings suggests potential code duplication. Consider extracting common event handling logic into a shared service or parent component.
src/main/webapp/app/course/learning-paths/components/competency-node/competency-node.component.ts (1)
Line range hint
41-65: Consider extracting value comparison logicThe repeated switch statements with similar logic across
isGreen,isYellow, andisGraymethods suggest potential for refactoring.+ private isValueType(type: CompetencyGraphNodeValueType): boolean { + return this.valueType() === type; + } + + private isProgressType(): boolean { + return this.isValueType(CompetencyGraphNodeValueType.MASTERY_PROGRESS) || + this.isValueType(CompetencyGraphNodeValueType.AVERAGE_MASTERY_PROGRESS); + } + isGreen(): boolean { - switch (this.valueType()) { - case CompetencyGraphNodeValueType.MASTERY_PROGRESS: - case CompetencyGraphNodeValueType.AVERAGE_MASTERY_PROGRESS: - return this.value() >= 100; - default: - return false; - } + return this.isProgressType() && this.value() >= 100; }src/test/javascript/spec/service/course-competency/course-competency-api.service.spec.ts (1)
Line range hint
94-102: Enhance test coverage for student progress dataThe test verifies the API call but doesn't validate the response data structure. Consider adding test cases for:
- Response data structure matching
CompetencyStudentProgressDTO- Edge cases (empty progress, null values)
- Error scenarios
it('should get course competencies with student progress by course id', async () => { const methodCall = courseCompetencyApiService.getCourseCompetenciesWithStudentProgressByCourseId(courseId); const response = httpClient.expectOne({ method: 'GET', url: `${getBasePath(courseId)}-student-progress`, }); - response.flush([]); + const mockData = [{ + competencyId: 1, + averageMasteryProgress: 85, + masteredStudentsCount: 10 + }]; + response.flush(mockData); + const result = await methodCall; + expect(result[0].averageMasteryProgress).toBe(85); + expect(result[0].masteredStudentsCount).toBe(10); });src/main/webapp/app/entities/competency.model.ts (1)
66-76: Strengthen type safety of the DTO interface.Consider the following improvements to make the interface more robust:
export interface CourseCompetencyStudentProgressDTO { id: number; title: string; - description?: string; + description: string | null; - taxonomy?: CompetencyTaxonomy; + taxonomy: CompetencyTaxonomy | null; - softDueDate?: dayjs.Dayjs; + softDueDate: dayjs.Dayjs | null; optional: boolean; numberOfStudents: number; numberOfMasteredStudents: number; - type: CourseCompetencyType; + type: CourseCompetencyType.COMPETENCY | CourseCompetencyType.PREREQUISITE; }This change:
- Uses explicit null types instead of optional fields
- Restricts the type field to valid enum values
src/test/javascript/spec/component/competencies/competency-management/competency-management.component.spec.ts (2)
102-110: Enhance test data coverage for edge cases.The mock competency data doesn't cover important edge cases:
- Missing test for 100% mastery (numberOfMasteredStudents = numberOfStudents)
- Missing test for 0 students
- Missing test for null/undefined optional fields
Add more test cases:
const competency = <CourseCompetencyStudentProgressDTO>{ id: 1, description: 'test', type: CourseCompetencyType.COMPETENCY, numberOfStudents: 8, numberOfMasteredStudents: 5, optional: false, title: 'Competency 1', }; + +const edgeCaseCompetencies = [ + { + ...competency, + numberOfStudents: 10, + numberOfMasteredStudents: 10, // 100% mastery + }, + { + ...competency, + numberOfStudents: 0, + numberOfMasteredStudents: 0, // No students + }, + { + ...competency, + optional: undefined, // Test undefined optional field + }, +] as CourseCompetencyStudentProgressDTO[];
192-192: Use specific HTTP error status for error testing.The error test uses a generic 500 status. Consider testing specific error scenarios.
-getIrisSettingsSpy.mockReturnValue(new HttpErrorResponse({ status: 500 })); +getIrisSettingsSpy.mockReturnValue(new HttpErrorResponse({ + status: 503, + statusText: 'Service Unavailable', + error: { message: 'IRIS settings service is temporarily unavailable' } +}));src/test/javascript/spec/component/competencies/components/course-competencies-management-table.component.spec.ts (2)
25-44: Enhance test data with more diverse scenarios.The mock data could be expanded to test more scenarios:
- Missing test for competencies with different taxonomies
- No test for competencies with soft due dates
- Limited testing of optional flag variations
Expand the test data:
const courseCompetencies: CourseCompetencyStudentProgressDTO[] = [ { id: 1, type: CourseCompetencyType.COMPETENCY, title: 'Competency 1', description: 'Description 1', numberOfMasteredStudents: 1, numberOfStudents: 2, optional: false, + taxonomy: 'UNDERSTAND', + softDueDate: new Date('2024-12-31'), }, { id: 2, type: CourseCompetencyType.PREREQUISITE, title: 'Prerequisite 1', description: 'Description 2', numberOfMasteredStudents: 3, numberOfStudents: 4, optional: false, + taxonomy: 'APPLY', }, + { + id: 3, + type: CourseCompetencyType.COMPETENCY, + title: 'Optional Competency', + description: 'Description 3', + numberOfMasteredStudents: 0, + numberOfStudents: 5, + optional: true, + taxonomy: 'CREATE', + }, ];
194-222: Add test for concurrent import operations.The import functionality should be tested for race conditions when multiple imports are triggered in quick succession.
Add a test case:
it('should handle concurrent imports correctly', async () => { fixture.detectChanges(); const openSpy = jest.spyOn(fixture.componentRef.injector.get(NgbModal), 'open') .mockReturnValue({ componentInstance: { disabledIds: [], competencyType: '', }, result: modalResult, } as any); const importAllCompetenciesSpy = jest.spyOn(competencyService, 'importAll') .mockImplementation(() => new Promise(resolve => setTimeout(resolve, 100))); // Trigger multiple imports const importPromises = [ component.importCompetencies(), component.importCompetencies(), ]; await Promise.all(importPromises); expect(importAllCompetenciesSpy).toHaveBeenCalledTimes(1); });src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java (1)
179-191: LGTM with caching consideration.The new endpoint is well-implemented with proper security controls and follows REST conventions. Consider adding caching for performance optimization, especially since this endpoint aggregates student progress data which might be computationally expensive for courses with many students.
Consider adding the following caching annotations:
+@Cacheable(value = "courseCompetencyStudentProgress", key = "#courseId") public ResponseEntity<List<CompetencyStudentProgressDTO>> getCourseCompetenciesWithStudentProgress(@PathVariable long courseId) {Also ensure to add cache eviction in relevant update methods:
+@CacheEvict(value = "courseCompetencyStudentProgress", key = "#courseId")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
src/main/java/de/tum/cit/aet/artemis/atlas/dto/CompetencyStudentProgressDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyProgressRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java(3 hunks)src/main/webapp/app/course/competencies/competency-management/competency-management.component.html(2 hunks)src/main/webapp/app/course/competencies/competency-management/competency-management.component.ts(7 hunks)src/main/webapp/app/course/competencies/components/course-competencies-management-table/course-competencies-management-table.component.html(1 hunks)src/main/webapp/app/course/competencies/components/course-competencies-management-table/course-competencies-management-table.component.scss(1 hunks)src/main/webapp/app/course/competencies/components/course-competencies-management-table/course-competencies-management-table.component.ts(1 hunks)src/main/webapp/app/course/competencies/services/course-competency-api.service.ts(2 hunks)src/main/webapp/app/course/learning-paths/components/competency-node/competency-node.component.ts(1 hunks)src/main/webapp/app/entities/competency.model.ts(1 hunks)src/main/webapp/i18n/de/competency.json(1 hunks)src/main/webapp/i18n/en/competency.json(1 hunks)src/test/javascript/spec/component/competencies/competency-management/competency-management.component.spec.ts(5 hunks)src/test/javascript/spec/component/competencies/components/course-competencies-management-table.component.spec.ts(1 hunks)src/test/javascript/spec/helpers/mocks/service/mock-alert.service.ts(1 hunks)src/test/javascript/spec/service/course-competency/course-competency-api.service.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/webapp/app/course/competencies/components/course-competencies-management-table/course-competencies-management-table.component.scss
- src/main/webapp/i18n/de/competency.json
- src/main/webapp/i18n/en/competency.json
🧰 Additional context used
📓 Path-based instructions (16)
src/main/webapp/app/course/competencies/components/course-competencies-management-table/course-competencies-management-table.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/test/javascript/spec/helpers/mocks/service/mock-alert.service.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/java/de/tum/cit/aet/artemis/atlas/dto/CompetencyStudentProgressDTO.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/learning-paths/components/competency-node/competency-node.component.ts (1)
src/test/javascript/spec/component/competencies/components/course-competencies-management-table.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyProgressRepository.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/repository/CourseCompetencyRepository.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/entities/competency.model.ts (1)
src/test/javascript/spec/service/course-competency/course-competency-api.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/course/competencies/components/course-competencies-management-table/course-competencies-management-table.component.ts (1)
src/main/webapp/app/course/competencies/services/course-competency-api.service.ts (1)
src/test/javascript/spec/component/competencies/competency-management/competency-management.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.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.component.ts (1)
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.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-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.
🔇 Additional comments (13)
src/test/javascript/spec/helpers/mocks/service/mock-alert.service.ts (1)
7-7: LGTM!
The addition of the warning method aligns with the existing mock alert methods and enhances the mock service functionality.
src/main/java/de/tum/cit/aet/artemis/atlas/dto/CompetencyStudentProgressDTO.java (1)
1-17: LGTM!
The CompetencyStudentProgressDTO record follows best practices by encapsulating the required data and providing a static factory method for object creation.
src/main/webapp/app/course/competencies/services/course-competency-api.service.ts (1)
43-44: Verify that all references to the old method are updated
Replacing getCourseCompetenciesByCourseId with getCourseCompetenciesWithStudentProgressByCourseId may impact other components relying on the old method. Ensure all usages are updated to prevent runtime errors.
Run the following script to identify any remaining references:
✅ Verification successful
Based on the search results, I can now generate the final response:
The method replacement is correctly implemented with no remaining references to the old method
The comprehensive search results show that:
- The new method
getCourseCompetenciesWithStudentProgressByCourseIdis being used in the codebase, specifically in the competency management component. - There are no occurrences of the old method name
getCourseCompetenciesByCourseIdin any TypeScript files. - All related components and tests are using the new method name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any usage of the old method in TypeScript files.
# Test: Expect no occurrences of 'getCourseCompetenciesByCourseId'
rg -t ts 'getCourseCompetenciesByCourseId'
Length of output: 42
Script:
#!/bin/bash
# Let's try a more comprehensive search to understand the context better
# 1. Search for any file containing 'CourseCompetency' to identify relevant files
echo "=== Files containing CourseCompetency ==="
rg -l "CourseCompetency"
# 2. Look for any method calls related to course competencies
echo -e "\n=== Method calls related to course competencies ==="
rg -t ts "getCourseCompetenc\w+" -A 2
# 3. Check the service class implementation for better context
echo -e "\n=== Course competency service implementation ==="
rg -t ts "class.*CourseCompetencyApiService" -A 10
Length of output: 23966
src/main/webapp/app/course/competencies/competency-management/competency-management.component.html (2)
4-4: Consider maintaining visual hierarchy with h2
Changing from h2 to h4 might affect the page's heading hierarchy and accessibility. Unless this is part of a broader heading structure update, consider keeping it as h2 for better semantic structure.
✅ Verification successful
Based on the search results, I can see that h4 headings are commonly used across the application for section titles and modal headers, particularly in management and configuration views. Some examples:
- Course management:
<h4 class="fw-medium" id="course-page-heading"> - Tutorial group management:
<h4>{{ 'artemisApp.pages.tutorialGroupSessionManagement.title' }} - Competency management:
<h4 class="m-0" jhiTranslate="artemisApp.competency.manage.title"> - Configuration modals:
<h4 class="modal-title">
The change from h2 to h4 in the competency management component aligns with this pattern, where h4 is used for section headers within management views. The h2 elements appear to be reserved for page-level headers and major sections.
The h4 heading is appropriate for this context
The change from h2 to h4 follows the established heading hierarchy pattern in the codebase, where h4 is consistently used for management view section headers. This maintains visual consistency across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check heading hierarchy in other components
rg -A 1 "<h[1-6].*>" "src/main/webapp/app/course/"
Length of output: 41962
34-40: Verify event handler implementations for renamed events
The component has been renamed and events updated to be more specific:
competencyDeleted→onCourseCompetencyDeletion- New event:
onCourseCompetenciesImport
These changes improve clarity but require verification of handler implementations.
✅ Verification successful
The previous ast-grep pattern didn't yield results, so let's try a broader search to:
- Find the component file to check the handler implementations
- Search for any references to these event handlers across the codebase
Based on the search results, I can now generate the final response as I have found clear evidence about the event handler implementations:
Event handlers are correctly implemented and working as expected
The event handlers have been properly implemented in the CompetencyManagementComponent:
onRemoveCompetency($event)is implemented to filter out the deleted competency from the listonCourseCompetenciesImport($event)is implemented to add the imported competencies to the list
Both handlers are correctly bound in the template and the component emits these events with the expected payloads.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for handler implementations
ast-grep --pattern 'onRemoveCompetency($event) {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Search for the component file and event handler implementations
fd "competency-management.component.ts" --exec cat {}
# Also search for any references to these event handlers
rg "onRemoveCompetency|onCourseCompetencyDeletion|onCourseCompetenciesImport" -A 3
Length of output: 44225
src/main/webapp/app/course/learning-paths/components/competency-node/competency-node.component.ts (1)
41-44: Verify value range assumptions
The condition value() >= 100 assumes the new 0-100 scale. This aligns with the PR objective but needs validation for edge cases.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the value range assumptions:
Value range is consistent with the 0-100 scale
The codebase consistently uses a 0-100 scale for mastery and progress values:
- The competency-node component's value comparison
value() >= 100aligns with other mastery checks in the codebase - Other components like course-competencies-details also use the same scale:
if (this.mastery >= 100) - The enum
CompetencyGraphNodeValueTypeshows this is specifically forAVERAGE_MASTERY_PROGRESS - Form validation enforces this range:
min="1" max="100"for mastery threshold inputs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other value comparisons to ensure consistent scale usage
rg -A 2 "value[^a-zA-Z]*(>|<|>=|<=|===|==|!=)" "src/main/webapp/app/**/*.ts"
Length of output: 146
Script:
#!/bin/bash
# Search for other value comparisons to ensure consistent scale usage
rg -A 2 "value[^a-zA-Z]*(>|<|>=|<=|===|==|!=)" src/main/webapp/app/course/learning-paths/components/competency-node/competency-node.component.ts
Length of output: 455
Script:
#!/bin/bash
# Look for value() method definition and usage
ast-grep --pattern 'value() { $$$ }'
Length of output: 162
Script:
#!/bin/bash
# Search for any mastery or progress related value comparisons
rg -A 2 "(mastery|progress).*(>|<|>=|<=|===|==|!=)" src/main/webapp/app
Length of output: 28151
Script:
#!/bin/bash
# Find the enum definition for CompetencyGraphNodeValueType
rg -A 5 "enum CompetencyGraphNodeValueType" src/main/webapp/app
Length of output: 633
src/main/webapp/app/course/competencies/components/course-competencies-management-table/course-competencies-management-table.component.html (2)
71-74:
Add safeguard against division by zero.
The percentage calculation could throw an error if numberOfStudents is zero, despite the ternary check.
Move the zero check into the calculation:
-{{ courseCompetency.numberOfStudents == 0 ? 0 : ((courseCompetency.numberOfMasteredStudents / courseCompetency.numberOfStudents) * 100 | number: '1.0-0') }}
+{{ courseCompetency.numberOfStudents > 0 ? ((courseCompetency.numberOfMasteredStudents / courseCompetency.numberOfStudents) * 100 | number: '1.0-0') : 0 }}Likely invalid or redundant comment.
69-69: Review innerHTML usage for security risks.
Using innerHTML with htmlForMarkdown pipe could expose XSS vulnerabilities if the markdown sanitization is not properly implemented.
Let's verify the implementation of the htmlForMarkdown pipe:
✅ Verification successful
Let me analyze the implementation of htmlForMarkdown to verify the security measures. Let's check its implementation:
Let me try a different approach to find the implementation:
Let me try to get the complete implementation of the markdown conversion utility:
Let me check how the pipe and markdown service use this function:
Based on the implementation analysis, I can now provide the final response:
The innerHTML usage with htmlForMarkdown pipe is secure.
The implementation shows proper security measures:
- Uses DOMPurify for HTML sanitization
- Implements a two-layer sanitization approach:
- DOMPurify sanitization in
htmlForMarkdown - Angular's DomSanitizer in the markdown service
- DOMPurify sanitization in
- Allows controlled whitelisting of HTML tags and attributes through parameters
- Properly handles markdown conversion using markdown-it with secure defaults
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of htmlForMarkdown pipe
# Expected: Find sanitization logic in the pipe implementation
rg -A 10 "htmlForMarkdown"
Length of output: 66671
Script:
#!/bin/bash
# Find the implementation of htmlForMarkdown in markdown.conversion.util.ts
ast-grep --pattern 'export function htmlForMarkdown($$$)'
Length of output: 59
Script:
#!/bin/bash
# Find the implementation of htmlForMarkdown using ripgrep with context
rg -U "export function htmlForMarkdown[\s\S]*?\{[\s\S]*?\}" -g "*.ts"
Length of output: 909
Script:
#!/bin/bash
# Get the complete content of markdown.conversion.util.ts
cat src/main/webapp/app/shared/util/markdown.conversion.util.ts
Length of output: 3743
Script:
#!/bin/bash
# Get the implementation of HtmlForMarkdownPipe and ArtemisMarkdownService
cat src/main/webapp/app/shared/pipes/html-for-markdown.pipe.ts src/main/webapp/app/shared/markdown.service.ts
Length of output: 4613
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java (1)
299-316: 🛠️ Refactor suggestion
Consider optimizing the query performance and handling edge cases.
The query implementation has a few areas that could be improved:
- The COUNT aggregation might not handle null values correctly in the mastery calculation.
- The GROUP BY clause uses the entire entity which could impact performance.
- The type discrimination using CASE WHEN TYPE(c) could be replaced with a more efficient approach.
Consider these improvements:
@Query("""
SELECT new de.tum.cit.aet.artemis.atlas.dto.CompetencyStudentProgressDTO(
c.id,
c.title,
c.description,
c.taxonomy,
c.softDueDate,
c.optional,
- CASE WHEN TYPE(c) = Competency THEN 'competency' ELSE 'prerequisite' END,
+ c.type,
COUNT(DISTINCT up),
- COUNT(CASE WHEN up.progress * up.confidence >= c.masteryThreshold THEN 1 ELSE 0 END)
+ COUNT(DISTINCT CASE WHEN up IS NOT NULL AND up.progress * up.confidence >= c.masteryThreshold THEN up ELSE NULL END)
)
FROM CourseCompetency c
LEFT JOIN c.userProgress up
WHERE c.course.id = :courseId
- GROUP BY c
+ GROUP BY c.id, c.title, c.description, c.taxonomy, c.softDueDate, c.optional, c.type
""")Let's verify the query performance:
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java (2)
31-31: LGTM!
Clean import statement following Java conventions.
135-145: LGTM with verification needed.
The new method is well-documented and follows the single responsibility principle. However, let's verify the repository method implementation to ensure proper error handling.
✅ Verification successful
Let me gather more information about the repository implementation and error handling patterns.
Let me try one more search pattern to find the repository implementation and error handling.
Let me try one final search to find the specific query implementation for findWithStudentProgressByCourseId.
Let me try one final search to find the specific query implementation and error handling patterns.
Based on the code analysis performed, I can now generate the final response:
Repository method appears to be properly implemented with standard Spring Data JPA error handling.
The method follows Spring Data JPA conventions where:
- It returns a List which handles empty results gracefully (returns empty list instead of null)
- The courseId parameter is validated through Spring's built-in parameter validation
- The method is part of a JPA repository which includes standard exception translation
- The service layer provides a clean abstraction and follows the single responsibility principle
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of findWithStudentProgressByCourseId in CourseCompetencyRepository
# Search for the repository method implementation
ast-grep --pattern 'interface CourseCompetencyRepository {
$$$
List<CompetencyStudentProgressDTO> findWithStudentProgressByCourseId($_);
$$$
}'
# Search for any existing error handling patterns in repository methods
rg -A 5 'findWithStudentProgressByCourseId|@Query.*student.*progress'
Length of output: 1586
Script:
#!/bin/bash
# Search for the @Query annotation and implementation details
rg -B 2 -A 10 '@Query.*findWithStudentProgressByCourseId'
# Look for the DTO class to understand the structure
ast-grep --pattern 'class CompetencyStudentProgressDTO {
$$$
}'
# Check if there are any exception handling patterns in the service layer
rg -A 5 'try.*catch.*courseCompetencyRepository'
Length of output: 302
Script:
#!/bin/bash
# Search for the repository interface and its annotations
rg -B 5 -A 15 'interface CourseCompetencyRepository'
# Look for the DTO class with a different pattern
rg -B 2 -A 10 'class.*CompetencyStudentProgressDTO'
# Check for any exception handling in the service class
rg -B 5 -A 10 'class CourseCompetencyService'
Length of output: 6092
Script:
#!/bin/bash
# Search for the specific query implementation
rg -B 5 -A 10 '@Query.*findWithStudentProgressByCourseId|@Query.*student.*progress'
# Look for any exception handling in similar repository methods
rg -B 2 -A 5 'throw.*Exception.*courseId|throw.*Exception.*competency'
# Check for any null handling or validation in the service
rg -B 2 -A 5 'if.*courseId.*null|Optional.*courseCompetencyRepository'
Length of output: 114877
Script:
#!/bin/bash
# Search for the repository method and its implementation
rg -B 5 -A 10 '@Query.*findWithStudentProgressByCourseId|@Query.*student.*progress.*course'
# Look for any exception handling in the service layer for courseId
rg -B 2 -A 5 'courseCompetencyService.*throw|courseCompetencyRepository.*throw'
# Check for any validation annotations on the repository method
ast-grep --pattern 'interface CourseCompetencyRepository {
$$$
@Query($_)
List<CompetencyStudentProgressDTO> findWithStudentProgressByCourseId($_);
$$$
}'
Length of output: 2761
src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java (2)
36-36: LGTM!
Clean import statements with appropriate security annotation for role-based access control.
Also applies to: 60-60
172-172: LGTM! Method rename improves clarity.
The rename from getCourseCompetenciesWithProgress to getCourseCompetenciesWithOwnProgress better describes the method's purpose.
...nents/course-competencies-management-table/course-competencies-management-table.component.ts
Show resolved
Hide resolved
...main/webapp/app/course/competencies/competency-management/competency-management.component.ts
Show resolved
Hide resolved
...main/webapp/app/course/competencies/competency-management/competency-management.component.ts
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyProgressRepository.java
Show resolved
Hide resolved
...nts/course-competencies-management-table/course-competencies-management-table.component.html
Show resolved
Hide resolved
...pec/component/competencies/components/course-competencies-management-table.component.spec.ts
Show resolved
Hide resolved
# Conflicts: # src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java # src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java (3)
299-316: Consider adding pagination support for large result sets.The query fetches all competencies for a course without pagination, which could impact performance for courses with many competencies.
Consider modifying the method signature to:
-List<CompetencyStudentProgressDTO> findWithStudentProgressByCourseId(@Param("courseId") long courseId); +Page<CompetencyStudentProgressDTO> findWithStudentProgressByCourseId(@Param("courseId") long courseId, Pageable pageable);
299-316: Add JavaDoc to document the complex query.This query performs important analytics calculations and should be documented for maintainability.
Add documentation like:
/** * Retrieves competency progress data for all students in a course. * * @param courseId the ID of the course * @return list of DTOs containing competency information and student progress */
299-315: Consider using a native SQL query for better performance.The current JPQL query with complex CASE expressions and aggregations might be more efficient as a native SQL query, especially considering the GROUP BY clause.
Example conversion to native SQL:
-@Query(""" - SELECT new de.tum.cit.aet.artemis.atlas.dto.CompetencyStudentProgressDTO( - c.id, - c.title, - ... - ) - FROM CourseCompetency c - LEFT JOIN c.userProgress up - WHERE c.course.id = :courseId - GROUP BY c - """) +@Query(value = """ + SELECT c.id, c.title, c.description, c.taxonomy, c.soft_due_date, + c.optional, + CASE WHEN c.dtype = 'Competency' THEN 'competency' ELSE 'prerequisite' END as type, + COUNT(up.id) as total_students, + COUNT(CASE WHEN up.progress * up.confidence >= c.mastery_threshold THEN 1 END) as mastered_students + FROM course_competency c + LEFT JOIN user_progress up ON up.competency_id = c.id + WHERE c.course_id = :courseId + GROUP BY c.id, c.title, c.description, c.taxonomy, c.soft_due_date, c.optional, c.dtype + """, + nativeQuery = true)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java(3 hunks)src/main/webapp/app/entities/competency.model.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/webapp/app/entities/competency.model.ts
- src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CourseCompetencyService.java
- src/main/java/de/tum/cit/aet/artemis/atlas/web/CourseCompetencyResource.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.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
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.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-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.
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/atlas/repository/CourseCompetencyRepository.java (2)
19-19: LGTM!
The import follows Java conventions and is properly grouped with other DTO imports.
308-309: Verify the mastery calculation logic.
The mastery calculation up.progress * up.confidence >= c.masteryThreshold needs verification to ensure it aligns with the PR objective of displaying mastery correctly on a 0-100 scale instead of 0-1.
✅ Verification successful
Let me try another search to find the relevant entity definitions.
Let me try a different search approach using ripgrep to find the entity definitions.
Let me try one more search to find the entity definitions in the domain/model packages.
Let me try one final search to find the mastery threshold field and its usage.
Based on the code analysis, I can now generate the final response:
The mastery calculation is correctly implemented for the 0-100 scale.
The code shows that:
progressandconfidenceare stored as values between 0 and 100 (seeMath.clamp(Math.round(progress), 0, 100)in CompetencyProgressService)masteryThresholdis also defined on a 0-100 scale (seeDEFAULT_MASTERY_THRESHOLD = 100in CourseCompetency)- The mastery calculation
up.progress * up.confidence >= c.masteryThresholdis correct because:- When multiplying two percentages (0-100), the result needs to be compared with a threshold on the same 0-100 scale
- For example: 80% progress * 90% confidence = 72% mastery, which is correctly compared against the masteryThreshold
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the scale of progress, confidence, and masteryThreshold fields
# Check field definitions in entity classes
echo "Checking UserProgress entity fields:"
rg -A 5 "class UserProgress|progress.*=|confidence.*="
echo "Checking CourseCompetency entity fields:"
rg -A 5 "class CourseCompetency|masteryThreshold.*="
Length of output: 65882
Script:
#!/bin/bash
# Search for UserProgress and CourseCompetency entity definitions
ast-grep --pattern 'class UserProgress {
$$$
}'
ast-grep --pattern 'class CourseCompetency {
$$$
}'
Length of output: 355
Script:
#!/bin/bash
# Search for UserProgress and CourseCompetency entity definitions with ripgrep
echo "Searching for UserProgress entity:"
rg -B 2 -A 10 "^public class UserProgress"
echo "Searching for CourseCompetency entity:"
rg -B 2 -A 10 "^public class CourseCompetency"
Length of output: 4706
Script:
#!/bin/bash
# Search for entity definitions in domain/model packages
echo "Searching for UserProgress entity:"
rg -B 2 -A 10 "class UserProgress" src/main/java/de/tum/cit/aet/artemis/atlas/domain/
echo "Searching for CourseCompetency entity:"
rg -B 2 -A 10 "class CourseCompetency" src/main/java/de/tum/cit/aet/artemis/atlas/domain/
Length of output: 1732
Script:
#!/bin/bash
# Search for mastery threshold field and its usage
echo "Searching for mastery threshold field and related code:"
rg -B 5 -A 5 "masteryThreshold" src/main/java/de/tum/cit/aet/artemis/atlas/
echo "Searching for progress and confidence fields:"
rg -B 5 -A 5 "progress.*=|confidence.*=" src/main/java/de/tum/cit/aet/artemis/atlas/
Length of output: 46777
Checklist
General
Server
Client
Motivation and Context
Fix smaller bugs in Atlas
Description
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
CompetencyStudentProgressDTOfor tracking student progress in competencies.Improvements
Localization
Bug Fixes