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

P20-848: Add error page for unsupported LTI message type #58024

Merged
merged 2 commits into from Apr 16, 2024

Conversation

artem-vavilov
Copy link
Contributor

Screenshot

lti-auth-error-page

Links

@artem-vavilov artem-vavilov requested a review from a team as a code owner April 16, 2024 00:28
Copy link
Contributor

Choose a reason for hiding this comment

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

I see changes in Portuguese-PR and Turkish, but we can't see the changes due to Git LFS. Would you mind summing up what changes in those files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#lti-unsupported-message-type.error-page
.error-parent
.error-child
= image_tag '/shared/images/sad-bee-avatar.png', alt: t('lti.error.unsupported_message_type')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use an empty alt text for the sad bee since:

  1. The image does NOT contain text.
  2. The image is NOT used in a link or a button.
  3. The image does NOT contribute meaning to the current page or context.
  4. The alt text is used in the title below.

Given those cases, I would consider the image decorative.

Copy link
Contributor

@mgc1194 mgc1194 left a comment

Choose a reason for hiding this comment

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

I left a couple of non-blocking comments.
Thanks for these improvements.

Copy link
Contributor

@carl-codeorg carl-codeorg left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -692,6 +692,8 @@ module SharedConstants
INTEGRATION_EARLY_ACCESS_URL: 'https://docs.google.com/forms/d/e/1FAIpQLScjfVR4CZs8Utf5vI4mz3e1q8vdH6RNIgTUWygZXN0oovBSQg/viewform',
INTEGRATION_BUG_REPORT_URL: 'https://support.code.org/hc/en-us/requests/new?ticket_form_id=14998494738829&tf_23889708=lms_eaf',
ADDITIONAL_FEEDBACK_URL: 'https://studio.code.org/form/lms_integration_modal_feedback',
# TODO(P20-873): Replace SUPPORTED_METHODS_URL with the link to the supported methods documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for creating the jira and leaving a reminder

@@ -7,6 +7,13 @@ module AccessTokenScopes
CONTEXT_MEMBERSHIP = 'https://purl.imsglobal.org/spec/lti-nrps/scope/contextmembership.readonly'.freeze
end

module MessageType
CLAIM = :'https://purl.imsglobal.org/spec/lti/claim/message_type'
SUPPORTED = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, should be easy to add deep linking message type support later.

@artem-vavilov artem-vavilov merged commit ab089c0 into staging Apr 16, 2024
2 checks passed
@artem-vavilov artem-vavilov deleted the P20-848/better-lti-auth-error branch April 16, 2024 17:06
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.

None yet

3 participants