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

Sakai LTI compatibility. LTI module combined with app. #1428

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

iturgeon
Copy link
Member

@iturgeon iturgeon commented Dec 28, 2022

This is a work in progress.

Add LTI compatibility with The Sakai LMS

  • /lti/assignment will now accept ContentItemSelectionRequest launches
  • Change var to const for js constants being injected into the page from the server
  • Allows the before_play_start event to determine if the lti content item picker should be displayed
  • Move picker display function into a common controller
  • Add an api method that’ll validate and sign a content item selection lti request
  • injects LTI_MESSAGE_TYPE and LTI_KEY js constants into the picker page template
  • LTILaunch gains a method to load the lti config that is associated with a lti key
  • Updated a few strings that specifically mention canvas
  • Moves all classes, tests, migrations, and configs from fuel/app/modules/lti into fuel/app
  • simplify lti launch picker code when sending content selection messages to lti/assignment
  • fixed issue with starting plays with referrer urls longer than 255 characters fixes If the referrer url is longer than 255 characters, creating a new play will fail #1418
  • Overhaul lit test provider a bit - mostly to simplify
  • attempt to fix yarn cache issue
  • Changes docker build process to ensure the built image has admin:make_paths_writable run to avoid having to run it every startup
  • new api endpoint for signing content item selection lti_sign_content_item_selection
  • improves oauth validation in fuel by ensuring the request uri is not masked by FuelPHP

Some specific testing should take place before merging:

  • Test Obojobo Next compatibility, it may not support the new picker integration
  • Test Canvas integration. (I tested the picker in an assignment on Canvas Free for Teachers)

This work is sponsored by LongSight

* Change var to const for js constants being injected into the page from the server
* Allows the before_play_start event to determine if the lti content item picker should be displayed
* Move picker display function into a common controller
* Add an api method that’ll validate and sign a content item selection lti request
* injects LTI_MESSAGE_TYPE and LTI_KEY js constants into the picker page template
* LTILaunch gains a method to load the lti config that is associated with a lti key
* Updated a few strings that specifically mention canvas
* Moves all classes, tests, migrations, and configs from fuel/app/modules/lti into fuel/app
* overhaul lti test provider to simplify
* simplify lti launch picker code when sending content selection messages to lti/assignment
* fixed issue with starting plays with referrer urls longer than 255 characters fixes ucfopen#1418
* updates oauth validation in fuel by ensuring the request uri is not masked by FuelPHP
* Overhaul lit test provider a bit - mostly to simplify and use lessons
* fix code sniff errors
* add deps to docker image
* attempt to fix yarn cache issue
* Changes docker build process to ensure the built image has admin:make_paths_writable run to avoid having to run it every startup
@iturgeon iturgeon force-pushed the issue/lti-compatability-with-sakai branch from cf80cc2 to 702747b Compare December 31, 2022 18:11
@iturgeon iturgeon force-pushed the issue/lti-compatability-with-sakai branch from 2b31389 to c705e91 Compare January 25, 2023 20:47
@@ -22,4 +22,4 @@ docker run \
--mount type=bind,source="$(pwd)"/../,target=/build \
--mount source=materia-asset-build-vol,target=/build/node_modules \
node:12.11.1-alpine \
/bin/ash -c "apk add --no-cache git && cd build && yarn install --frozen-lockfile --non-interactive --production --silent --pure-lockfile --force"
Copy link
Member Author

Choose a reason for hiding this comment

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

My Docker build process was dying regularly for some reason, this seems to have helped.

@@ -15,4 +15,4 @@ DCTEST="docker-compose -f docker-compose.yml -f docker-compose.override.test.yml
set -e
set -o xtrace

$DCTEST run --rm app /wait-for-it.sh mysql:3306 -t 20 -- composer run testci -- "$@"
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding -T allowed the commit hook to work for me

@@ -10,4 +10,4 @@ DCTEST="docker-compose -f docker-compose.yml -f docker-compose.override.test.yml
set -e
set -o xtrace

$DCTEST run --rm --no-deps app composer sniff-ci
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, working to add better support for pre-commit hooks

@@ -54,6 +54,13 @@ protected function tearDown(): void
}
}

protected static function remove_all_roles_for_user($user_id)
Copy link
Member Author

Choose a reason for hiding this comment

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

To get the tests to pass, I needed a way to easily wipe all roles for a user

\Js::push_inline('var BASE_URL = "'.\Uri::base().'";');
\Js::push_inline('var STATIC_CROSSDOMAIN = "'.\Config::get('materia.urls.static').'";');
\Js::push_inline('const BASE_URL = "'.\Uri::base().'";');
\Js::push_inline('const STATIC_CROSSDOMAIN = "'.\Config::get('materia.urls.static').'";');
Copy link
Member Author

Choose a reason for hiding this comment

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

hey, const is a thing!

@@ -53,7 +52,7 @@ public static function from_request()
return static::$launch;
}

public static function config()
public static function config_from_request()
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed because i added config_from_key

// When sending LTI ContentItem Selection requests, Sakai updated and required
// that GET params in the url aren't duplicated in the body of params
// as they caused oauth signature validation to fail in Sakai.
'enforce_unique_params' => $_ENV['LTI_ENFORCE_UNIQUE_PARAMS'] ?? true,
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this option as it's not super clear if other platforms will want this or not.

@@ -34,7 +34,14 @@
'preview/(:alnum)(/.*)?' => 'widgets/preview_widget/$1',
'preview-embed/(:alnum)(/.*)?' => 'widgets/play_embedded_preview/$1',
'embed/(:alnum)(/.*)?' => 'widgets/play_embedded/$1',
'lti/assignment?' => 'widgets/play_embedded/$1', // legacy LTI url
'lti/assignment?' => function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't use this much in Materia, so this may be difficult to find in the future. BUT, Fuel lets us add a function here to determine what controller receives a request. This is used to support lti implementations that only send requests to a single Materia url. To support this, lti/assignment will allow picker or content selection requests as well as widget play requests.

@@ -65,15 +65,15 @@ RUN composer install --no-cache --no-dev --no-progress --no-scripts --prefer-dis
# =====================================================================================================
FROM node:12.11.1-alpine AS yarn_stage

RUN apk add --no-cache git
RUN apk add --no-cache git python make g++
Copy link
Member Author

Choose a reason for hiding this comment

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

I ran into issues on new base php images building that were helped by adding these

@@ -89,3 +89,15 @@ USER www-data
COPY --from=composer_stage --chown=www-data:www-data /var/www/html /var/www/html
COPY --from=yarn_stage --chown=www-data:www-data /build/public /var/www/html/public
COPY --from=yarn_stage --chown=www-data:www-data /build/fuel/app/config/asset_hash.json /var/www/html/fuel/app/config/asset_hash.json

# set bogus values so admin task can run
ENV AUTH_SIMPLEAUTH_SALT=TEMPINVALIDVALUE
Copy link
Member Author

Choose a reason for hiding this comment

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

php8 was unhappy running admin commands without these environment vars

@@ -19,7 +19,7 @@
},
"dependencies": {
"fs-extra": "^8.0.1",
"materia-server-client-assets": "2.4.2",
"materia-server-client-assets": "https://github.com/iturgeon/Materia-Server-Client-Assets.git#842eaadcf38811bbaa37d804049cc36a4f643a9b",
Copy link
Member Author

Choose a reason for hiding this comment

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

THIS NEEDS TO BE UPDATED WHEN supporting MSCA PR is merged/tagged

@clpetersonucf
Copy link
Member

An update on this: I plan on addressing and implementing the changes on this PR after the react branch has shipped (which is hopefully imminent). The major changes required will be converting the PHP partials into their corresponding react views, and integrating the related changes to MSCA, since that repo is being deprecated.

@iturgeon
Copy link
Member Author

An update on this: I plan on addressing and implementing the changes on this PR after the react branch has shipped (which is hopefully imminent). The major changes required will be converting the PHP partials into their corresponding react views, and integrating the related changes to MSCA, since that repo is being deprecated.

Sounds good @clpetersonucf I can probably help out, so just ping me when it’s ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If the referrer url is longer than 255 characters, creating a new play will fail
2 participants