Skip to content

Programming exercises: Allow to choose preliminary feedback model #10441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

dmytropolityka
Copy link
Contributor

@dmytropolityka dmytropolityka commented Mar 5, 2025

Please test only on TS3
!!! Contains database migrations !!!

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

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).

  1. Deploy this branch to Test Server 3 (TS3).

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

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Screenshots

image
image

Summary by CodeRabbit

  • New Features

    • Introduced a new enum for module types, including FEEDBACK_SUGGESTIONS and PRELIMINARY_FEEDBACK.
    • Added a new Angular component for managing preliminary feedback options.
    • Enhanced feedback request handling to differentiate between manual and automated requests.
  • UI/Text Updates

    • Updated labels, tooltips, and buttons related to feedback suggestions and requests for clearer user guidance.
    • Adjusted the access control checks for feedback modules in the UI to reflect new logic.
  • Enhancements

    • Refined access controls and selection logic for feedback modules to ensure a more consistent and robust feedback experience.
    • Improved the handling of feedback request settings across multiple components and services.
    • Enhanced test coverage for feedback options, ensuring correct behavior under various conditions.

dmytropolityka and others added 28 commits December 3, 2024 11:28
…g-exercises/choose-preliminary-feedback-model
…oose-preliminary-feedback-model' into feature/programming-exercises/choose-preliminary-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
coderabbitai[bot]
coderabbitai bot previously approved these changes Apr 4, 2025
Copy link

github-actions bot commented Apr 4, 2025

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report201 ran197 passed3 skipped1 failed44m 53s 436ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/exercise/programming/ProgrammingExerciseStaticCodeAnalysis.spec.ts
ts.Static code analysis tests › Configures SCA grading and makes a successful submission with SCA errors❌ failure1m 45s 136ms

@github-actions github-actions bot removed the stale label Apr 5, 2025
maximiliansoelch and others added 2 commits April 7, 2025 16:38
…g-exercises/choose-preliminary-feedback-model

# Conflicts:
#	src/main/webapp/app/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.spec.ts
#	src/main/webapp/app/programming/shared/lifecycle/programming-exercise-lifecycle.component.spec.ts
coderabbitai[bot]
coderabbitai bot previously approved these changes Apr 7, 2025
Copy link

github-actions bot commented Apr 7, 2025

End-to-End (E2E) Test Results Summary

TestsPassed ✅SkippedFailedTime ⏱
End-to-End (E2E) Test Report1 ran1 passed0 skipped0 failed1s 810ms
TestResultTime ⏱
No test annotations available

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts (2)

77-84: Consider initializing dropdown state based on existing module

The dropdown state is set based only on the checkbox event, but isn't initialized based on whether the exercise already has a preliminary feedback module set.

ngOnInit(): void {
    const courseId = Number(this.activatedRoute.snapshot.paramMap.get('courseId'));
    this.athenaService.getAvailableModules(courseId, this.exercise()).subscribe((modules) => {
        this.availableAthenaModules = modules;
        this.modulesAvailable = modules.length > 0;
        this.cdr.detectChanges();
    });
    this.isAthenaEnabled$ = this.athenaService.isEnabled();
    this.initialAthenaModule = this.exercise().preliminaryFeedbackModule;
+   this.showDropdownList = !!this.exercise().preliminaryFeedbackModule;
}

86-88: Consider timezone handling in date comparison

The hasDueDatePassed method compares dates using the local timezone, which could lead to inconsistent behavior across different regions.

private hasDueDatePassed() {
-   return dayjs(this.exercise().dueDate).isBefore(dayjs());
+   return dayjs(this.exercise().dueDate).utc().isBefore(dayjs().utc());
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a7359d and 750d50a.

📒 Files selected for processing (1)
  • src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...

src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

  • src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts
🧠 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-04-07T13:18:39.389Z
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]`.
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-04-07T13:18:35.405Z
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-04-07T13:18:32.368Z
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]`.
🧬 Code Definitions (1)
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts (1)
src/main/webapp/app/exercise/feedback-suggestion/exercise-feedback-suggestion-options.component.ts (1)
  • Component (13-98)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
  • GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
  • GitHub Check: Build .war artifact
  • GitHub Check: server-tests
  • GitHub Check: Analyse
🔇 Additional comments (3)
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts (3)

1-18: Component setup looks good with signal-based inputs

The component is correctly set up using the new input() signal approach and proper imports. This aligns with the PR objective of allowing configuration of preliminary feedback modules.


80-80: The use of first() on array is consistent with Artemis patterns

Using this.availableAthenaModules.first() is appropriate since the Artemis codebase extends the Array prototype with first() and last() methods.


52-58:

❓ Verification inconclusive

Verify ngOnChanges behavior with signal inputs

When using the new signal-based inputs, the SimpleChanges object might not contain the properties as expected. Consider updating the change detection approach to use signal features instead.


🏁 Script executed:

#!/bin/bash
# Search for patterns of handling changes with signals in the codebase
rg "ngOnChanges.*input\(" --type ts -A 10

Length of output: 43


Attention: Verify Change Detection for Signal Inputs

The current implementation in
src/main/webapp/app/exercises/shared/preliminary-feedback/exercise-preliminary-feedback-options.component.ts (lines 52–58) relies on standard Angular change detection via ngOnChanges and the SimpleChanges object. Since new signal-based inputs may not propagate changes in the expected way (i.e. the expected change properties can be missing), please verify whether signal-based inputs are being used in this component (or elsewhere in the codebase). If they are, consider refactoring the change detection logic to leverage the appropriate signal APIs or lifecycle mechanisms.

Comment on lines +29 to +33
isAthenaEnabled$: Observable<boolean>;
modulesAvailable: boolean;
availableAthenaModules: string[];
initialAthenaModule?: string;
showDropdownList: boolean = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add memory leak prevention by unsubscribing observables

The component subscribes to observables but doesn't implement OnDestroy to clean them up, which could lead to memory leaks.

import { ChangeDetectorRef, Component, OnChanges, OnDestroy, OnInit, SimpleChanges, input } from '@angular/core';
import { AssessmentType } from 'app/assessment/shared/entities/assessment-type.model';
import { Exercise, ExerciseType } from 'app/exercise/shared/entities/exercise/exercise.model';
-import { Observable } from 'rxjs';
+import { Observable, Subject } from 'rxjs';
+import { takeUntil } from 'rxjs/operators';
import { ActivatedRoute } from '@angular/router';
import dayjs from 'dayjs/esm';
// ...rest of imports

@Component({
    selector: 'jhi-exercise-preliminary-feedback-options',
    imports: [TranslateDirective, NgStyle, HelpIconComponent, FormsModule, AsyncPipe],
    templateUrl: './exercise-preliminary-feedback-options.component.html',
})
-export class ExercisePreliminaryFeedbackOptionsComponent implements OnInit, OnChanges {
+export class ExercisePreliminaryFeedbackOptionsComponent implements OnInit, OnChanges, OnDestroy {
    // ...existing properties
    
+   private destroy$ = new Subject<void>();
    
    ngOnInit(): void {
        const courseId = Number(this.activatedRoute.snapshot.paramMap.get('courseId'));
        this.athenaService.getAvailableModules(courseId, this.exercise()).subscribe((modules) => {
-           this.availableAthenaModules = modules;
+           this.availableAthenaModules = modules;
            this.modulesAvailable = modules.length > 0;
            this.cdr.detectChanges();
-       });
+       }, undefined, () => {}, takeUntil(this.destroy$));
-       this.isAthenaEnabled$ = this.athenaService.isEnabled();
+       this.isAthenaEnabled$ = this.athenaService.isEnabled().pipe(takeUntil(this.destroy$));
        this.initialAthenaModule = this.exercise().preliminaryFeedbackModule;
    }
    
+   ngOnDestroy(): void {
+       this.destroy$.next();
+       this.destroy$.complete();
+   }
}

Committable suggestion skipped: line range outside the PR's diff.

Copy link

github-actions bot commented Apr 7, 2025

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report201 ran187 passed3 skipped11 failed58m 10s 944ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/exam/ExamTestRun.spec.ts
ts.Exam test run › Creates a test run❌ failure2m 2s 162ms
ts.Exam test run › Manage a test run › Changes test run working time❌ failure2m 2s 227ms
ts.Exam test run › Manage a test run › Conducts a test run❌ failure2m 2s 147ms
ts.Exam test run › Delete a test run › Deletes a test run❌ failure2m 2s 378ms
e2e/exam/ExamParticipation.spec.ts
ts.Exam participation › Early Hand-in › Participates as a student in a registered exam❌ failure2m 5s 625ms
ts.Exam participation › Early Hand-in › Using navigation sidebar to navigate within exam❌ failure2m 5s 639ms
ts.Exam participation › Early Hand-in › Using exercise overview to navigate within exam❌ failure2m 5s 376ms
e2e/exam/test-exam/TestExamParticipation.spec.ts
ts.Test exam participation › Early Hand-in › Participates as a student in a registered test exam❌ failure2m 5s 875ms
ts.Test exam participation › Early Hand-in › Using exercise sidebar to navigate within exam❌ failure2m 5s 550ms
ts.Test exam participation › Early Hand-in › Using exercise overview to navigate within exam❌ failure2m 5s 427ms
e2e/exercise/programming/ProgrammingExerciseStaticCodeAnalysis.spec.ts
ts.Static code analysis tests › Configures SCA grading and makes a successful submission with SCA errors❌ failure1m 42s 232ms

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/webapp/app/exercise/feedback-suggestion/feedback-suggestion-option.component.spec.ts (1)

121-160: Comprehensive test case for assessment type switching.

This test case thoroughly validates component behavior when switching between assessment types, ensuring:

  1. Controls are disabled and the dropdown is hidden when switching to automatic assessment
  2. Controls are re-enabled when switching back to semi-automatic assessment
  3. Checkbox state and properties are correctly reset

The test follows good practices with clear setup, actions, and expectations.

Consider adding comment separators between the test's logical sections to improve readability:

 // prepare data
 const modules = ['Module1', 'Module2'];
 jest.spyOn(athenaService, 'getAvailableModules').mockReturnValue(of(modules));
 component.exercise = { type: ExerciseType.PROGRAMMING, dueDate: futureDueDate, assessmentType: AssessmentType.SEMI_AUTOMATIC } as Exercise;

 fixture.detectChanges();

+// SECTION 1: Initial state with active module
 // assume a module is chosen, hence, the controls are active
 const event = { target: { checked: true } };
 component.toggleFeedbackSuggestions(event);

 expect(component.inputControlsDisabled()).toBeFalse();
 expect(component.showDropdownList).toBeTrue();

+// SECTION 2: Switch to automatic assessment
 // change assessment type
 component.exercise.assessmentType = AssessmentType.AUTOMATIC;
 fixture.detectChanges();

 // now, the input is unchecked and the controls are disabled
 expect(component.inputControlsDisabled()).toBeTrue();
 expect(component.showDropdownList).toBeFalse();

 let checkbox = fixture.debugElement.query(By.css('#feedbackSuggestionsEnabledCheck'));
 expect(checkbox.nativeElement.disabled).toBeTrue();

+// SECTION 3: Switch back to semi-automatic assessment
 // Now, change the assessment type back to semi automatic
 component.exercise.assessmentType = AssessmentType.SEMI_AUTOMATIC;
 component.exercise.feedbackSuggestionModule = undefined; // will be reset by parent component
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 750d50a and 1ef1d51.

📒 Files selected for processing (3)
  • src/main/webapp/app/exercise/feedback-suggestion/feedback-suggestion-option.component.spec.ts (3 hunks)
  • src/main/webapp/app/programming/shared/lifecycle/programming-exercise-lifecycle.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 (1)
  • src/test/javascript/spec/component/exercises/shared/feedback/preliminary-feedback-options.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...

src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

  • src/main/webapp/app/exercise/feedback-suggestion/feedback-suggestion-option.component.spec.ts
  • src/main/webapp/app/programming/shared/lifecycle/programming-exercise-lifecycle.component.spec.ts
🧬 Code Definitions (1)
src/main/webapp/app/exercise/feedback-suggestion/feedback-suggestion-option.component.spec.ts (1)
src/test/javascript/spec/helpers/mocks/service/mock-translate.service.ts (1)
  • MockTranslateService (9-56)
🪛 GitHub Check: client-tests
src/main/webapp/app/exercise/feedback-suggestion/feedback-suggestion-option.component.spec.ts

[failure] 11-11:
Cannot find module '../../../../../../test/javascript/spec/helpers/mocks/service/mock-translate.service' or its corresponding type declarations.

src/main/webapp/app/programming/shared/lifecycle/programming-exercise-lifecycle.component.spec.ts

[failure] 76-76:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?


[failure] 416-416:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?


[failure] 399-399:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?

🪛 GitHub Check: client-tests-selected
src/main/webapp/app/exercise/feedback-suggestion/feedback-suggestion-option.component.spec.ts

[failure] 11-11:
Cannot find module '../../../../../../test/javascript/spec/helpers/mocks/service/mock-translate.service' or its corresponding type declarations.

src/main/webapp/app/programming/shared/lifecycle/programming-exercise-lifecycle.component.spec.ts

[failure] 76-76:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?


[failure] 416-416:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?


[failure] 399-399:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
  • GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
  • GitHub Check: Build .war artifact
  • GitHub Check: server-tests
  • GitHub Check: Analyse
🔇 Additional comments (10)
src/main/webapp/app/programming/shared/lifecycle/programming-exercise-lifecycle.component.spec.ts (6)

22-22: Import of AthenaService is appropriate.

The AthenaService import is necessary for the new tests related to preliminary feedback functionality. This aligns with the PR objectives of configuring separate modules for preliminary feedback.


154-161: Rename from allowFeedbackRequests to allowManualFeedbackRequests is aligned with PR objectives.

The test has been properly updated to use allowManualFeedbackRequests and toggleManualFeedbackRequests instead of the previous naming. This change is consistent with the PR's objective of distinguishing between preliminary and manual feedback requests.


367-378: Test for ExerciseFeedbackSuggestionOptionsComponent is well structured.

This test properly verifies that the feedback suggestion component renders correctly when feedback suggestions are enabled, which is important for the new feedback configuration functionality.


380-391: Test correctly verifies component doesn't render in exam mode.

This test properly checks that the ExerciseFeedbackSuggestionOptionsComponent doesn't render in exam mode, which is an important constraint to verify.


393-408: Use of fakeAsync is appropriate for testing asynchronous behavior.

This test correctly uses fakeAsync and tick() to handle the asynchronous nature of the athenaService.isEnabled() observable. The test properly checks that the ExercisePreliminaryFeedbackOptionsComponent renders when Athena is enabled and preliminary feedback is configured.

🧰 Tools
🪛 GitHub Check: client-tests

[failure] 399-399:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?

🪛 GitHub Check: client-tests-selected

[failure] 399-399:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?


410-426: Test correctly handles the disabled state case.

This test properly verifies that the ExercisePreliminaryFeedbackOptionsComponent doesn't render when Athena is disabled, while still showing the manual feedback requests checkbox. This is an important edge case to verify for the preliminary feedback implementation.

🧰 Tools
🪛 GitHub Check: client-tests

[failure] 416-416:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?

🪛 GitHub Check: client-tests-selected

[failure] 416-416:
Cannot find name 'athenaService'. Did you mean 'AthenaService'?

src/main/webapp/app/exercise/feedback-suggestion/feedback-suggestion-option.component.spec.ts (4)

4-4: Import statements correctly added to support test functionality.

The imports for MockDirective, TranslateDirective, MockTranslateService, TranslateService, and By are well-structured and necessary for the enhanced test cases.

Also applies to: 10-13


29-30: Test providers correctly configured.

The addition of MockDirective(TranslateDirective) and the mock translation service provider are appropriate for the testing requirements.


101-119: Test case correctly generalized for any exercise type.

The test has been updated to check feedback suggestions functionality for any exercise type rather than just programming exercises, which aligns with the PR objective of enhancing flexibility in the feedback system.

The additional expectations for showDropdownList and allowManualFeedbackRequests properties correctly reflect the system changes from general to manual feedback requests.


11-11:

✅ Verification successful

Fix import path for the MockTranslateService.

The import path for the MockTranslateService is causing static analysis errors. The path with multiple ../ segments suggests it might be incorrectly referenced.

-import { MockTranslateService } from '../../../../../../test/javascript/spec/helpers/mocks/service/mock-translate.service';
+import { MockTranslateService } from 'test/javascript/spec/helpers/mocks/service/mock-translate.service';

Consider using a more direct path or creating an alias in the TypeScript configuration to simplify imports from test helpers.


🏁 Script executed:

#!/bin/bash
# Check for the correct location of the MockTranslateService file
fd --type f "mock-translate.service.ts" --exec echo {}

Length of output: 131


Correct Import Path for MockTranslateService

The file was confirmed to exist at ./src/test/javascript/spec/helpers/mocks/service/mock-translate.service.ts. With a properly configured TypeScript base URL (set to src), using the absolute path:

-import { MockTranslateService } from '../../../../../../test/javascript/spec/helpers/mocks/service/mock-translate.service';
+import { MockTranslateService } from 'test/javascript/spec/helpers/mocks/service/mock-translate.service';

ensures static analysis errors are resolved and eliminates the need for deeply nested relative imports. Please update the import accordingly.

🧰 Tools
🪛 GitHub Check: client-tests

[failure] 11-11:
Cannot find module '../../../../../../test/javascript/spec/helpers/mocks/service/mock-translate.service' or its corresponding type declarations.

🪛 GitHub Check: client-tests-selected

[failure] 11-11:
Cannot find module '../../../../../../test/javascript/spec/helpers/mocks/service/mock-translate.service' or its corresponding type declarations.

Copy link

github-actions bot commented Apr 7, 2025

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report201 ran197 passed3 skipped1 failed45m 9s 517ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/exercise/programming/ProgrammingExerciseStaticCodeAnalysis.spec.ts
ts.Static code analysis tests › Configures SCA grading and makes a successful submission with SCA errors❌ failure1m 43s 473ms

Copy link

github-actions bot commented Apr 7, 2025

End-to-End (E2E) Test Results Summary

TestsPassed ✅Skipped ⚠️FailedTime ⏱
End-to-End (E2E) Test Report201 ran198 passed3 skipped0 failed44m 33s 698ms
TestResultTime ⏱
No test annotations available

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/main/webapp/app/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.spec.ts (1)

547-563: Add Athena profile check for programming exercise test

Unlike the previous tests, this test doesn't mock the profileService to include the Athena profile. Consider adding this check to maintain consistency with the other tests, or include a comment explaining why it's intentionally different.

+    getProfileInfoSub = jest.spyOn(profileService, 'getProfileInfo');
+    getProfileInfoSub.mockReturnValue(
+        of({
+            activeProfiles: [PROFILE_ATHENA],
+        } as ProfileInfo),
+    );
+
     const exercise = { type: ExerciseType.PROGRAMMING } as ProgrammingExercise;
     exercise.studentParticipations = [
         {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea82055 and 71be42e.

📒 Files selected for processing (6)
  • src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java (4 hunks)
  • src/main/webapp/app/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.html (2 hunks)
  • src/main/webapp/app/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.spec.ts (2 hunks)
  • src/main/webapp/i18n/de/exercise.json (1 hunks)
  • src/main/webapp/i18n/en/exercise.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/main/webapp/i18n/de/exercise.json
  • src/main/webapp/i18n/en/exercise.json
  • src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java
  • src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...

src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

  • src/main/webapp/app/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.spec.ts
`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/**/*.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/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.html
🧬 Code Definitions (1)
src/main/webapp/app/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.spec.ts (2)
src/main/webapp/app/app.constants.ts (1)
  • PROFILE_ATHENA (50-50)
src/main/webapp/app/core/layouts/profiles/profile-info.model.ts (1)
  • ProfileInfo (6-63)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
  • GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
  • GitHub Check: Build .war artifact
  • GitHub Check: Analyse
  • GitHub Check: server-tests
  • GitHub Check: client-tests-selected
  • GitHub Check: client-tests
🔇 Additional comments (5)
src/main/webapp/app/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.spec.ts (3)

32-32: LGTM: Good import of constant

Using the constant PROFILE_ATHENA is a good practice instead of hardcoding the profile name.


505-524: Well-structured test for feedback button display in text exercises

This test correctly verifies that the feedback button is displayed for text exercises when the Athena profile is enabled and the exercise is before its due date, which aligns with the PR objectives of configuring separate modules for preliminary feedback.


526-545: Well-structured test for feedback button display in modeling exercises

The test follows the same pattern as the text exercise test, ensuring consistent verification of the feedback button visibility logic across different exercise types.

src/main/webapp/app/core/course/overview/exercise-details/student-actions/exercise-details-student-actions.component.html (2)

133-135: Simplified condition for programming exercises feedback button

The condition for displaying the feedback button for programming exercises has been appropriately simplified to only check for the existence of gradedParticipation and exercise type.


225-225: Updated condition for text/modeling exercises feedback button

The condition has been correctly updated to check if Athena is enabled and if the exercise is a text or modeling type before its due date, aligning with the PR objective of allowing instructors to configure separate modules for preliminary feedback.

Copy link

github-actions bot commented Apr 7, 2025

End-to-End (E2E) Test Results Summary

TestsPassed ☑️Skipped ⚠️Failed ❌️Time ⏱
End-to-End (E2E) Test Report201 ran197 passed3 skipped1 failed45m 33s 667ms
TestResultTime ⏱
End-to-End (E2E) Test Report
e2e/exercise/programming/ProgrammingExerciseStaticCodeAnalysis.spec.ts
ts.Static code analysis tests › Configures SCA grading and makes a successful submission with SCA errors❌ failure1m 45s 566ms

Copy link
Contributor

@ahmetsenturk ahmetsenturk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code

Comment on lines +255 to +258
athenaApi.ifPresentOrElse(api -> api.checkHasAccessToAthenaModule(programmingExercise, course, ModuleType.FEEDBACK_SUGGESTIONS, ENTITY_NAME),
() -> programmingExercise.setFeedbackSuggestionModule(null));
athenaApi.ifPresentOrElse(api -> api.checkHasAccessToAthenaModule(programmingExercise, course, ModuleType.PRELIMINARY_FEEDBACK, ENTITY_NAME),
() -> programmingExercise.setPreliminaryFeedbackModule(null));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this snippet not wrapped by try/catch? I saw you wrapped the same snippet in src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseExportImportResource.java

@helios-aet helios-aet bot temporarily deployed to artemis-test3.artemis.cit.tum.de April 11, 2025 06:49 Inactive
@helios-aet helios-aet bot temporarily deployed to artemis-test3.artemis.cit.tum.de April 11, 2025 08:58 Inactive
Copy link

End-to-End (E2E) Test Results Summary

TestsPassed ✅Skipped ⚠️FailedTime ⏱
End-to-End (E2E) Test Report201 ran198 passed3 skipped0 failed47m 28s 339ms
TestResultTime ⏱
No test annotations available

Copy link
Member

@krusche krusche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve the merge conflicts and get the PR finished asap.

Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assessment Pull requests that affect the corresponding module athena Pull requests that affect the corresponding module client Pull requests that update TypeScript code. (Added Automatically!) core Pull requests that affect the corresponding module database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. exercise Pull requests that affect the corresponding module modeling Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module server Pull requests that update Java code. (Added Automatically!) stale tests text Pull requests that affect the corresponding module
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

8 participants