<p><b>@tomhughes</b> requested changes on this pull request.</p>
<p>In addition to the inline comments, don't we need something on the server side to check that they actually agreed?</p>
<p>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 <code>read_tou</code> unset and then claim they have never agreed...</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2028#discussion_r246551163">app/assets/javascripts/user.js</a>:</p>
<pre style='color:#555'>> @@ -124,4 +124,9 @@ $(document).ready(function () {
$("#contributorTerms").html("<img src='" + OSM.SEARCHING + "' />");
$("#contributorTerms").load(url);
});
+
+ $("#read_tou").on("click", function () {
+ var agreebtn = document.getElementById('agree');
+ agreebtn.disabled=!this.checked;
</pre>
<p>I'd rather do this with jQuery like everything else - something like this I think:</p>
<pre><code>$("#agree").prop("disabled", !$(this).prop("checked"))
</code></pre>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2028#discussion_r246551972">app/views/users/terms.html.erb</a>:</p>
<pre style='color:#555'>>
<%= hidden_field_tag('referer', h(params[:referer])) unless params[:referer].nil? %>
<div class="buttons form-row inner20 clearfix">
- <p class="deemphasize"><%= t '.read and accept' %></p>
- <%= submit_tag(t('.agree'), :name => "agree", :id => "agree") %>
+ <%= submit_tag(t('.agree'), :name => "agree", :id => "agree", disabled: true) %>
</pre>
<p>Please use the <code>=></code> syntax for hashes.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2028#discussion_r246552235">app/views/users/terms.html.erb</a>:</p>
<pre style='color:#555'>> <div class="form-row">
- <label for="user_consider_pd">
- <%= check_box('user', 'consider_pd') %>
- <%= t '.consider_pd' %>
+ <label for="read_tou">
+ <%= check_box_tag 'read_tou',1,false %>
</pre>
<p>There's no need for the last two arguments - those are the default values.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2028#pullrequestreview-190945024">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABWnLZsoJae-oBQqpM1876J1wj_q_BNjks5vBmHngaJpZM4Xx7yL">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABWnLZ1ijn6uDx4GNxByizp9qhC8fXJXks5vBmHngaJpZM4Xx7yL.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/openstreetmap/openstreetmap-website","title":"openstreetmap/openstreetmap-website","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/openstreetmap/openstreetmap-website"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tomhughes requested changes on #2028"}],"action":{"name":"View Pull Request","url":"https://github.com/openstreetmap/openstreetmap-website/pull/2028#pullrequestreview-190945024"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/2028#pullrequestreview-190945024",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/2028#pullrequestreview-190945024",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>