AO3-6672 Allow some admins to set a generic username #4994

brianjaustin · 2024-12-15T19:55:14Z

Pull Request Checklist

Have you read "How to write the perfect pull request"?
Have you read the contributing guidelines?
Have you added tests for any changed functionality?
Have you added the Jira issue number
as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
Do you have fewer than 5 pull requests already open? If not, please wait
until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-6672

Purpose

Allow superadmins and policy_and_abuse to set a generic username for users (even when banned). Because #4991 is already open, I have not added the subscript for "Ticket ID" as noted in the ticket.

Side question: do we want to update some of the copy/variables here to be more consistent? I observed login (has to stay), user name, and username in different spots...

sarken · 2024-12-17T00:09:29Z

If you're up for changing copy in a whole bunch of places, I'm up for testing! We ultimately want to switch from "user name" to "username" but it's in soooo many places I didn't want to inflict that on anyone.

I think this covers all the references to this page so we could at least have some consistency in this area:

this change_username page, including error messages
the link in the /views/users/_edit_header_navigation.html.erb partial
the link in the /views/users/_edit_user_navigation.html.erb partial
the reference to this page on /app/views/pseuds/_pseuds_form.html.erb ("You cannot change the pseud that matches your user name. However, you can change your user name instead.")
the link in /app/views/preferences/index.html.erb

sarken

Just a couple quick frontend suggestions since we were talking about text changes!

app/views/users/_admin_change_username.html.erb

config/locales/models/en.yml

app/views/users/_admin_change_username.html.erb

app/views/preferences/index.html.erb

Bilka2

Besides the comments I left, I think it would be good to have a feature test that goes through renaming a user as an admin. Just to make sure nothing gets missed because the specs test pretty close to the implementation itself.

.rubocop.yml

app/controllers/pseuds_controller.rb

app/controllers/users_controller.rb

app/models/user.rb

app/views/challenge/gift_exchange/_challenge_signups.html.erb

config/locales/views/en.yml

Bilka2 · 2024-12-28T18:37:48Z

spec/controllers/users_controller_spec.rb

+    end
+
+    it_behaves_like "an action only authorized admins can access", authorized_roles: %w[superadmin policy_and_abuse] do
+      let(:user) { create(:user, banned: true) }


Why a banned user?

Non-exhaustive checking that users who cannot access this page themselves can be renamed by an admin

Bilka2 · 2024-12-28T18:44:07Z

spec/models/user_spec.rb

+        end
+
+        it "does not prevent changing the username" do
+          allow(existing_user).to receive(:justification_enabled?).and_return(false)


What about ticket_number?

Since https://github.com/otwcode/otwarchive/pull/4994/files#diff-a55bf747ae62cffdcdb15d898d6bde2ab06b44df40317d8c44330e1615b6c50fR133 already tests that, I disabled the ticket_number requirement here. I'd rather not turn it back on, because we have to mock quite a few things to make the Zoho validation happy

spec/models/user_spec.rb

sarken

I was reviewing in two tabs simultaneously so I could go back and forth between the views and the locales, so I'm sure this will be quality. If anything is confusing or contradictory, my bad, please yell!

app/controllers/pseuds_controller.rb

sarken · 2024-12-30T06:27:21Z

app/models/user.rb

@@ -564,11 +572,25 @@ def reindex_user_creations_after_rename
  end

  def add_renamed_at
+    return if User.current_user.is_a?(Admin)


I agree with Brian here. Since we're already supposed to be ignoring the renamed_at when an admin goes to change a name, I think it makes sense that admin renames just ignore the renamed_at entirely.

However, we could add a separate admin_renamed_at column to the users table for general tracking purposes. That does feel like something that could be both useful and clarifying if you're looking at an audit or just a user in the database.

app/views/admin/admin_invitations/find.html.erb

app/views/admin/admin_invitations/index.html.erb

app/views/invitations/_user_invitations.html.erb

app/views/users/_admin_change_username.html.erb

app/views/users/sessions/_passwd_small.html.erb

config/locales/controllers/en.yml

config/locales/devise/en.yml

brianjaustin · 2024-12-30T23:00:58Z

app/views/invitations/_invitation.html.erb

@@ -1,29 +1,29 @@
 <dl>
 	<dt>Sender</dt>
-    <dd><%= creator_link(invitation) %></dd>
+    <dd><%= creator_link(@invitation) %></dd>


I needed to make changes here to fix issues with how @invitation is referenced (locals versus just passing it in). For everyone's sanity, I'll do the rest of the erb lint fixes in #5000 since that has most of them already anyways

Bilka2

Thank you!

sarken · 2025-01-03T04:41:09Z

app/views/admin/admin_invitations/find.html.erb

 <!--/descriptions-->

 <!--subnav-->
 <!--/subnav-->

 <!--main content-->
-<%= form_tag url_for(:controller => 'admin/admin_invitations', :action => 'find'), :method => :get do %>
+<%= form_tag url_for(controller: "admin/admin_invitations", action: "find"), autocomplete: "off", method: :get do %>


Since we're now updating the actual form tags, let's add the classes that are missing on so many forms:

Suggested change

<%= form_tag url_for(controller: "admin/admin_invitations", action: "find"), autocomplete: "off", method: :get do %>

<%= form_tag url_for(controller: "admin/admin_invitations", action: "find"), autocomplete: "off", class: "invitation simple search", method: :get do %>

sarken · 2025-01-03T04:45:54Z

app/views/admin/admin_invitations/find.html.erb

+    <dd><%= text_field_tag "invitation[user_name]", params[:invitation][:user_name] %></dd>
+    <dt><%= label_tag "invitation[token]", t(".token") %></dt>
+    <dd><%= text_field_tag "invitation[token]", params[:invitation][:token] %></dd>
+    <dt><%= label_tag "invitee_email", t(".email") %></dt>
    <dd><%= text_field_tag "invitation[invitee_email]", params[:invitation][:invitee_email], id: "invitee_email" %></dd>
  </dl>
  <p class="submit actions"><%= submit_tag "Go" %></p>


Missed a string, although it could really be "Search" instead of "Go."

sarken · 2025-01-03T04:47:31Z

app/views/admin/admin_invitations/find.html.erb

    <dd><%= text_field_tag "invitation[invitee_email]", params[:invitation][:invitee_email], id: "invitee_email" %></dd>
  </dl>
  <p class="submit actions"><%= submit_tag "Go" %></p>
 <% end %>

 <% if @user %>
-<h3 class="heading"><%h= 'Invitations for' %> <%= link_to @user.login, user_invitations_path(@user) %></h3>
-<%= render :partial => 'invitations/user_invitations', :locals => {:invitations => @invitations} %>
+<h3 class="heading"><%= t(".invitations_for") %> <%= link_to @user.login, user_invitations_path(@user) %></h3>


I think this needs some interpolation so the link to the name is part of the translated text.

sarken · 2025-01-03T04:59:44Z

app/views/admin/admin_invitations/index.html.erb

+<%= form_tag url_for(controller: "admin/admin_invitations", action: :invite_from_queue), autocomplete: "off" do %>
  <fieldset class="simple">


Suggested change

<%= form_tag url_for(controller: "admin/admin_invitations", action: :invite_from_queue), autocomplete: "off" do %>

<fieldset class="simple">

<%= form_tag url_for(controller: "admin/admin_invitations", action: :invite_from_queue), class: "queue invitation simple post", autocomplete: "off" do %>

<fieldset>

sarken · 2025-01-03T05:00:55Z

app/views/admin/admin_invitations/index.html.erb

+<%= form_tag url_for(controller: "admin/admin_invitations", action: :create), autocomplete: "off" do |f| %>
  <fieldset class="simple">


Suggested change

<%= form_tag url_for(controller: "admin/admin_invitations", action: :create), autocomplete: "off" do |f| %>

<fieldset class="simple">

<%= form_tag url_for(controller: "admin/admin_invitations", action: :create), class: "invitation simple post", autocomplete: "off" do |f| %>

<fieldset>

Conveniently, this will move the submit button closer to the fields. simple was in the wrong place to have any effect before.

sarken · 2025-01-06T02:07:02Z

app/views/users/change_username.html.erb

+  <%= form_tag changed_username_user_path(@user), autocomplete: "off" do %>
+    <dl>


Same suggestion for the legend and heading as in the admin partial:

Suggested change

<%= form_tag changed_username_user_path(@user), autocomplete: "off" do %>

<dl>

<%= form_tag changed_username_user_path(@user), class: "simple post", autocomplete: "off" do %>

<legend><%= t(".legend") %></legend>

<h3 class="landmark heading"><%= t(".heading") %></h3>

<dl>

sarken · 2025-01-06T02:07:42Z

app/views/users/change_username.html.erb

+        </p>
+      </dd>
+      <dt><%= label_tag :password, t(".password") %></dt>
+      <dd><%= password_field_tag :password, nil, autocomplete: "disabled" %></dd>


Does this need autocomplete: "disabled" when the form has autocomplete: "off" because it's a password field?

sarken · 2025-01-06T02:27:47Z

app/views/admin/admin_invitations/find.html.erb

-<h3 class="heading"><%h= 'Invitations for' %> <%= link_to @user.login, user_invitations_path(@user) %></h3>
-<%= render :partial => 'invitations/user_invitations', :locals => {:invitations => @invitations} %>
+<h3 class="heading"><%= t(".invitations_for") %> <%= link_to @user.login, user_invitations_path(@user) %></h3>
+<%= render partial: "invitations/user_invitations", invitations: @invitations %>


Suggested change

<%= render partial: "invitations/user_invitations", invitations: @invitations %>

<%= render "invitations/user_invitations", invitations: @invitations %>

sarken · 2025-01-06T02:27:59Z

app/views/admin/admin_invitations/find.html.erb

 <% elsif @invitation %>
-<%= render :partial => 'invitations/invitation', :locals => {:invitation => @invitation} %>
+<%= render partial: "invitations/invitation", invitation: @invitation %>


Suggested change

<%= render partial: "invitations/invitation", invitation: @invitation %>

<%= render "invitations/invitation", invitation: @invitation %>

sarken · 2025-01-06T02:28:09Z

app/views/admin/admin_invitations/find.html.erb

 <% elsif @invitations %>
-<%= render :partial => 'invitations/user_invitations', :locals => {:invitations => @invitations} %>
+<%= render partial: "invitations/user_invitations", invitations: @invitations %>


Suggested change

<%= render partial: "invitations/user_invitations", invitations: @invitations %>

<%= render "invitations/user_invitations", invitations: @invitations %>

sarken · 2025-01-06T02:38:51Z

(Sorry for all the comments! If it makes you feel any better, the review took me three days to finish. 😆)

AO3-6672 Allow some admins to set a generic username

11005ef

github-actions bot added the Awaiting Review label Dec 15, 2024

sarken reviewed Dec 19, 2024

View reviewed changes

app/views/users/_admin_change_username.html.erb Outdated Show resolved Hide resolved

config/locales/models/en.yml Outdated Show resolved Hide resolved

app/views/users/_admin_change_username.html.erb Outdated Show resolved Hide resolved

brianjaustin added 4 commits December 19, 2024 14:01

Partially rename 'user name' -> username

3d7e2a7

Test fixes

864fe88

Review feedback

b4d6adf

Moar tests

66ad1d6

Hamham6 reviewed Dec 21, 2024

View reviewed changes

app/views/preferences/index.html.erb Outdated Show resolved Hide resolved

brianjaustin added 3 commits December 21, 2024 16:38

Missing i18n

23175d2

'user name' -> 'username' batch 2

5f52362

Fix tests

ae5f8be

Bilka2 suggested changes Dec 28, 2024

View reviewed changes

Bilka2 added Reviewed: Action Needed and removed Awaiting Review labels Dec 28, 2024

Review feedback

382acc9

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Dec 28, 2024

brianjaustin added 3 commits December 28, 2024 16:51

More fixes

8be6dc0

:thisisfine:

840b78b

Put arrows in i18n file

859b309

sarken reviewed Dec 30, 2024

View reviewed changes

brianjaustin added 3 commits December 30, 2024 15:47

Turn autocomplete off, i18n key improvements

7adf97f

Add admin_renamed_at

6bec283

Test fixes and more autocomplete fun

fac4e28

github-actions bot added the Has Migrations Contains migrations and therefore needs special attention when deploying label Dec 30, 2024

brianjaustin commented Dec 30, 2024

View reviewed changes

A couple oopsies

d72234f

Bilka2 approved these changes Jan 5, 2025

View reviewed changes

Bilka2 added the Reviewed: Ready to Merge label Jan 5, 2025

Bilka2 removed the Coder Has Actioned Review label Jan 5, 2025

sarken reviewed Jan 6, 2025

View reviewed changes

sarken added Reviewed: Action Needed and removed Reviewed: Ready to Merge labels Jan 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AO3-6672 Allow some admins to set a generic username #4994

AO3-6672 Allow some admins to set a generic username #4994

brianjaustin commented Dec 15, 2024

sarken commented Dec 17, 2024

sarken left a comment

Bilka2 left a comment

Bilka2 Dec 28, 2024

brianjaustin Dec 28, 2024

Bilka2 Dec 28, 2024

brianjaustin Dec 28, 2024

sarken left a comment

sarken Dec 30, 2024

brianjaustin Dec 30, 2024

Bilka2 left a comment

sarken Jan 3, 2025

sarken Jan 3, 2025

sarken Jan 3, 2025

sarken Jan 3, 2025

sarken Jan 3, 2025

sarken Jan 6, 2025

sarken Jan 6, 2025

sarken Jan 6, 2025

sarken Jan 6, 2025

sarken Jan 6, 2025

sarken commented Jan 6, 2025

	<%= form_tag url_for(controller: "admin/admin_invitations", action: "find"), autocomplete: "off", method: :get do %>
	<%= form_tag url_for(controller: "admin/admin_invitations", action: "find"), autocomplete: "off", class: "invitation simple search", method: :get do %>

		<%= form_tag url_for(controller: "admin/admin_invitations", action: :invite_from_queue), autocomplete: "off" do %>
		<fieldset class="simple">

		<%= form_tag changed_username_user_path(@user), autocomplete: "off" do %>
		<dl>

-  <%= form_tag changed_username_user_path(@user), autocomplete: "off" do %>
-    <dl>
+<%= form_tag changed_username_user_path(@user), class: "simple post", autocomplete: "off" do %>
+  <legend><%= t(".legend") %></legend>
+  <h3 class="landmark heading"><%= t(".heading") %></h3>
+  <dl>

	<%= render partial: "invitations/user_invitations", invitations: @invitations %>
	<%= render "invitations/user_invitations", invitations: @invitations %>

	<%= render partial: "invitations/invitation", invitation: @invitation %>
	<%= render "invitations/invitation", invitation: @invitation %>

AO3-6672 Allow some admins to set a generic username #4994

Are you sure you want to change the base?

AO3-6672 Allow some admins to set a generic username #4994

Conversation

brianjaustin commented Dec 15, 2024

Pull Request Checklist

Issue

Purpose

sarken commented Dec 17, 2024

sarken left a comment

Choose a reason for hiding this comment

Bilka2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bilka2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarken commented Jan 6, 2025