Merge pull request #5327 from consuldemocracy/users_url

Add a username slug to the user URL

app/controllers/direct_messages_controller.rb

@@ -1,31 +1,40 @@
 class DirectMessagesController < ApplicationController
-  load_and_authorize_resource
+  before_action :authenticate_user!
+  load_and_authorize_resource :user, instance_name: :receiver
+  before_action :check_slug
+  load_resource through: :receiver, through_association: :direct_messages_received
+  authorize_resource except: :new
 
   def new
-    @receiver = User.find(params[:user_id])
-    @direct_message = DirectMessage.new(receiver: @receiver)
+    authorize! :new, @direct_message, message: t("users.direct_messages.new.verified_only",
+                                                 verify_account: helpers.link_to_verify_account)
   end
 
   def create
-    @sender = current_user
-    @receiver = User.find(params[:user_id])
+    @direct_message.sender = current_user
 
-    @direct_message = DirectMessage.new(parsed_params)
     if @direct_message.save
       Mailer.direct_message_for_receiver(@direct_message).deliver_later
       Mailer.direct_message_for_sender(@direct_message).deliver_later
-      redirect_to [@receiver, @direct_message], notice: I18n.t("flash.actions.create.direct_message")
+
+      redirect_to user_direct_message_path(@receiver, @direct_message),
+                  notice: I18n.t("flash.actions.create.direct_message")
     else
       render :new
     end
   end
 
   def show
-    @direct_message = DirectMessage.find(params[:id])
   end
 
   private
 
+    def check_slug
+      slug = params[:user_id].split("-", 2)[1]
+
+      raise ActiveRecord::RecordNotFound unless @receiver.slug == slug.to_s
+    end
+
     def direct_message_params
       params.require(:direct_message).permit(allowed_params)
     end
@@ -33,8 +42,4 @@ class DirectMessagesController < ApplicationController
     def allowed_params
       [:title, :body]
     end
-
-    def parsed_params
-      direct_message_params.merge(sender: @sender, receiver: @receiver)
-    end
 end

app/controllers/users_controller.rb

@@ -1,5 +1,6 @@
 class UsersController < ApplicationController
   load_and_authorize_resource
+  before_action :check_slug
   helper_method :valid_interests_access?
 
   def show
@@ -8,6 +9,12 @@ class UsersController < ApplicationController
 
   private
 
+    def check_slug
+      slug = params[:id].split("-", 2)[1]
+
+      raise ActiveRecord::RecordNotFound unless @user.slug == slug.to_s
+    end
+
     def valid_interests_access?(user)
       user.public_interests || user == current_user
     end

app/models/abilities/everyone.rb

@@ -18,7 +18,6 @@ module Abilities
       can :read_results, Budget, id: Budget.finished.results_enabled.ids
       can :read_stats, Budget, id: Budget.valuating_or_later.stats_enabled.ids
       can :read_executions, Budget, phase: "finished"
-      can :new, DirectMessage
       can [:read, :debate, :draft_publication, :allegations, :result_publication,
            :proposals, :milestones], Legislation::Process, published: true
       can :summary, Legislation::Process,

app/models/user.rb

@@ -433,6 +433,14 @@ class User < ApplicationRecord
     (Tenant.current_secrets.dig(:security, :lockable, :unlock_in) || 1).to_f.hours
   end
 
+  def to_param
+    "#{id}-#{slug}"
+  end
+
+  def slug
+    username.to_s.parameterize
+  end
+
   private
 
     def clean_document_number

app/views/direct_messages/new.html.erb

@@ -6,21 +6,7 @@
       <%= t("users.direct_messages.new.title", receiver: @receiver.name) %>
     </h1>
 
-    <% if not current_user %>
-      <div class="callout primary">
-        <p>
-          <%= sanitize(t("users.login_to_continue",
-                         signin: link_to_signin,
-                         signup: link_to_signup)) %>
-        </p>
-      </div>
-    <% elsif not @receiver.email_on_direct_message? %>
-      <div class="callout primary">
-        <p>
-          <%= t("users.direct_messages.new.direct_messages_bloqued") %>
-        </p>
-      </div>
-    <% elsif can? :create, @direct_message %>
+    <% if @receiver.email_on_direct_message? %>
       <%= form_for [@receiver, @direct_message] do |f| %>
         <%= render "shared/errors", resource: @direct_message %>
 
@@ -32,10 +18,9 @@
         </div>
       <% end %>
     <% else %>
-      <div class="callout warning">
+      <div class="callout primary">
         <p>
-          <%= sanitize(t("users.direct_messages.new.verified_only",
-                         verify_account: link_to_verify_account)) %>
+          <%= t("users.direct_messages.new.direct_messages_bloqued") %>
         </p>
       </div>
     <% end %>

spec/controllers/direct_messages_controller_spec.rb

@@ -0,0 +1,62 @@
+require "rails_helper"
+
+describe DirectMessagesController do
+  before { sign_in(create :user, :level_two) }
+
+  describe "GET new" do
+    let!(:user) { create(:user, username: "James Jameson") }
+
+    it "finds a user by ID and slug" do
+      get :new, params: { user_id: "#{user.id}-james-jameson" }
+
+      expect(response).to be_successful
+    end
+
+    it "does not find a user by just an ID" do
+      expect do
+        get :new, params: { user_id: user.id }
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+
+    it "does not find a user by just a slug" do
+      expect do
+        get :new, params: { user_id: "james-jameson" }
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+
+    it "does not find a user with the wrong slug" do
+      expect do
+        get :new, params: { user_id: "#{user.id}-James Jameson" }
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+  end
+
+  describe "POST create" do
+    let!(:user) { create(:user, username: "James Jameson") }
+    let(:message_params) { { direct_message: { title: "Hello!", message: "How are you doing?" }} }
+
+    it "finds a user by ID and slug" do
+      post :create, params: message_params.merge(user_id: "#{user.id}-james-jameson")
+
+      expect(response).to be_successful
+    end
+
+    it "does not find a user by just an ID" do
+      expect do
+        post :create, params: message_params.merge(user_id: user.id)
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+
+    it "does not find a user by just a slug" do
+      expect do
+        post :create, params: message_params.merge(user_id: "james-jameson")
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+
+    it "does not find a user with the wrong slug" do
+      expect do
+        post :create, params: message_params.merge(user_id: "#{user.id}-James Jameson")
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+  end
+end

spec/controllers/users_controller_spec.rb

@@ -0,0 +1,39 @@
+require "rails_helper"
+
+describe UsersController do
+  describe "GET show" do
+    let!(:user) { create(:user, username: "James Jameson") }
+
+    it "finds a user by ID and slug" do
+      get :show, params: { id: "#{user.id}-james-jameson" }
+
+      expect(response).to be_successful
+    end
+
+    it "does not find a user by just an ID" do
+      expect do
+        get :show, params: { id: user.id }
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+
+    it "does not find a user by just a slug" do
+      expect do
+        get :show, params: { id: "james-jameson" }
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+
+    it "does not find a user with the wrong slug" do
+      expect do
+        get :show, params: { id: "#{user.id}-James Jameson" }
+      end.to raise_error ActiveRecord::RecordNotFound
+    end
+
+    it "finds users without username by ID" do
+      user.erase
+
+      get :show, params: { id: user.id }
+
+      expect(response).to be_successful
+    end
+  end
+end

spec/models/abilities/common_spec.rb

@@ -92,7 +92,7 @@ describe Abilities::Common do
   it { should_not be_able_to(:comment_as_administrator, proposal) }
   it { should_not be_able_to(:comment_as_moderator, proposal)     }
 
-  it { should     be_able_to(:new,    DirectMessage) }
+  it { should_not be_able_to(:new,    DirectMessage) }
   it { should_not be_able_to(:create, DirectMessage) }
   it { should_not be_able_to(:show,   DirectMessage) }
 

spec/system/direct_messages_spec.rb

@@ -54,7 +54,7 @@ describe "Direct messages" do
       visit new_user_direct_message_path(receiver)
 
       expect(page).to have_content "To send a private message verify your account"
-      expect(page).not_to have_link "Send private message"
+      expect(page).to have_current_path root_path
     end
 
     scenario "User not logged in" do
@@ -62,8 +62,8 @@ describe "Direct messages" do
 
       visit new_user_direct_message_path(receiver)
 
-      expect(page).to have_content "You must sign in or sign up to continue."
-      expect(page).not_to have_link "Send private message"
+      expect(page).to have_content "You must sign in or register to continue."
+      expect(page).to have_current_path new_user_session_path
     end
 
     scenario "Accessing form directly" do