`General`: Add LTI integration for Atlas, Iris, and Lectures #10338

raffifasaro · 2025-02-16T00:17:45Z

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

Currently, it is only possible for instructors to link Artemis exercises to Moodle learning activities. This PR enhances this functionality by adding Lectures, Competencies, Learning Activities and the Iris course dashboard to the linkable Artemis features.
To accomplish this, the UI for selecting the new content and the logic behind the LTI standard were adapted.

Description

This PR is a major addition to the existing LTI system.

Content selection table:
This PR reworks the content selection table, which instructors can use to link Artemis content to learning activities on Moodle.
The table now also features a selection for lectures, the competency page, the learning-activities page, as well as the course dashboard with a focus on the integrated Iris console.
New LTI features:
Server-side changes facilitate the new LTI features (Lectures, Competencies, Learning Paths and Iris). These changes are split into the linking process used by LTI to link the new content to a Moodle learning activity and a re-work of the authentication within the LTI services to be centered around courses instead of exercises.

Steps for Testing

Prerequisites:

Access to Moodle
Testserver 3

Make sure you are logged in to Artemis with a Test User 16)
Navigate to Moodle and login with the same test user (Test User 16}, same PW as for the Artemis Test Server)
Go to My Courses and open the lti test course
Activate Edit mode in top right corner
Add a learning activity by clicking on the blue + in "Test Topic"
Select Linhuber TS3
Click on "Select content"
Select the LTI test course
Select the different content types you want to test and click "link" in the bottom right corner
If the content was selected successfully, scroll down and hit "Save and display"

(If you get an error after linking, this is due to server sync, just try again by pressing the back button in your browser)
You should be able to see the new content now

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

Code Review

Code Review 1
Code Review 2

Manual Tests

Test 1
Test 2

Test Coverage

Client

Class/File	Line Coverage	Confirmation (assert/expect)
course-management.service.ts	86.66%	✅
lti13-deep-linking.component.ts	92.45%	✅
lti13-exercise-launch.component.ts	85.33%	✅

Server

Class/File	Line Coverage	Confirmation (assert/expect)
CourseResource.java	25%	✅
LtiResourceLaunch.java	76%	✅
DeepLinkingType.java	53%	✅
Lti13Service.java	74%	✅
LtiDeepLinkingService.java	46%	✅
LtiResource.java	52%	✅
PublicLtiResource.java	94%	✅

�

Screenshots

New content selection table:

LTI Lectures:

LTI Competencies:

LTI Learning-paths:

LTI Iris dashboard:

Summary by CodeRabbit

New Features
- Course retrieval now provides complete details including exercises, lectures, and competencies.
- Deep linking has been expanded to support additional content types such as lectures, competencies, learning paths, and IRIS.
User Interface Enhancements
- The deep linking interface has been redesigned with dropdown menus for a more organized and interactive content selection.
- Improved validation and selection feedback ensure a smoother linking process.
Localization
- New German and English translations enhance user guidance with updated labels and error messages.

…on-table' into chore/lti/extend-content-selection-table

sawys777

Tested on TS3, everything works as expected

MaximilianAnzinger

Code approvals & Manual tests all complete
Maintainer checked

TODO: I will lower the coverage threshold for lti - instructions in one single commit. All important functionality is covered (even the enum ...). I do not think that additional investigation into the coverage is reasonable to meet the required threshold.

If @krusche does not approve, we will have to revert the commit and somehow have to figure out how to improve the coverage.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

gradle/jacoco.gradle (1)

17-17: Revisit the LTI Module Coverage Threshold Setting.

The threshold for the LTI module is now set to an INSTRUCTION value of 0.810 with a CLASS threshold of 2. Please confirm that these specific values have been intentionally chosen to accommodate the new LTI integration features. It may be useful to double-check whether these thresholds align with the broader coverage expectations for the project and the additional functionality introduced in this PR.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1f2ebcb and a83ec9e.

⛔ Files ignored due to path filters (1)

src/main/resources/config/liquibase/master.xml is excluded by !**/*.xml

📒 Files selected for processing (1)

gradle/jacoco.gradle (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (9)

GitHub Check: Call Build Workflow / Build and Push Docker Image
GitHub Check: Call Build Workflow / Build .war artifact
GitHub Check: Codacy Static Code Analysis
GitHub Check: client-tests-selected
GitHub Check: client-tests
GitHub Check: server-tests
GitHub Check: server-style
GitHub Check: client-style
GitHub Check: Analyse

krusche · 2025-02-19T21:03:39Z

src/main/java/de/tum/cit/aet/artemis/lti/service/DeepLinkingType.java

+        for (DeepLinkingType deepLinkingType : values()) {
+            if (deepLinkingType.name().equalsIgnoreCase(type)) {
+                return deepLinkingType;
+            }
+        }
+        throw new IllegalArgumentException("Invalid deep linking type: " + type);


Suggested change

for (DeepLinkingType deepLinkingType : values()) {

if (deepLinkingType.name().equalsIgnoreCase(type)) {

return deepLinkingType;

}

}

throw new IllegalArgumentException("Invalid deep linking type: " + type);

return DeepLinkingType.valueOf(type.toUpperCase());

this is much easier ;-)

krusche · 2025-02-19T21:17:26Z

src/main/java/de/tum/cit/aet/artemis/lti/service/LtiDeepLinkingService.java

+        }
+    }
+
+    private Map<String, Object> createExerciseContentItem(Exercise exercise, String url) {


Does it make sense to use a map here? I would prefer a record type

krusche · 2025-02-19T21:17:40Z

src/main/java/de/tum/cit/aet/artemis/lti/service/LtiDeepLinkingService.java

@@ -125,6 +241,14 @@ private Map<String, Object> createContentItem(Exercise exercise, String url) {
        return item;
    }

+    private Map<String, Object> createLectureContentItem(Lecture lecture, String url) {


Does it make sense to use a map here? I would prefer a record type

krusche · 2025-02-19T21:17:55Z

src/main/java/de/tum/cit/aet/artemis/lti/service/LtiDeepLinkingService.java

+        return Map.of("type", "ltiResourceLink", "title", lecture.getTitle(), "url", url);
+    }
+
+    private Map<String, Object> createSingleUnitContentItem(String url) {


Does it make sense to use a map here? I would prefer a record type

krusche · 2025-02-19T21:19:03Z

src/main/java/de/tum/cit/aet/artemis/lti/service/LtiDeepLinkingService.java

+     *
+     * @param courseId The course ID.
+     */
+    private ArrayList<Map<String, Object>> populateCompetencyContentItems(String courseId) {


this is an ugly return type. Please use List<RecordType> if possible, also in all other places

and in general you should avoid using ArrayList as Type (except next to the new operator), always use List instead

krusche · 2025-02-19T21:23:08Z

src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java

-        String targetLink = ltiDeepLinkingService.performDeepLinking(idToken, clientRegistrationId, courseId, exerciseIds);
+        String targetLink;
+
+        if (exerciseIds != null) {


The long if-else block looks ugly. I wonder if there are other solutions to solve this.

krusche · 2025-02-19T21:25:40Z

src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java

-    @EnforceAtLeastInstructor
-    public ResponseEntity<String> lti13DeepLinking(@PathVariable Long courseId, @RequestParam(name = "exerciseIds") Set<Long> exerciseIds,
+    @EnforceAtLeastInstructorInCourse
+    public ResponseEntity<String> lti13DeepLinking(@PathVariable Long courseId, @RequestParam(name = "exerciseIds", required = false) Set<Long> exerciseIds,


I suggest to change the input parameters. It seems that all options are mutually exclusive so you could directly send a required type (potentially string or if possible and enum) whether exercises, lectures, competency, iris or learningPath is linked. Then you have a second parameter contentIds (required = false) which are either exerciseIds or lectureIds based the previously defined type. This would be much cleaner and would also allow you to improve the code below by e.g. using a switch expression

krusche · 2025-02-19T21:28:15Z

src/main/java/de/tum/cit/aet/artemis/lti/service/Lti13Service.java

+     * @param targetLinkUrl the target link URL
+     * @return true if the target link URL references a competency, false otherwise
+     */
+    private boolean targetLinkHasCompetency(String targetLinkUrl) {


targetLinkHasCompetency(...), targetLinkHasLearningPath(...) and targetLinkHasIris(...) are almost identical. This is really bad code. I kindly ask you to unify the code already here to avoid such issues. You could e.g. have one method that returns an enum type whether competency, learningPath or iris is included.

krusche · 2025-02-19T21:28:33Z

src/main/java/de/tum/cit/aet/artemis/lti/service/Lti13Service.java

+            return false;
+        }
+
+        log.info("Competency link detected: {}", targetLinkUrl);


do we need so many log.info?

krusche · 2025-02-19T21:29:01Z

src/main/java/de/tum/cit/aet/artemis/lti/service/Lti13Service.java

+        }
+
+        Map<String, String> pathVariables = null;
+        if (matcher.match(COURSE_PATH_PATTERN, targetLinkPath)) {


ugly if-else with lots of duplicated code. please refactor this

HawKhiem

Tested on TS3. Works as described

raffifasaro and others added 30 commits December 9, 2024 16:44

Mock test

134b7b8

Mock test 2

2f27e4b

mock table test

afdf7f0

mock table test fix

9e8c5f1

mock table fix

d19bc5d

mock table comment out dates

358d5c7

table fix

2bd644d

Merge branch 'develop' into chore/lti/extend-content-selection-table

5927c0a

expand table

8f2887d

find lectures

2909b48

fix

31a2fcb

Add working requests

2d0d385

fix

916a378

Add translation support

59ca782

fix

a51cd68

Iris selector + info text for competencies

469a78b

info text complete

23476f5

Fix selection

8c885d1

Update sendDeepLinkRequest() for use with new features

2816925

Merge branch 'develop' into chore/lti/extend-content-selection-table

fa28ef2

Merge branch 'develop' into chore/lti/extend-content-selection-table

2774b1e

Fix competency checkbox header

0844708

Merge remote-tracking branch 'origin/chore/lti/extend-content-selecti…

010c1bc

…on-table' into chore/lti/extend-content-selection-table

Merge branch 'develop' into chore/lti/extend-content-selection-table

e7f94ef

Merge branch 'develop' into chore/lti/extend-content-selection-table

46ea417

Merge branch 'develop' into chore/lti/extend-content-selection-table

e36102e

add competency support test

0ccd8d3

fix competency selection frontend

fc7b481

fix parameter requirements

1812260

fix exercise Id check

3b802f8

sawys777 previously approved these changes Feb 19, 2025

View reviewed changes

Merge branch 'develop' into chore/lti/extend-content-selection-table

e45b47d

raffifasaro changed the title ~~Development: Enhance LTI features~~ General: Enhance LTI features Feb 19, 2025

raffifasaro changed the title ~~General: Enhance LTI features~~ 'General': Enhance LTI features Feb 19, 2025

raffifasaro changed the title ~~'General': Enhance LTI features~~ General: Enhance LTI features Feb 19, 2025

MaximilianAnzinger changed the title ~~General: Enhance LTI features~~ General: Add LTI integration for Atlas, Iris, and Lectures Feb 19, 2025

MaximilianAnzinger previously approved these changes Feb 19, 2025

View reviewed changes

adjust lti module coverage

a83ec9e

MaximilianAnzinger dismissed stale reviews from SindiBuklaji, coderabbitai[bot], sawys777, pvlov, az108, Feras797, ahbitaqu, and themself via a83ec9e February 19, 2025 18:08

coderabbitai bot reviewed Feb 19, 2025

View reviewed changes

coderabbitai bot approved these changes Feb 19, 2025

View reviewed changes

krusche reviewed Feb 19, 2025

View reviewed changes

helios-aet bot temporarily deployed to artemis-test3.artemis.cit.tum.de February 20, 2025 05:59 Inactive

HawKhiem approved these changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`General`: Add LTI integration for Atlas, Iris, and Lectures #10338

`General`: Add LTI integration for Atlas, Iris, and Lectures #10338

raffifasaro commented Feb 16, 2025 •

edited by coderabbitai bot

Loading

sawys777 left a comment

MaximilianAnzinger left a comment

coderabbitai bot left a comment

krusche Feb 19, 2025

krusche Feb 19, 2025

krusche Feb 19, 2025

krusche Feb 19, 2025

krusche Feb 19, 2025

krusche Feb 19, 2025 •

edited

Loading

krusche Feb 19, 2025

krusche Feb 19, 2025

krusche Feb 19, 2025

krusche Feb 19, 2025

krusche Feb 19, 2025

krusche Feb 19, 2025

HawKhiem left a comment

General: Add LTI integration for Atlas, Iris, and Lectures #10338

Are you sure you want to change the base?

General: Add LTI integration for Atlas, Iris, and Lectures #10338

Conversation

raffifasaro commented Feb 16, 2025 • edited by coderabbitai bot Loading

Checklist

General

Server

Client

Motivation and Context

Description

Steps for Testing

Testserver States

Review Progress

Code Review

Manual Tests

Test Coverage

Client

Server

Screenshots

Summary by CodeRabbit

Summary by CodeRabbit

sawys777 left a comment

Choose a reason for hiding this comment

MaximilianAnzinger left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krusche Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HawKhiem left a comment

Choose a reason for hiding this comment

`General`: Add LTI integration for Atlas, Iris, and Lectures #10338

`General`: Add LTI integration for Atlas, Iris, and Lectures #10338

raffifasaro commented Feb 16, 2025 •

edited by coderabbitai bot

Loading

krusche Feb 19, 2025 •

edited

Loading