Converts LTI views to React #1415

cayb0rg · 2022-11-08T21:55:57Z

Changes

Converts LTI views to React
Removes jquery from test_provider and test_learner pages
Updates deprecated assertContains to assertContainsEquals in test functions

clpetersonucf

Some high level design comments:

It looks like you're trying to re-implement get_owners and request_access in the v1 API, but those API requests actually already exist - they're just not in v1. I know, it's confusing. post_request_access is a method in fuel/app/classes/controller/api/instance.php. The API in Materia is currently not great: older API requests are in v1, and newer API requests are in their respective API controller. It's something that has to be rebuilt from the ground up in the future python rewrite. get_owners probably isn't even required, the ownership is assessed in the LTI preview controller, and we're better off emitting that flag to the window like other JS constants in other React views.
Generally, try and avoid making changes to any of the old (or creating new) stylesheets in src/css. My goal is to kill these completely, and implement CSS in one of two places: a common stylesheet (include.scss) in the components directory, or a component-specific stylesheet in that same directory. With several LTI components making use of similar styles, it'd probably make sense for them all to inherit the same stylesheet.
I noticed you made a change to the post_login.php partial under themes/default/lti/partials. That's also part of the deprecated file structure, as those views are all being removed. Anything you're changing there should be changed in the corresponding components instead. The individual React views and their components are replacing the old PHP partials.

…t from certain tests

…cess to api/json request

clpetersonucf · 2023-03-13T20:20:19Z

Updated with changes to the main react branch. Something I'm noticing: the LTI picker isn't loading any widgets, which I suspect is due to pagination in the widget_paginate_instances_get, but haven't looked at it in detail.

clpetersonucf

The diff is looking better, but I left some additional feedback items below.

The test suite is failing consistently:

There was 1 failure:

1) Test_Api_V1::test_widget_instance_lock_for_another_user
Failed asserting that false is true.

/var/www/html/fuel/app/tests/api/v1.php:496

The only other thing I'm noticing is that attempting to test LTI Assignment Launch in the test provider yields the message LTI Assignment URL can not be blank! even when the the assignment URL is populated.

clpetersonucf · 2023-03-22T14:41:49Z

fuel/app/classes/materia/api/v1.php

@@ -1185,4 +1185,28 @@ static private function _get_instance_for_play_id($play_id)
 		if ( ! ($inst = Widget_Instance_Manager::get($inst_id))) throw new \HttpNotFoundException;
 		return $inst;
 	}
+
+	// Sends a notification to user requesting access to instance
+	static public function request_access(string $inst_id, int $owner_id) 


This API call is unnecessary, newer API endpoints are in the API controllers. post_request_access is in fuel/app/classes/controller/api/instance.php.

clpetersonucf · 2023-03-22T14:44:43Z

fuel/app/modules/lti/classes/controller/lti.php

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

 		if ($is_selector_mode && ! empty($return_url))
 		{
 			\Js::push_inline('var RETURN_URL = "'.$return_url.'"');
 		}
+		$this->insert_analytics();


I would double-check that these are necessary: insert_analytics is in the commoncontrollertemplate trait, so it may already be included.

clpetersonucf · 2023-03-22T14:50:38Z

src/util/api.js

+
+// Request access to widget
+export const apiRequestAccess = (instId, ownerId) => {
+	return fetch('/api/json/request_access', fetchOptions({ body: `data=${formatFetchBody([instId, ownerId])}` }))


You'll have to update this endpoint to api/instance/request_access to switch over from the api/v1 gateway to the instance API controller. You can look at other api/instance (or any other request besides api/json/...) to see if anything else has to be restructured for the request.

clpetersonucf · 2023-03-22T14:57:06Z

The only other thing I'm noticing is that attempting to test LTI Assignment Launch in the test provider yields the message LTI Assignment URL can not be blank! even when the the assignment URL is populated.

Update: this is probably because the test provider was relying on jquery for basic functionality, which is no longer bundled with Materia. We can probably update the page to use a CDN link for jQuery. I'd rather not bundle it with Materia's JS and CSS assets.

FrenjaminBanklin

It looks like the LTI-relevant XML files got caught up in sweeping deletions, but those should probably be restored.

There are a few spots I noticed that could use some cleanup re: extraneous white space and removing anything left over from debugging.

The LTI test provider's behavior seems a bit unpredictable, but I haven't really been able to nail down a pattern. Buttons that should be sending requests with bad OAuth signatures appear not to be causing any problems when they probably should, for example. Since so much is being changed, this may also be a good time to add the means to make additional checks - loading a widget as an instructor who does not own it, loading a widget in guest mode, loading an unknown assignment, etc.

Also, though it's minor, it looks like the course navigation screen's appearance is different. The two columns should be taking up the same amount of vertical space, but one is taller than the other.

FrenjaminBanklin · 2023-03-28T19:43:05Z

fuel/app/classes/materia/score/manager.php

@@ -145,6 +145,7 @@ static public function get_widget_score_distribution($inst_id)

 		// we have to post process this query O(n)
 		$semesters = [];
+		\Log::Error(count($result));


Debug leftovers?

FrenjaminBanklin · 2023-03-28T21:18:33Z

fuel/app/modules/lti/classes/controller/lti.php

-
-		$this->insert_analytics();
+		$instance_owner_list = $current_user_owns ? [] : (array_map(function ($object)
+		{ 


Try to clean up any trailing whitespace.

FrenjaminBanklin · 2023-03-28T21:19:16Z

fuel/app/modules/lti/classes/controller/lti.php

-		$this->theme->set_partial('content', 'partials/select_item');
-		$this->theme->set_partial('header', 'partials/header_empty');
-		$this->insert_analytics();
+


Try to clean up any trailing whitespace.

FrenjaminBanklin · 2023-03-28T21:19:24Z

fuel/app/modules/lti/classes/controller/lti.php

+		$this->theme->get_template()
+			->set('title', 'Widget Connected Successfully')
+			->set('page_type', 'preview');
+


Try to clean up any trailing whitespace.

FrenjaminBanklin · 2023-03-28T21:21:22Z

fuel/app/tests/api/v1.php

@@ -1564,4 +1569,4 @@ protected function assert_validation_error_message($msg)
 		$this->assertInstanceOf('\Materia\Msg', $msg);
 		$this->assertEquals('Validation Error', $msg->title);
 	}
-}
+}


Trailing new line is good practice.

FrenjaminBanklin · 2023-03-28T21:38:50Z

src/components/lti/select-item.jsx

+
+    let sectionRender = null
+    if (displayState == 'selectInstance') {
+        sectionRender = 


Try to clean up trailing whitespace.

FrenjaminBanklin · 2023-03-28T21:39:06Z

src/components/lti/select-item.jsx

+                // redirect the client to the return url with our new variables
+                window.location = `${window.RETURN_URL}${separator}embed_type=basic_lti&url=${url}`
+            }
+        } 


Try to clean up trailing whitespace.

FrenjaminBanklin · 2023-03-28T21:39:12Z

src/components/lti/select-item.jsx

+            let pgSpan = document.querySelector('.progress-container span')
+            pg.classList.add('success')
+            pgSpan.innerText = 'Success!'
+


Try to clean up trailing whitespace.

FrenjaminBanklin · 2023-03-28T21:39:22Z

src/components/lti/select-item.jsx

+    const { data, isFetching: isFetching, refetch: refetchInstances} = useQuery({
+		queryKey: 'instances',
+		queryFn: () => apiGetWidgetInstances(state.page),
+        staleTime: Infinity, 


Try to clean up trailing whitespace.

FrenjaminBanklin · 2023-03-28T21:42:41Z

fuel/app/themes/default/lti/partials/outcomes_xml.php

This seems like it's still being loaded as part of the on_score_updated_event LTI event. Is it being replaced with something? If not, restore this file.

…moves trailing whitespaces

clpetersonucf

I think this is good. The new components should be styled to match the new design language, but I can take care of that on the base react branch. I don't see any obvious red flags, and the bad oauth signature validation issue is also present on master, having to do with PHP short open tags on the dynamically generated forms.

…tch react design language. A few random fixes and tweaks.

Converts navigation launch, picker launch, and error lti views to React

f93a9bf

clpetersonucf requested changes Jan 20, 2023

View reviewed changes

cayb0rg added 4 commits January 24, 2023 10:50

Merge React Branch & revert changes to tests

58a8c96

Remove get_owners and request_access from v1.php and remove (int) cas…

3336f38

…t from certain tests

Remove LTI partials and other unnecessary functions

71de222

Passes instance_owner_list as JS global variable and moves request_ac…

ad59ed4

…cess to api/json request

clpetersonucf mentioned this pull request Feb 17, 2023

React conversion + Support Dashboard #1359

Merged

Merge branch 'issue/support-dashboard-in-react' into issue/LTI-React

24c007c

cayb0rg and others added 2 commits March 21, 2023 10:45

Fixes widgets not appearing in LTI picker

a628810

Merge branch 'issue/support-dashboard-in-react' into issue/LTI-React

c38ceaa

clpetersonucf requested changes Mar 22, 2023

View reviewed changes

clpetersonucf reviewed Mar 22, 2023

View reviewed changes

clpetersonucf added the React Branch Related to the React rewrite for Materia label Mar 22, 2023

cayb0rg added 2 commits March 28, 2023 13:22

Removes jquery, changes API endpoint, and uses commoncontrollertemplate

654319e

Updates assertContains to assertContainsEquals

f343a53

FrenjaminBanklin requested changes Mar 28, 2023

View reviewed changes

cayb0rg and others added 8 commits March 29, 2023 13:19

Fixes invalid oauth requests not failing, updates error view page, re…

2447bed

…moves trailing whitespaces

Tests post_request_access

4700606

Update instance.php

88e15cc

Update manager.php

88aaca7

Update config_xml.php

e5f2f1c

Update error-general.jsx

5bea1d2

Adds newline

5ba0bc9

Adds newline

6d7b12a

clpetersonucf approved these changes Apr 7, 2023

View reviewed changes

Resolving merge conflict and updating with changes on main react branch

e12e928

Heavy CSS reworks of most LTI views. Tweaked styles & behaviors to ma…

4e79944

…tch react design language. A few random fixes and tweaks.

clpetersonucf approved these changes Apr 11, 2023

View reviewed changes

clpetersonucf merged commit dfb116c into ucfopen:issue/support-dashboard-in-react Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converts LTI views to React #1415

Converts LTI views to React #1415

cayb0rg commented Nov 8, 2022 •

edited

Loading

clpetersonucf left a comment

clpetersonucf commented Mar 13, 2023

clpetersonucf left a comment •

edited

Loading

clpetersonucf Mar 22, 2023

clpetersonucf Mar 22, 2023

clpetersonucf Mar 22, 2023

clpetersonucf commented Mar 22, 2023

FrenjaminBanklin left a comment

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

FrenjaminBanklin Mar 28, 2023

clpetersonucf left a comment

Converts LTI views to React #1415

Converts LTI views to React #1415

Conversation

cayb0rg commented Nov 8, 2022 • edited Loading

clpetersonucf left a comment

Choose a reason for hiding this comment

clpetersonucf commented Mar 13, 2023

clpetersonucf left a comment • 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

clpetersonucf commented Mar 22, 2023

FrenjaminBanklin 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

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

clpetersonucf left a comment

Choose a reason for hiding this comment

cayb0rg commented Nov 8, 2022 •

edited

Loading

clpetersonucf left a comment •

edited

Loading