-
Notifications
You must be signed in to change notification settings - Fork 27
Add Terms of Service Acceptance Concept #6632
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
Conversation
I finished the basic functionality. Looks like this: @fm3 When I tested this, I stumbled a bit, since I had to set the tos version to 1 (since the lastTOSAcceptanceVersion defaults to 0). maybe it could default to -1? or one checks the last TOS acceptance time against zero.. |
…vate it via application.conf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frontend LGTM 👍
I think the backend still needs a review?
I'm a little bit worried that this might be fairly disruptive. In case the organization owner is not immediately available, all members need to wait for the ToS acceptance. I think a more user friendly pattern would be to prompt the organization owner for acceptance a couple of weeks before (repeatedly), noting the deadline and that work of the members will be disrupted if the ToS are not accepted by then. I know that this would be more work to implement, but I wanted to note it. Have you thought about or discussed this?
Would forward this to @normanrz – I think apart from the implementation effort, there may be legal things to consider? |
@normanrz and I briefly discussed this and agreed that we should try to find a simple solution to mitigate the described problem. One idea would be the following: The application.conf gets another ToS parameter which would be a What do you think about this @fm3 ? |
@philippotto I added the two new fields Note that we should set enabled to false again in the config before merging this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice stuff, backend mostly LGTM 👍 Only left some minor comments:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend is fine from my side, didn't do any testing though. Leaving the approval for the frontend reviewer.
conf/application.conf
Outdated
@@ -74,6 +74,13 @@ webKnossos { | |||
|
|||
Please add the information of the operator to comply with GDPR. | |||
""" | |||
termsOfService { | |||
enabled = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set to false before merging
@daniel-wer The front-end part is ready for a second review. I integrated a snooze functionality so that the ToS acceptance can be snoozed if the deadline hasn't passed yet. The modals will re-appear on the next page load if a specific duration has passed in the meantime (currently, 20 seconds for testing; probably will change to 3 days). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if the remaining ToDos are taken care of. Thank you for making it easy to test on the dev instance 🙏
- The ToS shouldn't include the hamburger menu or website footer. Not sure whether that's something for this PR or needs to be changed in the ToS weblium page.
I'd assume, it would be the easiest to change this on weblium. @normanrz Is this possible? Currently, it looks like this: |
Yeah. I'll check that. |
…cing * 'master' of github.com:scalableminds/webknossos: Automatically open (and close) quick select settings when labeling in… (#6706) Add Terms of Service Acceptance Concept (#6632) Fix crash in publication page and add error boundaries (#6700) temporarily disable vx related polling (#6702) add protected and private modifiers to DAO hierarchy (#6698)
New config keys
webKnossos.termsOfService.{enabled, version, content}
, new routesGET /termsOfService
,POST termsOfService/accept
,GET /termsOfService/acceptanceNeeded
The idea is that after login, the frontend checks acceptanceNeeded and shows a popup asking for new TOS acceptance. If the user is orga owner, they cann call
accept
and that’s it, if they are not, they are toldplease contact your orga owner
.Note that if termsOfService is not explicitly enabled in the config,
acceptanceNeeded
should always return false.Note that when migrating an existing instance, every orga should get one organization owner.
TODO Frontend
URL of deployed dev instance (used for testing):
Steps to test:
termsOfService.enabled
to true in application.confIssues:
(Please delete unneeded items, merge only when none are left open)