search

instructure / canvas-lms

The open LMS by Instructure, Inc.
https://github.com/instructure/canvas-lms/wiki
GNU Affero General Public License v3.0
5.52k stars 2.45k forks source link

Raise error when QTI Migration Tool is missing #2057

Closed LightAndLight closed 2 years ago

LightAndLight commented 2 years ago

On a system where the QTI Migration Tool is not installed, Canvas still tries to run it.

In this situation @migration_executable is nil, so Qti.get_conversion_command returns a bogus command string. This leads to unhelpful error messages in run_qti_converter. The error cases in said function assume that a valid command failed (and therefore look to the command's standard output for information about the failure).

When @migration_executable is nil, raising the error in Qti.get_conversion_command tells user that they haven't installed the tool.

This might make it easier to troubleshoot issues like #1436.

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

LightAndLight commented 2 years ago

bump

LightAndLight commented 2 years ago

Hi @slaughter550, how can I get this merged?

LightAndLight commented 2 years ago

It's important to me that I have no dangling pull requests. I'll bump this again in two weeks, and close it after four weeks if we don't make any progress on merging.

slaughter550 commented 2 years ago

@LightAndLight I brought this into our internal system for review and have assigned the appropriate team. I will come back shortly with feedback.

LightAndLight commented 2 years ago

Thanks!

LightAndLight commented 2 years ago

https://github.com/instructure/canvas-lms/pull/2057#issuecomment-1160977556:

... I'll bump this again in two weeks, and close it after four weeks if we don't make any progress on merging.

Closing because it's been four weeks.

slaughter550 commented 2 years ago

@LightAndLight this was merged - https://github.com/instructure/canvas-lms/commit/d1aa3366ef8a7195446dc4e34dc93b1530012208

LightAndLight commented 2 years ago

Cheers!

On Fri, 19 Aug 2022, 6:38 pm Alex Slaughter, @.***> wrote:

@LightAndLight https://github.com/LightAndLight this was merged

— Reply to this email directly, view it on GitHub https://github.com/instructure/canvas-lms/pull/2057#issuecomment-1220406506, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATLFOMMQS432XHC7PRILMTVZ5BX5ANCNFSM5VQHRPTA . You are receiving this because you were mentioned.Message ID: @.***>