[openstreetmap/openstreetmap-website] Re-arrange login and signup screens as discussed in #4128 (PR #4455)
Tom Hughes
notifications at github.com
Sun Jan 7 12:10:41 UTC 2024
@tomhughes requested changes on this pull request.
A first pass review, mostly concentrating on the server side.
I'm sure @gravitystorm will have more thoughts than me on the client side and bootstrap and whether there's any way to reduce the amount of custom CSS this is adding.
My main architectural question is whether we can avoid creating the two new actions, which creates a lot of duplication that is hard to avoid, and just use conditional sections in the views to change what is shown?
> @@ -42,11 +42,32 @@
<%= f.password_field :pass_crypt, :tabindex => 6 %>
<%= f.password_field :pass_crypt_confirmation, :tabindex => 7 %>
+ <p class="mb-3 text-muted"><%= t(".by_signing_up_html",
+ :tou_link => link_to(t("layouts.tou"),
+ "https://wiki.osmfoundation.org/wiki/Terms_of_Use",
+ :target => :new),
+ :contributor_terms_link => link_to(t(".contributor_terms"),
+ t(".contributor_terms_url"),
I think we need to check with LWG if they're happy for this URL to be translatable because I suspect that will be a problem as we need to be sure people are seeing the correct terms.
Did you make it translatable so the French and Italian versions could be linked to directly for those languages?
> redirect_to auth_url(current_user.auth_provider, current_user.auth_uid), :status => :temporary_redirect
else
# Save the user record
- session[:new_user] = current_user.attributes.slice("email", "display_name", "pass_crypt")
- redirect_to :action => :terms
+ session[:new_user] = current_user.attributes.slice("email", "display_name", "pass_crypt", "consider_pd")
We don't really need to go through the session here - it could just pass the attributes to `save_new_user` and then the other call could pass the attributes recovered from the session.
> @@ -1,29 +0,0 @@
-require "application_system_test_case"
-
-class UserSignupTest < ApplicationSystemTestCase
- test "Sign up from login page" do
What's the reason for removing this test? Obviously it will need to change to switch tab instead of clicking through the register link but it's still a valid test?
> @@ -99,7 +99,7 @@ class User < ApplicationRecord
validates :display_name, :if => proc { |u| u.display_name_changed? },
:characters => { :url_safe => true },
:whitespace => { :leading => false, :trailing => false }
- validates :email, :presence => true, :confirmation => true, :characters => true
+ validates :email, :presence => true, :characters => true
I did say in #4128 that I was unsure about removing the email confirmation but as far as I can see there was no further discussion of it.
It was added for a reason, to try and stop people mistyping their email address and then not being able to confirm their account but I'll accept that it's not clear how successful that is and how much people just copy and paste to the second field.
> <h4><%= t ".about.header" %></h4>
<p><%= t ".about.paragraph_1" %></p>
- <p><%= t ".about.paragraph_2" %></p>
I'm not sure about completely removing this - we can probably drop the bit about sending an email but the "Signup to get started" call to action bit at the start might be good to keep? Maybe we add it to the end of the first paragraph?
> + session[:referer] = safe_referer(params[:referer]) if params[:referer]
+
+ Rails.logger.info "create: #{session[:referer]}"
+
+ if current_user.invalid?
+ # Something is wrong with a new user, so rerender the form
+ render :action => "new"
+ else
+ # Save the user record
+ session[:new_user] = current_user.attributes.slice("email", "display_name", "pass_crypt", "consider_pd")
+ save_new_user
+ end
+ end
+ end
+
+ def new_association
I'm not sure if there's anything we can easily do about it but creating the separate `new_association` and `create_association` actions means there is quite a bit of duplication with the code in the `new` and `create` actions.
> # Something is wrong with a new user, so rerender the form
- render :action => "new"
- elsif current_user.auth_provider.present?
+ render :action => "new_association"
+ else
# Verify external authenticator before moving on
session[:new_user] = current_user.attributes.slice("email", "display_name", "pass_crypt", "consider_pd")
redirect_to auth_url(current_user.auth_provider, current_user.auth_uid), :status => :temporary_redirect
Do we still need to do this verification I wonder? The purpose of this was that if the user selected a third party using the drop down on the signup screen then we needed to verify it, which also got us the authentication ID to store, but if that dropdown goes away and we have always done the third party auth via a button then we already have the ID and we don't really need to do a second call to the third party?
> + <h1><%= t ".title" %></h1>
+ </div>
+ <div class='header-illustration new-user-arm d-none d-md-block'></div>
+<% end %>
+
+<div class="auth-container">
+ <div class="text-muted col-sm form-container">
+
+ <h4><%= t ".welcome" %></h4>
+
+ <%= bootstrap_form_for current_user, :url => { :action => "create_association" } do |f| %>
+ <%= hidden_field_tag("referer", h(@referer)) unless @referer.nil? %>
+ <%= f.hidden_field :auth_provider %>
+ <%= f.hidden_field :auth_uid %>
+
+ <% if current_user.errors[:email].empty? %>
What's the benefit of actively hiding the email field when we can? If it's shown but pre-populated then it makes no difference for most people because they can just click OK to continue but they still have the option to change it if they want...
> + new_association:
+ title: New Association
+ welcome: "Welcome to OpenStreetMap"
+ display name description: "Your publicly displayed username. You can change this later in the preferences."
+ email_help_html: 'Your address is not displayed publicly, see our %{privacy_policy_link} for more information.'
+ by_signing_up_html: "By signing up, you agree to our %{tou_link}, %{privacy_policy_link} and %{contributor_terms_link}."
+ tou: "terms of use"
+ contributor_terms_url: "https://wiki.osmfoundation.org/wiki/Licence/Contributor_Terms"
+ contributor_terms: "contributor terms"
+ continue: Sign Up
+ privacy_policy: privacy policy
+ privacy_policy_url: https://wiki.osmfoundation.org/wiki/Privacy_Policy
+ privacy_policy_title: OSMF privacy policy including section on email addresses
+ consider_pd_html: "I consider my contributions to be in the %{consider_pd_link}."
+ consider_pd: "public domain"
+ consider_pd_url: https://wiki.osmfoundation.org/wiki/Licence_and_Legal_FAQ/Why_would_I_want_my_contributions_to_be_public_domain
There's an awful lot of duplication in these resources which again is a result of splitting the actions in the user controller...
> @@ -15,6 +15,16 @@ def new
override_content_security_policy_directives(:form_action => []) if Settings.csp_enforce || Settings.key?(:csp_report_url)
session[:referer] = safe_referer(params[:referer]) if params[:referer]
+
+ if session[:referer]
Can we move this logic into the `SessionMethods` concern so we don't need to duplicate it in the users and sessions controllers? It would just be a method that took the referer as an argument?
> @@ -23,6 +23,7 @@ def new
ref_params = CGI.parse referer_query
preferred = ref_params["preferred_auth_provider"].first
@preferred_auth_provider = preferred if preferred && Settings.key?(:"#{preferred}_auth_id")
+ @client_app_name = Oauth2Application.find_by(:uid => ref_params["client_id"].first)&.name if ref_params["client_id"].first
This is probably better as:
```ruby
Oauth2Application.where(:uid => ref_params["client_id"].first).pluck(:name).first
```
> @@ -68,6 +68,7 @@ def new
ref_params = CGI.parse referer_query
preferred = ref_params["preferred_auth_provider"].first
@preferred_auth_provider = preferred if preferred && Settings.key?(:"#{preferred}_auth_id")
+ @client_app_name = Oauth2Application.find_by(:uid => ref_params["client_id"].first)&.name if ref_params["client_id"].first
Same here...
> @@ -4,6 +4,10 @@
<% end %>
<div class="auth-container">
+ <% if @client_app_name %>
+ <p class="association-note mt-4"><%= t(".login_to_authorize_html", :client_app_name => @client_app_name) %></p>
That translation name should probably be `signup_to_authorize_html` here? Though see later comment about the translations...
> @@ -1808,6 +1808,7 @@ en:
new:
title: "Login"
tab_title: "Login"
+ login_to_authorize_html: "You need an OpenStreeMap account to access %{client_app_name}."
Should this be more like `Login to OpenStreetMap to access...` maybe? At the very least it needs to spell OpenStreetMap correctly ;-)
> @@ -2695,6 +2696,7 @@ en:
new:
title: "Sign Up"
tab_title: "Sign up"
+ login_to_authorize_html: "You need an OpenStreeMap account to access %{client_app_name}."
Same here, maybe `Signup with OpenStreetMap to access...` or if we are going to have the same text in both places we should try and share one resource.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4455#pullrequestreview-1807783631
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/4455/review/1807783631 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240107/0b168fd3/attachment-0001.htm>
More information about the rails-dev
mailing list