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

Optional terms of use dialog for user #825

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

schuettloeffel-elsa
Copy link
Contributor

@schuettloeffel-elsa schuettloeffel-elsa commented Jul 9, 2024

closes #296

When using the admin frontend for more than a few admins (e.g. lecturers that upload videos directly from the admin ui), there has to be a way to supply a terms of use form on first login. The displayed text should off course be configurable and the user would have to accept the terms to continue.

Copy link
Contributor

github-actions bot commented Jul 9, 2024

Use docker or podman to test this pull request locally.

Run test server using develop.opencast.org as backend:

podman run --rm -it -p 127.0.0.1:3000:3000 ghcr.io/opencast/opencast-admin-interface:pr-825

Specify a different backend like stable.opencast.org:

podman run --rm -it -p 127.0.0.1:3000:3000 -e PROXY_TARGET=https://stable.opencast.org ghcr.io/opencast/opencast-admin-interface:pr-825

It may take a few seconds for the interface to spin up.
It will then be available at http://127.0.0.1:3000.
For more options you can pass on to the proxy, take a look at the README.md.

Copy link
Contributor

github-actions bot commented Jul 9, 2024

This pull request is deployed at test.admin-interface.opencast.org/825/2024-10-21_08-07-12/ .
It might take a few minutes for it to become available.

@schuettloeffel-elsa
Copy link
Contributor Author

Backend PR: opencast/opencast#6010

@mtneug
Copy link
Member

mtneug commented Jul 9, 2024

@schuettloeffel-elsa, thanks for this PR. Linking terms of use makes sense. However, I'm wondering, since we now have a couple of links in the footer, if it would be better to have a generic way of putting links there. Something like a configuration map containing the link name and URL. Opinions?

@schuettloeffel-elsa
Copy link
Contributor Author

Linking terms of use makes sense. However, I'm wondering, since we now have a couple of links in the footer, if it would be better to have a generic way of putting links there. Something like a configuration map containing the link name and URL. Opinions?

I dont really get what you mean, this PR doesnt provide any links in the footer, it just displays the terms if a user hasnt already accepted them. Or am I missing something here?

@mtneug
Copy link
Member

mtneug commented Jul 10, 2024

I dont really get what you mean, this PR doesnt provide any links in the footer, it just displays the terms if a user hasnt already accepted them. Or am I missing something here?

Oh, my bad! I somehow assumed that this would add a "Terms of Use" link in the footer of the Admin UI similar to #306.

@schuettloeffel-elsa
Copy link
Contributor Author

No worries :)

Copy link
Member

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

I didn't do a full review. This is more just a few comments.

src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Show resolved Hide resolved
@lkiesow
Copy link
Member

lkiesow commented Jul 11, 2024

One more request I forgot and which also applies to opencast/opencast#6010

  • It would be great if you could give your pull request a description of what the problem is, what you tried to achieve and if there is anything special about the way you fixed the problem. Even if you linked an issue, it's somewhat cumbersome for reviewers to always try to get information from somewhere else. Also, things can change, and you may get additional insights during the implementation. Definitely not the end of the world, but you will make reviewers happy.
  • The pull requests titles will become part of the changelog. So it's great if the title describes what's happening. And this is no longer a feature request ;-P

@lkiesow lkiesow changed the title Feature request: terms of use for new user Optional terms of use dialog for user Jul 11, 2024
@schuettloeffel-elsa
Copy link
Contributor Author

One more request I forgot and which also applies to opencast/opencast#6010

  • It would be great if you could give your pull request a description of what the problem is, what you tried to achieve and if there is anything special about the way you fixed the problem. Even if you linked an issue, it's somewhat cumbersome for reviewers to always try to get information from somewhere else. Also, things can change, and you may get additional insights during the implementation. Definitely not the end of the world, but you will make reviewers happy.

Sure! From other projects I know the workflow to just link to the issue (like I die here) and always retrieve the informations there, just to prevent doubled informations in the issue and PR. But if it helps I will give some informations in the PR as well in the future :)

  • The pull requests titles will become part of the changelog. So it's great if the title describes what's happening. And this is no longer a feature request ;-P

Haha, will do, thanks for editing

Copy link
Member

@JulianKniephoff JulianKniephoff left a comment

Choose a reason for hiding this comment

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

Apart from the inline comments:

  • We should really try to not add any more ts-expect-error and eslint-disable comments.

It would be nice if you could include a screenshot, as well.

.gitignore Outdated Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
src/components/Header.tsx Outdated Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Show resolved Hide resolved
src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
Copy link
Member

@JulianKniephoff JulianKniephoff left a comment

Choose a reason for hiding this comment

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

Another small thing.

Also here is a screenshot:
image

Note that it has a little extra white padding at the bottom that we should probably get rid of.

src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
@schuettloeffel-elsa
Copy link
Contributor Author

Note that it has a little extra white padding at the bottom that we should probably get rid of.

Done.

src/components/shared/TermsOfUseModal.tsx Outdated Show resolved Hide resolved
setTermsContent(response.data);
})
.catch(error => {
axios.get(getURL(typeof i18n.options.fallbackLng === 'string' ? i18n.options.fallbackLng : 'en-US'))
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite working right. I can see it requesting my region's terms (which correctly 404), and I can see it calling setTermsContent with the correct en-US terms. But the modal itself never pops up.

Copy link
Contributor Author

@schuettloeffel-elsa schuettloeffel-elsa Oct 21, 2024

Choose a reason for hiding this comment

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

Works fine for me, what user are you trying this with? ROLE_ADMIN will never see this, also the property org.opencastproject.admin.display_terms has to be true

@gregorydlogan gregorydlogan self-assigned this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: terms of use for new user
6 participants