Programming exercises: Allow to choose preliminary feedback model
#10441
Conversation
…odules: client side
…g-exercises/choose-preliminary-feedback-model
…iminary-feedback-model
…oose-preliminary-feedback-model' into feature/programming-exercises/choose-preliminary-feedback-model
…iminary-feedback-model
…g-exercises/choose-preliminary-feedback-model # Conflicts: # src/main/webapp/app/exercises/modeling/manage/modeling-exercise.module.ts # src/main/webapp/app/exercises/programming/shared/lifecycle/programming-exercise-lifecycle.module.ts # src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts # src/main/webapp/app/exercises/text/manage/text-exercise/text-exercise.module.ts # src/main/webapp/app/overview/exercise-details/exercise-details-student-actions.component.html # src/main/webapp/i18n/de/exercise.json # src/test/javascript/spec/component/exercises/shared/feedback/feedback-suggestion-option.component.spec.ts # src/test/javascript/spec/component/modeling-exercise/modeling-exercise-update.component.spec.ts # src/test/javascript/spec/component/overview/exercise-details/exercise-details-student-actions.component.spec.ts # src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.spec.ts # src/test/javascript/spec/component/programming-exercise/programming-exercise-lifecycle.component.spec.ts # src/test/javascript/spec/component/text-exercise/text-exercise-update.component.spec.ts
github-actions
bot
added
tests
server
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
...app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts
Outdated
Show resolved
Hide resolved
...p/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts
Outdated
Show resolved
Hide resolved
...app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html (1)
33-34:⚠️ Potential issueRemove extraneous closing brace
There appears to be an extra closing brace at the end of the file that doesn't match any opening brace. This is likely a syntax error.
} 33 } 34 -33 -} -34
🧹 Nitpick comments (5)
src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts (3)
56-59: Consider using more concise observable testing pattern.You're subscribing to an observable within the test which makes the test more verbose. Since this is a test for a simple boolean value, you could use the
firstValueFromutility or testing utilities for more concise observable assertions.-component.isAthenaEnabled$.subscribe((result) => { - expect(result).toBeTrue(); -}); +import { firstValueFrom } from 'rxjs'; + +// Later in the test +const isEnabled = await firstValueFrom(component.isAthenaEnabled$); +expect(isEnabled).toBeTrue();
101-113: Avoid mutating test objects directly.Instead of mutating the
eventobject directly at line 109, create a new event object for the second test case. This makes the test more readable and isolates test scenarios better.-event.target.checked = false; -component.togglePreliminaryFeedback(event); +const uncheckedEvent = { target: { checked: false } }; +component.togglePreliminaryFeedback(uncheckedEvent);
41-42: Consider using more explicit object construction for test data.Using
as Exercisetype assertion with incomplete objects may hide potential type errors. Consider constructing a more complete test object for better test readability and maintainability.-component.exercise = { type: ExerciseType.TEXT, dueDate: futureDueDate, preliminaryFeedbackModule: undefined } as Exercise; +component.exercise = new Exercise( + undefined, // id + undefined, // title + ExerciseType.TEXT, // type + undefined, // categories + undefined, // difficulty + { dueDate: futureDueDate } +); +component.exercise.preliminaryFeedbackModule = undefined;Alternatively, if full object construction is too verbose, at least create a helper function that returns a properly typed object.
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html (1)
17-17: Add translation directive to the label for consistencyThe label for the athenaModuleSelect dropdown is not using the translation directive unlike other text elements in the template. For consistency with the app's internationalization approach, consider adding jhiTranslate.
- <label for="athenaModuleSelect">Athena Preliminary Feedback Module</label> + <label for="athenaModuleSelect" jhiTranslate="artemisApp.exercise.athenaPreliminaryFeedbackModule"></label>You'll need to add the corresponding translation key to the i18n files.
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts (1)
75-82: Add type for event parameterThe event parameter in the
togglePreliminaryFeedbackmethod should have a more specific type thananyfor better type safety.- togglePreliminaryFeedback(event: any) { + togglePreliminaryFeedback(event: Event) { this.showDropdownList = event.target.checked; if (event.target.checked) { this.exercise().preliminaryFeedbackModule = this.availableAthenaModules.first(); } else { this.exercise().preliminaryFeedbackModule = undefined; } }You'll need to add a type assertion for
event.target as HTMLInputElementto access thecheckedproperty.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts(2 hunks)src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html(1 hunks)src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts(1 hunks)src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts(1 hunks)src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts(3 hunks)src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(1 hunks)src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts(1 hunks)src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts(1 hunks)src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(2 hunks)src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html(1 hunks)src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts(1 hunks)src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts(2 hunks)src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(0 hunks)src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts(1 hunks)src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts(2 hunks)src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts(0 hunks)src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html(1 hunks)src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts(5 hunks)src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts
- src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts
- src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
- src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
- src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts
- src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html
- src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts
- src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts
- src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
- src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts
- src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html
- src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts
- src/main/webapp/app/exercises/shared/feedback-suggestion/exercise-feedback-suggestion-options.component.ts
🧰 Additional context used
📓 Path-based instructions (3)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.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/exercises/shared/feedback/preliminary-feedback-options.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/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html
🧠 Learnings (1)
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts (3)
Learnt from: dmytropolityka
PR: ls1intum/Artemis#10441
File: src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts:76-83
Timestamp: 2025-03-05T22:14:15.408Z
Learning: In the Artemis client code, the Array prototype is extended with `first()`, `last()`, and `shuffle()` methods (defined in src/main/webapp/app/shared/util/array.extension.ts). The `first()` method returns the first element of an array or undefined if the array is empty, while `last()` returns the last element or undefined if empty. These extensions allow for calls like `array.first()` and `array.last()` instead of using standard array indexing like `array[0]` or `array[array.length-1]`.
Learnt from: dmytropolityka
PR: ls1intum/Artemis#10441
File: src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts:76-83
Timestamp: 2025-03-05T22:14:15.408Z
Learning: In the Artemis client code, the Array prototype is extended with `first()` and `last()` methods (defined in src/main/webapp/app/shared/util/array.extension.ts), allowing for calls like `array.first()` and `array.last()` instead of using standard array indexing like `array[0]` or `array[array.length-1]`.
Learnt from: dmytropolityka
PR: ls1intum/Artemis#10441
File: src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts:76-83
Timestamp: 2025-03-05T22:14:15.408Z
Learning: In the Artemis client code, the Array prototype is extended with `first()` and `last()` methods, allowing for calls like `array.first()` and `array.last()` instead of using standard array indexing like `array[0]` or `array[array.length-1]`.
🔇 Additional comments (10)
src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts (5)
38-47: Good use of Jest spies and asynchronous testing.The test correctly uses Jest spying to mock service responses and properly awaits async operations before assertions.
22-24: Effective use of MockProvider for dependency injection.Good job using
MockProviderfrom ng-mocks to provide test doubles for the component's dependencies.
1-10: Clean imports with appropriate dependencies.The imports are well-organized and only include what's necessary for the tests.
62-71: Good test for conditional disabling of UI controls.This test effectively verifies that input controls are disabled under appropriate conditions (assessment type and read-only state).
96-113: Comprehensive test for toggle behavior.The test properly verifies that toggling the preliminary feedback checkbox sets the module and controls the dropdown visibility.
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.html (3)
25-27: LGTM on the new @for syntaxThe new
@forsyntax is correctly implemented, replacing the older*ngFordirective as per the coding guidelines.
1-1: LGTM on the new @if syntaxThe new
@ifsyntax is correctly implemented, replacing the older*ngIfdirective as per the coding guidelines.
15-15: LGTM on the new @if syntaxThe nested conditional for showing the dropdown list correctly uses the new
@ifsyntax.src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts (2)
19-21: LGTM on using input signalsYou've correctly implemented the new Angular Signal-based input API (
input()) as recommended in the previous review comments.
78-78: Correctly using array extensionsThe use of the
first()method on arrays is consistent with the Artemis client code conventions, as noted in the previous review thread.
...app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts (4)
46-46: UsetoBeTrue()instead oftoBeTruthy()According to the coding guidelines for tests, boolean assertions should use the more specific
toBeTrue()ortoBeFalse()rather thantoBeTruthy().- expect(component.modulesAvailable).toBeTruthy(); + expect(component.modulesAvailable).toBeTrue();
66-66: UsetoBeTrue()instead oftoBeTruthy()For boolean assertions, use
toBeTrue()as specified in the coding guidelines for better expectation specificity.- expect(result).toBeTruthy(); + expect(result).toBeTrue();
70-70: UsetoBeTrue()instead oftoBeTruthy()For consistent test assertions, replace
toBeTruthy()withtoBeTrue()as per your test guidelines.- expect(result).toBeTruthy(); + expect(result).toBeTrue();
77-77: UsetoBeTrue()instead oftoBeTruthy()Replace
toBeTruthy()withtoBeTrue()for boolean assertions to follow the coding guidelines.- expect(result).toBeTruthy(); + expect(result).toBeTrue();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(1 hunks)src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(2 hunks)src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(0 hunks)src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(4 hunks)src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
- src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
- src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
- src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
`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/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: server-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (1)
src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts (1)
1-114: Test structure and coverage looks goodThe test file is well-structured with comprehensive coverage of the component's functionality. It correctly uses MockProvider from ng-mocks and follows most of the test guidelines with appropriate assertions for different data types.
…iminary-feedback-model
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
🔭 Outside diff range comments (7)
src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java (1)
123-126: 💡 Verification agent🧩 Analysis chain
Fix file structure issue - misplaced code.
There appears to be a structural issue in the file. Lines 123-126 seem to be misplaced and likely belong to a different method. The file doesn't compile correctly with the current structure.
The lines:
} GROUP BY b.exerciseId """) BuildJobStatisticsDTO findBuildJobStatisticsByExerciseId(@Param("exerciseId") Long exerciseId);appear to be part of a different query method but are incorrectly placed after the closing brace of the repository interface and before additional methods. Please review the file structure and correct the placement of these lines.
🏁 Script executed:
#!/bin/bash # Description: Verify file structure and locate the correct context for this method # Look for findBuildJobStatisticsByExerciseId method in the repository rg "findBuildJobStatisticsByExerciseId" -A 2 -B 2 --no-headingLength of output: 1464
Critical Structural Issue: Misplaced Query Parts in BuildJobRepository
It appears that the query for the method
findBuildJobStatisticsByExerciseIdinsrc/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.javais broken due to misplaced lines. In particular, the linesGROUP BY b.exerciseId """) BuildJobStatisticsDTO findBuildJobStatisticsByExerciseId(@Param("exerciseId") Long exerciseId);seem to be detached from their intended
@Queryannotation block. It looks like an extraneous closing brace (}) was erroneously inserted before these lines, which disrupts the formation of a complete query string and likely causes compilation errors.Actionable items:
- Review the location of the stray closing brace (likely before the query continuation) to ensure that the entire SQL query (including the
GROUP BYclause and the closing""") is part of the proper@Queryannotation.- Adjust the file structure so that
findBuildJobStatisticsByExerciseIdis correctly annotated with its complete query string.package.json (2)
201-203:⚠️ Potential issueDuplicate Script Key: compile:ts:tests
The script key"compile:ts:tests"is defined twice. Even though both entries have the same value, JSON keys should be unique to avoid unintended behavior. Please remove the duplicate entry.
218-219:⚠️ Potential issueDuplicate Script Key: testw8
The script"testw8"appears twice in the scripts section. Remove the duplicate to prevent potential confusion or override issues.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.java (1)
124-136:⚠️ Potential issueRedundant duplicate method
Lines 124-136 re-implement the exactparseDockerFlagsfunctionality from lines 70-90. Having two methods with identical logic poses a maintainability risk.Remove this duplicate to maintain DRY principles and avoid confusion regarding which method should be used.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java (1)
103-104:⚠️ Potential issuePotential merge conflict residue
PACKAGE_NAME_REGEX_FOR_JAVA_KOTLINis declared here, but reappears again on lines 110-111. This is likely leftover code from a merge, producing two definitions of the same constant.Remove one definition to avoid repeated constants and ensure clarity.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (2)
419-434: 🛠️ Refactor suggestionPartially duplicated logic for results parsing
These lines resemble the block around 405-416, leading to potential confusion or mismatch in handling test results. Consolidate if possible.Refactor to a single code path or method that handles test result files consistently to minimize duplication.
490-498:⚠️ Potential issueConfirm single source for SCA parsing
Lines 490-498 replicate the logic from lines 470-473. Consolidate the method to avoid confusion or drifting logic.Eliminate the duplicate method and use a single version to ensure DRY principles.
🧹 Nitpick comments (19)
src/main/resources/templates/aeolus/javascript/default_static.sh (4)
1-3: Shell Script Initialization: Enhance Robustness
The shebang andset -eusage are correct to catch errors. Consider addingset -o pipefailto ensure that errors in piped commands are also captured.
9-12:static_code_analysisFunction: Linting Exit Code Handling
Usingnpm run lint:ci || [ $? -eq 1 ]permits an exit code of 1 to pass without failing the script. For clarity and maintainability, consider explicitly capturing the exit code or adding a comment explaining the rationale behind allowing exit code 1.
26-30: Subshell Invocation: Consider Improved Quoting
The subshell invocations (e.g.,bash -c "source ${_script_name} aeolus_sourcing; install_dependencies") work as intended; however, if the script path contains spaces, this could lead to issues. It is recommended to quote${_script_name}within the command as follows:- bash -c "source ${_script_name} aeolus_sourcing; install_dependencies" + bash -c "source \"${_script_name}\" aeolus_sourcing; install_dependencies"This change should be applied similarly to lines 28 and 30.
34-34: Trailing Line: Consistency Check
There is an extra line (line 34) at the end of the file. Please verify if this blank line is intentional; if not, consider removing it to maintain consistency with the rest of the file.src/test/javascript/spec/component/standardized-competencies/standardized-competency.service.spec.ts (4)
9-31: Consider using NgMocks for better test isolation.The test setup correctly configures the testing module with HTTP providers, but according to the project coding guidelines, NgMocks should be used for mocking. Consider refactoring to use NgMocks for better component isolation.
import { HttpResponse, provideHttpClient } from '@angular/common/http'; import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing'; import { TestBed, fakeAsync, tick } from '@angular/core/testing'; import { KnowledgeAreaDTO, Source, StandardizedCompetency } from 'app/entities/competency/standardized-competency.model'; +import { MockBuilder, MockRender, ngMocks } from 'ng-mocks'; import { take } from 'rxjs'; import { CompetencyTaxonomy } from 'app/entities/competency.model'; import { StandardizedCompetencyService } from 'app/shared/standardized-competencies/standardized-competency.service'; describe('StandardizedCompetencyService', () => { let standardizedCompetencyService: StandardizedCompetencyService; let httpTestingController: HttpTestingController; let defaultStandardizedCompetency: StandardizedCompetency; beforeEach(() => { - TestBed.configureTestingModule({ - providers: [provideHttpClient(), provideHttpClientTesting()], - }); - - standardizedCompetencyService = TestBed.inject(StandardizedCompetencyService); - httpTestingController = TestBed.inject(HttpTestingController); + return MockBuilder(StandardizedCompetencyService) + .provide(provideHttpClient()) + .provide(provideHttpClientTesting()); + }); + + beforeEach(() => { + standardizedCompetencyService = ngMocks.findInstance(StandardizedCompetencyService); + httpTestingController = ngMocks.findInstance(HttpTestingController); defaultStandardizedCompetency = { id: 1, title: 'Standardized Competency1', taxonomy: CompetencyTaxonomy.ANALYZE, }; });
33-48: Use more specific assertions for improved test clarity.According to the project coding guidelines, use more specific assertions for better test clarity. Instead of
.toEqual(), consider using.toContainEntries()for the object comparison.it('should get competency', fakeAsync(() => { let actualCompetency = new HttpResponse<StandardizedCompetency>(); const expectedCompetency = defaultStandardizedCompetency; const returnedFromService: StandardizedCompetency = { ...expectedCompetency }; standardizedCompetencyService .getStandardizedCompetency(1) .pipe(take(1)) .subscribe((resp) => (actualCompetency = resp)); const req = httpTestingController.expectOne({ method: 'GET' }); req.flush(returnedFromService); tick(); - expect(actualCompetency.body).toEqual(expectedCompetency); + expect(actualCompetency.body).toContainEntries(Object.entries(expectedCompetency)); }));
50-86: Update assertions for better specificity and test isolation.The test for
getAllForTreeViewshould use more specific assertions as per the project guidelines. Also, consider adding additional assertions to verify the structure of the returned data.it('should get all for tree view', fakeAsync(() => { let actualResult = new HttpResponse<KnowledgeAreaDTO[]>(); const expectedResult: KnowledgeAreaDTO[] = [ { id: 1, title: 'KnowledgeAreaDTO1', children: [ { id: 11, title: 'KnowledgeAreaDTO1.1', }, ], competencies: [ { id: 1, title: 'Standardized Competency', }, ], }, { id: 2, title: 'KnowledgeAreaDTO2', }, ]; const returnedFromService: KnowledgeAreaDTO[] = [...expectedResult]; standardizedCompetencyService .getAllForTreeView() .pipe(take(1)) .subscribe((resp) => (actualResult = resp)); const req = httpTestingController.expectOne({ method: 'GET' }); req.flush(returnedFromService); tick(); - expect(actualResult.body).toEqual(expectedResult); + expect(actualResult.body).toContainEntries(Object.entries(expectedResult)); + expect(actualResult.body?.[0].children).toBeDefined(); + expect(actualResult.body?.[0].competencies).toBeDefined(); }));
88-106: Use more specific assertions for the sources test.Following the project guidelines for assertion specificity, update the assertion in the
getSourcestest.it('should get sources', fakeAsync(() => { let actualSources = new HttpResponse<Source[]>(); const expectedSources: Source[] = [ { id: 1, title: 'source1' }, { id: 2, title: 'source2' }, ]; const returnedFromService = [...expectedSources]; standardizedCompetencyService .getSources() .pipe(take(1)) .subscribe((resp) => (actualSources = resp)); const req = httpTestingController.expectOne({ method: 'GET' }); req.flush(returnedFromService); tick(); - expect(actualSources.body).toEqual(expectedSources); + expect(actualSources.body).toContainEntries(Object.entries(expectedSources)); + expect(actualSources.body?.length).toBe(2); }));src/main/java/de/tum/cit/aet/artemis/programming/repository/BuildJobRepository.java (2)
108-115: Method implementation looks good but consider using enum constant.The new query method
fetchSuccessfulBuildJobsByExerciseIdWithLimitfollows good repository patterns with appropriate parameter annotations and SQL formatting. However, the query uses a string literal 'SUCCESSFUL' for the build status rather than referencing theBuildStatusenum constant. This could potentially become brittle if enum values change in the future.Consider modifying the query to use a parameter for the build status:
- WHERE b.exerciseId = :exerciseId AND b.buildStatus = 'SUCCESSFUL' + WHERE b.exerciseId = :exerciseId AND b.buildStatus = :buildStatusAnd update the method signature:
- List<BuildJob> fetchSuccessfulBuildJobsByExerciseIdWithLimit(@Param("exerciseId") Long exerciseId, @Param("limit") int limit); + List<BuildJob> fetchSuccessfulBuildJobsByExerciseIdWithLimit(@Param("exerciseId") Long exerciseId, @Param("buildStatus") BuildStatus buildStatus, @Param("limit") int limit);Then call with
BuildStatus.SUCCESSFULto maintain type safety.
117-122: Method implementation looks good but consider using enum constant.Similar to the previous method, this query also uses a string literal 'SUCCESSFUL' for the build status instead of the enum constant. The method otherwise follows repository best practices with proper parameter annotations and SQL formatting.
Consider parameterizing the build status:
- WHERE b.exerciseId = :exerciseId AND b.buildStatus = 'SUCCESSFUL' + WHERE b.exerciseId = :exerciseId AND b.buildStatus = :buildStatusAnd update the method signature:
- long fetchSuccessfulBuildJobCountByExerciseId(@Param("exerciseId") Long exerciseId); + long fetchSuccessfulBuildJobCountByExerciseId(@Param("exerciseId") Long exerciseId, @Param("buildStatus") BuildStatus buildStatus);src/main/webapp/app/exercises/programming/shared/service/aeolus.service.ts (2)
15-23: Update JSDoc comments to reflect parameter removal.The JSDoc comments don't reflect the removal of the
coverageparameter that was previously in the method signature. Documentation should be updated to match the actual method signature./** * Fetches the aeolus template file for the given programming language * @param {ProgrammingLanguage} language * @param {ProjectType} projectType (if available) * @param staticAnalysis (if available) whether static code analysis should be enabled * @param sequentialRuns (if available) whether sequential test runs should be enabled - * @param coverage (if available) whether test coverage should be enabled * @returns WindFile or undefined if no template is available */
31-39: Update JSDoc comments to reflect parameter removal.Similarly, these JSDoc comments need to be updated to remove the mention of the
coverageparameter that was removed from the method signature./** * Fetches the aeolus template file for the given programming language * @param {ProgrammingLanguage} language * @param {ProjectType} projectType (if available) * @param staticAnalysis (if available) whether static code analysis should be enabled * @param sequentialRuns (if available) whether sequential test runs should be enabled - * @param coverage (if available) whether test coverage should be enabled * @returns json test file */src/main/java/de/tum/cit/aet/artemis/buildagent/dto/DockerFlagsDTO.java (1)
5-5:❓ Verification inconclusive
Consider using
Integerinstead of primitiveintfor optional parameters.The new parameters (
cpuCount,memory,memorySwap) use primitiveinttypes, which means they can't be null. This might cause issues with backward compatibility if these values are optional in some scenarios.-public record DockerFlagsDTO(String network, Map<String, String> env, int cpuCount, int memory, int memorySwap) { +public record DockerFlagsDTO(String network, Map<String, String> env, Integer cpuCount, Integer memory, Integer memorySwap) {This change would allow these fields to be null when not specified, providing better backward compatibility.
🏁 Script executed:
#!/bin/bash # Find existing usages of the DockerFlagsDTO to verify backward compatibility concerns rg -A 3 "new DockerFlagsDTO" --type javaLength of output: 42
🏁 Script executed:
#!/bin/bash # Search for all references to DockerFlagsDTO constructor usage rg "DockerFlagsDTO\\s*\\(" --type javaLength of output: 224
Optional Parameter Compatibility Concern
The current implementation declares the fields as primitive
int, which means there is no way to representnullfor cases where these parameters might not be specified. Our search for usages ofDockerFlagsDTOdid not reveal any external references that provide further context on how these fields are used. This leaves us with two possibilities:
- If these parameters are always explicitly provided (i.e., they're effectively required), then using primitives may be acceptable.
- If, however, there's an expectation in some scenarios for these fields to be optional (and thus potentially
null), then the fields should be changed to useIntegerto better support backward compatibility.Actionable recommendation:
Review the intended usage of
cpuCount,memory, andmemorySwap.
If they are meant to be optional, update the record definition as follows:-public record DockerFlagsDTO(String network, Map<String, String> env, int cpuCount, int memory, int memorySwap) { +public record DockerFlagsDTO(String network, Map<String, String> env, Integer cpuCount, Integer memory, Integer memorySwap) {Please verify this change within the broader context of the codebase and confirm that any serialization or downstream processing correctly handles
nullvalues.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.java (1)
60-73: Improve memory string parsing robustness.The memory parsing implementation makes some assumptions about the format of memory strings (e.g., ending with quotes). This could lead to unexpected behavior with differently formatted inputs.
private static long parseMemoryStringToMB(String memoryString) { - if (memoryString.endsWith("g\"")) { - return Long.parseLong(memoryString.replaceAll("[^0-9]", "")) * 1024L; - } - else if (memoryString.endsWith("m\"")) { - return Long.parseLong(memoryString.replaceAll("[^0-9]", "")); - } - else if (memoryString.endsWith("k\"")) { - return Long.parseLong(memoryString.replaceAll("[^0-9]", "")) / 1024L; - } - else { - return Long.parseLong(memoryString); + String cleanString = memoryString.replaceAll("\"", "").trim(); + if (cleanString.endsWith("g") || cleanString.endsWith("G")) { + return Long.parseLong(cleanString.replaceAll("[^0-9]", "")) * 1024L; + } + else if (cleanString.endsWith("m") || cleanString.endsWith("M")) { + return Long.parseLong(cleanString.replaceAll("[^0-9]", "")); + } + else if (cleanString.endsWith("k") || cleanString.endsWith("K")) { + return Long.parseLong(cleanString.replaceAll("[^0-9]", "")) / 1024L; + } + else { + try { + return Long.parseLong(cleanString); + } catch (NumberFormatException e) { + // Log error or handle the exception + return 0; + } } }src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (2)
303-304: Network isolation during script run
InvokingrunScriptInContainerwithisNetworkDisabledis consistent, though ensure your container itself is configured accordingly (e.g., Docker run param or container-level setting).
311-314: Repeated log message
The log messages for “Collecting test results” seem duplicated across these ranges. This might confuse logs or result in extra clutter.Consider centralizing such messages or merging them to maintain a succinct log flow.
Also applies to: 320-323
src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts (3)
125-128: Type safety concern for CPU count.
event.target.valueis likely a string, which could cause type mismatch or unintended comparisons. Convert it to a number (e.g.,parseInt) before assignment tocpuCount.-onCpuCountChange(event: any) { - this.cpuCount = event.target.value; +onCpuCountChange(event: any) { + this.cpuCount = parseInt(event.target.value, 10); this.parseDockerFlagsToString(); }
130-133: Type safety concern for memory.Same reasoning as CPU count. Convert
event.target.valueto a number to prevent type-related issues.-onMemoryChange(event: any) { - this.memory = event.target.value; +onMemoryChange(event: any) { + this.memory = parseInt(event.target.value, 10); this.parseDockerFlagsToString(); }
135-138: Type safety concern for memory swap.Likewise, parse the input as a number to ensure consistent arithmetic operations.
-onMemorySwapChange(event: any) { - this.memorySwap = event.target.value; +onMemorySwapChange(event: any) { + this.memorySwap = parseInt(event.target.value, 10); this.parseDockerFlagsToString(); }
🛑 Comments failed to post (10)
supporting_scripts/course-scripts/quick-course-setup/create_course.py (2)
35-35:
⚠️ Potential issueAPI endpoint structure changed.
The URL for course creation has been updated to include a
/coreprefix. This change aligns with the PR objective of enhancing the Artemis platform's feedback system and reflects an API reorganization mentioned in the summary.This change might break backward compatibility with external scripts or clients that rely on the old URL structure. Ensure that:
- All client code has been updated to use the new URL pattern
- API documentation is updated to reflect this change
- Consider if a temporary redirect from the old endpoint to the new one is needed
21-21:
⚠️ Potential issueConfiguration source changed for SPECIAL_CHARACTERS_REGEX.
The regex configuration has been moved from the 'Settings' section to the 'CourseSettings' section, which is a good organizational improvement that keeps course-related settings together. However, this change might break existing configuration files that don't have this field in the 'CourseSettings' section.
Ensure all existing configuration files are updated accordingly, or consider adding a fallback mechanism:
-SPECIAL_CHARACTERS_REGEX: str = config.get('CourseSettings', 'special_character_regex') +# Try CourseSettings first, fall back to Settings for backward compatibility +SPECIAL_CHARACTERS_REGEX: str = config.get('CourseSettings', 'special_character_regex') if config.has_option('CourseSettings', 'special_character_regex') else config.get('Settings', 'special_character_regex')📝 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.# Try CourseSettings first, fall back to Settings for backward compatibility SPECIAL_CHARACTERS_REGEX: str = config.get('CourseSettings', 'special_character_regex') if config.has_option('CourseSettings', 'special_character_regex') else config.get('Settings', 'special_character_regex')src/main/java/de/tum/cit/aet/artemis/atlas/web/StandardizedCompetencyResource.java (1)
34-34: 💡 Verification agent
🧩 Analysis chain
Request mapping updated with correct prefix.
The request mapping has been properly updated to include the "atlas" segment, which aligns with the profile change. This ensures a consistent API structure.
Note that the Javadoc comments on lines 55, 71, and 86 still reference the old API paths and should be updated to match the new paths.
🏁 Script executed:
#!/bin/bash # Check if there are any client references to the old API path that might break with this change rg -i "api/standardized-competencies/" --type=ts --type=java --glob=!**/StandardizedCompetencyResource.javaLength of output: 602
Action Required: Update Documentation and Client References to New API Paths
- The request mapping in
StandardizedCompetencyResource.javahas been correctly updated to include the "atlas" prefix.- However, the Javadoc comments on lines 55, 71, and 86 in the same file still reference the old API paths.
- Additionally, the search revealed that
AdminStandardizedCompetencyResource.javacontains active references to the outdated endpoint (/api/standardized-competencies/), which may lead to runtime issues.Please update the Javadoc comments and the URI constructors in
AdminStandardizedCompetencyResource.javato reflect the new API structure (including the "atlas" segment) to ensure consistency across the codebase.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.java (1)
45-56: 🛠️ Refactor suggestion
Add validation for Docker flags array length.
The loop assumes
defaultDockerFlagshas an even number of elements (flags and values), but there's no validation to ensure this is the case. A malformed configuration could cause anIndexOutOfBoundsException.List<String> defaultDockerFlags = programmingLanguageConfiguration.getDefaultDockerFlags(); +if (defaultDockerFlags.size() % 2 != 0) { + // Log warning or handle the error + return; +} + for (int i = 0; i < defaultDockerFlags.size(); i += 2) { String flag = defaultDockerFlags.get(i); String value = defaultDockerFlags.get(i + 1);📝 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.List<String> defaultDockerFlags = programmingLanguageConfiguration.getDefaultDockerFlags(); if (defaultDockerFlags.size() % 2 != 0) { // Log warning or handle the error return; } for (int i = 0; i < defaultDockerFlags.size(); i += 2) { String flag = defaultDockerFlags.get(i); String value = defaultDockerFlags.get(i + 1); switch (flag) { case "--cpus" -> builder.withDetail(Constants.DOCKER_FLAG_CPUS, Long.parseLong(value.replaceAll("[^0-9]", ""))); case "--memory" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY_MB, parseMemoryStringToMB(value)); case "--memory-swap" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY_SWAP_MB, parseMemoryStringToMB(value)); } }src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.java (1)
70-90: 🛠️ Refactor suggestion
Duplicate
parseDockerFlagsmethod implementation
TheparseDockerFlagsmethod appears again near lines 124-136, creating a duplicate definition. This duplication is prone to future inconsistencies and bloats the code.Refactor the file to remove one of the duplicated methods, merge any missing logic, and ensure all references call a single source of truth.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java (1)
110-111: 🛠️ Refactor suggestion
Duplicate
PACKAGE_NAME_REGEX_FOR_JAVA_KOTLIN
The constant is declared a second time, duplicating the one defined on lines 103-104.Eliminate the second occurrence so the code remains coherent and DRY.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (2)
439-442:
⚠️ Potential issueInconsistent test file extension checks
Here,.jsonis not included, yet lines 460-461 do consider.json. This leads to contradictory acceptance rules. Please unify the file extension logic.Ensure .json logic is consistent across the entire file to avoid mistakes in test result detection.
467-468: 🛠️ Refactor suggestion
Duplicate
processStaticCodeAnalysisReportFile
A second definition near lines 490-498 also processes static code analysis. Duplicate methods risk divergence over time.Remove the duplication and unify the logic in one location.
Also applies to: 470-473
src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts (1)
83-91: 🛠️ Refactor suggestion
Potential zero-value handling issue.
Using
if (!this.cpuCount) { ... }overrides valid zero values (e.g.0) with defaults. If a user intends to setcpuCount = 0, it will be replaced byprofileInfo.defaultContainerCpuCount. Consider explicitly checking forundefinedornullinstead to avoid ignoring zero values.-if (!this.cpuCount) { +if (this.cpuCount == null) { this.cpuCount = profileInfo.defaultContainerCpuCount; }📝 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.if (this.cpuCount == null) { this.cpuCount = profileInfo.defaultContainerCpuCount; } if (!this.memory) { this.memory = profileInfo.defaultContainerMemoryLimitInMB; } if (!this.memorySwap) { this.memorySwap = profileInfo.defaultContainerMemorySwapLimitInMB; }build.gradle (1)
309-309:
⚠️ Potential issueDuplicate library references with different versions.
The file references
com.nimbusds:nimbus-jose-jwt:9.48(line 309) and alsocom.nimbusds:nimbus-jose-jwt:10.0.2(line 326). This can create version conflicts. Consider unifying to one version.-implementation "com.nimbusds:nimbus-jose-jwt:9.48" +// Remove or align this to match the other versionAlso applies to: 326-326
…iminary-feedback-model
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/javascript/spec/helpers/mocks/service/mock-profile.service.ts (1)
5-8: MockProfileService now returns a default ProfileInfo instead of undefinedThe
getProfileInfomethod has been updated to return a BehaviorSubject initialized with a concrete object{ activeProfiles: [] }instead ofundefined. This change ensures that components that consume this service won't have to handle undefined values, aligning with the PR objectives of enhancing the feedback system.However, the type assertion
as unknown as ProfileInfosuggests that the returned object doesn't fully satisfy the ProfileInfo interface. Consider creating a more complete mock object that properly implements the interface.- getProfileInfo = () => - new BehaviorSubject<ProfileInfo | undefined>({ - activeProfiles: [], - } as unknown as ProfileInfo); + getProfileInfo = () => + new BehaviorSubject<ProfileInfo>({ + activeProfiles: [], + ribbonEnv: '', + inProduction: false, + version: '' + });src/main/webapp/app/shared/user-settings/science-settings/science-settings.service.ts (3)
23-29: Constructor subscription is functionally correct
Subscribing in a service constructor to set up profile-dependent logic is reasonable for continuous service availability; however, confirm if this subscription completes once and doesn't need explicit teardown.
56-60: Consider consolidating ProfileService checks
You repeat the profile check here and in the constructor. Extracting this logic could reduce duplication and improve maintainability.-refreshScienceSettings(): void { - this.profileService.getProfileInfo().subscribe((profileInfo) => { - if (!profileInfo.activeProfiles.includes(PROFILE_ATLAS)) { - return; - } - ... - }); +private isAtlasActive(profileInfo: ProfileInfo): boolean { + return profileInfo.activeProfiles.includes(PROFILE_ATLAS); +} + +refreshScienceSettings(): void { + this.profileService.getProfileInfo().subscribe((profileInfo) => { + if (!this.isAtlasActive(profileInfo)) { + return; + } + ... + }); }
61-71: Include an error handling path in the subscription
At present, only thenextcallback is specified. Consider adding anerrorcallback for better fault tolerance and debugging. This can help track potential backend failures.this.userSettingsService.loadSettings(UserSettingsCategory.SCIENCE_SETTINGS).subscribe({ next: (res: HttpResponse<Setting[]>) => { ... }, + error: (err) => { + console.error('Failed to load science settings', err); + }, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/webapp/app/shared/competency-selection/competency-selection.component.ts(4 hunks)src/main/webapp/app/shared/user-settings/science-settings/science-settings.service.ts(2 hunks)src/test/javascript/spec/component/competencies/edit-competency.component.spec.ts(2 hunks)src/test/javascript/spec/component/lecture-unit/attachment-unit/create-attachment-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/lecture-unit/attachment-unit/edit-attachment-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/lecture-unit/online-unit/create-online-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/lecture-unit/online-unit/edit-online-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/lecture-unit/text-unit/create-text-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/lecture-unit/text-unit/edit-text-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/lecture-unit/video-unit/create-video-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/lecture-unit/video-unit/edit-video-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/programming-exercise/update-components/programming-exercise-problem.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/competency-selection.component.spec.ts(4 hunks)src/test/javascript/spec/component/shared/user-settings/science-settings.service.spec.ts(2 hunks)src/test/javascript/spec/helpers/mocks/service/mock-profile.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`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/attachment-unit/create-attachment-unit.component.spec.tssrc/test/javascript/spec/component/competencies/edit-competency.component.spec.tssrc/test/javascript/spec/component/lecture-unit/attachment-unit/edit-attachment-unit.component.spec.tssrc/test/javascript/spec/component/lecture-unit/video-unit/create-video-unit.component.spec.tssrc/test/javascript/spec/helpers/mocks/service/mock-profile.service.tssrc/test/javascript/spec/component/shared/user-settings/science-settings.service.spec.tssrc/test/javascript/spec/component/programming-exercise/update-components/programming-exercise-problem.component.spec.tssrc/test/javascript/spec/component/lecture-unit/online-unit/create-online-unit.component.spec.tssrc/test/javascript/spec/component/lecture-unit/online-unit/edit-online-unit.component.spec.tssrc/test/javascript/spec/component/lecture-unit/text-unit/create-text-unit.component.spec.tssrc/test/javascript/spec/component/lecture-unit/video-unit/edit-video-unit.component.spec.tssrc/test/javascript/spec/component/shared/competency-selection.component.spec.tssrc/test/javascript/spec/component/lecture-unit/text-unit/edit-text-unit.component.spec.ts
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/shared/user-settings/science-settings/science-settings.service.tssrc/main/webapp/app/shared/competency-selection/competency-selection.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (25)
src/test/javascript/spec/component/competencies/edit-competency.component.spec.ts (1)
19-20: LGTM: Added ProfileService dependency to the test setupThe ProfileService is correctly mocked using MockProfileService in the test bed configuration, following proper Angular testing practices. This change ensures that the EditCompetencyComponent can interact with profile-related functionality during testing, supporting the PR objective of enhancing the feedback system configuration.
Also applies to: 33-33
src/test/javascript/spec/component/lecture-unit/attachment-unit/create-attachment-unit.component.spec.ts (1)
22-23: LGTM: Added ProfileService dependency to the test setupThe ProfileService is correctly mocked using MockProfileService in the test bed configuration, following proper Angular testing practices. This change ensures that the CreateAttachmentUnitComponent can interact with profile-related functionality during testing, which supports the PR's objective of improving the flexibility of the feedback system.
Also applies to: 65-65
src/test/javascript/spec/component/programming-exercise/update-components/programming-exercise-problem.component.spec.ts (1)
13-14: LGTM: Added ProfileService dependency to the test setupThe ProfileService is correctly mocked using MockProfileService in the test bed configuration, following proper Angular testing practices. This change is particularly relevant for the PR objective since it enables the programming exercise component to differentiate between preliminary and graded feedback modules.
Also applies to: 31-31
src/test/javascript/spec/component/lecture-unit/text-unit/edit-text-unit.component.spec.ts (1)
23-24: The ProfileService integration follows testing best practices.The addition of ProfileService and its mock implementation follows the pattern of dependency injection in Angular testing. This is in line with the coding guidelines that recommend mocking irrelevant dependencies.
Also applies to: 61-61
src/test/javascript/spec/component/lecture-unit/text-unit/create-text-unit.component.spec.ts (1)
23-24: Good use of MockProfileService.The implementation correctly provides MockProfileService as a substitute for ProfileService in the test bed, which is consistent with the project's testing approach.
Also applies to: 37-37
src/test/javascript/spec/component/lecture-unit/video-unit/create-video-unit.component.spec.ts (1)
20-21: Consistent mock implementation.The ProfileService integration matches the pattern used across other component tests, maintaining consistency throughout the test suite.
Also applies to: 66-66
src/test/javascript/spec/component/lecture-unit/online-unit/edit-online-unit.component.spec.ts (1)
20-21: Properly follows test guidelines for dependency mocking.The addition of ProfileService with its mock implementation adheres to the project's testing guidelines that emphasize mocking irrelevant dependencies for focused unit testing.
Also applies to: 64-64
src/test/javascript/spec/component/lecture-unit/online-unit/create-online-unit.component.spec.ts (1)
20-21: Good addition of ProfileService mockThe addition of ProfileService and MockProfileService imports along with their registration in the TestBed providers array follows good testing practices by properly mocking dependencies instead of using real implementations.
Also applies to: 33-33
src/test/javascript/spec/component/lecture-unit/attachment-unit/edit-attachment-unit.component.spec.ts (1)
22-23: Good addition of ProfileService mockAdding the ProfileService mock is appropriate and aligns with the testing best practices by properly isolating the component tests from actual service implementations.
Also applies to: 44-44
src/test/javascript/spec/component/lecture-unit/video-unit/edit-video-unit.component.spec.ts (1)
19-20: Good addition of ProfileService mockThe ProfileService mock implementation is correctly added to the providers array, ensuring that component tests don't rely on actual service implementations.
Also applies to: 62-62
src/test/javascript/spec/component/shared/user-settings/science-settings.service.spec.ts (3)
12-14: Good addition of required importsThe addition of ProfileService, ProfileInfo, and PROFILE_ATLAS imports is appropriate for the enhanced test setup.
39-43: Well-implemented ProfileService mock setupThe setup of the ProfileService mock with a specific profile configuration is well-implemented, ensuring that the service behaves correctly during tests.
46-48: Good test cleanup practiceAdding the afterEach block with jest.restoreAllMocks() is a best practice that ensures each test runs with a clean state by resetting all mocks between tests.
src/main/webapp/app/shared/competency-selection/competency-selection.component.ts (7)
1-1: Imports are properly organized and follow Angular guidelines.The addition of
OnDestroyfrom Angular core,Subscriptionfrom rxjs, and theProfileServicealong with thePROFILE_ATLASconstant are appropriately imported to support the new lifecycle management and profile feature integration.Also applies to: 16-16, 22-23
38-38: Component implements OnDestroy to prevent memory leaks.Good implementation of the
OnDestroyinterface to properly clean up subscriptions when the component is destroyed.
43-43: ProfileService injection follows Angular DI pattern.Using the inject function for dependency injection is consistent with modern Angular practices.
71-71: Good subscription management practice.The private subscription property is properly initialized as undefined and typed as Subscription. This facilitates proper cleanup in ngOnDestroy to prevent memory leaks.
73-81: Well-structured subscription to profile info.The implementation correctly subscribes to the profile service and conditionally initializes based on the presence of the ATLAS profile. The comment explaining the design decision adds valuable context for other developers.
83-85: Proper implementation of ngOnDestroy.The component correctly unsubscribes from the profile subscription using the optional chaining operator, preventing memory leaks when the component is destroyed.
87-87: Extracted initialization logic improves readability.Moving the initialization logic to a separate method makes the code more modular and easier to understand, especially with the conditional execution based on profile information.
src/test/javascript/spec/component/shared/competency-selection.component.spec.ts (3)
5-5: Appropriate imports for testing with profile service.The addition of
BehaviorSubjectfrom rxjs and the profile-related imports are necessary for properly testing the new functionality in the component.Also applies to: 18-21
42-42: MockProfileService provider correctly added.The test module configuration has been properly updated to include the MockProfileService, following the pattern used for other service mocks.
54-60: Test setup properly mocks profile service behavior.The test correctly sets up the ProfileService mock to return an observable containing the profile information with ATLAS enabled. This ensures the component's conditional initialization will be triggered during tests.
The use of BehaviorSubject is appropriate for providing observable test data, and the spying on getProfileInfo ensures the mock implementation will be used during testing.
src/main/webapp/app/shared/user-settings/science-settings/science-settings.service.ts (2)
9-10: Imports align with Angular standards
These imports cleanly reference the required services and constants, and they follow cultural Angular style guides with single quotes.
18-18: Good use of Angular’sinjectAPI
This elegantly injects the ProfileService without needing constructor parameters. No issues found.
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
…iminary-feedback-model
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
…iminary-feedback-model
| @@ -261,14 +261,17 @@ <h3 jhiTranslate="artemisApp.exercise.sections.grading" id="artemisApp.exercise. | |||
| </div> | |||
| } | |||
| @if (!isExamMode) { | |||
| <jhi-exercise-feedback-suggestion-options [exercise]="modelingExercise" [dueDate]="modelingExercise.dueDate" /> | |||
There was a problem hiding this comment.
Deleting this line is incorrect as it deletes the option to configure Athena's feedback suggestions for modeling exercises. The above checkmark regarding automatic assessment refers to Compass, which is another way to generate feedback suggestions for tutors.
This should be removed in the future.
The UI should look like this:
But it looks like this currently:
Please test only on TS3
!!! Contains database migrations !!!
Checklist
General
Server
Client
Changes affecting Programming Exercises
Motivation and Context
Currently, Artemis allows only one module to be set for Athena Feedback Requests, be it for preliminary or graded feedback. This PR addresses the limitation and enables setting separate modules for each type of feedback.
Description
This PR introduces functionality that allows instructors to configure different Athena Feedback Request modules for preliminary feedback and graded feedback. Preliminary feedback can now be assigned a dedicated module without affecting the graded feedback settings.
Steps for Testing
Disclaimer:
Currently, only LLM modules support preliminary feedback. You can easily test this functionality with non-LLM modules to verify if the correct module is being used (non-LLM modules will not work).
As an instructor:
2. Navigate to the course settings.
3. Create a Programming, Text, or Modeling exercise.
4. Select a module for preliminary feedback (no due date required). For graded feedback, enable Feedback Suggestions, set a due date, and select a module(text or programming).
5. Save the changes.
As a student:
6. Open the created exercise.
7. Submit something.
8. Submit an AI feedback request.
9. Verify that the appropriate module is used based on the type of feedback.
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Screenshots
Summary by CodeRabbit
New Features
Documentation