Iris: Upgrade settings system for Pyris V2

[Hialus]

Warning

CONTAINS DATABASE MIGRATION INCOMPATIBLE WITH DEVELOP!

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

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 added multiple integration tests (Spring) related to the features (with a high test coverage).
• I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
• 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 added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
• 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.

Motivation and Context

After the introduction of PyrisV2 the settings system for Iris was outdated. We want to update it to support PyrisV2 fully.
Additionally, we discontinue the Iris Hestia integration.

Description

• Removed old settings fields
• allowedModels and preferredModels were transformed into allowedVariants and selectedVariant
• All Iris Hestia related stuff was removed
• Updated UI

Steps for Testing

Prerequisites:

• 1 Admin account
• Local setup with Pyris (unless otherwise announced)
• FastAPI: Support new settings system Pyris#149 merged or deployed on pyris-test

1. Backup your database
2. Log in to Artemis
3. In the nav bar in the admin drop down go to Iris settings
4. Play around with the settings
5. After you are done set everything back to original (all enabled)
6. Go to course management page of your test course
7. Open Iris settings
8. Play around
9. Go to the management page of a programming exercise in your course
10. Open Iris settings
11. Play around
12. Optional: Restore your database

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

Test Coverage

Client

Class/File	Line Coverage	Confirmation (assert/expect)
course-detail.component.ts	91.4%	✅
iris-settings.model.ts	81.81%	✅
iris-sub-settings.model.ts	100%	✅
iris-common-sub-settings-update.component.ts	96.22%	✅
iris-settings-update.component.ts	88.6%	✅
iris-enabled.component.ts	100%	✅
iris-settings.service.ts	90.47%	✅

Server

Class/File	Line Coverage	Confirmation (assert/expect)
IrisSession.java	100%	✅
IrisChatSubSettings.java	100%	✅
IrisCompetencyGenerationSubSettings.java	100%	✅
IrisCourseSettings.java	100%	✅
IrisExerciseSettings.java	91%	✅
IrisGlobalSettings.java	100%	✅
IrisSettings.java	100%	✅
IrisSubSettings.java	100%	✅
IrisSubSettingsType.java	100%	✅
IrisCombinedChatSubSettingsDTO.java	100%	✅
IrisCombinedCompetencyGenerationSubSettingsDTO.java	100%	✅
IrisCombinedSettingsDTO.java	100%	✅
IrisSettingsRepository.java	100%	✅
IrisSessionService.java	76%	✅
PyrisConnectorService.java	78%	✅
IrisExerciseChatSessionService.java	90%	✅
IrisSettingsService.java	86%	✅
IrisSubSettingsService.java	89%	✅
IrisSettingsResource.java	95%	✅
CodeHintService.java	96%	✅
CodeHintResource.java	85%	✅

�

Screenshots

Summary by CodeRabbit

Summary by CodeRabbit

• New Features

  • Added new methods for assessment management, including the ability to cancel and delete assessments.
  • Introduced a new BonusResultDTO record to encapsulate bonus grade results.
  • Updated BonusSourceResultDTO to include additional fields for tracking bonus results.
  • Updated method for fetching variants based on specified features in PyrisConnectorService.
  • Enhanced role-based access control in IrisSettingsResource for various settings operations.
  • Introduced a new REST controller IrisVariantsResource for managing variants.

• Documentation

  • Enhanced guidelines on database relationships and fetching strategies, including lazy fetching and best practices.

• Refactor

  • Improved organization of import statements across several classes for better readability.

• Style

  • Reordered import statements to follow a consistent structure across various files.

• Chores

  • Removed dependency on Iris functionality from CodeHintResource, simplifying the class.

• Tests

  • Streamlined test setups by removing unnecessary components and adjusting expectations in several test suites.

[coderabbitai]

Actionable comments posted: 4

[coderabbitai]

Actionable comments posted: 1

[MichaelOwenDyer]

Tested locally, works great. Code looks good too

[github-actions]

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.

[MichaelOwenDyer]

This is a much-needed change and I have tested it continuously for over a month now, imo it is ready to merge.

[kaancayli]

Changes look good. Tested locally, no issues encountered. LGTM

[yassinsws]

Tested this locally works good !

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 3869102 and 1c0db46.

📒 Files selected for processing (3)

• src/test/javascript/spec/component/iris/settings/iris-course-settings-update.component.spec.ts (1 hunks)
• src/test/javascript/spec/component/iris/settings/iris-enabled.component.spec.ts (3 hunks)
• src/test/javascript/spec/component/iris/settings/iris-exercise-settings-update.component.spec.ts (0 hunks)

💤 Files with no reviewable changes (1)

• src/test/javascript/spec/component/iris/settings/iris-exercise-settings-update.component.spec.ts

🧰 Additional context used

📓 Path-based instructions (2)

src/test/javascript/spec/component/iris/settings/iris-course-settings-update.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

src/test/javascript/spec/component/iris/settings/iris-enabled.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

🔇 Additional comments (2)

src/test/javascript/spec/component/iris/settings/iris-enabled.component.spec.ts (2)

Line range hint 50-59: LGTM! Consider using constants for sub-settings types.

The removal of IrisSubSettingsType.HESTIA from the test cases aligns with the PR objectives. The test still covers the relevant sub-settings types (CHAT and COMPETENCY_GENERATION).

As suggested in a previous review, consider using constants for the sub-settings types to improve maintainability:

const EXERCISE_SUB_SETTINGS_TYPES = [
  IrisSubSettingsType.CHAT,
  IrisSubSettingsType.COMPETENCY_GENERATION
] as const;

it.each(EXERCISE_SUB_SETTINGS_TYPES)('should load exercise', async (subSettingsType) => {
  // ... rest of the test
});

This approach would make it easier to maintain the list of sub-settings types across different test cases.

---

89-90: LGTM! Consider parameterizing the test for different sub-settings types.

The changes correctly update the sub-settings type from HESTIA to CHAT and use the corresponding irisChatSettings. This aligns with the removal of HESTIA references mentioned in the PR objectives.

To improve test coverage and flexibility, consider parameterizing this test for different sub-settings types as suggested in a previous review:

it.each([
  [IrisSubSettingsType.CHAT, 'irisChatSettings'],
  [IrisSubSettingsType.COMPETENCY_GENERATION, 'irisCompetencyGenerationSettings'],
  [IrisSubSettingsType.LECTURE_INGESTION, 'irisLectureIngestionSettings'],
])('should set course enabled for %s', async (subSettingsType, settingsProperty) => {
  const setSettingsSpy = jest.spyOn(irisSettingsService, 'setCourseSettings').mockReturnValue(of(new HttpResponse({ body: null as any as IrisSettings })));
  comp.course = course;
  comp.irisSettings = irisSettings;
  comp.irisSubSettingsType = subSettingsType;
  comp.irisSubSettings = irisSettings[settingsProperty];

  comp.setEnabled(true);
  expect(setSettingsSpy).toHaveBeenCalledOnce();
  await Promise.resolve();
  expect(comp.irisSubSettings?.enabled).toBeTrue();
});

This approach would test the setEnabled functionality for all relevant sub-settings types in a single, parameterized test case.

[MichaelOwenDyer]

Reapprove

[kaancayli]

Reapprove

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 1c0db46 and ce497fc.

📒 Files selected for processing (1)

• src/main/java/de/tum/cit/aet/artemis/iris/web/IrisVariantsResource.java (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/main/java/de/tum/cit/aet/artemis/iris/web/IrisVariantsResource.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

[yassinsws]

reapprove