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: Fix issues with LTI 1.3 for open edx #7931

Merged
merged 190 commits into from
Feb 26, 2024
Merged

Conversation

basak-akan
Copy link
Contributor

@basak-akan basak-akan commented Jan 21, 2024

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

openEdx supports LTI 1.3 within their xblock-lti-consumer components. Each xblock-lti-consumer component acts as an external platform who consumes content (which means each xblock-lti-consumer has its own configuration details such as unique client id, authorization uri, jwkset uri, token uri). Also openEdx does not support dynamic configuration service (the service that you only provide a dynamic configuration link and all the configuration information communications done automatically). So Artemis administrators needs to manually fill in each xblock-lti-consumer components information into Artemis.
Besides that since each xblock-lti-consumer components act as separate consumer it is impossible to keep track of which xblock-lti-consumer component launched an exercise in current setting.

Description

To adress above issues;

  • new admin ui added to manually create lti platform
  • added a new column to lti_resource_launch table which is a fk to the lti_platform_configuration id. In this way we can keep track of exercise launches and send the results back to correct xblock-lti-consumer component.

Warning

PLEASE ONLY DEPLOY TO TS11, CONTAINS DATABASE CHANGES!!!

Steps for Testing

Prerequisites:

  • 1 Student
  1. Log into edX ( https://openedx.ase.cit.tum.de/ ) using one of the student credentials (i.e. artemis_test_user_1)

  2. Navigate to LTI 1.3 Test Course

  3. Select Introduction then Hello World section

  4. Select launch exercise under Lti Consumer and then press ok

  5. Verify iframe opens and nagivates to Artemis

  6. Login artemis using same student credentials

  7. Participate exercise and submit

  8. Close iframe

  9. Select progress on top bar

  10. Verify you see exercise participation result

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)
edit-lti-configuration.component.ts 85%
lti-configuration.component.ts 94.73%
lti-configuration.service.ts 88.88%
course-lti-configuration.component.ts 96.77%
edit-course-lti-configuration.component.ts 82.5%
course-management-tab-bar.component.ts 93.97%
course-management.service.ts 87.28%
online-course-configuration.model.ts 100%
lti-course-card.component.ts 100%
lti13-exercise-launch.component.ts 95.16%
lti13-select-course.component.ts 92.3%

Server

Class/File Line Coverage Confirmation (assert/expect)
LtiPlatformConfiguration.java 92%
Lti13AgsClaim.java 10% ✅ ❌
Lti13ClientRegistration.java 84%
Lti13LaunchRequest.java 97%
LtiResourceLaunch.java 76%
CourseRepository.java 76%
LtiPlatformConfigurationRepository.java 76%
Lti13LaunchFilter.java 100%
CourseService.java 90%
OnlineCourseConfigurationService.java 95%
Lti13Service.java 88%
LtiService.java 97%
CourseResource.java 94%
LtiResource.java 73%
AdminLtiConfigurationResource.java 86%
OnlineCourseDTO.java 100%

Screenshots

  • edX instructor + artemis admin
  1. edx instructor provides lti consumer information to artemis admin.
  2. Artemis admin creates new lti platform
  3. edx instructor fills in artemis information into edx lti consumer.
Screen.Recording.1-21-2024.11-06-07.PM.mp4
  • Student partipates into Artemis exercise via edX and see result on edX progress page
Screen.Recording.1-21-2024.11-24-49.PM.mp4

Summary by CodeRabbit

  • New Features
    • Enhanced LTI 1.3 launch request handling with new client registration ID support.
    • Improved LTI platform configuration management, including the ability to add new configurations and retrieve them by ID.
    • Updated UI for LTI configuration in the admin section, including better handling of platform data and new nested routing for configuration editing.
    • Added a new button for creating LTI platform configurations, improving navigation and user experience.
  • Bug Fixes
    • Updated conditional assignment for the lineItem variable to support various LMS storage locations.
  • Style
    • Improved code readability by replacing specific import statements with wildcard imports in several Java files.
  • Tests
    • Expanded test coverage with new tests for LTI platform configuration creation, online course retrieval for LTI dashboards, and service layer interactions.
    • Improved test reliability in the JavaScript component test by removing unnecessary tick function call and adding a spy for service method.

rstief
rstief previously approved these changes Feb 26, 2024
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.

Code looks good 👍

…t-course-lti-configuration.component.ts

Co-authored-by: Andreas Resch <andreas@resch.io>
milljoniaer
milljoniaer previously approved these changes Feb 26, 2024
Copy link
Contributor

@milljoniaer milljoniaer left a comment

Choose a reason for hiding this comment

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

LGTM

@basak-akan basak-akan requested a review from rstief February 26, 2024 15:13
rstief
rstief previously approved these changes Feb 26, 2024
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.

💯

reschandreas
reschandreas previously approved these changes Feb 26, 2024
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.

reapprove

Jan-Thurner
Jan-Thurner previously approved these changes Feb 26, 2024
Copy link
Contributor

@Jan-Thurner Jan-Thurner left a comment

Choose a reason for hiding this comment

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

reapprove

nata-81
nata-81 previously approved these changes Feb 26, 2024
Copy link

@nata-81 nata-81 left a comment

Choose a reason for hiding this comment

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

reapprove

@krusche krusche changed the title Development: Fix issues with LTI 1.3 for openedX Development: Fix issues with LTI 1.3 for open edx Feb 26, 2024
@krusche krusche added this to the 6.9.0 milestone Feb 26, 2024
@krusche krusche merged commit fdbd2e1 into develop Feb 26, 2024
12 of 18 checks passed
@krusche krusche deleted the feature/fix-edx-lti1p3 branch February 26, 2024 19:54
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!) component:LTI database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants