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

Fix issues with LTI launch while logged in and course creation #6835

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

pretendWhale
Copy link
Contributor

@pretendWhale pretendWhale commented Nov 9, 2023

Motivation and Context

Two bugs were discovered with lti functionality:

  1. when launching LTI from canvas while logged in to MarkUs, users were not properly redirected to the choose_course path
  2. When attempting to create a new course, an uncaught error would occur if a course with the same name is already in the markus database.

Your Changes

Description:

  1. modified the login method to properly redirect logged-in users.
  2. Catch the case when the course already exists and display an error message if the user attempts to create a new course.

Type of change (select all that apply):

  • Bug fix (non-breaking change which fixes an issue)

Testing

Wrote tests for the changed methods, and tested in the web interface.

Questions and Comments (if applicable)

Checklist

  • I have performed a self-review of my own code.
  • I have verified that the pre-commit.ci checks have passed.
  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported on Coveralls.
  • I have added tests for my changes.

Pull request to make documentation changes (if applicable)

@coveralls
Copy link
Collaborator

coveralls commented Dec 15, 2023

Pull Request Test Coverage Report for Build 7463603247

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 91.679%

Totals Coverage Status
Change from base Build 7226755465: 0.1%
Covered Lines: 38757
Relevant Lines: 41653

💛 - Coveralls

@david-yz-liu david-yz-liu added this to the v2.4.2 milestone Jan 4, 2024
app/controllers/lti_deployments_controller.rb Outdated Show resolved Hide resolved
app/controllers/lti_deployments_controller.rb Outdated Show resolved Hide resolved
app/controllers/main_controller.rb Outdated Show resolved Hide resolved
@david-yz-liu david-yz-liu merged commit 405c787 into MarkUsProject:master Jan 9, 2024
6 checks passed
donny-wong pushed a commit to donny-wong/Markus that referenced this pull request Jan 12, 2024
* fix issue with lti launch while logged in and new course with existing name
* make lti routes available in production
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.

4 participants