Iris: Add lecture transcription storage
#10176
Conversation
…:ls1intum/Artemis into feature/iris/video-transcription-storage
…:ls1intum/Artemis into feature/iris/video-transcription-storage
…:ls1intum/Artemis into feature/iris/video-transcription-storage
github-actions
bot
added
tests
server
WalkthroughThe pull request introduces a comprehensive feature for lecture transcription management in the Artemis application. The changes span both backend and frontend components, adding support for creating, ingesting, and managing lecture transcriptions. Key additions include new domain entities for transcriptions and transcription segments, repositories, services, and REST endpoints. The frontend receives a new admin component for transcription ingestion, along with corresponding services and tests. The implementation supports integration with the Pyris system for transcription processing and provides a robust mechanism for handling transcription data across different parts of the application. Changes
Sequence DiagramsequenceDiagram
participant User
participant LectureTranscriptionIngestionComponent
participant LectureTranscriptionService
participant LectureTranscriptionResource
participant PyrisWebhookService
User->>LectureTranscriptionIngestionComponent: Enter transcription details
LectureTranscriptionIngestionComponent->>LectureTranscriptionService: ingestTranscription(courseId, lectureId)
LectureTranscriptionService->>LectureTranscriptionResource: createLectureTranscription
LectureTranscriptionResource-->>PyrisWebhookService: addTranscriptionsToPyrisDB
PyrisWebhookService->>PyrisWebhookService: executeTranscriptionAdditionWebhook
PyrisWebhookService-->>User: Transcription Ingested
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
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: 25
🧹 Nitpick comments (15)
src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTranscriptionResource.java (1)
73-81: Consider refactoring to eliminate code duplicationThe two methods
getStatusOfTranscriptionIngestionandgetStatusOfLectureUnitsTranscriptionIngestionshare similar code structures, including exception handling and authorization checks. Refactoring common logic into a private helper method can improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle.For example, you could create a private method to handle common authorization and error handling logic.
Also applies to: 101-109
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/TranscriptionIngestionWebhookJob.java (1)
8-9: Fix typographical error in class documentationThere's a typo in the class comment: "then Ingestion" should be "the Ingestion."
Apply this diff to correct the typo:
- * This job is used to reference the details of then Ingestion when Pyris sends a status update. + * This job is used to reference the details of the Ingestion when Pyris sends a status update.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/transcriptionIngestion/PyrisTranscriptionIngestionWebhookDTO.java (2)
14-14: Consider minimizing the DTO fields for data economy.The DTO includes descriptive fields (lectureName, courseName, courseDescription) that might not be necessary for the transcription ingestion process. Consider if these fields are actually used by Pyris.
-public record PyrisTranscriptionIngestionWebhookDTO(Transcription transcription, long lectureId, String lectureName, long courseId, String courseName, String courseDescription) { +public record PyrisTranscriptionIngestionWebhookDTO(Transcription transcription, long lectureId, long courseId) {
7-11: Enhance documentation with @param annotations.The Javadoc should include @param annotations for each field to better document their purpose and requirements.
/** * Represents a webhook data transfer object for lecture units in the Pyris system. * This DTO is used to encapsulate the information related to updates of lecture units, * providing necessary details such as lecture and course identifiers, names, and descriptions. + * + * @param transcription The transcription data to be ingested + * @param lectureId The unique identifier of the lecture + * @param lectureName The name of the lecture + * @param courseId The unique identifier of the course + * @param courseName The name of the course + * @param courseDescription The description of the course */src/main/java/de/tum/cit/aet/artemis/lecture/repository/TranscriptionRepository.java (1)
23-29: Consider adding index hint for performance.The query by ID with segments could benefit from an index hint.
@Query(""" SELECT t FROM Transcription t + /*+ INDEX(t transcription_id_idx) */ LEFT JOIN FETCH t.segments WHERE t.id = :id """)src/main/java/de/tum/cit/aet/artemis/lecture/domain/Transcription.java (2)
34-37: Consider adding index for ordered segments.The
@OrderByannotation suggests frequent ordering of segments by start time. Consider adding a database index to optimize these queries.Add index annotation:
@OneToMany(cascade = CascadeType.ALL, fetch = FetchType.LAZY, orphanRemoval = true) @OrderBy("startTime asc") + @Index(name = "idx_transcription_segment_start_time") @JoinColumn(name = "transcription_id") private List<TranscriptionSegment> segments = new ArrayList<>();
72-75: Enhance toString method with more relevant information.The current
toStringimplementation excludes the ID field, which could be useful for debugging.Consider including the ID in the string representation:
@Override public String toString() { - return "Transcription [language=" + language + ", segments=" + segments + "]"; + return "Transcription [id=" + getId() + ", language=" + language + ", segments.size=" + segments.size() + "]"; }src/main/java/de/tum/cit/aet/artemis/lecture/domain/TranscriptionSegment.java (1)
28-29: Consider adding length constraint for text field.The
@Lobannotation allows for large text storage, but consider adding a reasonable maximum length to prevent excessive storage usage.Add length constraint:
@Lob + @Column(length = 10000) // Adjust the length based on your requirements private String text;src/main/java/de/tum/cit/aet/artemis/lecture/web/TranscriptionResource.java (2)
31-31: Consider using a more specific base path.The current base path "api/" is too generic. Consider using a more specific path like "api/transcriptions/" to better reflect the resource hierarchy.
-@RequestMapping("api/") +@RequestMapping("api/transcriptions/")
65-67: Avoid explicit null assignment.Setting the ID to null is unnecessary as it's already null for new entities.
Transcription transcription = new Transcription(lecture, transcriptionDTO.language(), segments); - transcription.setId(null);src/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java (1)
54-56: Avoid array initialization with List.of.The current implementation uses an array initialization with List.of which is less readable. Consider using a direct List.of call.
- Transcription transcription = new Transcription(this.lecture1, "en", List.of(new TranscriptionSegment[] { segment1, segment2 })); + Transcription transcription = new Transcription(this.lecture1, "en", List.of(segment1, segment2));src/main/java/de/tum/cit/aet/artemis/lecture/domain/Lecture.java (1)
63-66: Consider using Set instead of List for transcriptions.Since the order of transcriptions doesn't seem to be important (no @OrderColumn), using a Set might be more appropriate and could prevent duplicate entries.
@OneToMany(mappedBy = "lecture", cascade = CascadeType.ALL, orphanRemoval = true) @JsonIgnoreProperties("lecture") @Cache(usage = CacheConcurrencyStrategy.NONSTRICT_READ_WRITE) - private List<Transcription> transcriptions = new ArrayList<>(); + private Set<Transcription> transcriptions = new HashSet<>();src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java (1)
112-126: Extract timeout constants and add parameter validation.The implementation looks good but could be improved in two areas:
- The timeout values (60 minutes) are duplicated with
addIngestionWebhookJob. Consider extracting these as class constants.- Missing validation for negative course/lecture IDs.
Apply this diff to improve the implementation:
+ private static final long WEBHOOK_JOB_TIMEOUT = 60; + private static final TimeUnit WEBHOOK_JOB_TIMEOUT_UNIT = TimeUnit.MINUTES; + public String addTranscriptionIngestionWebhookJob(long courseId, long lectureId) { + if (courseId < 0 || lectureId < 0) { + throw new IllegalArgumentException("Course ID and Lecture ID must be non-negative"); + } var token = generateJobIdToken(); var job = new TranscriptionIngestionWebhookJob(token, courseId, lectureId); - long timeoutWebhookJob = 60; - TimeUnit unitWebhookJob = TimeUnit.MINUTES; - jobMap.put(token, job, timeoutWebhookJob, unitWebhookJob); + jobMap.put(token, job, WEBHOOK_JOB_TIMEOUT, WEBHOOK_JOB_TIMEOUT_UNIT); return token; }src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisWebhookService.java (1)
88-100: Extract constants and document empty lists.The implementation could be improved in two areas:
- The variant "fullIngestion" is hardcoded and duplicated with
executeLectureAdditionWebhook.- The purpose of empty lists in DTO construction is not documented.
Apply this diff to improve the implementation:
+ private static final String FULL_INGESTION_VARIANT = "fullIngestion"; + private String executeTranscriptionAdditionWebhook( PyrisTranscriptionIngestionWebhookDTO toUpdateTranscription) { String jobToken = pyrisJobService.addTranscriptionIngestionWebhookJob( toUpdateTranscription.courseId(), toUpdateTranscription.lectureId()); + // TODO: Document the purpose of empty lists in DTO construction PyrisPipelineExecutionSettingsDTO settingsDTO = new PyrisPipelineExecutionSettingsDTO(jobToken, List.of(), artemisBaseUrl); PyrisWebhookTranscriptionIngestionExecutionDTO executionDTO = new PyrisWebhookTranscriptionIngestionExecutionDTO( toUpdateTranscription, settingsDTO, List.of()); - pyrisConnectorService.executeTranscriptionAdditionWebhook("fullIngestion", executionDTO); + pyrisConnectorService.executeTranscriptionAdditionWebhook( + FULL_INGESTION_VARIANT, executionDTO); return jobToken; }src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.html (1)
1-18: Enhance accessibility and user experience.The component would benefit from the following improvements:
- Add ARIA labels and roles for better accessibility
- Add form validation feedback
- Add loading state indicators during API calls
Apply this diff to improve the implementation:
-<div> +<div role="main" aria-labelledby="page-title"> - <h1>Lecture Transcription</h1> + <h1 id="page-title">Lecture Transcription</h1> - <div> + <div role="form" aria-label="Transcription Ingestion Form"> <h2>Ingest</h2> <p>Ingest transcription into Pyris Weaviate DB</p> - <input placeholder="Course Id" [(ngModel)]="courseIdInput" /> - <input placeholder="Lecture Id" [(ngModel)]="lectureIdInput" /> + <input + placeholder="Course Id" + [(ngModel)]="courseIdInput" + aria-label="Course ID" + [attr.aria-invalid]="!isValidCourseId" + required /> + <div *ngIf="!isValidCourseId" class="error-message" role="alert"> + Please enter a valid course ID + </div> + <input + placeholder="Lecture Id" + [(ngModel)]="lectureIdInput" + aria-label="Lecture ID" + [attr.aria-invalid]="!isValidLectureId" + required /> + <div *ngIf="!isValidLectureId" class="error-message" role="alert"> + Please enter a valid lecture ID + </div> - <jhi-button id="ingest-transcription-button" [icon]="faCheck" (onClick)="ingestTranscription()" /> + <jhi-button + id="ingest-transcription-button" + [icon]="faCheck" + (onClick)="ingestTranscription()" + [loading]="isIngesting" + [disabled]="!isValidForm || isIngesting" + aria-label="Ingest Transcription" /> </div> - <div class="insert-db-container"> + <div class="insert-db-container" role="form" aria-label="Transcription Creation Form"> <h2>Create</h2> <p>Create transcription into Artemis DB</p> <div class="insert-transcription-row"> - <textarea placeholder="Transcription as JSON" class="insert-transcription-area" [(ngModel)]="transcriptionInput"></textarea> - <jhi-button id="create-transcription-button" [icon]="faCheck" (onClick)="createTranscription()" /> + <textarea + placeholder="Transcription as JSON" + class="insert-transcription-area" + [(ngModel)]="transcriptionInput" + aria-label="Transcription JSON" + [attr.aria-invalid]="!isValidJson" + required></textarea> + <div *ngIf="!isValidJson" class="error-message" role="alert"> + Please enter valid JSON + </div> + <jhi-button + id="create-transcription-button" + [icon]="faCheck" + (onClick)="createTranscription()" + [loading]="isCreating" + [disabled]="!isValidJson || isCreating" + aria-label="Create Transcription" /> </div> </div> </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20250113154600_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (27)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisConnectorService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisJobService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisWebhookService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/transcriptionIngestion/PyrisTranscriptionIngestionWebhookDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/transcriptionIngestion/PyrisWebhookTranscriptionIngestionExecutionDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/TranscriptionIngestionWebhookJob.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTranscriptionResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/domain/Lecture.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/domain/Transcription.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/domain/TranscriptionSegment.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/dto/TranscriptionDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/dto/TranscriptionSegmentDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/repository/TranscriptionRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/TranscriptionResource.java(1 hunks)src/main/webapp/app/admin/admin.routes.ts(1 hunks)src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.html(1 hunks)src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.scss(1 hunks)src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.ts(1 hunks)src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription.service.ts(1 hunks)src/main/webapp/i18n/en/global.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java(1 hunks)src/test/javascript/spec/component/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.spec.ts(1 hunks)src/test/javascript/spec/component/admin/lecture-transcription-ingestion/lecture-transcription.service.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/java/de/tum/cit/aet/artemis/lecture/dto/TranscriptionSegmentDTO.java
- src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.scss
- src/main/java/de/tum/cit/aet/artemis/lecture/dto/TranscriptionDTO.java
🧰 Additional context used
📓 Path-based instructions (23)
src/main/webapp/app/admin/admin.routes.ts (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisConnectorService.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/iris/service/pyris/dto/transcriptionIngestion/PyrisWebhookTranscriptionIngestionExecutionDTO.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/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.ts (1)
src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.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/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/transcriptionIngestion/PyrisTranscriptionIngestionWebhookDTO.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/lecture/repository/TranscriptionRepository.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/lecture/service/LectureService.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/iris/service/pyris/job/TranscriptionIngestionWebhookJob.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/lecture/web/TranscriptionResource.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/lecture/repository/LectureRepository.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/iris/service/pyris/PyrisWebhookService.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/admin/lecture-transcription-ingestion/lecture-transcription.service.ts (1)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.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/lecture/domain/TranscriptionSegment.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/test/javascript/spec/component/admin/lecture-transcription-ingestion/lecture-transcription.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/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/javascript/spec/component/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.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/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTranscriptionResource.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/iris/service/pyris/PyrisJobService.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/lecture/domain/Lecture.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/lecture/domain/Transcription.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
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (7)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/transcriptionIngestion/PyrisWebhookTranscriptionIngestionExecutionDTO.java (1)
1-13: Implementation aligns with DTO best practicesThe
PyrisWebhookTranscriptionIngestionExecutionDTOis correctly implemented as a Java record, which is suitable for a simple data carrier. The use of@JsonInclude(JsonInclude.Include.NON_EMPTY)ensures that only non-empty fields are serialized, which is efficient for JSON payloads.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/TranscriptionIngestionWebhookJob.java (1)
13-20: Verify the intended behavior ofcanAccessmethodsBoth
canAccessmethods returnfalse, which might restrict all access. Please verify if this is the intended behavior. If so, consider adding a comment to explain why access is always denied, improving code clarity.src/main/webapp/app/admin/admin.routes.ts (1)
156-163: LGTM! Route configuration follows best practices.The route configuration is well-structured with:
- Proper lazy loading
- Correct authority checks
- Consistent naming with other routes
src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java (2)
40-40: LGTM!The import statement follows the project's import organization pattern.
175-182: LGTM!The implementation follows the established pattern for mock methods in this class, with proper request handling and response generation.
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (1)
54-56: LGTM!The imports and field declaration follow the project's conventions.
src/main/webapp/i18n/en/global.json (1)
120-121: LGTM!The new translation key follows the existing pattern and naming conventions.
src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTranscriptionResource.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTranscriptionResource.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/web/IrisTranscriptionResource.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/lecture/domain/TranscriptionSegment.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisWebhookService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java
Outdated
Show resolved
Hide resolved
…:ls1intum/Artemis into feature/iris/video-transcription-storage
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java (1)
61-86: 🛠️ Refactor suggestionEnhance test coverage with additional test cases.
The current test suite could be expanded to cover more scenarios:
- Missing negative test cases (e.g., invalid input)
- Limited assertions (only checking auth token)
- No validation of the actual transcription data
Consider adding these test cases:
@Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void testIngestTranscriptionWithInvalidJson() { activateIrisFor(lecture1.getCourse()); request.put("/api/courses/" + lecture1.getCourse().getId() + "/lectures/" + lecture1.getId() + "/ingest-transcription", Optional.of("invalid json"), HttpStatus.BAD_REQUEST); } @Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void testIngestTranscriptionVerifyData() { activateIrisFor(lecture1.getCourse()); irisRequestMockProvider.mockTranscriptionIngestionWebhookRunResponse(dto -> { assertThat(dto.settings().authenticationToken()).isNotNull(); assertThat(dto.data()).isNotNull(); // Add more specific data validations }); request.put("/api/courses/" + lecture1.getCourse().getId() + "/lectures/" + lecture1.getId() + "/ingest-transcription", Optional.empty(), HttpStatus.OK); }
🧹 Nitpick comments (4)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java (2)
28-28: Follow Java naming conventions for repository variable.The variable name
lecturetranscriptionRepositoryshould be in camelCase format.- private LectureTranscriptionRepository lecturetranscriptionRepository; + private LectureTranscriptionRepository lectureTranscriptionRepository;
41-59: Improve test setup organization and documentation.The setup method could be improved in several ways:
- Add JavaDoc explaining the test setup and data
- Extract helper methods for better organization
- Use constants for test data values
Consider refactoring like this:
+ private static final double SEGMENT_START_TIME = 0.0; + private static final double SEGMENT_END_TIME = 12.0; + private static final String SEGMENT_1_TEXT = "Welcome to today's lecture"; + private static final String SEGMENT_2_TEXT = "Today we will talk about Artemis"; + + /** + * Sets up test environment with: + * - Users: 1 instructor, 1 student, 1 tutor + * - Course with exercises and lectures + * - Lecture with two transcription segments + * - Additional users not in the course + */ @BeforeEach void initTestCase() throws Exception { + setupUsers(); + setupCourseAndLecture(); + setupTranscription(); + } + + private void setupUsers() { userUtilService.addUsers(TEST_PREFIX, 1, 1, 0, 1); + setupAdditionalUsers(); + }src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (2)
301-307: Update the JavaDoc description.The current description appears to be copied from another endpoint. It should describe the transcription ingestion functionality instead.
- * This endpoint is for starting the ingestion of all lectures or only one lecture when triggered in Artemis. + * This endpoint is for ingesting lecture transcriptions into Pyris. * - * @param lectureId If this id is present then only ingest this one lecture of the respective course + * @param courseId the ID of the course containing the lecture + * @param lectureId the ID of the lecture whose transcriptions should be ingested * @return the ResponseEntity with status 200 (OK) and a message success or null if the operation failed
324-325: Remove unnecessary Set conversion.The
transcriptionsvariable is already a Set, making the HashSet conversion redundant.- Set<LectureTranscription> transcriptionsToIngest = new HashSet<>(transcriptions); - lectureService.ingestTranscriptionInPyris(transcriptionsToIngest, course, lecture); + lectureService.ingestTranscriptionInPyris(transcriptions, lecture.getCourse(), lecture);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureTranscriptionRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureUnitRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(5 hunks)src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.ts(1 hunks)src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription.service.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureUnitRepository.java
- src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription.service.ts
- src/main/webapp/app/admin/lecture-transcription-ingestion/lecture-transcription-ingestion.component.ts
- src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureTranscriptionRepository.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.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/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: Analyse
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisTranscriptionIngestionTest.java (1)
71-76: 🛠️ Refactor suggestionImprove error handling test expectations.
The test expects
INTERNAL_SERVER_ERRORfor an invalid lecture ID, which is not the most appropriate status code. ANOT_FOUNDstatus would be more suitable for this scenario.- request.put("/api/courses/" + lecture1.getCourse().getId() + "/lectures/" + 9999L + "/ingest-transcription", Optional.empty(), HttpStatus.INTERNAL_SERVER_ERROR); + request.put("/api/courses/" + lecture1.getCourse().getId() + "/lectures/" + 9999L + "/ingest-transcription", Optional.empty(), HttpStatus.NOT_FOUND);Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (2)
53-53: LGTM!The imports are specific and follow Java best practices.
Also applies to: 56-56
74-75: LGTM!The changes follow Spring best practices:
- Constructor injection is used
- Repository field is properly encapsulated
Also applies to: 99-110
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureTranscriptionResource.java (1)
101-106:⚠️ Potential issueFix inconsistencies in endpoint implementation.
- URL pattern is inconsistent with POST endpoint (should be
courses/{courseId}/lecture/{lectureId}/transcriptions/{transcriptionId})- Method name should be
getTranscription(singular) to match its functionality- Improve exception handling for not found case
- @GetMapping("lectures/{lectureId}/transcription/{transcriptionId}") + @GetMapping("courses/{courseId}/lecture/{lectureId}/transcriptions/{transcriptionId}") @EnforceAtLeastStudent - public ResponseEntity<LectureTranscription> getLectureTranscriptions(@PathVariable Long lectureId, @PathVariable Long transcriptionId) { + public ResponseEntity<LectureTranscription> getTranscription( + @PathVariable Long courseId, + @PathVariable Long lectureId, + @PathVariable Long transcriptionId) { log.debug("REST request to get transcription {}", transcriptionId); - LectureTranscription transcription = lectureTranscriptionRepository.findById(transcriptionId).orElseThrow(); + LectureTranscription transcription = lectureTranscriptionRepository.findById(transcriptionId) + .orElseThrow(() -> new EntityNotFoundException("Transcription not found with id " + transcriptionId));
🧹 Nitpick comments (7)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisWebhookService.java (3)
75-82: Enhance JavaDoc with @throws documentation.Add
@throws IllegalArgumentExceptionto document the validation exceptions./** * adds the transcription to the vector database in Pyris * * @param transcriptions The transcription that got Updated * @param course The course of the transcriptions * @param lecture The lecture of the transcriptions * @return jobToken if the job was created else null + * @throws IllegalArgumentException if transcriptions is null/empty or if any transcription is invalid */
83-107: Consider consolidating validation checks.The implementation looks good but could benefit from consolidating the validation checks for better maintainability.
public String addTranscriptionsToPyrisDB(Set<LectureTranscription> transcriptions, Course course, Lecture lecture) { - if (transcriptions == null || transcriptions.isEmpty()) { - throw new IllegalArgumentException("Transcriptions cannot be empty"); - } - - if (!lectureIngestionEnabled(course)) { - return null; - } - - transcriptions.forEach(transcription -> { - if (transcription.getLecture() == null) { - throw new IllegalArgumentException("Transcription must be associated with a lecture"); - } - else if (!transcription.getLecture().equals(lecture)) { - throw new IllegalArgumentException("All transcriptions must be associated with the same lecture"); - } - }); + validateTranscriptions(transcriptions, lecture); + + if (!lectureIngestionEnabled(course)) { + return null; + } List<PyrisTranscriptionIngestionWebhookDTO> pyrisTranscriptionIngestionWebhookDTOs = transcriptions.stream() .map(transcription -> new PyrisTranscriptionIngestionWebhookDTO(transcription, lecture.getId(), lecture.getTitle(), course.getId(), course.getTitle(), course.getDescription())) .toList(); return executeTranscriptionAdditionWebhook(pyrisTranscriptionIngestionWebhookDTOs, course, lecture); } + + private void validateTranscriptions(Set<LectureTranscription> transcriptions, Lecture lecture) { + if (transcriptions == null || transcriptions.isEmpty()) { + throw new IllegalArgumentException("Transcriptions cannot be empty"); + } + + transcriptions.forEach(transcription -> { + if (transcription.getLecture() == null) { + throw new IllegalArgumentException("Transcription must be associated with a lecture"); + } + else if (!transcription.getLecture().equals(lecture)) { + throw new IllegalArgumentException("All transcriptions must be associated with the same lecture"); + } + }); + }
109-122: Document all parameters and hardcoded values.The implementation looks good but needs better documentation:
- Add missing parameter documentation.
- Document the hardcoded "fullIngestion" variant.
/** * executes executeTranscriptionAdditionWebhook to insert the transcription into the vector database of Pyris * * @param toUpdateTranscriptions The transcription that are going to be Updated + * @param course The course containing the transcriptions + * @param lecture The lecture containing the transcriptions * @return jobToken if the job was created */ private String executeTranscriptionAdditionWebhook(List<PyrisTranscriptionIngestionWebhookDTO> toUpdateTranscriptions, Course course, Lecture lecture) { String jobToken = pyrisJobService.addTranscriptionIngestionWebhookJob(course.getId(), lecture.getId()); PyrisPipelineExecutionSettingsDTO settingsDTO = new PyrisPipelineExecutionSettingsDTO(jobToken, List.of(), artemisBaseUrl); PyrisWebhookTranscriptionIngestionExecutionDTO executionDTO = new PyrisWebhookTranscriptionIngestionExecutionDTO(toUpdateTranscriptions, lecture.getTitle(), settingsDTO, List.of()); + // "fullIngestion" variant indicates a complete ingestion of all transcription data pyrisConnectorService.executeTranscriptionAdditionWebhook("fullIngestion", executionDTO); return jobToken; }src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (1)
301-308: Update the JavaDoc description.The current JavaDoc description appears to be copy-pasted from another endpoint and incorrectly describes the functionality. Update it to accurately describe the transcription ingestion endpoint.
- * This endpoint is for starting the ingestion of all lectures or only one lecture when triggered in Artemis. + * This endpoint triggers the ingestion of transcriptions for a specific lecture into Pyris.src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureTranscriptionResource.java (3)
39-39: Consider adding API version prefix to base path.The base path
api/should include version information (e.g.,api/v1/) to support future API versioning and backward compatibility.-@RequestMapping("api/") +@RequestMapping("api/v1/")
86-88: Remove redundant ID nullification.Setting ID to null is redundant as new entities should not have an ID set in the DTO.
LectureTranscription lectureTranscription = new LectureTranscription(lecture, transcriptionDTO.language(), segments); - lectureTranscription.setId(null);
91-91: Use UriComponentsBuilder for URI construction.Use Spring's
UriComponentsBuilderto safely construct URIs instead of string concatenation.- return ResponseEntity.created(new URI("/api/lecture/" + lectureId + "/transcriptions/" + result.getId())).body(result); + return ResponseEntity.created( + UriComponentsBuilder.fromPath("/api/lecture/{lectureId}/transcriptions/{id}") + .buildAndExpand(lectureId, result.getId()) + .toUri() + ).body(result);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisWebhookService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureTranscriptionResource.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureService.java
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.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/iris/service/pyris/PyrisWebhookService.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/lecture/web/LectureTranscriptionResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (4)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisWebhookService.java (1)
33-34: LGTM! Clean import statements.The imports are well-organized and follow Java best practices by avoiding wildcard imports.
Also applies to: 39-39
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (2)
53-53: LGTM!The imports are specific and follow Java conventions.
Also applies to: 56-56
74-75: LGTM!The dependency injection follows the constructor injection pattern and maintains consistency with other repository fields.
Also applies to: 99-111
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureTranscriptionResource.java (1)
42-61: Well-structured dependency injection!Good practices observed:
- Constructor injection
- Final fields
- Proper logger setup
| Set<LectureTranscription> transcriptions = lectureTranscriptionRepository.findAllWithTranscriptionSegmentsByLectureId(lectureId); | ||
| if (transcriptions.isEmpty()) { | ||
| return ResponseEntity.badRequest().headers(HeaderUtil.createAlert(applicationName, "artemisApp.iris.ingestionAlert.noTranscriptionError", "noTranscription")) | ||
| .body(null); | ||
| } | ||
| Set<LectureTranscription> transcriptionsToIngest = new HashSet<>(transcriptions); | ||
| lectureService.ingestTranscriptionInPyris(transcriptionsToIngest, course, lecture); | ||
| return ResponseEntity.ok().build(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant course variable and simplify transcription ingestion.
The course variable is only used as a parameter for ingestTranscriptionInPyris. Since we can get the course from the lecture object, we can simplify this code.
Set<LectureTranscription> transcriptions = lectureTranscriptionRepository.findAllWithTranscriptionSegmentsByLectureId(lectureId);
if (transcriptions.isEmpty()) {
return ResponseEntity.badRequest().headers(HeaderUtil.createAlert(applicationName, "artemisApp.iris.ingestionAlert.noTranscriptionError", "noTranscription"))
.body(null);
}
Set<LectureTranscription> transcriptionsToIngest = new HashSet<>(transcriptions);
- lectureService.ingestTranscriptionInPyris(transcriptionsToIngest, course, lecture);
+ lectureService.ingestTranscriptionInPyris(transcriptionsToIngest, lecture.getCourse(), lecture);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Set<LectureTranscription> transcriptions = lectureTranscriptionRepository.findAllWithTranscriptionSegmentsByLectureId(lectureId); | |
| if (transcriptions.isEmpty()) { | |
| return ResponseEntity.badRequest().headers(HeaderUtil.createAlert(applicationName, "artemisApp.iris.ingestionAlert.noTranscriptionError", "noTranscription")) | |
| .body(null); | |
| } | |
| Set<LectureTranscription> transcriptionsToIngest = new HashSet<>(transcriptions); | |
| lectureService.ingestTranscriptionInPyris(transcriptionsToIngest, course, lecture); | |
| return ResponseEntity.ok().build(); | |
| Set<LectureTranscription> transcriptions = lectureTranscriptionRepository.findAllWithTranscriptionSegmentsByLectureId(lectureId); | |
| if (transcriptions.isEmpty()) { | |
| return ResponseEntity.badRequest().headers(HeaderUtil.createAlert(applicationName, "artemisApp.iris.ingestionAlert.noTranscriptionError", "noTranscription")) | |
| .body(null); | |
| } | |
| Set<LectureTranscription> transcriptionsToIngest = new HashSet<>(transcriptions); | |
| lectureService.ingestTranscriptionInPyris(transcriptionsToIngest, lecture.getCourse(), lecture); | |
| return ResponseEntity.ok().build(); |
| public ResponseEntity<LectureTranscription> createLectureTranscription(@RequestBody LectureTranscriptionDTO transcriptionDTO, @PathVariable Long courseId, | ||
| @PathVariable Long lectureId) throws URISyntaxException { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation annotations to DTO parameters.
Add @Valid annotation to validate the DTO and consider adding validation constraints to the DTO fields.
- public ResponseEntity<LectureTranscription> createLectureTranscription(@RequestBody LectureTranscriptionDTO transcriptionDTO, @PathVariable Long courseId,
+ public ResponseEntity<LectureTranscription> createLectureTranscription(@Valid @RequestBody LectureTranscriptionDTO transcriptionDTO, @PathVariable Long courseId,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ResponseEntity<LectureTranscription> createLectureTranscription(@RequestBody LectureTranscriptionDTO transcriptionDTO, @PathVariable Long courseId, | |
| @PathVariable Long lectureId) throws URISyntaxException { | |
| public ResponseEntity<LectureTranscription> createLectureTranscription(@Valid @RequestBody LectureTranscriptionDTO transcriptionDTO, @PathVariable Long courseId, | |
| @PathVariable Long lectureId) throws URISyntaxException { |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureTranscriptionResource.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureTranscriptionResource.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.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
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
- GitHub Check: Build and Push Docker Image
🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (5)
53-56: LGTM!The new imports are specific and necessary for the transcription functionality.
74-75: LGTM!The new repository field and constructor parameter follow Spring's dependency injection best practices and maintain consistency with existing code patterns.
Also applies to: 99-110
301-308: LGTM!The JavaDoc documentation is clear and comprehensive.
309-312: LGTM!The endpoint configuration with @Profile(PROFILE_IRIS) and @EnforceAtLeastInstructorInCourse is appropriate.
319-326: 🛠️ Refactor suggestionSimplify transcription ingestion logic.
Remove redundant course variable and simplify the ingestion call.
Set<LectureTranscription> transcriptions = lectureTranscriptionRepository.findAllWithTranscriptionSegmentsByLectureId(lectureId); if (transcriptions.isEmpty()) { - return ResponseEntity.badRequest().headers(HeaderUtil.createAlert(applicationName, "artemisApp.iris.ingestionAlert.noTranscriptionError", "noTranscription")) - .body(null); + throw new BadRequestAlertException("No transcriptions found for the lecture", ENTITY_NAME, "noTranscription"); } - Set<LectureTranscription> transcriptionsToIngest = new HashSet<>(transcriptions); - lectureService.ingestTranscriptionInPyris(transcriptionsToIngest, course, lecture); + lectureService.ingestTranscriptionInPyris(new HashSet<>(transcriptions), lecture.getCourse(), lecture); return ResponseEntity.ok().build();Likely invalid or redundant comment.
|
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. |
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
This is a temporary solution to ingest lecture video transcriptions into the Artemis MySQL database until the transcription generation is implemented. Those transcriptions are needed for Pyris, so that Iris can later on reference lecture videos in student questions.
Description
Steps for Testing
Prerequisites:
example:
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
Test Coverage
Summary by CodeRabbit
Summary by CodeRabbit
Here are the updated release notes for the provided pull request:
New Features
Backend Improvements
Frontend Improvements
Security