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

Support editing assignment from launch error dialog #5163

Merged
merged 4 commits into from
Mar 16, 2023

Conversation

robertknight
Copy link
Member

@robertknight robertknight commented Mar 8, 2023

Add an "Edit assignment" link to the launch error dialog, if the user is an instructor and assignment editing is enabled. This makes it possible to fix certain errors by editing the assignment, when previously it would have required re-creating the assignment.

Edit assignment link in LaunchErrorDialog

See #5163 (comment) for testing notes.

@robertknight
Copy link
Member Author

We discussed the UI design in Slack and have consensus to add an "Edit assignment" button to this dialog, which is always present if the user is an instructor. This should

  • Provide a more obvious call-to-action when appropriate
  • Avoid potential problems where a developer adds a new error message to this component but forgets to add a link to the edit assignment view

@robertknight
Copy link
Member Author

robertknight commented Mar 10, 2023

Following some discussion, the UI has been changed to expose the option to edit an assignment via a link at the bottom of the dialog, which is always available. This makes the behavior more predictable, regardless of what error occurred.

Edit assignment v2

To test the dialog, you can either configure a new assignment with an invalid configuration (eg. pick an empty group set) or apply the diff below to trigger the dialog on any assignment launch:

diff --git a/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx b/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx
index ae14a625..3e06b9f3 100644
--- a/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx
+++ b/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx
@@ -177,6 +177,9 @@ export default function BasicLTILaunchApp() {
    * Fetch the URL of the content to display in the iframe
    */
   const fetchContentURL = useCallback(async () => {
+    setErrorState('error-fetching');
+    return false;
+
     if (!viaAPICallInfo) {
       // If no "callback" URL was supplied for the frontend to use to fetch
       // the URL, then the backend must have provided the Via URL in the

@lyzadanger
Copy link
Contributor

Following some discussion, the UI has been changed to expose the option to edit an assignment via a link at the bottom of the dialog, which is always available. This makes the behavior more predictable, regardless of what error occurred.

This UI is looking better to me! Perhaps underline the "Edit assignment"?

Base automatically changed from launch-error-common-props to main March 10, 2023 14:29
@robertknight robertknight force-pushed the launch-error-edit-links branch 2 times, most recently from cd750b7 to b6f7f1f Compare March 10, 2023 15:51
This can be used to add additional actions beyond "Cancel" and "Try
again" to the dialog.
If the current user is an instructor, and the LMS supports it, show an action to
edit the assignment on the LTI launch error screen. This is useful for cases
where an instructor can fix the assignment by changing its configuration.
@robertknight robertknight marked this pull request as ready for review March 10, 2023 15:57
/**
* Content of the dialog. This should be a human-readable explanation of the
* problem that occurred and may include hints on how to fix it.
*/
children?: ComponentChildren;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added comment is unrelated to the other changes in this PR.

@@ -78,6 +92,7 @@ export default function ErrorModal({
{cancelLabel}
</Button>
)}
{extraActions}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extraActions here is rendered, when present, after any cancel button, when present. Are there situations in which we could end up with:

<cancel button> [Edit assignment] <retry button>

or

<cancel button> [Edit assignment]

? Perhaps extraActions could come first? Also perhaps you have already thought through these cases...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the current use case in LaunchErrorDialog, there is no Cancel option, so the issue doesn't arise. I am not sure which will make the most sense for future use cases. When "Edit assignment" had a more button-like appearance it seemed sensible to put it next to the primary action. With the revised link-like appearance, that makes less sense. I'll move it to the left of "Cancel" for now, but I expect we might need to revisit in future.

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but it's worth addressing Lyza's comment, I think: https://github.com/hypothesis/lms/pull/5163/files#r1137071669

Following discussion in [1], we think putting additional actions to the left of
OK/Cancel makes sense currently, although for the use case of the initial use in
`LaunchErrorDialog` there is no Cancel button in any case.

[1] #5163 (comment)
@robertknight
Copy link
Member Author

Thanks for the feedback. I have moved extraActions to the left of the OK/Cancel buttons in the dialog for the moment, although this doesn't affect this particular dialog as there is no "Cancel" action available. OK for me to get this merged @acelaya?

@acelaya
Copy link
Contributor

acelaya commented Mar 16, 2023

OK for me to get this merged @acelaya?

@robertknight yes! Go for it

@robertknight robertknight merged commit 4533ae6 into main Mar 16, 2023
@robertknight robertknight deleted the launch-error-edit-links branch March 16, 2023 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants