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 alternative cookie messages in UI (iss. #107) #109

Merged
merged 7 commits into from
Jun 25, 2024

Conversation

lsloan
Copy link
Contributor

@lsloan lsloan commented Jun 24, 2024

Resolves #107.

I noticed that OIDCLoginInitView's inherited method View.as_view(), being called from an @tl-its-umich-edu LTI project, accepted a list of various keyword arguments. I experimented with that and learned that by defining the arguments as attributes of OIDCLoginInitView, they would be accepted and could later be accessed as instance properties where DjangoOIDCLogin.enable_check_cookies() is called.

This could be used as…

    path("init/<uuid:registration_uuid/", OIDCLoginInitView.as_view(
        main_msg=(
            "Your browser prevents embedded content from using cookies.  To work "
            "around this, the content must be opened in a new tab or window.  "
        ),
        click_msg="Open a new tab or window now.",
        loading_msg="Loading..."
    ), name="init")

This could be used as…

```python
    path("init/<uuid:registration_uuid>/", OIDCLoginInitView.as_view(
        main_msg='Your browser prevents embedded content from using cookies.',
        click_msg='Instead, you must open the content in a new tab.',
        loading_msg='Loading…'
    ), name="init"),
```
@lsloan
Copy link
Contributor Author

lsloan commented Jun 24, 2024

@michaelwheeler and @jonespm: This is what I thought would be a simple implementation to support other messages, but requiring minimal changes to code that would use it.

I've not yet checked the code with nox as suggested, but I will. What do you think? Is this approach good enough to mark this PR ready for review?

@michaelwheeler
Copy link
Collaborator

@lsloan Thanks for the PR! What would you think about providing initial values for the three new properties of OIDCLoginInitView, rather than having them as Optionals? By default maybe OIDCLoginInitView could just use grammar-corrected versions of what pylti1p3 uses.

https://github.com/dmitry-viskov/pylti1.3/blob/d8fa43e1ace2885e84dfa4067f0f7379aaa86826/pylti1p3/oidc_login.py#L35-L41

Documenting this in https://github.com/academic-innovation/django-lti/blob/main/docs/oidc_initiation.rst would also be great.

After running `nox`, `black` said a blank line was needed.  After I looked, I decided a comment would be helpful, too.
@lsloan
Copy link
Contributor Author

lsloan commented Jun 24, 2024

Yes, I can provide defaults. I thought of that, but it wasn't necessary, so I just thought I'd wait and see what others had to say.

I'm happy to document as well.

I'll mark this PR as ready for review, since we're starting to do that anyway. 😉

@lsloan lsloan marked this pull request as ready for review June 24, 2024 18:26
@lsloan
Copy link
Contributor Author

lsloan commented Jun 24, 2024

@michaelwheeler, I've committed changes with better default messages and documentation. Tell me if you see anything else that needs to be added or formatting improved.

I think I've handled the things nox warned about. The output from flake8 was a huge mess because it checked everything in my .venv directory. 🙄

Copy link
Collaborator

@michaelwheeler michaelwheeler left a comment

Choose a reason for hiding this comment

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

Thanks @lsloan! Just a few items to address before we can get this merged.

lti_tool/views.py Outdated Show resolved Hide resolved
lti_tool/views.py Outdated Show resolved Hide resolved
docs/oidc_initiation.rst Show resolved Hide resolved
docs/oidc_initiation.rst Show resolved Hide resolved
@lsloan lsloan requested a review from michaelwheeler June 25, 2024 17:54
Copy link
Collaborator

@michaelwheeler michaelwheeler left a comment

Choose a reason for hiding this comment

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

Thanks @lsloan I think we're all set here. Really appreciate the contribution!

docs/oidc_initiation.rst Show resolved Hide resolved
@lsloan
Copy link
Contributor Author

lsloan commented Jun 25, 2024

@michaelwheeler, will you merge this soon or are you waiting for a specific time?

@michaelwheeler michaelwheeler merged commit 3eb1afd into academic-innovation:main Jun 25, 2024
17 checks passed
@michaelwheeler
Copy link
Collaborator

Sorry @lsloan! Was waiting for the checks to pass and got distracted with other things!

@lsloan
Copy link
Contributor Author

lsloan commented Jun 25, 2024

@michaelwheeler, no problem. Please tell me whenever a pip-installable release is available. @tl-its-umich-edu will be happy to see the changes in a current project.

Thanks! :shipit:

@lsloan lsloan deleted the 107-cookie-messages branch June 26, 2024 19:22
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.

prevent poor grammar: override some pylti1p3 messages
2 participants