Prepare the library for being hosted on packagist #1

dbhynds · 2021-01-07T21:23:43Z

This PR is admittedly massive, and there's not really any way around it. I'll do my best to categorize the changes I've made:

Change the namespace from ImsGlobal to Packback\Lti1p3
Rename all classes to StudlyCase and methods to camelCase.
Various formatting changes for PSR-0, PSR-1, and PSR-4
Implemented interfaces core objects
Implemented automated tests
Updated the readme
Added a Laravel Implementation Guide

Code coverage is currently at 52%. Not great, but it's a lot better than 0. I also ported over our certification tests from PBQ and (with not a small amount of struggling) managed to get the tests implemented for IMS LTI 1.3 Certification.

There are several areas of the code marked with @todo, mostly revolving around testing. If I have time later this sprint, I'll come back to these, but I wanted to get something out there first.

I have a local branch on PBQ that references this one to verify at the very least that nothing I did here breaks anything over there. https://github.com/packbackbooks/questions/tree/lti-13

Add support for the groups service and submission review

…rary into updates

… into updates

Watercycle · 2021-01-07T22:12:14Z

README.md

-use \IMSGlobal\LTI;
+use Firebase\JWT\JWT;
+
+JWT::$leeway = 5;


As somebody not familiar with Firebase or the $leeway field, I have no clue what this bootstrapping is doing.

That makes two of us 😆

Watercycle · 2021-01-07T22:16:24Z

README.md

+- Packback\Lti1p3\Interfaces\Cookie
+- Packback\Lti1p3\Interfaces\Database
+
+Cache and Cookie storage have legacy implementations at Packback\Lti1p3\ImsStorage if you do not wish to implement your own. However you must implement your own database.


Just general thoughts here - are the legacy Packback\Lti1p3\ImsStorage used by default? I don't know what these are doing, but generally it's good for a package to start with "batteries included" and then explain more advanced usages like implementing your own storage mechanism.

They're not used by default. Honestly, I'm hesitant to have them used out of the box. They're not the most flexible and make a lot of assumptions about implementation. For example, Laravel's Cache and Cookie stores are completely incompatible with the IMS implementations. If you drop them into a fresh Laravel install and try to use them, it'll 500.

Watercycle · 2021-01-07T22:21:12Z

README.md


 If a lineitem with the same `tag` exists, that lineitem will be used, otherwise a new lineitem will be created.

+## Laravel Implementation Guide


Do we have any idea how other folks have used this package? Is it solely with Laravel or are there more general use cases? This README starts off hitting you with a lot of technical details. Which may make sense, but it's still pretty intimidating for someone with minimal LTI 1.3 experience.

No idea. I know there's at least one person using this branch, but have no idea who else might user it or how.

Any recommendations for how to make the readme more accessible to new users?

Mhmm, I don't have any practical suggestions.

It largely depends on target audience and how much effort we want to put into it. What comes to mind is either walking through integrating this with a fresh Laravel app or linking to another open-source repo demoing the already setup library. At least for me, it's hard to see how all of these pieces are tied together at first when all the technical details are laid out without runnable examples. Though, the newly added tests do help out here.

I say Laravel because most PHP folks are familiar with Laravel. It's easy to engineer things once you understand how they work. Starting with all the implementation details being generic is tricky to follow at first.

Watercycle · 2021-01-07T22:30:28Z

README.md

@@ -1,181 +1,196 @@
+# LTI 1.3 Advantage Library


This is nit-picking, but I don't see "Advantage" mentioned anywhere else in the project. Seems kind of random for the main page title.

LTI 1.3's full name is "LTI v1.3 and LTI Advantage" ... I'm not exactly sure what the terms mean, tbh. I think the "Advantage" part may refer to things like roster pull and gradebook sync, whereas the "1.3" part maybe refers to the actual launch process?

Watercycle · 2021-01-07T22:40:04Z

README.md

@@ -1,181 +1,196 @@
+# LTI 1.3 Advantage Library



Briefly skimming through the README, I'm not sure I understand what all this library offers to help get an app LTI 1.3 compatible. It's okay if the answer is no, but is there anything that can add to the overview to make the benefits of this package clearer at first glance?

EricTendian

Nice work. Besides the comments below, I noticed some other places where we were still using old constant names - run this package thru a linter and you'll be able to find more of them. (I just used PhpStorm)

src/Interfaces/Cache.php

src/LtiCourseGroupsService.php

src/LtiMessageLaunch.php

src/LtiAssignmentsGradesService.php

src/LtiServiceConnector.php

EricTendian

Noticed a few more issues and two things not addressed yet. (used PhpStorm for linting)

EricTendian · 2021-01-11T18:05:10Z

src/ImsStorage/ImsCache.php

    }

-    public function cacheLaunchData($key, $jwt_body)
+    public function cacheLaunchData($key, $jwtbody)


This case doesn't match the case in the interface.

EricTendian · 2021-01-11T18:13:27Z

src/LtiMessageLaunch.php

+            throw new LtiException('Missing Nonce');
+        }
+        if (!$this->cache->checkNonce($this->jwt['body']['nonce'])) {
+            //throw new LtiException("Invalid Nonce");


Should this be commented out?

It's commented out in the current branch that we're using. I checked the blame, and it's been commented out for as long as we've been using it. I'm not sure, but can look into it. Probably shouldn't be commented out, but who knows.

Ah, that's fine then - just looked weird.

Well, not sure why it was commented out, but added it back in successfully.

src/Redirect.php

src/Interfaces/LtiServiceConnectorInterface.php

src/LtiAssignmentsGradesService.php

dbhynds · 2021-01-12T15:24:58Z

Now that we're actually validating the Nonce, it's causing our LTI 1.3 cert suite to fail. Looking into why exactly.

dbhynds · 2021-01-12T16:51:06Z

Alright, everything passes. I also tested manual launching with LTI 1.3

EricTendian

Looks good, I just see one comment that still isn't addressed. https://github.com/packbackbooks/lti-1-3-php-library/pull/1/files#r555240560

EricTendian

Awesome work!

First commit of a pr workflow github action

MartinLenord and others added 30 commits June 3, 2020 13:09

add support for groups and submission review

5ee80e2

move regx to constant

b124206

Merge pull request #47 from IMSGlobal/groups_and_sub_review

3a192de

Add support for the groups service and submission review

Merge branch 'master' into master

218719f

rename constants

35e10f6

Merge branch 'master' of https://github.com/IMSGlobal/lti-1-3-php-lib…

955395a

…rary into updates

Merge branch 'master' of https://github.com/dbhynds/lti-1-3-php-library…

00d8049

… into updates

Rename everything to have name schemes appropriate for autoloading

4e53ba8

rename all method names according psr-1

68502e1

Add automated tests

d724dbe

fix indendation

fccb497

Import interfaces

0dbf00f

Add basic tests that just instantiate objects and are currently failing

d8e0de8

Fix tests

6b00be4

Add a registration interface

2d12f26

Fully test jwks and tidy up services

c2fb951

Rename Namespace

07cd970

fix method call

8e1bcc5

fix missing import

1d3d5ba

fix find/replace error

88f6c5e

add more robust tests

be90976

add more tests

8f7649b

add more tests

3566b0a

add more tests

a772cf1

add more tests

8d2478f

add more tests

842c3fa

add more tests

8203a2b

add more tests

ffac0fb

implement certification suite tests

eb1350b

fix a few references to the class name instead of LTI

7589144

Watercycle reviewed Jan 7, 2021

View reviewed changes

dbhynds added 5 commits January 7, 2021 16:51

Update comments

30a8efe

Update readme

181a72e

Add one more constant

ff0254e

update readme

1f3ca6e

update readme

20e8b61

EricTendian suggested changes Jan 8, 2021

View reviewed changes

dbhynds added 2 commits January 8, 2021 15:46

PR Feedback

02447b4

Add sub roles

08b4dbf

dbhynds requested review from EricTendian and Watercycle January 8, 2021 21:57

EricTendian suggested changes Jan 11, 2021

View reviewed changes

PR Feedback

f2aca27

EricTendian self-assigned this Jan 11, 2021

dbhynds added 2 commits January 11, 2021 16:31

Throw exception

7587b4e

Fix tests

50b454c

dbhynds requested a review from EricTendian January 12, 2021 16:51

EricTendian reviewed Jan 13, 2021

View reviewed changes

Fix caps typo

583bf2d

dbhynds requested a review from EricTendian January 13, 2021 16:36

EricTendian approved these changes Jan 13, 2021

View reviewed changes

dbhynds merged commit 879addb into master Jan 13, 2021

dbhynds deleted the updates branch January 13, 2021 16:50

pbcraig pushed a commit that referenced this pull request May 25, 2021

Merge pull request #1 from craigmbooth/add-ci

75f919b

First commit of a pr workflow github action


		If a lineitem with the same `tag` exists, that lineitem will be used, otherwise a new lineitem will be created.

		## Laravel Implementation Guide

Prepare the library for being hosted on packagist #1

Prepare the library for being hosted on packagist #1

Uh oh!

Conversation

dbhynds commented Jan 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbhynds Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Watercycle Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Watercycle Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Watercycle Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EricTendian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EricTendian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbhynds commented Jan 12, 2021

Uh oh!

dbhynds commented Jan 12, 2021

Uh oh!

EricTendian left a comment

Choose a reason for hiding this comment

Uh oh!

EricTendian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbhynds Jan 7, 2021 •

edited

Loading

Watercycle Jan 7, 2021 •

edited

Loading

Watercycle Jan 7, 2021 •

edited

Loading

Watercycle Jan 7, 2021 •

edited

Loading