diff --git a/app/controllers/user_name_change_requests_controller.rb b/app/controllers/user_name_change_requests_controller.rb index 6e1f290cd..3f7874f5c 100644 --- a/app/controllers/user_name_change_requests_controller.rb +++ b/app/controllers/user_name_change_requests_controller.rb @@ -1,47 +1,29 @@ class UserNameChangeRequestsController < ApplicationController before_action :member_only, :only => [:index, :show] before_action :gold_only, :only => [:new, :create] - before_action :admin_only, :only => [:approve, :reject] respond_to :html, :json, :xml def new @change_request = UserNameChangeRequest.new(change_request_params) respond_with(@change_request) end - + def create - @change_request = UserNameChangeRequest.create(change_request_params) - - if @change_request.errors.any? - render :action => "new" - else - @change_request.approve! - redirect_to user_name_change_request_path(@change_request), :notice => "Your name has been changed" - end + @change_request = UserNameChangeRequest.create_with(user: CurrentUser.user, original_name: CurrentUser.name).create(change_request_params) + flash[:notice] = "Your name has been changed" + respond_with(@change_request, location: profile_path) end - + def show @change_request = UserNameChangeRequest.find(params[:id]) check_privileges!(@change_request) respond_with(@change_request) end - + def index @change_requests = UserNameChangeRequest.visible.order("id desc").paginate(params[:page], :limit => params[:limit]) respond_with(@change_requests) end - - def approve - @change_request = UserNameChangeRequest.find(params[:id]) - @change_request.approve! - redirect_to user_name_change_request_path(@change_request), :notice => "Name change request approved" - end - - def reject - @change_request = UserNameChangeRequest.find(params[:id]) - @change_request.reject!(params[:reason]) - redirect_to user_name_change_request_path(@change_request), :notice => "Name change request rejected" - end private @@ -51,6 +33,6 @@ class UserNameChangeRequestsController < ApplicationController end def change_request_params - params.fetch(:user_name_change_request, {}).permit(%i[desired_name change_reason]) + params.fetch(:user_name_change_request, {}).permit(%i[desired_name]) end end diff --git a/app/models/user_name_change_request.rb b/app/models/user_name_change_request.rb index 385090a8f..b0415b651 100644 --- a/app/models/user_name_change_request.rb +++ b/app/models/user_name_change_request.rb @@ -1,75 +1,34 @@ class UserNameChangeRequest < ApplicationRecord - after_initialize :initialize_attributes, if: :new_record? - validates_presence_of :original_name, :desired_name - validates_inclusion_of :status, :in => %w(pending approved rejected) belongs_to :user - belongs_to :approver, :class_name => "User", optional: true - validate :not_limited, :on => :create + belongs_to :approver, class_name: "User", optional: true + + validate :not_limited, on: :create validates :desired_name, user_name: true + validates_presence_of :original_name, :desired_name - def initialize_attributes - self.user_id ||= CurrentUser.user.id - self.original_name ||= CurrentUser.user.name - end - - def self.pending - where(:status => "pending") - end - - def self.approved - where(:status => "approved") - end + after_create :approve! def self.visible(viewer = CurrentUser.user) if viewer.is_admin? all elsif viewer.is_member? - joins(:user).merge(User.undeleted).where("user_name_change_requests.status = 'approved' OR user_name_change_requests.user_id = ?", viewer.id) + joins(:user).merge(User.undeleted).where("user_name_change_requests.user_id = ?", viewer.id) else none end end - - def rejected? - status == "rejected" - end - - def approved? - status == "approved" - end - def pending? - status == "pending" - end - - def feedback - user.feedback.order("id desc") - end - def approve! - update(status: "approved", approver_id: CurrentUser.user.id) user.update_attribute(:name, desired_name) body = "Your name change request has been approved. Be sure to log in with your new user name." Dmail.create_automated(:title => "Name change request approved", :body => body, :to_id => user_id) UserFeedback.create(:user_id => user_id, :category => "neutral", :body => "Name changed from #{original_name} to #{desired_name}") ModAction.log("Name changed from #{original_name} to #{desired_name}",:user_name_change) end - - def reject!(reason) - update(status: "rejected", rejection_reason: reason) - body = "Your name change request has been rejected for the following reason: #{rejection_reason}" - Dmail.create_automated(:title => "Name change request rejected", :body => body, :to_id => user_id) - end - + def not_limited - if UserNameChangeRequest.where("user_id = ? and created_at >= ?", CurrentUser.user.id, 1.week.ago).exists? - errors.add(:base, "You can only submit one name change request per week") + if UserNameChangeRequest.unscoped.where(user: user).where("created_at >= ?", 1.week.ago).exists? + errors[:base] << "You can only submit one name change request per week" end end - - def api_attributes - attributes = super - attributes -= [:change_reason, :rejection_reason] unless CurrentUser.is_admin? || user == CurrentUser.user - attributes - end end diff --git a/app/views/user_name_change_requests/index.html.erb b/app/views/user_name_change_requests/index.html.erb index e18bd5a40..ab438ba93 100644 --- a/app/views/user_name_change_requests/index.html.erb +++ b/app/views/user_name_change_requests/index.html.erb @@ -6,9 +6,8 @@ User - Request - Reason - Status + Old Name + New Name Date @@ -17,23 +16,8 @@ <% @change_requests.each do |change_request| %> <%= link_to_user change_request.user %> - - <%= change_request.original_name %> -> - <%= change_request.desired_name %> - - - <% if CurrentUser.is_admin? || CurrentUser.user == change_request.user %> - <%= change_request.change_reason %> - <% end %> - - - <%= change_request.status %> - <% if change_request.approved? %> - by <%= link_to_user change_request.approver %> - <% elsif change_request.rejected? %> - for reason: <%= link_to change_request.rejection_reason %> - <% end %> - + <%= change_request.original_name %> + <%= change_request.desired_name %> <%= compact_time change_request.created_at %> <%= link_to "view", user_name_change_request_path(change_request) %> diff --git a/app/views/user_name_change_requests/show.html.erb b/app/views/user_name_change_requests/show.html.erb index ae6871a64..440746635 100644 --- a/app/views/user_name_change_requests/show.html.erb +++ b/app/views/user_name_change_requests/show.html.erb @@ -22,66 +22,8 @@ <%= @change_request.desired_name %> - - Reason - <%= @change_request.change_reason %> - - - Status - - <%= @change_request.status %> - <% if @change_request.approved? %> - by <%= link_to_user @change_request.approver %> - <% elsif @change_request.rejected? %> - for reason: <%= link_to @change_request.rejection_reason %> - <% end %> - - - - <% if @change_request.pending? && CurrentUser.user.is_admin? %> -
-

Feedback

- -
- -
-

Statistics

- -
- -
-

Options

- <%= form_tag(approve_user_name_change_request_path(@change_request)) do %> - <%= submit_tag "Approve" %> - <% end %> - - <%= form_tag(reject_user_name_change_request_path(@change_request), :class => "simple_form") do %> -
- - <%= text_field_tag "reason" %> -
- -
- <%= submit_tag "Reject" %> -
- <% end %> -
- <% end %> diff --git a/config/routes.rb b/config/routes.rb index 9982bc794..38f5962e5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -300,12 +300,7 @@ Rails.application.routes.draw do get :search end end - resources :user_name_change_requests do - member do - post :approve - post :reject - end - end + resources :user_name_change_requests, only: [:new, :create, :show, :index] resource :user_revert, :only => [:new, :create] resources :wiki_pages do member do diff --git a/db/migrate/20190926000912_remove_reason_and_status_from_user_name_change_requests.rb b/db/migrate/20190926000912_remove_reason_and_status_from_user_name_change_requests.rb new file mode 100644 index 000000000..fd8efd218 --- /dev/null +++ b/db/migrate/20190926000912_remove_reason_and_status_from_user_name_change_requests.rb @@ -0,0 +1,8 @@ +class RemoveReasonAndStatusFromUserNameChangeRequests < ActiveRecord::Migration[6.0] + def change + remove_column :user_name_change_requests, :change_reason, :text + remove_column :user_name_change_requests, :rejection_reason, :text + remove_column :user_name_change_requests, :approver_id, :integer + remove_column :user_name_change_requests, :status, :string, null: false, default: "pending" + end +end diff --git a/db/structure.sql b/db/structure.sql index 733f0f334..3a09b6393 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -3010,13 +3010,9 @@ ALTER SEQUENCE public.user_feedback_id_seq OWNED BY public.user_feedback.id; CREATE TABLE public.user_name_change_requests ( id integer NOT NULL, - status character varying DEFAULT 'pending'::character varying NOT NULL, user_id integer NOT NULL, - approver_id integer, original_name character varying, desired_name character varying, - change_reason text, - rejection_reason text, created_at timestamp without time zone, updated_at timestamp without time zone ); @@ -7340,6 +7336,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20190908031103'), ('20190908035317'), ('20190919175836'), -('20190923071044'); +('20190923071044'), +('20190926000912'); diff --git a/test/factories/user_name_change_request.rb b/test/factories/user_name_change_request.rb index 6ca0b5d32..509ce82f1 100644 --- a/test/factories/user_name_change_request.rb +++ b/test/factories/user_name_change_request.rb @@ -1,6 +1,7 @@ FactoryBot.define do factory(:user_name_change_request) do + user + original_name {FFaker::Internet.user_name} desired_name {FFaker::Internet.user_name} - change_reason {FFaker::Lorem.sentence} end end diff --git a/test/functional/user_name_change_requests_controller_test.rb b/test/functional/user_name_change_requests_controller_test.rb index b967d9469..f64be590f 100644 --- a/test/functional/user_name_change_requests_controller_test.rb +++ b/test/functional/user_name_change_requests_controller_test.rb @@ -5,16 +5,8 @@ class UserNameChangeRequestsControllerTest < ActionDispatch::IntegrationTest setup do @user = create(:gold_user) @admin = create(:admin_user) - as(@user) do - @change_request = UserNameChangeRequest.create!( - :user_id => @user.id, - :original_name => @user.name, - :desired_name => "abc", - :change_reason => "hello" - ) - end end - + context "new action" do should "render" do get_auth new_user_name_change_request_path, @user @@ -25,13 +17,19 @@ class UserNameChangeRequestsControllerTest < ActionDispatch::IntegrationTest context "create action" do should "work" do post_auth user_name_change_requests_path, @user, params: { user_name_change_request: { desired_name: "zun" }} - assert_response :success + + assert_response :redirect + assert_equal("zun", @user.reload.name) end end - + context "show action" do + setup do + @change_request = as(@user) { create(:user_name_change_request, user_id: @user.id) } + end + should "render" do - get_auth user_name_change_request_path(@change_request), @user + get_auth user_name_change_request_path(@change_request), @admin assert_response :success end @@ -43,7 +41,7 @@ class UserNameChangeRequestsControllerTest < ActionDispatch::IntegrationTest end end end - + context "for actions restricted to admins" do context "index action" do should "render" do @@ -51,20 +49,6 @@ class UserNameChangeRequestsControllerTest < ActionDispatch::IntegrationTest assert_response :success end end - - context "approve action" do - should "succeed" do - post_auth approve_user_name_change_request_path(@change_request), @admin - assert_redirected_to(user_name_change_request_path(@change_request)) - end - end - - context "reject action" do - should "succeed" do - post_auth reject_user_name_change_request_path(@change_request), @admin - assert_redirected_to(user_name_change_request_path(@change_request)) - end - end end end end diff --git a/test/unit/user_name_change_request_test.rb b/test/unit/user_name_change_request_test.rb index c6c8a64f3..bc8babb36 100644 --- a/test/unit/user_name_change_request_test.rb +++ b/test/unit/user_name_change_request_test.rb @@ -19,7 +19,6 @@ class UserNameChangeRequestTest < ActiveSupport::TestCase @change_request = UserNameChangeRequest.create( :user_id => @requester.id, :original_name => @requester.name, - :status => "pending", :desired_name => "abc" ) CurrentUser.user = @admin @@ -50,37 +49,12 @@ class UserNameChangeRequestTest < ActiveSupport::TestCase end end - context "rejecting a request" do - setup do - @change_request = UserNameChangeRequest.create( - :user_id => @requester.id, - :original_name => @requester.name, - :status => "pending", - :desired_name => "abc" - ) - CurrentUser.user = @admin - end - - should "create a dmail" do - assert_difference("Dmail.count", 1) do - @change_request.reject!("msg") - end - end - - should "preserve the username" do - @change_request.reject!("msg") - @requester.reload - assert_not_equal("abc", @requester.name) - end - end - context "creating a new request" do should "not validate if the desired name already exists" do assert_difference("UserNameChangeRequest.count", 0) do req = UserNameChangeRequest.create( :user_id => @requester.id, :original_name => @requester.name, - :status => "pending", :desired_name => @requester.name ) assert_equal(["Desired name already exists"], req.errors.full_messages)