ranked-modelを使うときは :with_sameオプションを付けよう

前置き

自作中のTODOアプリではタスクの表示順を自由に入れ替えできるようになっています。

f:id:naito-coding0322:20180922192059p:plain

indexページには User の Task 一覧を表示

表示の順番はranked-model gemを使って :row_order の値で制御

それぞれの Task はjQueryUIのsortableを導入してドラッグ&ドロップで入れ替えが可能

という感じです。

class Task < ApplicationRecord
  include RankedModel
  belongs_to :user
  scope :donetask,  -> { where(:done => false ) }
  ranks :row_order, :scope => :donetask
end


問題点

並び替えが上手くいかないときがある。

並び替えをしてページを更新すると、ひとつ下にズレたりすることがたびたびありました。
ログを見ると一応 :row_order の値はきちんと更新されています。


原因

新規にテストユーザーを作成し、タスクを4つ作成しました。
DB Browserで確認します。
f:id:naito-coding0322:20180922193453p:plain

1番目のタスクを1つ下(2と3のあいだ)へ移動すると :row_order の値が更新されます。 f:id:naito-coding0322:20180922193637p:plain

本来であれば、1番目のタスクの:row_order の値は「1162961895」と「1655222771」の間で設定されなければいけません。

では「-2146624780」は何かというと、これはデータベースに登録されたすべての Task のなかで上から2番目に位置する数値です。
(実際には「すべての未完了なタスク」が正しいです。Taskモデルの画像参照。) データベース上の全てのタスクの中ではなく、そのユーザーが持っているタスクの中で順番を変更しないといけません。

対処法

ranks :row_orderに :with_same => :(親モデルの外部キー) というオプションを付け足します。

class Task < ApplicationRecord
  include RankedModel
  belongs_to :user
  scope :donetask,  -> { where(:done => false ) }
  ranks :row_order, :scope => :donetask, :with_same => :user_id ←これ
end

これでユーザーごと、つまり同じ user_id を持った複数の Task ごとにrow_orderの数値が割り振られるようになります。

長々と書きましたが、単純に「gemを使うときはREADMEを必ず読もうぜ」という話ですね。気をつけよう

テンプレート側で出力する値の処理を行うのは避ける

表示に必要なデータはプログラム側で用意し、
テンプレートではデータを埋め込む場所や表示方法などを記述する。

前置き

「そのタスクがいくつのサブタスクを保持しているか」をindex画面に表示しています。
f:id:naito-coding0322:20180919155248p:plain

この部分のコードが以下です。

<td><%= task.subtasks.count unless task.subtasks.empty? %><%= "-" unless task.subtasks.present? %></td>


問題点

これだとテンプレート側で出力に必要なデータの処理を行っているため、ビューとコントローラの役割分担が上手くできていません。
冒頭に述べたように、テンプレートではあらかじめ用意されたデータの「埋め込み場所や表示方法」を記述するのが正しい使い方です。


解決策

該当部分をメソッドに切り出します。テンプレートで使うメソッドなのでhelperに定義します。

module TasksHelper

  #タスクにサブタスクがあれば数を表示
  def subtask_present?(task)
    if task.subtasks.present?
      task.subtasks.count
    else  
      "-"
    end
  end

end

これでindexページでの記述が以下のようにシンプルになり、処理された結果を表示するだけになりました。

<td><%= subtask_present?(task) %></td>


今回はtasks#indexでしか使わないヘルパーメソッドなのでtasks_helperに記述しましたが、アプリケーション全体で使うようであればapplication_helperに記述しましょう。

繰り返されるインスタンス変数の初期化をメソッドに切り出す

前置き

作成中のTODOアプリでは、ユーザーがタスクを複数所有し、それぞれのタスクがサブタスクを複数所有しています。

アクションメソッド(数個だけ抜粋)

  def show
    @user = User.find(params[:user_id])
    @task = Task.find(params[:task_id])
    @subtask = Subtask.find(params[:id])
  end

  def update
    @user = User.find(params[:user_id])
    @task = Task.find(params[:task_id])    
    @subtask = Subtask.find(params[:id])
    
    if @subtask.update(subtask_params)
      redirect_to user_task_path(@user, @task, @subtask)
    end
  end
  
  def destroy
    @user = User.find(params[:user_id])
    @task = Task.find(params[:task_id])
    @subtask = Subtask.find(params[:id])
    
    @subtask.destroy
    redirect_to user_task_path(@user, @task)
  end

今回はこれらのコードをリファクタリングしていきます。


問題点

OnceAndOnlyOnce原則に反していることです。
インスタンス変数の初期化が繰り返し記述されています。


対処

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

このようなprivateメソッドを作ってbefore_actionで適用させれば、記述はこの一度で済ませることができます。

対処後

class SubtasksController < ApplicationController
 before_action :set_user
 before_action :set_task
 before_action :set_subtask

  def show
  end

  def update
    if @subtask.update(subtask_params)
      redirect_to user_task_path(@user, @task, @subtask)
    end
  end
  
  def destroy
   @subtask.destroy
    redirect_to user_task_path(@user, @task)
  end


  private
      def set_user
        @user = User.find(params[:user_id])
      end
      
      def set_task
        @task = Task.find(params[:task_id])
      end
      
      def set_subtask
        @subtask = Subtask.find(params[:id])
      end
end

ただこのset_○○には色々な考え方があり、

  • 機能が増えていくにつれてbefore_actionが大量に増えてしまう

  • before_actionの並び順によってエラーが起きる可能性もある

  • アクションをパッと見た時にどのようなインスタンス変数を扱うのかがすぐ分からない

という問題もあるようです。今回のようにシンプルなアプリケーションであれば上記のやり方で良いと思います。
インスタンス変数の初期化の処理が複雑なのであれば、find_○○というメソッドを定義してそれぞれのアクションの中で使うと分かりやすくなります。

参考リンク

ソースコードの減らし方 - 基本的な考え方と10個の方法 - クラウドワークス エンジニアブログ

Rails の before_action :set_* って必要? - ネットの海の片隅で

入力したテキストの改行をviewに反映する方法

f:id:naito-coding0322:20180913132958p:plain このように、テキストの改行が反映されるようになります。

simple_formatメソッドを使う

このメソッドは与えられた文字列に対して

文字列を<p>で括る
改行は<br/>を付ける
連続した改行は</p><p>に変換

という処理を行います。

ただ、HTMLタグなどをサニタイズする働きもあるようです。

Railsソースコード

    def simple_format(text, html_options = {}, options = {})
      wrapper_tag = options.fetch(:wrapper_tag, :p)

      text = sanitize(text) if options.fetch(:sanitize, true)
      paragraphs = split_paragraphs(text)

      if paragraphs.empty?
        content_tag(wrapper_tag, nil, html_options)
      else
        paragraphs.map! { |paragraph|
          content_tag(wrapper_tag, raw(paragraph), html_options)
        }.join("\n\n").html_safe
      end
    end


ということは、前回の記事で行ったテキストへの処理が無効化されてしまいます。
(テキスト内のURLがaタグに変換されるようにする - naito-coding0322’s diary)

筆者の場合は、リンクをクリックしたときに別ページでリンク先を開いてほしいのに、 target="_blank"が無効化されてしまい困っていました。


解決方法

オプションでsanitizeをfalseにすることができます!

<%= simple_format(文字列, {}, sanitize: false) %>


前回の記事に反映させると

<%= simple_format(convert_url_into_a_tag(h(@task.description)).html_safe, {}, sanitize: false) %>

これで、文中のURLをaタグに変換したテキストの改行を反映してviewで表示することができました。

もちろんですがsimple_formatのサニタイズをfalseにするのは、その前段階でテキストにhtml_escapeメソッドを使ってユーザーからの入力に対処していることが前提です。

テキスト内のURLがaタグに変換されるようにする

目的

タスク管理アプリのタスクの説明部分にリンクを追加できるようにしたい。

つまり、text_areaなどで入力したテキストを表示する際にURLを含んでいたら自動的にリンクを作るようにしたい、ということです。

helperメソッドを作成

URIライブラリを使います。

module ApplicationHelper
require 'uri'  
  def convert_url_into_a_tag(text)
    URI.extract(text, ['http', 'https']).uniq.each do |url|
      sub_text = ""
      sub_text << "<a href=" << url << " target=\"_blank\">" << url << "</a>"

      text.gsub!(url, sub_text)
    end
   return text
  end
end

詳しい解説

URI.extract(text, ['http', 'https']).uniq.each do |url|

第一引数に与えられたテキスト(text)のなかから第二引数に与えられたスキーム名(httpとhttps)にマッチするものを探し出し、.uniqで重複を削除してurlとしてブロックが受け取ります。

 sub_text = ""
 sub_text << "<a href=" << url << " target=\"_blank\">" << url << "</a>"

 text.gsub!(url, sub_text)

ブロック内の処理は、用意した空のローカル変数sub_textにそれぞれの文字列を追加していきtext.gsub!(url, sub_text)でtext内のurlに該当する部分をsub_textに置き換えます。

注意
レシーバ.gsub(引数) → 置き換えを行った新しい文字列を返す
レシーバ.gsub!(引数) → レシーバ自身を置き換えて返す

最終的に、return textで「URLをaタグに置き換えたテキスト(text)」を返します。

使い方

以下のように記述して使います。

 <%= convert_url_into_a_tag(h(@task.description)).html_safe %>

h と html_safe

(h(@task.description)).html_safe

hというのはhtml_escapeメソッドの略名で、HTMLタグなどをエスケープするメソッドです。

.html_safeというのは、「レシーバに含まれる文字列は問題ないのでエスケープしないでね」というメソッドです。

2つは反対の意味を持つメソッドのようですが、なぜ一緒に使われているのでしょうか……

セキュリティ対策としてとても重要

まず、tasksコントローラから渡された@taskのdescription(タスクの説明文)から、html_escapeメソッドによってHTMLタグがエスケープされます。
これはユーザーが悪意のあるスクリプトを埋め込む事を防止する意味合いがあります。(クロスサイトスクリプティングXSS

そうしてXSSの対策をしたあとで、convert_url_into_a_tagメソッドによってテキスト内のURLをaタグに置き換えます。
そのときにつけたHTMLタグは安全ですので、html_safeメソッドで有効化する、という流れです。

ユーザーによる入力をエスケープした上で、改めてこちらでaタグを付与する ということですね。

参考リンク

[Ruby][Rails]テキスト内のURLをaタグに書き換える

[Rails]ERBのエスケープを自在に扱おうぜ

「オブジェクト指向でなぜつくるのか」を読んだ

オブジェクト指向に対する誤解が解けた

プログラミング言語は、機械語によるプログラミングからアセンブリ言語高級言語→構造化言語というように、「より便利に」を目指してそれぞれの課題を解決するように進化してきました。
なかでも「無駄を省く」「保守と再利用をしやすいようにする」という課題の解決に向かって生まれたのがオブジェクト指向プログラミングです。
あくまでもプログラミング技術の1つの到達点であり、現在もさらに便利になるように新しい理論や言語が開発され続けています。


オブジェクト指向は2つの側面を持つ

オブジェクト指向プログラミングの考え方は図式表現、モデリング、設計などの上流工程の分野にも応用されました。
オブジェクト指向」という言葉は下流工程の「プログラミング技術」と上流工程の「汎用の整理術」の2つの側面を持っています。
この2つを混同してしまうことがオブジェクト指向の理解を難しいものにしており、この本は2つを明確に区別して書かれています。


それぞれの技術・手法について

クラス、ポリモーフィズム、継承などの要素について、第五章で静的領域、ヒープ領域、スタック領域からなるプログラムのメモリ領域がOOPではどのように管理/活用されているかという内部の動きを解説することで、それぞれの要素を抽象的な概念ではなくシステムとしての働きとして理解できるようになっています。

上流工程についても、業務分析・要求定義・設計の一般的な説明をしたうえで、実際にUMLを活用して図書館の貸出業務をモデリングするプロセスを3ステップに分けて解説してくれるので、ついていくことができました。


感想

初学者の自分はオブジェクト指向とは「なにか全く新しい完成した万能の技術」だと思っていましたが、その誤解が解けたのがいちばんの収穫でした。
以前のプログラミング技術ではどんな課題があってオブジェクト指向がそれをどう解決したか、という視点でオブジェクト指向プログラミングを解説している点が分かりやすかったです。
パソコンの中で動くプログラムだけでなく、実際の開発業務の流れさえもそれぞれをリソースとして捉え、より効率的に行うことを目指して世界中でノウハウが共有されていることに驚きました。プログラマーには「無駄を省いて効率的に」を追求し続ける精神が必要なんだと感じました。

ネストしたリソースのユーザー制御

完成したタスク管理アプリに、「ひとつのタスク内にサブタスクを作成できる機能」をアップデートで追加しました。

Subtaskモデルを作成したので、現在ルーティングはこうなっています。

Rails.application.routes.draw do
 
  get 'static_pages/home'
  root to: "static_pages#home"
  devise_for :users

  resources :users, only: [:index, :show] do  
    resources :tasks do
      put :sort
        resources :subtasks, only: [:create, :new, :show, :update, :destroy]  do
          put :sort
        end
    end
  end

end

コントローラーのアクションもひと通り実装し、機能テストも作成しました。
ユーザー制御がややこしかったので、失敗するテストを先に書いてTDDで進めました。
例えばsubtasks#updateのURLは

/users/:user_id/tasks/:task_id/subtasks/:id

というふうに ID を3つ使います。
サブタスクモデルはtask_idでタスクモデルと紐付き、タスクモデルはuser_idでユーザーモデルと紐付いています。
user_idのみでユーザー制御をするとURLのtask_idを書き換えた場合他人の情報にアクセスできてしまうという問題がありました。

そして、考えに考えた結果実装した before_actionのメソッドが以下です。

    #正しいユーザーかどうか確認する
    def correct_user
       if params[:id].present?
         @subtask = Subtask.find(params[:id])
         @task = Task.find(@subtask.task_id)
         redirect_to(root_path, notice: "URLが間違っています") unless @task.user_id == current_user.id
       else
         @task = Task.find(params[:task_id])
         @user = User.find(@task.user_id)
         redirect_to(root_path, notice: "URLが間違っています") unless @user == current_user
       end
     end

長いし、モデルを追加するたびにこの処理も追加しなくてはいけないのかと思うと先が思いやられます。
モデル側のバリデーションという形でユーザー制御を実施できるというような情報をチラッと見たので調べてみます。

テストファイル内の個々のテストやsetup、コントローラーのprivateアクションなどが複雑になってきました。
リファクタリングの必要がありそうです。


ちなみにPATCHリクエストを送る際、パラメータのtask_idを他人のIDに変更して送信できてしまうと結果として「他人のサブタスクを作成できてしまう」
状態になるのですが、それはストロングパラメーターの仕組みによって制御されています。

      def subtask_params
        params.require(:subtask).permit( :title, :description, :row_order_position)
      end