Lectures: Fix edge cases that can lead to invalid lecture unit configurations
#11648
Conversation
additional improvements in the related web resources
github-actions
bot
added
tests
server
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
avoid unnecessary REST calls to the server
We now use DTOs *yeah*. This prevents that the lecture unit order gets destroyed It also properly supports adding, updating or deleting competency links
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java (1)
221-245: Avoid unsafegetLast()cast; reuse the managedtextUnitinstance insteadThe pattern
Lecture updatedLecture = lectureRepository.saveAndFlush(lecture); TextUnit persistedUnit = (TextUnit) updatedLecture.getLectureUnits().getLast();is still fragile:
- It assumes the last lecture unit is always the newly created
TextUnit.- It performs an unchecked cast that will throw
ClassCastExceptionif the last unit is of another subtype.- It relies on collection ordering semantics that might change with future refactors.
After
saveAndFlush(lecture), the originaltextUnitinstance should already be managed and have its ID populated, so you can safely use it directly and avoid depending on collection order.A safer implementation would be:
- lecture.addLectureUnit(textUnit); - Lecture updatedLecture = lectureRepository.saveAndFlush(lecture); - TextUnit persistedUnit = (TextUnit) updatedLecture.getLectureUnits().getLast(); - // From now on, only use persistedUnit - lectureUnitService.saveWithCompetencyLinks(persistedUnit, textUnitRepository::saveAndFlush); - competencyProgressApi.ifPresent(api -> api.updateProgressByLearningObjectAsync(persistedUnit)); + lecture.addLectureUnit(textUnit); + lectureRepository.saveAndFlush(lecture); + // From now on, only use textUnit (now managed with an ID) + lectureUnitService.saveWithCompetencyLinks(textUnit, textUnitRepository::saveAndFlush); + competencyProgressApi.ifPresent(api -> api.updateProgressByLearningObjectAsync(textUnit)); @@ - lectureUnitService.disconnectCompetencyLectureUnitLinks(persistedUnit); - return ResponseEntity.created(new URI("/api/text-units/" + persistedUnit.getId())).body(persistedUnit); + lectureUnitService.disconnectCompetencyLectureUnitLinks(textUnit); + return ResponseEntity.created(new URI("/api/text-units/" + textUnit.getId())).body(textUnit);This removes the unsafe cast and the dependency on
getLast()while preserving the intended behavior.#!/bin/bash # Verify that no other resources rely on `getLectureUnits().getLast()` in a similar way. rg -n "getLectureUnits\(\).*getLast\(" src/main/java || true
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureUnit.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/repository/TextUnitRepository.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java(5 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/lecture/repository/TextUnitRepository.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureUnit.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
🧠 Learnings (4)
📓 Common learnings
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts:356-361
Timestamp: 2025-10-10T17:12:35.471Z
Learning: In the LectureSeriesCreateComponent (src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts), the rawExistingLectures input contains only persisted Lecture entities from the database, so their id field is guaranteed to be non-null despite the Lecture type having id?: number. The cast to ExistingLecture at line 359 is therefore safe.
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/java/de/tum/cit/aet/artemis/iris/web/IrisLectureChatSessionResource.java:79-81
Timestamp: 2025-10-10T13:22:16.754Z
Learning: The `visibleDate` property of the `Lecture` entity in the Artemis codebase is deprecated and no longer supported. Visibility checks based on `lecture.isVisibleToStudents()` or `lecture.getVisibleDate()` should not be flagged as missing security controls, as this functionality has been intentionally removed across the codebase (tracked in issue #11479).
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/java/de/tum/cit/aet/artemis/iris/web/IrisLectureChatSessionResource.java:138-140
Timestamp: 2025-10-10T13:22:13.454Z
Learning: In the IrisLectureChatSessionResource.java file, the checkWhetherLectureIsVisibleToStudentsElseThrow visibility checks are intentionally commented out as part of deprecating the Lecture.visibleDate property. This deprecation is temporary to monitor user feedback before full removal (tracked in issue #11479).
📚 Learning: 2025-10-10T17:12:35.471Z
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts:356-361
Timestamp: 2025-10-10T17:12:35.471Z
Learning: In the LectureSeriesCreateComponent (src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts), the rawExistingLectures input contains only persisted Lecture entities from the database, so their id field is guaranteed to be non-null despite the Lecture type having id?: number. The cast to ExistingLecture at line 359 is therefore safe.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: JohannesStoehr
Repo: ls1intum/Artemis PR: 8679
File: src/main/java/de/tum/in/www1/artemis/web/rest/tutorialgroups/TutorialGroupSessionResource.java:37-37
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The DTOs `CompetencyProgressForLearningPathDTO`, `ProgrammingExerciseResetOptionsDTO`, and `CourseWithIdDTO` do not contain nullable values or `Optional` types, making the `JsonInclude` annotation unnecessary for them.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: julian-christl
Repo: ls1intum/Artemis PR: 8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java (4)
src/main/webapp/app/atlas/shared/entities/competency.model.ts (1)
CompetencyLectureUnitLink(112-119)src/main/webapp/app/lecture/shared/entities/lecture.model.ts (1)
Lecture(9-26)src/main/webapp/app/lecture/manage/lecture-units/edit-text-unit/edit-text-unit.component.ts (1)
updateTextUnit(58-78)src/main/webapp/app/lecture/manage/lecture-units/create-text-unit/create-text-unit.component.ts (1)
createTextUnit(39-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: bean-instantiation-check
- GitHub Check: submit-gradle
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java (2)
64-76: Constructor wiring and new dependencies look correctThe added
LectureUnitRepositoryandCompetencyRepositoryfields are final, injected via constructor, and used below; this aligns with the DI and coding guidelines. No issues here.
188-210: DTO and interface design is clean and aligns with guidelinesUsing nested records for
TextUnitDTO,CompetencyLinkDTO, andCompetencyDTO, plus theLectureUnitDTOinterface abstraction, keeps the API surface minimal and strongly typed. The@JsonInclude(JsonInclude.Include.NON_EMPTY)and@JsonIgnoreProperties(ignoreUnknown = true)annotations are reasonable here for payload minimization and forward compatibility. No change requested.
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java (1)
234-241: Critical: Unsafe cast will cause ClassCastException.The cast to
AttachmentVideoUnitat line 239 is performed without aninstanceofcheck, despite the JavaDoc stating that aBAD_REQUESTshould be returned if the lecture unit is not of this type. This will throwClassCastException(500 error) instead of the documented 400 error.Apply this diff to add the type check:
public ResponseEntity<Void> ingestLectureUnitInPyris(LectureUnit lectureUnit) { if (irisLectureApi.isEmpty()) { log.error("Could not send Lecture Unit to Pyris: Pyris webhook service is not available, check if IRIS is enabled."); return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).build(); } + if (!(lectureUnit instanceof AttachmentVideoUnit)) { + log.warn("Cannot ingest non-AttachmentVideoUnit into Pyris: {}", lectureUnit.getClass().getSimpleName()); + return ResponseEntity.status(HttpStatus.BAD_REQUEST).build(); + } boolean isIngested = irisLectureApi.get().addLectureUnitToPyrisDB((AttachmentVideoUnit) lectureUnit) != null; return ResponseEntity.status(isIngested ? HttpStatus.OK : HttpStatus.BAD_REQUEST).build(); }
♻️ Duplicate comments (2)
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java (2)
107-107: Critical: Returns 500 instead of 404 for missing text unit.Line 107 uses
orElseThrow()which throwsNoSuchElementException, resulting in a 500 error. This is inconsistent withgetTextUnit(line 83) which throwsEntityNotFoundExceptionfor a proper 404 response.Apply this diff to fix the error handling:
- var existingTextUnit = textUnitRepository.findByIdWithCompetencies(textUnitDto.id()).orElseThrow(); + var existingTextUnit = textUnitRepository.findByIdWithCompetencies(textUnitDto.id()) + .orElseThrow(() -> new EntityNotFoundException("TextUnit"));
147-169: Major: Unsafe cast remains despite past comment claiming it was addressed.Lines 161-162 still use the unsafe cast pattern flagged in previous reviews. The past comment states this was "Addressed in commit 89dc376", but the fragile
getLast()cast toTextUnitwithout aninstanceofcheck remains in the current code.The safer approach is to use the
textUnitreference directly:lecture.addLectureUnit(textUnit); -Lecture updatedLecture = lectureRepository.saveAndFlush(lecture); -TextUnit persistedUnit = (TextUnit) updatedLecture.getLectureUnits().getLast(); +lectureRepository.saveAndFlush(lecture); // From now on, only use persistedUnit -lectureUnitService.saveWithCompetencyLinks(persistedUnit, textUnitRepository::saveAndFlush); -competencyProgressApi.ifPresent(api -> api.updateProgressByLearningObjectAsync(persistedUnit)); +lectureUnitService.saveWithCompetencyLinks(textUnit, textUnitRepository::saveAndFlush); +competencyProgressApi.ifPresent(api -> api.updateProgressByLearningObjectAsync(textUnit)); // TODO: return a DTO instead to avoid manipulation of the entity before sending it to the client -lectureUnitService.disconnectCompetencyLectureUnitLinks(persistedUnit); -return ResponseEntity.created(new URI("/api/text-units/" + persistedUnit.getId())).body(persistedUnit); +lectureUnitService.disconnectCompetencyLectureUnitLinks(textUnit); +return ResponseEntity.created(new URI("/api/text-units/" + textUnit.getId())).body(textUnit);
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java (1)
157-157: Clarify or remove the ambiguous comment.The comment
// Alternativeis unclear. Consider either removing it or replacing it with a more descriptive explanation of when to use this overload versus the other one (e.g., "Use this overload when original competency IDs are known but the original learning object is not available").Apply this diff to improve clarity:
- // Alternative + /** + * Asynchronously update the existing progress for all changed competencies linked to the given learning object. + * Use this overload when the original competency IDs are known but the original learning object is not available. + * + * @param originalCompetencyIds The IDs of competencies linked to the learning object before the update + * @param updatedLearningObject The updated learning object after the update + */ @Async public void updateProgressForUpdatedLearningObjectAsync(Set<Long> originalCompetencyIds, LearningObject updatedLearningObject) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRepositoryApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/dto/CompetencyDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/dto/CompetencyLinkDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/dto/LectureUnitDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/dto/OnlineUnitDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/dto/TextUnitDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java(9 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/TextUnitDTO.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/CompetencyLinkDTO.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/OnlineUnitDTO.javasrc/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRepositoryApi.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/CompetencyDTO.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/LectureUnitDTO.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.javasrc/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.javasrc/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
🧠 Learnings (13)
📓 Common learnings
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/java/de/tum/cit/aet/artemis/iris/web/IrisLectureChatSessionResource.java:138-140
Timestamp: 2025-10-10T13:22:13.454Z
Learning: In the IrisLectureChatSessionResource.java file, the checkWhetherLectureIsVisibleToStudentsElseThrow visibility checks are intentionally commented out as part of deprecating the Lecture.visibleDate property. This deprecation is temporary to monitor user feedback before full removal (tracked in issue #11479).
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts:356-361
Timestamp: 2025-10-10T17:12:35.471Z
Learning: In the LectureSeriesCreateComponent (src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts), the rawExistingLectures input contains only persisted Lecture entities from the database, so their id field is guaranteed to be non-null despite the Lecture type having id?: number. The cast to ExistingLecture at line 359 is therefore safe.
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/java/de/tum/cit/aet/artemis/iris/web/IrisLectureChatSessionResource.java:79-81
Timestamp: 2025-10-10T13:22:16.754Z
Learning: The `visibleDate` property of the `Lecture` entity in the Artemis codebase is deprecated and no longer supported. Visibility checks based on `lecture.isVisibleToStudents()` or `lecture.getVisibleDate()` should not be flagged as missing security controls, as this functionality has been intentionally removed across the codebase (tracked in issue #11479).
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: julian-christl
Repo: ls1intum/Artemis PR: 8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.javasrc/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: JohannesStoehr
Repo: ls1intum/Artemis PR: 8679
File: src/main/java/de/tum/in/www1/artemis/web/rest/tutorialgroups/TutorialGroupSessionResource.java:37-37
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The DTOs `CompetencyProgressForLearningPathDTO`, `ProgrammingExerciseResetOptionsDTO`, and `CourseWithIdDTO` do not contain nullable values or `Optional` types, making the `JsonInclude` annotation unnecessary for them.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/dto/TextUnitDTO.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/CompetencyLinkDTO.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/OnlineUnitDTO.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/CompetencyDTO.java
📚 Learning: 2025-05-24T16:06:41.454Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 10812
File: src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRepository.java:70-88
Timestamp: 2025-05-24T16:06:41.454Z
Learning: In CompetencyRepository JPQL queries that join from LearningPath to course competencies, DISTINCT is not necessary because a learning path belongs to exactly one course, preventing duplicate competencies in the result set.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRepositoryApi.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.javasrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java
📚 Learning: 2025-09-20T16:43:32.823Z
Learnt from: MoritzSpengler
Repo: ls1intum/Artemis PR: 11382
File: src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizQuestionProgressService.java:130-137
Timestamp: 2025-09-20T16:43:32.823Z
Learning: The findAllDueQuestions method exists in QuizQuestionRepository and accepts Set<Long> ids, Long courseId, and Pageable parameters, returning Page<QuizQuestion>. This method is properly implemented and available for use in QuizQuestionProgressService.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRepositoryApi.java
📚 Learning: 2025-09-20T16:43:32.823Z
Learnt from: MoritzSpengler
Repo: ls1intum/Artemis PR: 11382
File: src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizQuestionProgressService.java:130-137
Timestamp: 2025-09-20T16:43:32.823Z
Learning: The findAllDueQuestions method exists in QuizQuestionRepository and accepts Set<Long> ids, Long courseId, and Pageable parameters, returning Page<QuizQuestion>. The method has a Query annotation that filters by courseId, isOpenForPractice = TRUE, and excludes questions with IDs in the provided set using NOT IN.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRepositoryApi.java
📚 Learning: 2025-09-25T20:28:36.905Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java
📚 Learning: 2025-10-10T17:12:35.471Z
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts:356-361
Timestamp: 2025-10-10T17:12:35.471Z
Learning: In the LectureSeriesCreateComponent (src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts), the rawExistingLectures input contains only persisted Lecture entities from the database, so their id field is guaranteed to be non-null despite the Lecture type having id?: number. The cast to ExistingLecture at line 359 is therefore safe.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.javasrc/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
📚 Learning: 2025-10-10T13:22:16.754Z
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/java/de/tum/cit/aet/artemis/iris/web/IrisLectureChatSessionResource.java:79-81
Timestamp: 2025-10-10T13:22:16.754Z
Learning: The `visibleDate` property of the `Lecture` entity in the Artemis codebase is deprecated and no longer supported. Visibility checks based on `lecture.isVisibleToStudents()` or `lecture.getVisibleDate()` should not be flagged as missing security controls, as this functionality has been intentionally removed across the codebase (tracked in issue #11479).
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/dto/LectureUnitDTO.java
📚 Learning: 2024-06-18T07:02:44.462Z
Learnt from: JohannesWt
Repo: ls1intum/Artemis PR: 8791
File: src/main/java/de/tum/in/www1/artemis/service/learningpath/LearningPathRecommendationService.java:402-410
Timestamp: 2024-06-18T07:02:44.462Z
Learning: JohannesWt agrees with using the `orElse` method from the Optional API for simplifying the handling of `optionalCompetencyProgress` in `LearningPathRecommendationService.java` to enhance code readability and conciseness.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/main/java/de/tum/in/www1/artemis/web/rest/StudentExamResource.java:892-892
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Valentin-boehm has indicated that including detailed error messages for self-explanatory exceptions such as a BadRequestException when a student exam is already submitted or abandoned is not necessary in the context of `StudentExamResource.java`.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
📚 Learning: 2024-10-08T15:35:48.768Z
Learnt from: florian-glombik
Repo: ls1intum/Artemis PR: 8858
File: src/test/javascript/spec/component/shared/exercise-filter/exercise-filter-modal.component.spec.ts:201-225
Timestamp: 2024-10-08T15:35:48.768Z
Learning: Non-null assertions should not be flagged or suggested for removal in the context of PR ls1intum/Artemis#8858.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
📚 Learning: 2024-07-09T19:10:33.905Z
Learnt from: florian-glombik
Repo: ls1intum/Artemis PR: 8858
File: src/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.ts:97-107
Timestamp: 2024-07-09T19:10:33.905Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
🧬 Code graph analysis (2)
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java (1)
src/main/webapp/app/atlas/shared/entities/competency.model.ts (1)
CompetencyLearningObjectLink(93-101)
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java (4)
src/main/webapp/app/atlas/shared/entities/competency.model.ts (1)
CompetencyLearningObjectLink(93-101)src/main/webapp/app/lecture/shared/entities/lecture.model.ts (1)
Lecture(9-26)src/main/webapp/app/lecture/manage/lecture-units/edit-text-unit/edit-text-unit.component.ts (1)
updateTextUnit(58-78)src/main/webapp/app/lecture/manage/lecture-units/create-text-unit/create-text-unit.component.ts (1)
createTextUnit(39-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: bean-instantiation-check
- GitHub Check: client-tests
🔇 Additional comments (17)
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRepositoryApi.java (1)
28-30: LGTM!The new API method follows the established delegation pattern and provides a clean interface for retrieving competencies by ID with exception handling.
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java (1)
48-50: LGTM!The new API overload provides a clean alternative when competency IDs are available instead of the full original learning object, properly delegating to the service layer.
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java (2)
158-173: LGTM!The implementation correctly compares original and updated competency IDs to determine which progress records need updating. The use of
updatedLearningObjectfor added competencies (line 171) is appropriate for this overload.
195-196: Good optimization reminder.The TODO comment appropriately flags a performance concern. Loading competencies once outside the loop would indeed improve efficiency when many users and competencies are involved.
src/main/java/de/tum/cit/aet/artemis/lecture/dto/LectureUnitDTO.java (1)
6-13: LGTM!The interface provides a clean contract for lecture unit DTOs with essential fields for identification, release scheduling, and competency linking.
src/main/java/de/tum/cit/aet/artemis/lecture/dto/CompetencyLinkDTO.java (1)
6-10: LGTM!The record correctly models the link between a competency and its weight, with appropriate Jackson annotations for JSON serialization.
src/main/java/de/tum/cit/aet/artemis/lecture/dto/CompetencyDTO.java (1)
6-10: LGTM!The record appropriately uses a primitive
longfor the ID since competency references must always have a valid identifier. This differs intentionally from unit DTOs that use boxedLongto support creation scenarios where ID is not yet assigned.src/main/java/de/tum/cit/aet/artemis/lecture/dto/TextUnitDTO.java (1)
9-12: LGTM!The record correctly implements
LectureUnitDTOand includes all necessary fields for text unit representation. The use of boxedLongfor ID appropriately supports creation scenarios where the ID is not yet assigned.src/main/java/de/tum/cit/aet/artemis/lecture/dto/OnlineUnitDTO.java (1)
9-12: LGTM!The record correctly implements
LectureUnitDTOwith online-specific fields (description, source). The structure aligns well withTextUnitDTOwhile accommodating the distinct properties of online units.src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java (4)
76-90: LGTM!The dependency injection of
CompetencyRepositoryApifollows best practices: constructor injection withOptionalwrapper for the optional Atlas module dependency.
129-154: LGTM!The refactoring to use a generic type parameter and create a modifiable copy of the input list is good defensive programming. This prevents unintended side effects on the caller's collection while maintaining type safety.
182-182: LGTM!Delegating the removal to
lecture.removeLectureUnitById()improves encapsulation and aligns with the PR's refactoring to properly manage lecture unit ordering.
250-258: LGTM!The added guard to check if competency links are initialized prevents lazy loading exceptions and ensures only managed collections are processed. The null-safe clearing of bidirectional references is proper defensive programming.
src/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.java (2)
71-80: LGTM!The addition of
LectureUnitRepositoryfollows proper dependency injection patterns with constructor injection.
106-106: Verify authorization scope change is intentional.The authorization was changed from
@EnforceAtLeastEditorInLectureUnitto@EnforceAtLeastEditorInLecture. This broadens the authorization scope—any editor of the lecture can now update any unit in that lecture, rather than requiring unit-specific authorization.Ensure this aligns with the intended security model changes in the PR.
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java (2)
51-68: LGTM!The dependency injection and field reordering follow proper patterns.
100-100: Verify authorization scope change is intentional.The authorization was changed from
@EnforceAtLeastEditorInLectureUnitto@EnforceAtLeastEditorInLecture, broadening the scope. Ensure this aligns with the intended security model changes.
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java (2)
107-107: Return 404 instead of 500 when text unit is missing.The
orElseThrow()without an argument throwsNoSuchElementException, resulting in a 500 error instead of a 404 when the text unit does not exist. This is inconsistent withgetTextUnit(line 83), which throwsEntityNotFoundException("TextUnit").Apply this diff to align the behavior:
- var existingTextUnit = textUnitRepository.findByIdWithCompetencies(textUnitDto.id()).orElseThrow(); + var existingTextUnit = textUnitRepository.findByIdWithCompetencies(textUnitDto.id()) + .orElseThrow(() -> new EntityNotFoundException("TextUnit"));
163-171: Unsafe cast and fragile retrieval logic.Line 164 unsafely casts
getLast()toTextUnitwithout aninstanceofcheck and assumes the last element is the newly added unit. This is fragile and could throwClassCastExceptionif the collection order differs or contains other unit types.The safer approach is to use the
textUnitreference directly aftersaveAndFlush, as Hibernate will have populated its ID and merged it into the persistence context:lecture.addLectureUnit(textUnit); -Lecture updatedLecture = lectureRepository.saveAndFlush(lecture); -TextUnit persistedUnit = (TextUnit) updatedLecture.getLectureUnits().getLast(); +lectureRepository.saveAndFlush(lecture); // From now on, only use persistedUnit -lectureUnitService.saveWithCompetencyLinks(persistedUnit, textUnitRepository::saveAndFlush); -competencyProgressApi.ifPresent(api -> api.updateProgressByLearningObjectAsync(persistedUnit)); +lectureUnitService.saveWithCompetencyLinks(textUnit, textUnitRepository::saveAndFlush); +competencyProgressApi.ifPresent(api -> api.updateProgressByLearningObjectAsync(textUnit)); // TODO: return a DTO instead to avoid manipulation of the entity before sending it to the client -lectureUnitService.disconnectCompetencyLectureUnitLinks(persistedUnit); -return ResponseEntity.created(new URI("/api/text-units/" + persistedUnit.getId())).body(persistedUnit); +lectureUnitService.disconnectCompetencyLectureUnitLinks(textUnit); +return ResponseEntity.created(new URI("/api/text-units/" + textUnit.getId())).body(textUnit);src/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.java (1)
174-183: Unsafe cast and fragile retrieval logic.Line 176 unsafely casts
getLast()toOnlineUnitwithout aninstanceofcheck and assumes the last element is the newly added unit. This is fragile and could throwClassCastExceptionif the collection order differs or contains other unit types.The safer approach is to use the
onlineUnitreference directly aftersaveAndFlush, as it will be managed and have its ID populated:lecture.addLectureUnit(onlineUnit); -Lecture updatedLecture = lectureRepository.saveAndFlush(lecture); - -OnlineUnit persistedUnit = (OnlineUnit) updatedLecture.getLectureUnits().getLast(); +lectureRepository.saveAndFlush(lecture); // From now on, only use persistedUnit -lectureUnitService.saveWithCompetencyLinks(persistedUnit, onlineUnitRepository::saveAndFlush); -competencyProgressApi.ifPresent(api -> api.updateProgressByLearningObjectAsync(persistedUnit)); +lectureUnitService.saveWithCompetencyLinks(onlineUnit, onlineUnitRepository::saveAndFlush); +competencyProgressApi.ifPresent(api -> api.updateProgressByLearningObjectAsync(onlineUnit)); // TODO: return a DTO instead to avoid manipulation of the entity before sending it to the client -lectureUnitService.disconnectCompetencyLectureUnitLinks(persistedUnit); -return ResponseEntity.created(new URI("/api/online-units/" + persistedUnit.getId())).body(persistedUnit); +lectureUnitService.disconnectCompetencyLectureUnitLinks(onlineUnit); +return ResponseEntity.created(new URI("/api/online-units/" + onlineUnit.getId())).body(onlineUnit);src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java (1)
319-321: Add null check for competency to prevent NPE.Line 320 calls
dtoLink.competency().id()without checking ifcompetencyis null. If a client sends a DTO entry withcompetency: null, this will throwNullPointerException, resulting in a 500 error instead of a 400 validation error.Apply this diff to add validation:
for (var dtoLink : lectureUnitDto.competencyLinks()) { + if (dtoLink.competency() == null || dtoLink.competency().id() == null) { + throw new BadRequestAlertException("Competency and competency ID must not be null", "lectureUnit", "competencyNull"); + } long competencyId = dtoLink.competency().id(); double weight = dtoLink.weight();
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/lecture/OnlineUnitIntegrationTest.java (1)
159-174: Order-preservation test is correct but could snapshot expected ordering more defensivelyUsing
lectureUtilService.createOnlineUnit(lecture1)and updating viaPUTwithOnlineUnitDTO(Lines 163-164, 170) fits the new API surface and correctly targets the second lecture unit for the ordering check. However, the assertionList<LectureUnit> orderedUnits = lecture1.getLectureUnits(); // ... List<LectureUnit> updatedOrderedUnits = lectureRepository.findByIdWithLectureUnitsAndAttachments(lecture1.getId()).orElseThrow().getLectureUnits(); assertThat(updatedOrderedUnits).containsExactlyElementsOf(orderedUnits);could become fragile if
getLectureUnits()ever returns the mutable backing collection instead of a copy—orderedUnitsmight then reflect any reordering performed during the update, leading to a false positive.To decouple expected vs. actual, consider snapshotting the list explicitly when capturing the expected order, e.g.:
-List<LectureUnit> orderedUnits = lecture1.getLectureUnits(); +List<LectureUnit> orderedUnits = List.copyOf(lecture1.getLectureUnits());(or
new ArrayList<>(...)), so the test will fail if the backend accidentally reorders units.src/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.java (1)
109-111: Use descriptive exception for consistency.Line 109 throws a generic
BadRequestException()when the ID is null, whileTextUnitResource.updateTextUnit(line 104) throws a more descriptiveBadRequestAlertExceptionwith entity name and error code. Consider aligning the exception handling for consistency.Apply this diff:
if (onlineUnitDto.id() == null) { - throw new BadRequestException(); + throw new BadRequestAlertException("An online unit must have an ID to be updated", ENTITY_NAME, "idNull"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java(9 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java(5 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/OnlineUnitIntegrationTest.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/TextUnitIntegrationTest.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java
- src/test/java/de/tum/cit/aet/artemis/lecture/TextUnitIntegrationTest.java
- src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.javasrc/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/test/java/de/tum/cit/aet/artemis/lecture/OnlineUnitIntegrationTest.java
🧠 Learnings (9)
📓 Common learnings
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts:356-361
Timestamp: 2025-10-10T17:12:35.471Z
Learning: In the LectureSeriesCreateComponent (src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts), the rawExistingLectures input contains only persisted Lecture entities from the database, so their id field is guaranteed to be non-null despite the Lecture type having id?: number. The cast to ExistingLecture at line 359 is therefore safe.
📚 Learning: 2025-10-10T17:12:35.471Z
Learnt from: marlonnienaber
Repo: ls1intum/Artemis PR: 11436
File: src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts:356-361
Timestamp: 2025-10-10T17:12:35.471Z
Learning: In the LectureSeriesCreateComponent (src/main/webapp/app/lecture/manage/lecture-series-create/lecture-series-create.component.ts), the rawExistingLectures input contains only persisted Lecture entities from the database, so their id field is guaranteed to be non-null despite the Lecture type having id?: number. The cast to ExistingLecture at line 359 is therefore safe.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.javasrc/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/main/java/de/tum/in/www1/artemis/web/rest/StudentExamResource.java:892-892
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Valentin-boehm has indicated that including detailed error messages for self-explanatory exceptions such as a BadRequestException when a student exam is already submitted or abandoned is not necessary in the context of `StudentExamResource.java`.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: julian-christl
Repo: ls1intum/Artemis PR: 8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.javasrc/test/java/de/tum/cit/aet/artemis/lecture/OnlineUnitIntegrationTest.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java
📚 Learning: 2024-10-08T15:35:48.768Z
Learnt from: florian-glombik
Repo: ls1intum/Artemis PR: 8858
File: src/test/javascript/spec/component/shared/exercise-filter/exercise-filter-modal.component.spec.ts:201-225
Timestamp: 2024-10-08T15:35:48.768Z
Learning: Non-null assertions should not be flagged or suggested for removal in the context of PR ls1intum/Artemis#8858.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java
📚 Learning: 2024-10-15T11:33:17.915Z
Learnt from: alexjoham
Repo: ls1intum/Artemis PR: 9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/lecture/OnlineUnitIntegrationTest.java
📚 Learning: 2025-09-25T20:28:36.905Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/lecture/OnlineUnitIntegrationTest.javasrc/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: jakubriegel
Repo: ls1intum/Artemis PR: 8050
File: src/test/java/de/tum/in/www1/artemis/plagiarism/PlagiarismUtilService.java:62-66
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `createCourseWithUsers` method in `PlagiarismUtilService.java` uses fixed inputs as it is designed to be a test helper method for simplifying the setup of courses and users in tests.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/lecture/OnlineUnitIntegrationTest.java
📚 Learning: 2025-05-24T16:06:41.454Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 10812
File: src/main/java/de/tum/cit/aet/artemis/atlas/repository/CompetencyRepository.java:70-88
Timestamp: 2025-05-24T16:06:41.454Z
Learning: In CompetencyRepository JPQL queries that join from LearningPath to course competencies, DISTINCT is not necessary because a learning path belongs to exactly one course, preventing duplicate competencies in the result set.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureUnitService.java
🧬 Code graph analysis (2)
src/main/java/de/tum/cit/aet/artemis/lecture/web/TextUnitResource.java (3)
src/main/webapp/app/atlas/shared/entities/competency.model.ts (1)
CompetencyLearningObjectLink(93-101)src/main/webapp/app/lecture/manage/lecture-units/edit-text-unit/edit-text-unit.component.ts (1)
updateTextUnit(58-78)src/main/webapp/app/lecture/manage/lecture-units/create-text-unit/create-text-unit.component.ts (1)
createTextUnit(39-66)
src/main/java/de/tum/cit/aet/artemis/lecture/web/OnlineUnitResource.java (4)
src/main/webapp/app/atlas/shared/entities/competency.model.ts (1)
CompetencyLearningObjectLink(93-101)src/main/webapp/app/lecture/shared/entities/lecture.model.ts (1)
Lecture(9-26)src/main/webapp/app/lecture/manage/lecture-units/edit-online-unit/edit-online-unit.component.ts (1)
updateOnlineUnit(61-81)src/main/webapp/app/lecture/manage/lecture-units/create-online-unit/create-online-unit.component.ts (1)
createOnlineUnit(39-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
🔇 Additional comments (2)
src/test/java/de/tum/cit/aet/artemis/lecture/OnlineUnitIntegrationTest.java (2)
38-38: OnlineUnitDTO import and usage in update test look consistentThe new import on Line 38 and the switch to
OnlineUnitDTOas the response type inupdateOnlineUnit_asInstructor_shouldUpdateOnlineUnit(Lines 145-155) align with a DTO-based update API. The assertion onupdatedOnlineUnit.description()and the verification ofupdateProgressForUpdatedLearningObjectAsyncWithOriginalCompetencyIds(Set.of(competency.getId()), onlineUnit)correctly exercise the new competency-progress contract for updates.Also applies to: 145-155
69-86: Explicitly setting lecture on test OnlineUnit matches non-null DB constraintAssigning the lecture in
initTestCaseviathis.onlineUnit.setLecture(this.lecture1);(Line 76) ensures the testOnlineUnitis always associated with aLecture, which is consistent with a non-nulllectureIdconstraint on lecture units and avoids persistence edge cases during test setup.
End-to-End (E2E) Test Results Summary
|
|||||||||||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test9.artemis.cit.tum.de
|
tested on test server 9 course with quiz were deadline is in the past to be imported: also not sure if this is intended but if you edit a lecture unit the checkbox for "automatic unit processing" is unchecked and one cannot view the previously uploaded pdf in the page anymore |
lana-ati
left a comment
lana-ati
left a comment
There was a problem hiding this comment.
Tested during working session on TS9. All described features working as expected from both student and instructor view.
maxgutke
left a comment
maxgutke
left a comment
There was a problem hiding this comment.
I tested this PR on TS9. I followed the steps for testing and encountered no issues
kristi-balla
left a comment
kristi-balla
left a comment
There was a problem hiding this comment.
Tested on t9, created a text and exercise unit: deleting, importing units and linking competencies works. The automatic unit processing also works
|
Tested everything as perscribed. I created a lecture, added units automatically, added more units with several competencies, deleted the units and edited them. Everything worked without issues. |
MoritzSpengler
left a comment
MoritzSpengler
left a comment
There was a problem hiding this comment.
Re-tested. The previous errors did not occur anymore. Creating, editing and deleting lectures with lecture units works without issues.
I can confirm that exercises that are past the due date are not getting imported when the lecture gets importet (maybe this is expected behavior).
|
@hanna20022005 the "view isolated" on a lecture unit opens a blank page bug is also happening on production. Could you create an issue and inform @bassner since he's the current maintainer of the lectures module. |
github-project-automation
bot
moved this from Ready For Review
to Merged
in Artemis Development
This PR fixes many issues when using lecture units:
nullin the database, which leads to issues in the user interface, in particular unexpected behavior and unresolvable bugs (an admin would need to fix this directly in the database).To improve the situation, I carefully redesigned the Java relationship between Lecture and LectureUnit:
nullable = falseforlecture_unit.lectureIdandlecture_unit.lecture_unit_orderto prevent the above issues 1 and 2.@OrderColumndoes not support this, see https://hibernate.atlassian.net/browse/HHH-13287.lecture_unit_ordermanually. Developers who interact won't notice this because this is hidden in the corresponding methodsaddLectureUnit,removeLectureUnit,setLectureUnits, andreorderLectureUnits. JUnit and ArchUnit tests make sure this is implemented correctly, and developers cannot use it wrongly.Lists without@OrderColumnas Bags, we must change toSetwith aLinkedHashSet. This way, existing queries, e.g. fetching lecture + lecture unit + competency links, are safe and do not return more elements than available in the database.LinkedHashSetmaintains an order, and theLectureclass provides getters and setters with the List interface for developers who need to interact with it.Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Steps for Testing
Prerequisites:
Automatic unit processingwhen creating a lecture and make sure it works without issues.Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
No changes
Screenshots
No noticeable changes (only very small improvements in the Attachments view)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor