search

academic-innovation / django-lti

LTI Advantage support for Django projects.
https://academic-innovation.github.io/django-lti/
MIT License
15 stars 7 forks source link

prevent poor grammar: override some `pylti1p3` messages #107

Closed lsloan closed 5 months ago

lsloan commented 5 months ago

When using django-lti, I've seen messages like this in the UI…

Your browser prohibits to save cookies in the iframes. Click here to open content in the new tab.

(Note: I'm not blaming this module for the messages, I know they come from https://github.com/dmitry-viskov/pylti1.3.)

I'm embarrassed to let users see that, because they may think I wrote those messages with bad grammar and technobabble. I'd rather it give messages like…

Your browser disallows embedded content to use cookies. Instead, you must open the content in a new tab.

It's a little better grammar, less technical, and avoids the "click here" type of link.

I thought of opening a PR to https://github.com/dmitry-viskov/pylti1.3 to correct those messages, but I see it hasn't been updated in two years and has a backlog of about 13 PRs. So, I think the next best approach is to appeal to you to provide some way to pass messages in.

According to the pylti1.3 documentation, its enable_check_cookies method supports message arguments…

oidc_login.enable_check_cookies(main_msg, click_msg, loading_msg)

Where main_msg and click_msg correspond to the two sentences in the messages I quoted above. (I understand that my hope to move the link in the second sentence may not be feasible.)

I see that method is called in your lti_tool.views.OIDCLoginInitView.get_oidc_response() method…

https://github.com/academic-innovation/django-lti/blob/afae63068a6b2ac82b512819e9b08928ddd8223a/lti_tool/views.py#L55

Could the OIDCLoginInitView class be updated to use a constructor that accepts optional message arguments? Or could the messages be provided as parameters, for example, as target_link_uri is?

michaelwheeler commented 5 months ago

This seems totally reasonable.

Or could the messages be provided as parameters, for example, as target_link_uri is?

I'd avoid this approach, since the params here are specific to the OIDC third-party initiation workflow.

Maybe something similar to get_redirect_url() would work? Possibly coupled with an updated class constructor?

https://github.com/academic-innovation/django-lti/blob/afae63068a6b2ac82b512819e9b08928ddd8223a/lti_tool/views.py#L41-L43

cmurtaugh commented 5 months ago

I think this also raises a larger concern about the future of pylti1p3. Since it's a key dependency of this project I worry about it essentially going unmaintained. I wonder if the original maintainer would be amenable to turning it over or if it should be forked.

michaelwheeler commented 5 months ago

I think this also raises a larger concern about the future of pylti1p3. Since it's a key dependency of this project I worry about it essentially going unmaintained. I wonder if the original maintainer would be amenable to turning it over or if it should be forked.

This is a good point that has come up before. I've just created #108 as a place for suggestions and discussion.