Issue/662 Add API coverage for LTI resource links

[jsmnhou]

Proposed Changes

• Add support for LTI resource links
• Implement get_lti_resource_links(), get_lti_resource_link(), create_lti_resouce_link()
• Add associated tests/fixtures (see test_courses.py, lti_resource_link.json)

Fixes #662

Effects

• With these changes, all tests are passing and coverage report remains at 100%.

Note: Local testing proved difficult as I do not have access to a paid Canvas instance where I can generate dummy LTI resource links to view actual server responses (referencing here and here). Resorted to using mock responses to test, but should revisit with access to a paid Canvas account.

[jonespm]

Thanks for the contribution!

I tested out the 2 get methods and they both work fine!

I tested the create method without the custom hash and it also works. It looks like there's no test case for including custom.

Custom being incorrect does return this error

UnprocessableEntity: {"errors":[{"code":"invalid_custom","message":"'custom' param must contain key/value pairs"}]}

which all looks good. I tested with {"message": "Hello World"} and it works as expected.

It looks like Canvas added 3 more API's to this since the ticket was filed for Bulk Create, Update and Delete. It's fine if you don't have time to get to those and I'd take this as-is but we should get an issue filed to capture those additional tasks.

[jsmnhou]

@jonespm Thank you for the comments and for testing them out! Just fixed the import issue in my last commit, so hopefully the PR workflows should pass this time pending your next approval. I'll see to merging this PR first but I'll also create a new issue for the 3 additional endpoints and link this thread to that post for reference. Not sure how much time I'll have this weekend to get to them but I'll definitely take a stab at it!

Edit: New issue for additional endpoints linked here

[jsmnhou]

@jonespm Trying to debug this module import error but having some trouble locating where it's coming from. When I run scripts/find_missing_modules.py, it states that the lti_resource_link module is missing from inspect and needs to be imported (even indirectly) to __init__. However when I check the __init__, and consequently Canvas, imports, I'm confused why lti_resource_link isn't being recognized as an import since it's being imported in the course.py file. I tried consolidating the individual imports with the related functions in course.py into one overarching import at the top of the file (e.g. from canvasapi.lti_resource_link import LTIResourceLink), but this issue persists. Is there potentially something I'm understanding incorrectly regarding how imports work?

[jonespm]

Hmm, I'm not sure, checking this out

[jonespm]

Okay, I think the fix you suggested (removing the imports from in the function and moving it to the top) is needed and will likely get it to pass on Github Actions.

There's appears to be a bug in the test scripts/find_missing_modules.py where it won't run correctly if you have canvasapi installed globally or in a venv. I don't think it's installed on the Github process which makes me think it will pass there.

I can submit a new issue for a future fix for this if it does pass. I'd filed #675

diff --git a/canvasapi/course.py b/canvasapi/course.py
index 13467aa..f1d644f 100644
--- a/canvasapi/course.py
+++ b/canvasapi/course.py
@@ -23,6 +23,7 @@ from canvasapi.gradebook_history import (
 from canvasapi.grading_period import GradingPeriod
 from canvasapi.grading_standard import GradingStandard
 from canvasapi.license import License
+from canvasapi.lti_resource_link import LTIResourceLink
 from canvasapi.module import Module
 from canvasapi.new_quiz import NewQuiz
 from canvasapi.outcome_import import OutcomeImport
@@ -2771,8 +2772,6 @@ class Course(CanvasObject):
 
         :rtype: :class:`canvasapi.paginated_list.PaginatedList`
         """
-        from canvasapi.lti_resource_link import LTIResourceLink
-
         return PaginatedList(
             LTIResourceLink,
             self._requester,
@@ -2793,8 +2792,6 @@ class Course(CanvasObject):
 
         :rtype: :class:`canvasapi.lti_resource_link.LTIResourceLink`
         """
-        from canvasapi.lti_resource_link import LTIResourceLink
-
         lti_resource_link_id = obj_or_id(
             lti_resource_link, "lti_resource_link", (LTIResourceLink,)
         )
@@ -2824,8 +2821,6 @@ class Course(CanvasObject):
 
         :rtype: :class:`canvasapi.lti_resource_link.LTIResourceLink`
         """
-        from canvasapi.lti_resource_link import LTIResourceLink
-
         if not url:
             raise RequiredFieldMissing("The 'url' paramter is required.")

[jonespm]

Specifically I think the test (find_missing_modules.py) needs to insert rather than append. But it will only be a problem if other canvasapi's are in the path as well. I can file a issue for this.

sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))

[jsmnhou]

@jonespm Just pushed the fix you mentioned consolidating the imports to the top. Fingers crossed this works 🤞

[jonespm]

Okay, well that one does pass now. 3 more alphabetical order issues.

canvasapi.course.Course
-----------
create_module (canvasapi.course:442) came before create_lti_resource_link (canvasapi.course:2808)
get_lti_resource_links (canvasapi.course:2766) came before get_lti_resource_link (canvasapi.course:2784)
get_migration_systems (canvasapi.course:164[9](https://github.com/ucfopen/canvasapi/actions/runs/12243577619/job/34153541816?pr=673#step:12:10)) came before get_lti_resource_links (canvasapi.course:2766)

Found 3 method(s) not in alphabetical order. 💥

Guess these function names need to be in order too. It did seem to be nice to have them together but I guess that's not the expectation.

[jsmnhou]

@jonespm Reordered them! I ran scripts/alphabetic.py in previous commits so a bit confused why they weren't caught then, perhaps we should take another look at this script as well?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5e4f260) to head (e001286).
Report is 11 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop      #673   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           73        74    +1     
  Lines         3740      3761   +21     
=========================================
+ Hits          3740      3761   +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jsmnhou]

@jonespm My bad, I forgot to run coverage report after some recent changes. Let me add a few tests for these

[jonespm]

It looks like checking import sorting is separate from checking if module names are alphabetical. I'd say that's probably fine but the ordering of the checks should probably be moved around to keep the alphabetical checks together. Looks like all the tests pass now except codecov. Looks like a few cases aren't covered by tests.

[jsmnhou]

@jonespm Just pushed, sorry took a little longer than expected digging through other files to reference. Hopefully this works!

[jsmnhou]

@jonespm Damn.. I'm so sorry for the spam. Triple checked ./scripts/run_tests.sh so hopefully this is it

[jonespm]

Looks like all tests have passed! 🎉 It also looks like for first time contributors Github requires each run to be approved.

I'm fine merging but going to wait to see if @Thetwam has any comments or wants to click the button.

[Thetwam]

Thanks for your contribution - looks good! Just have a few minor tweaks for docs. Also need to add to the changelog and add you to authors.

I'll take care of these for ya and get this merged in. Thanks again!

[jsmnhou]

@Thetwam Sounds great, glad I could help! Please let me know if there is anything else I can do on my end. Thanks @jonespm for all your help!

[Thetwam]

Thanks again!