Add links to the ToU and include them in signup

[simonpoole]

• add links to the ToU in the Welcome box and on the About page
• add tou_agreed to users table
• require checking a checkbox that the ToU have been read for signup

Missing:

- [ ] tests

[simonpoole]

http://upc.poole.ch:3000/ is running this right now (against a local test database) if anybody wants to see if the way that this works now is OK.

[tomhughes]

In addition to the inline comments the PR descriptions claims that a link is added to the About page but that doesn't seem to be true?

[simonpoole]

@tomhughes the change on the about page is on line 1274 on en.yml (I needed some squinting at the screen to re-.find it :-)).

[tomhughes]

@simonpoole I did actually try looking at it on your test instance and couldn't see the link but now that I understand the change that was likely because I was seeing the en-GB version of the string that hadn't changed.

[mmd-osm]

I'm wondering how this new flag field affects existing users. Will editing be blocked unless they have ticked some checkbox?

[simonpoole]

Sorry, I wanted to explain this in the comment but in the usual hackweekend environment I forgot.

The current changes do not effect existing accounts at all with the exception of those that haven't accepted the contributor terms.
Making the Tou acceptance mandatory for other existing accounts is way down the road, if ever, and will require additional coding not to mention discussion. This is in line with the LWG plans.

PS: it is not a flag it's a time stamp for obvious reasons.

[simonpoole]

Feedback from @systemed

• doesn't like the new position of the PD checkbox (surprise :-))
• suggests not having the ToU link in the Welcome box and instead moving it in to the attribution "footer"

[simonpoole]

@gravitystorm & @tomhughes this now "just" needs a test to verify that the "Accept" button is only enabled then the ToU checkbox is checked, and the corresponding database field is populated when accept is pressed.

I couldn't seem to find a test that actually checked this code path (there is a test in https://github.com/openstreetmap/openstreetmap-website/blob/master/test/integration/user_terms_seen_test.rb which however doesn't actually simulate pressing the accept button), can you confirm?

[tomhughes]

Well https://github.com/openstreetmap/openstreetmap-website/blob/master/test/integration/user_creation_test.rb#L75 tests the signup flow including accepting the terms?

If you actually want to check what happens to the button on the client then you will need a system test though obviously you want to check that the backend works right as well as you don't want to trust client side javascript ;-)

[mmd-osm]

The current changes do not effect existing accounts at all with the exception of those that haven't accepted the contributor terms.
Making the Tou acceptance mandatory for other existing accounts is way down the road, if ever, and will require additional coding not to mention discussion. This is in line with the LWG plans.

@simonpoole : somehow this doesn't fit well with what is stated here: https://wiki.openstreetmap.org/wiki/GDPR/Affected_Services:

rare case: For a transition period, there will be old user accounts that have not yet accepted the ToU. These would not be given access to "sensitive" material.

In the context of GDPR there needs to be some mechanism to accept the ToU even for existing users, otherwise you won't see any of the to-be hidden fields anymore even as logged on user.

[simonpoole]

@mmd-osm I was saying that in the context of making acceptance mandatory with this PR. As you point out we need to either:

• make acceptance mandatory latest before the API restrictions go in to force (when the code is available and so on, I'm not holding my breath on that),

• or provide a mechanism with which people could accept when necessary.

The later doesn't seem to make much sense as there are other issues than just GDPR compliance that are addressed in the ToU and at least -in principle- we would want everybody to agree to them. I'm just being a bit pragmatic about the whole thing as there are going to be a lot more signups going forward than there are now, and we are not in a hurry to force acceptance as long as there is no sign of the GDPR related code being worked on.

[simonpoole]

Well https://github.com/openstreetmap/openstreetmap-website/blob/master/test/integration/user_creation_test.rb#L75 tests the signup flow including accepting the terms?

If you actually want to check what happens to the button on the client then you will need a system test though obviously you want to check that the backend works right as well as you don't want to trust client side javascript ;-)

I'll punt on the client side javascript :-) (assuming that we don't actually have any kind of test harness that directly simulates user interaction). But since the requirement that the checkbox is checked is the only functional aspect that could be tested separately I'm a bit at a loss to find any sensible additional testable condition, so IMHO I would let things stand as they are now (when and if we add code to force ToU acceptance for existing users that can be revisited).

[tomhughes]

In addition to the inline comments, don't we need something on the server side to check that they actually agreed?

It looks to me like currently it relies on the submit button being disabled until javascript enables it when the checkbox is ticked but that leaves it open to somebody to hack a post of the form with read_tou unset and then claim they have never agreed...

[simonpoole]

In addition to the inline comments, don't we need something on the server side to check that they actually agreed?

It looks to me like currently it relies on the submit button being disabled until javascript enables it when the checkbox is ticked but that leaves it open to somebody to hack a post of the form with read_tou unset and then claim they have never agreed...

I now protect against this, not really sure if "users_controler.save" is the best place, and redirect back to the terms page. Further I've added a test that tests for the redirct if the check box parameter is missing/not checked (not that I actually understand how it works :-)).

[tomhughes]

So looking at the UI this present I have to say I find it very confusing... This is what the terms page now looks like:

So I have a checkbox to say I accept the ToU and then underneath that we have Agree/Decline buttons, which actually refer to the contributor terms at the top but that's like totally not obvious to me at least.

I think fundamentally it's just odd to have two agree/disagree questions which are answered in different ways - one by checking a box and one by choosing which submit button to press?

[simonpoole]

So looking at the UI this present I have to say I find it very confusing... This is what the terms page now looks like:

.....

So I have a checkbox to say I accept the ToU and then underneath that we have Agree/Decline buttons, which actually refer to the contributor terms at the top but that's like totally not obvious to me at least.

I think fundamentally it's just odd to have two agree/disagree questions which are answered in different ways - one by checking a box and one by choosing which submit button to press?

In principle I agree, our professionals (lawyers) would have liked to have the text displayed plus a checkbox, and this is the compromise.

It would perhaps be less confusing to actually have a 2nd (well 1st) check box for the contributor terms, and use a different text for the "Agree" button, perhaps "Continue" (and relabel the Decline button too, which we should do in any case as accepting the ToU is mandatory for everybody that gets shown the term screen now, so for the 1-2 people per year from pre-licence change and actually click decline, that option won't do anything reasonable any more).

Mockup:

[simonpoole]

@tomhughes should I the current state available on a local test instance as I did in October?

[simonpoole]

I can rebase this to get rid of the new conflict, but it only makes sense to do so if there is a chance of it getting merged.

[tomhughes]

What are we actually waiting on? I didn't really understand your previous comment?

[simonpoole]

What are we actually waiting on? I didn't really understand your previous comment?

Well in October I was running a test instance for public review, see #2028 (comment) I was just offering to do it again (with the current layout), if it could move this forward.

In any case from my pov I don't see anything further that needs doing except if we want to in some way display the ToU text, but IMHO there simply isn't enough screen real estate to do that in any meaningful way.

[tomhughes]

You could do, or I could put up a dev instance.

[simonpoole]

You could do, or I could put up a dev instance.

http://upc.poole.ch:3000/ is running again, I need to fix the ESlint errors though ("moving target").

[simonpoole]

Thanks (in the name of the LWG)!