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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
702747b
LTI overhaul to support Sakai 22.x
iturgeon Nov 13, 2022
c705e91
fix for improving content item lti post
iturgeon Jan 24, 2023
3ed6072
fix score passback bug after simplifying lti module
iturgeon Feb 2, 2023
e021f5a
Merge branch 'master' into issue/lti-compatability-with-sakai
iturgeon Feb 2, 2023
acbfbd7
code cleanup for lti_sign_content_item_selection
iturgeon Feb 4, 2023
63384fe
renames LtiLaunch::config for clarity
iturgeon Feb 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docker/run_build_assets.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

/bin/ash -c "apk add --no-cache git && cd build && yarn install --frozen-lockfile --non-interactive --production --silent --pure-lockfile --force --check-files --cache-folder .ycache && rm -rf .ycache"
2 changes: 1 addition & 1 deletion docker/run_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

$DCTEST run -T --rm app /wait-for-it.sh mysql:3306 -t 20 -- composer run testci -- "$@"
2 changes: 1 addition & 1 deletion docker/run_tests_lint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

$DCTEST run -T --rm --no-deps app composer sniff-ci
45 changes: 36 additions & 9 deletions fuel/app/classes/basetest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

{
\DB::delete('perm_role_to_user')
->where('user_id', $user_id)
->execute();
}

protected static function clear_fuel_input()
{
// reset fuelphp's input class
Expand Down Expand Up @@ -196,6 +203,10 @@ protected function _as_student()
\Fuel\Tasks\Admin::new_user($uname, 'test', 'd', 'student', '[email protected]', $pword);
$user = \Model_User::find_by_username($uname);
}
else
{
static::remove_all_roles_for_user($user->id);
}

$login = \Service_User::login($uname, $pword);
$this->assertTrue($login);
Expand All @@ -215,13 +226,16 @@ protected function _as_author()
{
require_once(APPPATH.'/tasks/admin.php');
\Fuel\Tasks\Admin::new_user($uname, 'Prof', 'd', 'Author', '[email protected]', $pword);
\Fuel\Tasks\Admin::give_user_role($uname, 'basic_author');
$user = \Model_User::find_by_username($uname);
}
else
{
static::remove_all_roles_for_user($user->id);
}

\Materia\Perm_Manager::add_users_to_roles_system_only([$user->id], [\Materia\Perm_Role::AUTHOR]);
$login = \Service_User::login($uname, $pword);
$this->assertTrue($login);

$this->users_to_clean[] = $user;
return $user;
}
Expand All @@ -238,10 +252,14 @@ protected function _as_author_2()
{
require_once(APPPATH.'/tasks/admin.php');
\Fuel\Tasks\Admin::new_user($uname, 'test', 'd', 'author', '[email protected]', $pword);
\Fuel\Tasks\Admin::give_user_role($uname, 'basic_author');
$user = \Model_User::find_by_username($uname);
}
else
{
static::remove_all_roles_for_user($user->id);
}

\Materia\Perm_Manager::add_users_to_roles_system_only([$user->id], [\Materia\Perm_Role::AUTHOR]);
$login = \Service_User::login($uname, $pword);
$this->assertTrue($login);
$this->users_to_clean[] = $user;
Expand All @@ -260,10 +278,14 @@ protected function _as_author_3()
{
require_once(APPPATH.'/tasks/admin.php');
\Fuel\Tasks\Admin::new_user($uname, 'test', 'd', 'author', '[email protected]', $pword);
\Fuel\Tasks\Admin::give_user_role($uname, 'basic_author');
$user = \Model_User::find_by_username($uname);
}
else
{
static::remove_all_roles_for_user($user->id);
}

\Materia\Perm_Manager::add_users_to_roles_system_only([$user->id], [\Materia\Perm_Role::AUTHOR]);
$login = \Service_User::login($uname, $pword);
$this->assertTrue($login);
$this->users_to_clean[] = $user;
Expand All @@ -282,12 +304,14 @@ protected function _as_super_user()
{
require_once(APPPATH.'/tasks/admin.php');
\Fuel\Tasks\Admin::new_user($uname, 'test', 'd', 'su', '[email protected]', $pword);
// TODO: super_user should get all these rights inherently right??????!!!!
\Fuel\Tasks\Admin::give_user_role($uname, 'super_user');
\Fuel\Tasks\Admin::give_user_role($uname, 'basic_author');
$user = \Model_User::find_by_username($uname);
}
else
{
static::remove_all_roles_for_user($user->id);
}

\Materia\Perm_Manager::add_users_to_roles_system_only([$user->id], [\Materia\Perm_Role::AUTHOR, \Materia\Perm_Role::SU]);
$login = \Service_User::login($uname, $pword);
$this->assertTrue($login);
$this->users_to_clean[] = $user;
Expand All @@ -305,11 +329,14 @@ protected function _as_noauth()
{
require_once(APPPATH.'/tasks/admin.php');
\Fuel\Tasks\Admin::new_user($uname, 'test', 'd', 'noauth', '[email protected]', $pword);
// TODO: super_user should get all these rights inherently right??????!!!!
\Fuel\Tasks\Admin::give_user_role($uname, 'no_author');
$user = \Model_User::find_by_username($uname);
}
else
{
static::remove_all_roles_for_user($user->id);
}

\Materia\Perm_Manager::add_users_to_roles_system_only([$user->id], [\Materia\Perm_Role::NOAUTH]);
$login = \Service_User::login($uname, $pword);
$this->assertTrue($login);
$this->users_to_clean[] = $user;
Expand Down
25 changes: 13 additions & 12 deletions ...pp/modules/lti/classes/controller/lti.php → fuel/app/classes/controller/lti.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
* License outlined in licenses folder
*/

namespace Lti;

class Controller_Lti extends \Controller
{
use \Trait_Analytics;
Expand All @@ -20,7 +18,7 @@ public function before()
*/
public function action_index()
{
$cfg = \Config::get('lti::lti.consumers.default');
$cfg = \Config::get('lti.consumers.default');
// TODO: this is hard coded for Canvas, figure out if the request carries any info we can use to figure this out
$this->theme->set_template('partials/config_xml');
$this->theme->get_template()
Expand Down Expand Up @@ -58,8 +56,8 @@ public function action_login()
$this->theme->set_partial('content', 'partials/post_login');
$this->insert_analytics();

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


\Css::push_group('core');

Expand All @@ -77,8 +75,10 @@ public function action_picker(bool $authenticate = true)
$launch = LtiLaunch::from_request();
if ($authenticate && ! LtiUserManager::authenticate($launch)) return \Response::redirect('/lti/error/unknown_user');

$system = ucfirst(\Input::post('tool_consumer_info_product_family_code', 'this system'));
$is_selector_mode = \Input::post('selection_directive') === 'select_link' || \Input::post('lti_message_type') === 'ContentItemSelectionRequest';
$system = \Input::post('tool_consumer_info_product_family_code', 'this system');
$lti_message_type = \Input::post('lti_message_type', 'none');
$lti_key = \Input::post('oauth_consumer_key', '');
$is_selector_mode = \Input::post('selection_directive') === 'select_link' || $lti_message_type === 'ContentItemSelectionRequest';
$return_url = \Input::post('launch_presentation_return_url') ?? \Input::post('content_item_return_url');

\Materia\Log::profile(['action_picker', \Input::post('selection_directive'), $system, $is_selector_mode ? 'yes' : 'no', $return_url], 'lti');
Expand All @@ -89,17 +89,18 @@ public function action_picker(bool $authenticate = true)
\Js::push_inline('var BASE_URL = "'.\Uri::base().'";');
\Js::push_inline('var WIDGET_URL = "'.\Config::get('materia.urls.engines').'";');
\Js::push_inline('var STATIC_CROSSDOMAIN = "'.\Config::get('materia.urls.static').'";');
\Js::push_inline($this->theme->view('partials/select_item_js')
->set('system', $system));
\Css::push_group(['core', 'lti']);

\Js::push_inline('var LTI_MESSAGE_TYPE = "'.$lti_message_type.'"');
\Js::push_inline('var system = "'.htmlentities($system).'"');
\Js::push_inline('const LTI_KEY = "'.$lti_key.'"');
if ($is_selector_mode && ! empty($return_url))
{
\Js::push_inline('var RETURN_URL = "'.$return_url.'"');
}

\Css::push_group(['core', 'lti']);

$this->theme->get_template()
->set('title', 'Select a Widget for Use in '.$system)
->set('title', 'Select a Widget for Use in '.ucfirst($system))
->set('page_type', 'lti-select');

$this->theme->set_partial('content', 'partials/select_item');
Expand Down
3 changes: 1 addition & 2 deletions .../modules/lti/classes/controller/error.php → fuel/app/classes/controller/lti/error.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
* License outlined in licenses folder
*/

namespace Lti;

class Controller_Error extends \Controller
class Controller_Lti_Error extends \Controller
{
use \Trait_Analytics;
protected $_content_partial = 'partials/error_general';
Expand Down
Loading