wikiリダイレクトのリセット機能 #11

[ishikawa999]

• WikiRedirectController + destroyを作る
• view 組み込む
• インデックスを貼る
• 権限を設定する
  • 権限あるときのみ、リンクを出す
  • APIで権限を確認する
  • 関係ないプロジェクトのURLで削除できないようにする
• 多用言語化
• 画面に通知を出す

[ishikawa999]

最初テストがうまく動かないことで戸惑っていたが、テストの実行コマンドでRAILS_ENV=testを入れるのを忘れていたのが原因だった。
RAILS_ENV=test bundle exec rake test TEST=test/functional/wiki_redirects_controller_test.rb

現在はdestroyアクションは作ったので、それをViewからaタグでリクエストできるようにするところから次はじめる

画面案

Wiki表示画面の三点リーダ

History
Lock
Rename
+ Testからのリダイレクトをやめる
+ Test2からのリダイレクトをやめる
Delete
New Wiki Page

やること

• テスト(テスト駆動で)
• WikiRedirectsControllerをつくる => Done
• destroyアクションを作る(routes) => Done
• WikiPageに紐付くWikiRedirectを調べる上でredirects_toで検索することになるので、redirects_toにインデックス貼る(?)後からでも良いかも
• destroyアクションを実行するリンクをcontextualの三点リーダメニューの中に入れる

テスト(テスト駆動で)

• fixtureがないとDBがリセットされない
• wiki_redirects fixtureを作るか？
• ちゃんと単体テストするなら

  # allow(WikiRedirect).to receive(:find).with(redirect_id).and_return (mock)
  # allow(mock).to receive(:destroy)
  # DO
  # expect(mock).to have_received(:destroy)

project_redirect_path DELETE /projects/:project_id/wiki/redirects/:id(.:format)
wiki_redirects#destroy
wiki_redirect_path DELETE /wiki_redirects/:id
=> 「プロジェクトを指定していない = 強い権限が必要になる」なので大変になりそう
=> プロジェクトIDを指定する

ID=1でユーザーを指定してもなぜかアノニマスユーザーになる => RAILS_ENV=testをつけていなかったのが原因

[ishikawa999]

あと、権限チェックの部分の実装も

[kfischer-okarin]

@ishikawa999 rebaseはもうしていたので、手元のブランチを作り直してくださいね。

[juno-nishizaki]

第5回パッチ会

やったこと

• destroyアクションを実行するリンクをcontextualの三点リーダメニューの中に入れる
  • wiki_controller_test.rb に view のテストを追加する（ test_show_delete_redirect_links） ⇒ Done
    • 2 回リダイレクトさせた状態で、view の表示が正しいかテストしている
  • 表示内容の実装＆テスト ⇒ Done
  • リンクの実装＆テスト ⇒ WIP

課題

パスが /projects/:project_id/wiki/redirects/:id(.:format) だと、「redirects」という名前の Wiki ページを表示しようとしてしまう。

次にやること

• 正しくアクセスできるパスを検討する
• delete メソッドで呼び出せるように埋め込む

[sanak]

@kfischer-okarin
諸々、動作を確認し、動きとしては問題ないように思いました。

細かいところで、UI(フロントエンド)周りについては、下記の2点が少し気になりました。

---

1. リダイレクト削除メニューのインデント(字下げ)が必要かどうか
   
   (上のコメントに記載の + 部分の箇所になります。)

   Rename
   + Testからのリダイレクトをやめる
   + Test2からのリダイレクトをやめる
   

2. リダイレクト削除時の通知メッセージが、ページ削除時と同じ、削除しました。となるが、 リンクを削除しました。などの別の文言の方が良いかどうか
   

---

上記いずれもUI/UX的に問題なさそうであれば、大丈夫だと思います。

なお、私のRedmine公式サイトのアカウントは、GitHubと同じ@sanakとなりますので、よろしくお願いします 🙇‍♂️

[ishikawa999]

2点ほど細かい気になった点をコメントしました。挙動自体には問題ないと思います。

[ishikawa999]

Suggested change

	<% @redirects_to_self.map { |redirect| %>
	<%= link_to(l(:button_delete_redirect, :page_title => WikiPage.pretty_title(redirect.title)), {:controller => 'wiki_redirects', :action => 'destroy', :project_id => @project.identifier, :wiki_page_id => @page.title, :id => redirect.id}, :method => :delete, :class => 'icon icon-link-break') %>
	<% } %>
	<% @redirects_to_self.map do |redirect| %>
	<%= link_to(l(:button_delete_redirect, :page_title => WikiPage.pretty_title(redirect.title)), {:controller => 'wiki_redirects', :action => 'destroy', :project_id => @project.identifier, :wiki_page_id => @page.title, :id => redirect.id}, :method => :delete, :class => 'icon icon-link-break') %>
	<% end %>

細かいのですが、ここのブロックはdo...endの方が他の書き方にあっていて良いと思います。

[ishikawa999]

• test_destroyとtest_destroy_without_permissionの間で結局何の権限の違いによって結果が違うのかが分かりづらい(rename_wiki_pages権限の有無によって違うはず)
• test_destroyとtest_destroy_without_permissionで別のユーザーを使っているため、rename_wiki_pages権限以外の前提にも違い出てしまっている

という2点が気になりました。
前提(@request.session[:user_id] = 2)は合わせた上で、権限無しのテストでだけ Role.find(1).remove_permission! :rename_wiki_pages という書き方のほうが個人的には好きです。

diff --git a/test/functional/wiki_redirects_controller_test.rb b/test/functional/wiki_redirects_controller_test.rb
index 7949ec3b5..e9ae9f2f7 100644
--- a/test/functional/wiki_redirects_controller_test.rb
+++ b/test/functional/wiki_redirects_controller_test.rb
@@ -31,6 +31,8 @@ class WikiRedirectsControllerTest < Redmine::ControllerTest
   end
 
   def test_destroy
+    @request.session[:user_id] = 2
+
     wiki_page = WikiPage.find(2)
     old_title = wiki_page.title
     wiki_page.title = 'Test'
@@ -45,8 +47,9 @@ class WikiRedirectsControllerTest < Redmine::ControllerTest
     assert_not WikiRedirect.exists?(id: wiki_redirect.id)
   end
 
-  def test_destroy_without_permission
-    @request.session[:user_id] = User.generate!.id
+  def test_destroy_without_rename_wiki_pages_permission
+    @request.session[:user_id] = 2
+    Role.find(1).remove_permission! :rename_wiki_pages # User 2 role
 
     wiki_page = WikiPage.find(2)
     old_title = wiki_page.title

(気になった点としてコメントしただけなので、この書き方でも問題なくコミットされると思います。)

[ishikawa999]

このパッチって何か終わっていない作業がありますか？
ぱっと見投稿しても良さそうに見えます。

[sanak]

すいません、私の方で要らないコメントをしてましたが、良く読み返すと、 + はツリーのインデントでなく、追加の意味でしたね...😓
通知メッセージの方も問題ないと思いますので、残り作業はないと思います。
#22 (comment)

[github-actions]

Patch can be downloaded here