Lectures: Add reference between video unit and attachment unit
#10468
Conversation
github-actions
bot
added
tests
server
WalkthroughThis update establishes a bidirectional one-to-one relationship between video units and attachment units. On the backend, new fields and accessor methods are added to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant LectureService
participant VideoForm
User->>Component: Initiate create/edit video unit
Component->>LectureService: Request lecture details
LectureService-->>Component: Return lecture details (with attachment units)
Component->>Component: Filter available attachment units
Component->>VideoForm: Supply formData and availableAttachmentUnits
VideoForm-->>User: Render form with attachment unit selection
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 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 (8)
🪧 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: 2
🔭 Outside diff range comments (1)
src/test/javascript/spec/component/lecture/lecture-units.component.spec.ts (1)
82-99: 🛠️ Refactor suggestionCheck the response type in mocked lecture data.
You're assigningnew Lecture()toHttpResponse<VideoUnit>. Update the generic type toLectureif that’s truly the returned object.
🧹 Nitpick comments (2)
src/test/javascript/spec/component/lecture-unit/video-unit/create-video-unit.component.spec.ts (1)
163-163: Fix typo in test description.There's a typo in the test description: "expect" should be "except".
- it('should provide all attachment units expect attachment unit a attached to a different video unit', fakeAsync(() => { + it('should provide all attachment units except attachment unit attached to a different video unit', fakeAsync(() => {src/main/webapp/app/lecture/lecture-units/lecture-units.component.ts (1)
340-355: Well-implemented method for fetching attachment units.The fetchAvailableAttachmentUnitsForVideoUnit method is well-structured with proper error handling and filtering logic. Consider adding a comment explaining the purpose of the empty object at the beginning of the availableAttachmentUnits array (it's likely for creating a "none" option in a dropdown).
this.availableAttachmentUnits = [ - {}, + {}, // Empty object to provide a "None" option in the dropdown ...(lecture.body?.lectureUnits ?? []).filter((unit: LectureUnit) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20250312010510_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (18)
src/main/java/de/tum/cit/aet/artemis/lecture/domain/AttachmentUnit.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/domain/VideoUnit.java(2 hunks)src/main/webapp/app/entities/lecture-unit/attachmentUnit.model.ts(1 hunks)src/main/webapp/app/entities/lecture-unit/videoUnit.model.ts(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-video-unit/create-video-unit.component.html(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-video-unit/create-video-unit.component.ts(3 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/edit-video-unit/edit-video-unit.component.html(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/edit-video-unit/edit-video-unit.component.ts(3 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/video-unit-form/video-unit-form.component.html(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/video-unit-form/video-unit-form.component.ts(3 hunks)src/main/webapp/app/lecture/lecture-units/lecture-units.component.html(1 hunks)src/main/webapp/app/lecture/lecture-units/lecture-units.component.ts(7 hunks)src/main/webapp/i18n/de/lectureUnit.json(1 hunks)src/main/webapp/i18n/en/lectureUnit.json(1 hunks)src/test/javascript/spec/component/lecture-unit/video-unit/create-video-unit.component.spec.ts(3 hunks)src/test/javascript/spec/component/lecture-unit/video-unit/edit-video-unit.component.spec.ts(7 hunks)src/test/javascript/spec/component/lecture-unit/video-unit/video-unit-form.component.spec.ts(1 hunks)src/test/javascript/spec/component/lecture/lecture-units.component.spec.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/lectureUnit.json
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/entities/lecture-unit/videoUnit.model.tssrc/main/webapp/app/entities/lecture-unit/attachmentUnit.model.tssrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/video-unit-form/video-unit-form.component.tssrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-video-unit/create-video-unit.component.tssrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/edit-video-unit/edit-video-unit.component.tssrc/main/webapp/app/lecture/lecture-units/lecture-units.component.ts
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
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/javascript/spec/component/lecture-unit/video-unit/video-unit-form.component.spec.tssrc/test/javascript/spec/component/lecture/lecture-units.component.spec.tssrc/test/javascript/spec/component/lecture-unit/video-unit/edit-video-unit.component.spec.tssrc/test/javascript/spec/component/lecture-unit/video-unit/create-video-unit.component.spec.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
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/webapp/app/lecture/lecture-unit/lecture-unit-management/video-unit-form/video-unit-form.component.htmlsrc/main/webapp/app/lecture/lecture-units/lecture-units.component.htmlsrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-video-unit/create-video-unit.component.htmlsrc/main/webapp/app/lecture/lecture-unit/lecture-unit-management/edit-video-unit/edit-video-unit.component.html
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/VideoUnit.javasrc/main/java/de/tum/cit/aet/artemis/lecture/domain/AttachmentUnit.java
🧠 Learnings (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/video-unit-form/video-unit-form.component.ts (1)
Learnt from: florian-glombik
PR: ls1intum/Artemis#9656
File: src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts:69-71
Timestamp: 2025-03-12T13:33:48.254Z
Learning: In the `src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachment-unit-form/attachment-unit-form.component.ts`, the `isFormValid` computed property intentionally uses a logical OR between `this.statusChanges() === 'VALID'` and `this.fileName()` to mirror the original `isSubmitPossible` getter, ensuring consistent form validation behavior.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (53)
src/main/webapp/app/entities/lecture-unit/attachmentUnit.model.ts (1)
4-4: Looks good - bidirectional relationship establishedThe addition of the
VideoUnitimport and thecorrespondingVideoUnitproperty correctly implements the one-to-one relationship betweenAttachmentUnitandVideoUnitas mentioned in the PR objectives.Also applies to: 11-11
src/main/webapp/app/entities/lecture-unit/videoUnit.model.ts (1)
2-2:❓ Verification inconclusive
Code looks good, but be aware of potential circular dependency
The addition of the
AttachmentUnitimport and thecorrespondingAttachmentUnitproperty correctly implements the bidirectional relationship. However, there's now a circular dependency between these two model files that could potentially cause issues with some JavaScript bundlers.Also applies to: 9-9
🏁 Script executed:
#!/bin/bash # Check if the circular dependency might cause issues with the current bundler configuration # Look for any TypeScript configuration files that might handle circular dependencies fd -e json -e js "tsconfig|webpack" --exec cat {} # Check if there are any existing circular dependencies in the codebase as precedent rg -p "import.*from.*" --glob "*.ts" | rg -A 1 -B 1 "circular|cycle"Length of output: 7315
Circular Dependency Caution for Video Unit Model
The updated import for
AttachmentUnitcorrectly establishes the bidirectional relationship; however, this introduces a circular dependency between the model files. Our search of the TS and bundler configuration files didn’t reveal any special handling for circular dependencies. Although there’s no current evidence of issues in production, please double-check that your bundler can safely resolve such cycles. The same caution applies to the corresponding update on lines 9-9.src/main/webapp/app/lecture/lecture-units/lecture-units.component.html (1)
36-36: Property binding looks goodThe addition of the
[availableAttachmentUnits]input property to the video unit form component correctly passes the list of available attachment units to be used in the dropdown.src/main/webapp/i18n/de/lectureUnit.json (1)
73-73: Translation looks goodThe German translation "Dateieinheit" for "attachmentUnit" follows the informal style (dutzen) as required by the coding guidelines.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/edit-video-unit/edit-video-unit.component.html (1)
2-2: New property added for mapping attachment units to video units.The component now passes available attachment units to the video form component, supporting the new one-to-one relationship between video units and attachment units as described in the PR objectives.
src/test/javascript/spec/component/lecture-unit/video-unit/video-unit-form.component.spec.ts (1)
128-128: Test assertion updated to include new attachment relationship.The test now correctly verifies that the form submission includes the
correspondingAttachmentUnitIdproperty, which is properly initialized tonull. This ensures the new relationship between video and attachment units is properly tested.src/main/webapp/i18n/en/lectureUnit.json (1)
73-73: Added localization for new attachment unit field.The new localization entry supports the UI for selecting a corresponding attachment unit in the video unit form, aligning with the PR objective to establish a connection between video and attachment units.
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/video-unit-form/video-unit-form.component.html (1)
108-115: Added form field for attachment unit selection.The form now includes a select dropdown that allows users to choose a corresponding attachment unit for the video unit. This implements the UI portion of the one-to-one relationship described in the PR objectives.
A few observations:
- The component correctly uses the new
availableAttachmentUnits()method- The form uses modern Angular syntax with
@forinstead of*ngForas per guidelines- The field is optional (no required marker or validation), which is appropriate for this relationship
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-video-unit/create-video-unit.component.html (1)
2-2: Correctly implemented inputs for attachment unit reference.The addition of
[formData]="formData"and[availableAttachmentUnits]="availableAttachmentUnits"inputs properly enables the reference between video and attachment units, which aligns with the PR objectives.src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/video-unit-form/video-unit-form.component.ts (4)
13-13: Appropriate import of AttachmentUnit model.The addition of the import statement follows the coding guidelines by avoiding star imports and maintaining proper organization.
21-21: Model extended with corresponding attachment unit reference.The addition of the optional
correspondingAttachmentUnitIdproperty to theVideoUnitFormDatainterface properly supports the bidirectional relationship being established.
76-76: Input property added for available attachment units.The new input property follows Angular best practices and the coding guidelines by using camelCase and proper typing.
96-96: Form control added for corresponding attachment unit.The form control properly integrates with the existing form structure and allows for the optional selection of an attachment unit.
src/main/java/de/tum/cit/aet/artemis/lecture/domain/VideoUnit.java (4)
6-7: Appropriate imports for JPA annotations.The JPA annotations are correctly imported individually rather than using star imports, adhering to the coding guidelines.
9-9: JsonIgnoreProperties import properly added.The import is correctly added to support the JSON serialization annotations required for the bidirectional relationship.
39-42: One-to-one relationship properly established.The
@OneToOneand@JoinColumnannotations correctly set up the relationship withAttachmentUnit, and the@JsonIgnorePropertiesprevents infinite recursion during serialization by ignoring the back-reference.
44-50: Accessor methods follow least access principle.The getter and setter methods adhere to the Java coding guidelines by following the least access principle and CamelCase naming conventions.
src/main/java/de/tum/cit/aet/artemis/lecture/domain/AttachmentUnit.java (2)
35-37: Bidirectional one-to-one relationship correctly configured.The
@OneToOne(mappedBy = "correspondingAttachmentUnit")annotation properly establishes the bidirectional relationship, withVideoUnitbeing the owning side. The@JsonIgnorePropertiesprevents infinite recursion during serialization.
73-79: Accessor methods for corresponding video unit.The getter and setter methods correctly follow Java naming conventions and encapsulation principles, providing a clean API for accessing and modifying the relationship.
src/test/javascript/spec/component/lecture-unit/video-unit/edit-video-unit.component.spec.ts (7)
4-4: Good use of fakeAsync and tick.
No concerns with adding asynchronous testing utilities.
19-23: New imports look fine.
Bringing in theProfileService,LectureService,AttachmentUnit, andTextUnitis valid and aligns with the added functionality.
35-35: Consistent approach for mocking dependencies.
UsingMockProvider(LectureService)is consistent with how other services are handled.
71-76: Compile and component creation steps are correct.
No issues found with the updated approach to compile components and set up the fixtures.
125-134: Duplicate concern about type mismatch.
This block repeats the same pattern of usingHttpResponse<VideoUnit>for aLectureobject.
177-220: Recurrent type mismatch in test.
Please update the generic type to match theLectureobject in the response:- const lectureResponse: HttpResponse<VideoUnit> = ... + const lectureResponse: HttpResponse<Lecture> = ...
222-272: 1) Same type inconsistency.
Again, this test sets aLectureobject inHttpResponse<VideoUnit>. The same fix applies as before.2) Grammar nitpick in the test name.
Consider changing it from "expect attachment unit a attached" to "except the attachment unit attached."src/test/javascript/spec/component/lecture/lecture-units.component.spec.ts (7)
2-2: Proper import for asynchronous test utilities.
No issues with addingfakeAsyncandtick.
33-33: Importing LectureService is appropriate.
Aligns with the new test logic.
54-54: Mocking LectureService is consistent.
The approach matches other mock provider patterns.
771-779: Repeat type mismatch in test.
Consistently applyHttpResponse<Lecture>wherenew Lecture()is returned.
789-798: Recurrent type mismatch.
Similar fix required forHttpResponse<VideoUnit>vs.Lectureobject.
809-840: Similar type mismatch in lecture-based data.
Use the correct generic type in the mocked response.
842-882: 1) Wrong test name wording.
Change “expect attachment unit a attached” to “except the attachment unit attached.”2) Reused mismatch concern.
HttpResponse<VideoUnit>is populated with aLecture.src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-video-unit/create-video-unit.component.ts (3)
8-16: New imports appear valid.
Inclusion offinalize,switchMap,take, and additional model/service imports aligns with the updated data flow.
28-36: Added dependencies and properties look correct.
InjectingLectureServiceand declaringformDataplus a list ofavailableAttachmentUnitsis consistent with the new feature requirements.
74-82: Form data destructuring is clear.
The logic responsibly checks ifcorrespondingAttachmentUnitIdexists before settingcorrespondingAttachmentUnit, ensuring a correct one-to-one link.src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/edit-video-unit/edit-video-unit.component.ts (7)
9-15: Good job with adding the necessary imports!The imports are well-organized and follow a logical order. You've correctly imported the required dependencies for the newly added functionality.
26-26: Correctly injected the LectureService.The LectureService is properly injected as a private service, following Angular's dependency injection pattern.
33-35: Good approach storing videoUnitId as a class property.Moving videoUnitId from a local variable to a class property improves reusability across methods. The addition of availableAttachmentUnits property is also appropriate for the new functionality.
46-48: Good use of forkJoin for concurrent requests.Using forkJoin to fetch both the video unit and lecture details concurrently is an excellent performance optimization compared to sequential requests.
55-64: Properly handling the response data.The destructuring of results from forkJoin and setting up the formData with the correspondingAttachmentUnitId is well implemented.
66-74: Well-implemented filtering logic for attachment units.The filtering logic ensures that only relevant attachment units are displayed - either those not linked to any video unit or those linked to the current video unit being edited.
81-87: Correctly handling the correspondingAttachmentUnit relationship.The updateVideoUnit method properly destructures the formData to include correspondingAttachmentUnitId and sets the correspondingAttachmentUnit property with the correct type.
src/test/javascript/spec/component/lecture-unit/video-unit/create-video-unit.component.spec.ts (4)
20-25: Appropriate imports added for testing.The new imports for LectureService, AttachmentUnit, TextUnit, and Lecture are correctly added to support the new test cases.
37-37: Correctly added MockProvider for LectureService.The LectureService is properly mocked in the testing module setup.
134-161: Good test case for verifying attachment units retrieval.This test thoroughly verifies that all attachment units are correctly retrieved from the lecture and filtered properly, while text units are excluded.
164-194: Good test for filtering out attached units.This test correctly verifies that attachment units linked to other video units are excluded from the available options.
src/main/webapp/app/lecture/lecture-units/lecture-units.component.ts (6)
20-27: Appropriate imports added.The HttpResponse import and LectureService are correctly added to support the new functionality.
50-50: Correctly injected the LectureService.The LectureService is properly injected as a protected service, following the pattern used for other services in this component.
85-85: Good addition of availableAttachmentUnits property.This property is needed to store the attachment units available for selection in the video unit form.
107-107: Properly integrating attachment unit fetching in workflow.The fetchAvailableAttachmentUnitsForVideoUnit method is correctly called when creating a video unit.
161-171: Well-implemented video unit update with attachment relationship.The createEditVideoUnit method correctly handles the correspondingAttachmentUnitId from the form data and properly sets the correspondingAttachmentUnit property with the correct type.
307-314: Good integration in edit workflow.The fetchAvailableAttachmentUnitsForVideoUnit method is correctly called when editing a video unit, passing the videoUnitId parameter. The correspondingAttachmentUnitId is also properly included in the videoUnitFormData.
src/test/javascript/spec/component/lecture-unit/video-unit/edit-video-unit.component.spec.ts
Outdated
Show resolved
Hide resolved
Checklist
General
Server
Client
Motivation and Context
Iris needs to know which video unit belongs to which attachment unit.
Description
To enhance Iris chats and answer with more context from video units and attachment units, Iris needs to know exactly which video unit is connected to which attachment unit. This ensures both types of units are used when answering students queries. For that a OneToOne reference between video units and attachment units has been added.
Steps for Testing
Prerequisites:
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
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Tests