Redirect in destroy action not working properly-CodePudding

I am building a simple blog app using Ruby on Rails that allows users to log in/out, sign up and perform actions on their articles and profiles based on permissions and restrictions. I have encountered a problem with the destroy User action. In the users/index view(where all existing users are listed), it causes no errors due to the url path containing no {:id}, but the redirect_to root_path does not work. If the same action is executed in the users/show page(personal profile page with some info and associated articles), due to the url being localhost/users/id, when the user is deleted I get "Couldn't find User with 'id'=33" error shown below. If I manually go to the root route, the successful account deletion message shows up and the action is performed correctly. So this is not a metter of DESTROY not working, but of redirection I believe. I have tried redirecting to different paths but it still doesn't work. Here are the related files:

routes.rb

Rails.application.routes.draw do
root "pages#home"
get "about", to: "pages#about"

resources :articles

get "signup", to: "users#new"
resources :users, except: [:new] 

get 'login', to: 'sessions#new'
post 'login', to: 'sessions#create'
get 'logout' => :destroy, to: 'sessions#destroy'
end

pages_controller

class PagesController < ApplicationController
def home
    redirect_to articles_path if logged_in?
end

def about
end         
end

users_controller

class UsersController < ApplicationController
before_action :set_user, only: [:show, :edit, :update, :destroy]
before_action :require_user, only: [:edit, :update]
before_action :require_same_user, only: [:edit, :update, :destroy]

def index
    @users = User.all
end

def show
    @articles = @user.articles
end

def new
    @user = User.new
end

def edit
end

def create
    @user = User.new(user_params)
    if(@user.save)
        session[:user_id] = @user.id  #logs user in automatically once they are signed up
        flash[:notice] = "Welcome to AlphaBlog, #{@user.username}!"
        redirect_to articles_path
    else
        render 'new'
    end
end

def update
    if @user.update(user_params)
        flash[:notice] = "Account updated!"
        redirect_to @user
    else
        render 'edit'
    end
end

def destroy
    @user.destroy
    session[:user_id] = nil
    flash[:notice] = "Account and all associated articles deleted!"
    redirect_to root_path
end

private
def user_params
    params.require(:user).permit(:username, :email, :password)
end

def set_user
    @user = User.find(params[:id])
end

def require_same_user
    if current_user != @user
        flash[:alert] = "You can only edit your own profile!"
        redirect_to current_user
    end
end

end

sessions_controller

class SessionsController < ApplicationController

def new
end

def create
    user = User.find_by(email: params[:session][:email].downcase)
    if user && user.authenticate(params[:session][:password])
        session[:user_id] = user.id
        flash[:notice] = "Logged in successfully!"
        redirect_to user
    else
        flash.now[:alert] = "There was something wrong with your login details!"
        render 'new'
    end

end

def destroy
    session[:user_id] = nil
    flash[:notice] = "Logged out."
    redirect_to root_path
end

end

users/index.html.erb

<div class = "header">
<h1>
    AlphaBlog
    <% if logged_in? %>
        <%= link_to 'Articles', articles_path, method: :get, class: "index-button-to" %>
    <% else %>
        <%= link_to 'Home', root_path(), method: :get, class: "index-button-to" %>
        <%= link_to 'Articles', articles_path, method: :get, class: "index-button-to" %>
    <% end %>
    <%= render 'layouts/log_in_out_navigation'%>
</h1>
</div>

<h2>Alpha Bloggers</h2>

<div >
<%# cycle through all articles and show them all in a table %>
<% @users.each do |user| %>

    <div class = "index-article-container">

        <div  style = "color:rgb(16, 136, 255);">
            <%= user.username %>
        </div>

        <div >

            <div >
                <%= gravatar_for(user, size: 150) %>
            </div>

            <div >
                <%# gives the plural word for multiple articles %>
                <%= pluralize(user.articles.count, "article") %>
            </div>

            <div >
                <%# shows selected article page %>
                <%= link_to 'View Profile', user, class: "index-link-to show" %>
                <% if logged_in? && current_user.username == user.username %>
                    <%# shows selected article EDIT page. edit_article_path because in routes, 
the prefix for edit is edit_article && (article) because we need the id for the path as well%>
                    <%= link_to 'Edit Profile', edit_user_path(user), data: { turbo_method: 
:get}, class: "index-link-to edit" %>
                    <%= link_to 'Delete Profile', user_path(current_user), data: { 
turbo_method: :delete, turbo_confirm: "Are you sure? (This will also delete all of your 
articles)" }, class: "index-link-to delete" %>
                <% end %>
            </div>
                    
        </div>
                    

        <div >
            Joined <%= time_ago_in_words(user.created_at) %> ago.
        </div>

    </div>
    
<% end %>

users/show.html.erb

<div class = "header">
<h1>
    AlphaBlog
    <% if logged_in? %>
        <%= link_to 'Articles', articles_path, method: :get, class: "index-button-to" %>
        <%= link_to 'Bloggers', users_path, method: :get, class: "index-button-to" %>
    <% else %>
        <%= link_to 'Home', root_path(), method: :get, class: "index-button-to" %>
        <%= link_to 'Articles', articles_path, method: :get, class: "index-button-to" %>
        <%= link_to 'Bloggers', users_path, method: :get, class: "index-button-to" %>
    <% end %>
    <%= render 'layouts/log_in_out_navigation'%>
</h1>
</div>

<h2> <%= @user.username %>'s profile </h2>

<div >
<%# gravatar_for method created in helpers/application_helper %>
<%= gravatar_for @user, size: 200 %>
<% if logged_in? && current_user.username == @user.username %>
    <div >
        <%= link_to "Edit Profile", edit_user_path(@user), class: "index-link-to edit" %>
        <%= link_to 'Delete Profile', user_path(current_user), data: { turbo_method: :delete, 
turbo_confirm: "Are you sure? (This will also delete all of your articles)" }, class: "index- 
link- 
to delete", style: "margin-top:0.3vh" %>
    </div>
<% end %>
</div>

<h3 style = "text-align:center">Articles</h3>
<%= render 'articles/article' %>

error page

CodePudding user response：

I think the answer here is really a very different layout of your routes and controller (or to not reivent the wheel in the first place). Passing the user id through the parameters would be fine if your making a system where you are managing other users - but its pretty wonky when users are managing their own profiles.

For example this is how users CRUD their own profiles in a vanilla Devise setup:

Verb    URI Pattern               Controller#Action                    
------------------------------------------------------------------------
GET     /users/cancel(.:format)   devise/registrations#cancel 
GET     /users/sign_up(.:format)  devise/registrations#new
GET     /users/edit(.:format)     devise/registrations#edit
PATCH   /users(.:format)          devise/registrations#update                                                                          
PUT     /users(.:format)          devise/registrations#update                                                                          
DELETE  /users(.:format)          devise/registrations#destroy                                                                         
POST    /users(.:format)          devise/registrations#create

Note the lack of the :id parameter in the URI Pattern. Thats because its implied that the resource in question is the currently signed in user, and that the user is identified through the session (or a token).

The controller is named Registrations to avoid the ambiguity if the programmer later wants to add a UsersController to manage other users.

If you want to do something similiar you can generate singular routes by using the resource macro instead of resources.

# routes for user registration
resource :registrations, 
    only: [:new, :edit, :update, :create, :destroy]

# routes for viewing other users 
resources :users, only: [:index, :show]

Which will generate:

Prefix                Verb   URI Pattern                    Controller#Action
-----------------------------------------------------------------------
new_registrations     GET    /registrations/new(.:format)   registrations#new
edit_registrations    GET    /registrations/edit(.:format)  registrations#edit
registrations         GET    /registrations(.:format)       registrations#show    
                      PATCH  /registrations(.:format)       registrations#update  
                      PUT    /registrations(.:format)       registrations#update  
                      DELETE /registrations(.:format)       registrations#destroy
                      POST   /registrations(.:format)       registrations#create

Name it whatever you want. The core takeaway here is to not confuse two completely different problems - user management and user registrations and have separate endpoints and controllers for each responsibilty.

Then in your controller you simply authenticate the user from the session and redirect the user if they are not authenticated:

# Handles user account registration, updates and deleting accounts
class RegistrationsController < ApplicationController
  before_action :require_user, except: [:new, :create]

  # Displays the form for signing up a user
  # GET /registrations
  def new
    @user = User.new
  end

  # Register a new user and sign them in
  # POST /registrations
  def create
    @user = User.new(user_params)
    if @user.save 
      reset_session # avoids session fixation attacks
      session[:user_id] = @user.id  #logs user in automatically once they are signed up
      flash[:notice] = "Welcome to AlphaBlog, #{@user.username}!"
      redirect_to articles_path
    else
      render :new
    end
  end

  # Form for editing the users own profile
  # GET /registrations/edit
  def edit
    @user = current_user
  end

  # Update the currently signed in user
  # PATCH /registrations
  def update
    @user = current_user
    if @user.update(user_params)
      flash[:notice] = "Account updated!"
      redirect_to current_user
    else
      render :new
    end
  end

  # Cancel the current users registration 
  # DELETE /registrations
  def delete
    current_user.delete
    reset_session # avoids session fixation attacks
    flash[:notice] = "Account and all associated articles deleted!"
    redirect_to root_path
  end
  
  private 

  # this should be moved up to the parent controller or be extracted into a concern
  def current_user
    @current_user ||= User.find(session[:user_id]) if session[:user_id]
  end 

  def user_params
    params.require(:user).permit(:username, :email, :password)
  end
end

# Displays users
# Managing accounts is handled by RegistrationsController
class UsersController < ApplicationController
  # GET /users
  def index
    @users = User.all
  end
  
  # GET /users/1
  def show
    @user = User.find(params[:id])
    @articles = @user.articles
  end
end

Since their is no id in the path you you need to set the delete button to send to the right path:

<%= button_to "Delete your account", registrations_path, method: :delete %>

And adjust your forms:

<%= form_with(model: @user, url: registrations_path) do |form| %>
  # ...
<% end %>

Doing this the correct way would really be to do it the same way that Devise does and have a normal link that sends a GET request to a "Are you sure you want to delete your account?" page when then requires the user to enter their password or email and submits a DELETE request so that users don't accidentially delete their accounts.

But then again don't reinvent the authentication wheel unless you want a long and tedious lesson into wheelmaking.

CodePudding user response：

So I finally solved this myself using a simplified version of max's answer.

The error was happening due to the fact that the destroy action in users_controller assigned a value of nil to the session's user id in this line

session[:user_id] = nil

This was causing the application to throw that ugly error, although the action itself was being performed correctly, because a user_id is needed in users_controller's other actions (in the error's case, set_user.

I solved the problem by taking the destroy action out of users_controller and into sessions_controller as destroy_user, this way it had a sort of independence from the rest of the users_controller code, and created a specific route for this action.

So to sum it up:

1.Remove destroy from users_controller

2.Add destroy_user in sessions_controller as follows:

def destroy_user
    @user = User.find_by(id: session[:user_id])
    if current_user != @user
        flash[:alert] = "You can only edit your own profile!"
        redirect_to current_user
    end
    @user.destroy
    session[:user_id] = nil
    flash[:notice] = "Account and all associated articles deleted!"
    redirect_to root_path
end

3.Add this path to routes.rb

get 'destroy_user' => :destroy, to: 'sessions#destroy_user'

4.Change the Delete Profile links into this:

<%= link_to 'Delete Profile', destroy_user_path(current_user), data: { 
turbo_method: :delete, turbo_confirm: "Are you sure? (This will also delete all 
of your articles)" }, class: "index-link-to delete" %>