Skip to content
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

Development: Implement multiple exercise selection in LTI deep linking service #7812

Merged
merged 176 commits into from
Feb 4, 2024

Conversation

basak-akan
Copy link
Contributor

@basak-akan basak-akan commented Dec 20, 2023

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the 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.
  • I followed the 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

This update introduces the capability to add multiple exercises in a single LTI deep-linking service response. Previously, instructors had to add exercises individually. By enabling batch addition of exercises, this change enhances usability, and reduces the administrative burden on instructors. Additionally, it includes an improved arrangement of the exercises list view, further optimizing the overall workflow and user interface.

Description

  • Multiple exercises can be selected to Link to external LMS (Moodle).
  • If the Artemis exercise is a graded or bonus exercise, it will be added to the grade book of the external LMS.
  • If the Artemis exercise is not a graded exercise, it won't appear on the grade book of the external LMS.
  • Exercise selection page rearranged:
    • Course title and semester information removed
    • Exercise start date added
    • Exercise graded information added

Steps for Testing

Prerequisites:

  • Test on Test Server 11 where lti profile is enabled.
  • 1 Instructor
  1. Login to Moodle (https://moodle.ase.in.tum.de/) using instructor credentials (e.g. artemis_test_user_11)
  2. Select "LTI Testing Course"
  3. Enable edit mode (on top right corner of the page)
  4. Select add an activity or resource
  5. Select "External Tool"
  6. You will be directed to editing external tool page
  7. Under preconfigured tools select "Artemis - https://artemis-test11.artemis.in.tum.de/" and click "select content" button.
  8. Verify you see login page of artemis and login using same instructor credentials.
  9. Verify you see "LTI Single Configuration Test" course and select this course
  10. Select multiple exercises where graded column is yes and no and click on "link" button
  11. Verify you see selected exercises names and graded information on navigated Moodle page (Please note that if exercise is a graded exercise you will see maximum graded value as 100 - this is the intended behavior to match with current results service)
  12. Click on add and verify the selected exercises are added to the Moodle course.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked







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
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
lti13-deep-linking.component.ts 90.62%

Server

Class/File Line Coverage Confirmation (assert/expect)
LtiDeepLinkingService.java 97%
LtiResource.java 100%

Screenshots

video

Summary by CodeRabbit

  • New Features

    • Enhanced the LTI Deep Linking service to support multiple exercise IDs for deep linking, improving integration flexibility.
    • Updated the LTI deep linking UI to display exercise start dates and inclusion in overall scores, enhancing user information.
  • Refactor

    • Refactored LTI deep linking components and services for improved handling of multiple exercises.
    • Updated test suites to align with the new multi-exercise deep linking functionality.
  • Style

    • Adjusted UI elements in the LTI deep linking component for a clearer presentation of exercise details.

@krusche krusche dismissed stale reviews from MaximilianJG, Predixx, and konrad2002 January 27, 2024 18:02

The base branch was changed.

@krusche krusche added the too-long-open !!! This is an antipattern, we should aim for small PRs that are only open for a short time !!! label Jan 27, 2024
Copy link
Contributor

@egekurt123 egekurt123 left a comment

Choose a reason for hiding this comment

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

Reapprove after base merge

Copy link
Contributor

@RY997 RY997 left a comment

Choose a reason for hiding this comment

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

Tested on ts11, worked as expected

Copy link

@Gusti2010 Gusti2010 left a comment

Choose a reason for hiding this comment

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

Manually tested on ts11_Legacy, works as expected! Thanks for the detailed testing steps + video :)

Copy link
Contributor

@reschandreas reschandreas left a comment

Choose a reason for hiding this comment

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

code lgtm, also demonstrated in testing session, works as expected, thanks!

Copy link
Contributor

@laurenzfb laurenzfb left a comment

Choose a reason for hiding this comment

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

demonstrated on ts11, works. code also lgtm

Copy link
Contributor

@rstief rstief left a comment

Choose a reason for hiding this comment

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

Demonstrated in testing session, code also lgtm

@krusche krusche added this to the 6.8.0 milestone Feb 4, 2024
@krusche krusche merged commit afb4317 into develop Feb 4, 2024
26 of 29 checks passed
@krusche krusche deleted the feature/lti-link-multiple-exercise branch February 4, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) ready to merge server Pull requests that update Java code. (Added Automatically!) tests too-long-open !!! This is an antipattern, we should aim for small PRs that are only open for a short time !!!
Projects
None yet
Development

Successfully merging this pull request may close these issues.