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

User agreement #862

Merged
merged 36 commits into from Sep 17, 2020
Merged

User agreement #862

merged 36 commits into from Sep 17, 2020

Conversation

Atmire-Kristof
Copy link
Contributor

@Atmire-Kristof Atmire-Kristof commented Sep 8, 2020

References

Description

This PR adds a User Agreement at /info/end-user-agreement that all registered users have to agree with before being able to do anything else.

This PR also adds the privacy statement page. You can find it at /info/privacy. It isn’t referenced anywhere yet in this PR. That is done by #861, which adds a footer link and mentions it in the cookie popup. That PR also adds a footer link to the end user agreement.

Instructions for Reviewers

Changes made:

  • EndUserAgreementService: A service to check or update whether or not a user has accepted the agreement. There's currently two ways an agreement can be accepted:
    • A cookie in the user's browser storing a boolean
    • Logged in e-person's dspace.agreements.end-user metadata
  • Two new guards: EndUserAgreementCookieGuard returning true when the cookie is accepted and EndUserAgreementCurrentUserGuard returning true when the user's metadata contains "true" OR no user is logged in (anonymous). In case these guards consider the agreement not accepted, it'll return a UrlTree to a new EndUserAgreementComponent with the original destination as a redirect url in the query parameters.
  • EndUserAgreementCookieGuard is configured at route /register/:token. This is so the User Agreement can be accepted during registration.
  • EndUserAgreementCurrentUserGuard is configured on most routes (with the exception of /reload, /register, /login, /logout, /info (new) /unauthorized and **)
  • EndUserAgreementComponent: A component displaying the User Agreement (currently Lorem Ipsum) with a checkbox at the bottom to accept the agreement. This checkbox is automatically selected when: The cookie contains "true" OR the current user's metadata contains "true". When the checkbox is selected, you're able to click "Save". Clicking "Save" will set the cookie (when anonymous) or metadata (when authenticated) and redirect the user back to their original destination.

How to un-agree during testing:

  • You can remove the cookie by using your browser's dev tools.
  • You can reset the metadata by saving with an unchecked "agree" checkbox. The save button will be disabled if you uncheck, but you can remove the disabled attribute from the save button in the dev tools, and save anyway to reset the metadata for a user that has already agreed.

Ensure both the cookie and user metadata are not set to "true", before attempting any of these scenarios:

  • Visit any route blocked by authentication and login to an authorised account after being redirected to /login. You should be redirected to the User Agreement page. Accepting the User Agreement should set the user's metadata to "true" and redirect you back to your original destination.
  • Visit any public page and login using the dropdown at the top. Again, you should be redirected to the User Agreement page and after accepting, redirected back to the public page you were on.
  • Register a new user. After clicking the link in your email, the User Agreement should pop up. Accepting it should set the cookie to "true" and redirect you back to /register/:token to continue the registration.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

Atmire-Kristof and others added 30 commits August 19, 2020 17:47
…er-agreement-and-Privacy-statement

Conflicts:
	src/app/app-routing.module.ts
	src/app/core/core.module.ts
…er-agreement-and-Privacy-statement

Conflicts:
	src/app/app-routing.module.ts
Conflicts:
	src/app/app-routing.module.ts
	src/app/shared/log-in/log-in.component.ts
…er-agreement-and-Privacy-statement

Conflicts:
	src/app/+collection-page/collection-page-routing.module.ts
	src/app/+community-page/community-page-routing.module.ts
	src/app/+item-page/item-page-routing.module.ts
	src/app/+workflowitems-edit-page/workflowitems-edit-page-routing.module.ts
	src/app/app-routing.module.ts
	src/app/core/shared/operators.ts
Conflicts:
	src/app/app-routing.module.ts
@lgtm-com
Copy link

lgtm-com bot commented Sep 8, 2020

This pull request introduces 1 alert when merging c5e9756 into dd03745 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@artlowel artlowel added this to Needs Reviewers Assigned in DSpace 7 Beta 4 via automation Sep 8, 2020
@artlowel artlowel added this to the 7.0beta4 milestone Sep 8, 2020
@heathergreerklein heathergreerklein moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 4 Sep 8, 2020
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 @Atmire-Kristof : Code looks good. I did some basic testing of the "User Agreement" today after authenticating.

  • Verified clicking Cancel logs you out
  • Verified I cannot bypass the User Agreement by trying to jump to a different page in the app
  • Verified accepting the user agreement works well (and only possible after I click checkbox)
  • Verified the privacy statement text appears a /info/privacy

One question I ran into:

  • How do I force users to (re)accept a changed User agreement? For example, if the text of the user agreement requires major updates, I'd expect there should be a way for an Administrator to "reset" all users and force them to agree again.

Overall, I think this code & feature looks good enough as-is, so I'll approve it now. Any improvements could be done in a follow-up PR. I was just unclear how to "reset" the User Agreement acceptance for one user (or all users)...I'm not seeing a way to get that to work.

DSpace 7 Beta 4 automation moved this from Under Review to Reviewer Approved Sep 16, 2020
@artlowel
Copy link
Member

artlowel commented Sep 17, 2020

@tdonohue:

One question I ran into:

  • How do I force users to (re)accept a changed User agreement? For example, if the text of the user agreement requires major updates, I'd expect there should be a way for an Administrator to "reset" all users and force them to agree again.

Overall, I think this code & feature looks good enough as-is, so I'll approve it now. Any improvements could be done in a follow-up PR. I was just unclear how to "reset" the User Agreement acceptance for one user (or all users)...I'm not seeing a way to get that to work.

The only way to do it right now, is using a db query to reset those metadata values for one or more EPersons. Doing it in bulk from the UI would require a dedicated rest endpoint.

Since this is a task that will only be necessary every once in a while, perhaps we can create a script to do this. It could have a param to do it for one, multiple or all EPersons. And that way we don't need to create a custom endpoint or UI to do so.

@LucaGiamminonni
Copy link
Contributor

@artlowel

Since this is a task that will only be necessary every once in a while, perhaps we can create a script to do this. It could have a param to do it for one, multiple or all EPersons. And that way we don't need to create a custom endpoint or UI to do so.

I am currently working on something like this: I thought about creating a script that, given a metadata, deletes all its values. I thought of structuring it in a general way so that maybe it can also be used for other similar purposes. What do you think about it?

@artlowel
Copy link
Member

@LucaGiamminonni Sure, that sounds good!

Copy link
Contributor

@LucaGiamminonni LucaGiamminonni left a comment

Choose a reason for hiding this comment

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

I looked at the code and it looks okay. I have tested the behavior that is expected from the changes in this PR and the main things I have verified are:

  • at the login of a user who has never accepted the user agreements, the page with the user agreements to be accepted is shown
  • it is not possible to change the page until the conditions are accepted
  • since the user accepts the conditions he can browse the application without problem
  • the metadata that stores the information that the user agrees to the End User Agreement is saved correctly
  • at the next login of the same user, the End User Agreement page is not shown again
  • the privacy statement text is shown on the page /info/privacy

@tdonohue
Copy link
Member

Just to close up the discussion above. I agree with the analysis of @artlowel and @LucaGiamminonni. To "reset" the user agreement, we could just create a script (ideally one that supports "Scripts & Processes" so that it can be run from either CLI or Admin UI) which updates the database by clearing out the user agreement metadata field. I'll create a new ticket for that work, and schedule it for Beta 5

So, this PR can be merged as is. Thanks @Atmire-Kristof !

@tdonohue tdonohue merged commit 9f396f7 into DSpace:main Sep 17, 2020
DSpace 7 Beta 4 automation moved this from Reviewer Approved to Done Sep 17, 2020
@artlowel artlowel mentioned this pull request Sep 30, 2020
5 tasks
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

User Agreement / Privacy Statement
4 participants