Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ユーザーの検索結果に、管理者でログインしたときだけそのユーザーの相談部屋へのリンクを表示 #4147

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
8790657
first commit
ot0m1 Feb 5, 2022
f859a3f
相談部屋へのリンクを表示
ot0m1 Feb 10, 2022
5b38b4a
冗長な条件分岐をリファクタリング
ot0m1 Feb 10, 2022
b0f7b84
さらにリファクタリング
ot0m1 Feb 10, 2022
1142061
rubocop通した
ot0m1 Feb 10, 2022
4059f8d
さらにリファクタリング
ot0m1 Feb 10, 2022
4c85f20
クリップボードのテストを通るように修正
ot0m1 Feb 14, 2022
1bea353
talk?メソッドの返り値をbooleanのみに
ot0m1 Feb 17, 2022
8e9eea5
メソッド名を具体的に
ot0m1 Feb 17, 2022
11ea071
管理者だけにリンクが表示されるように変更
ot0m1 Feb 17, 2022
9cdc350
不要な条件分岐を削除
ot0m1 Feb 17, 2022
f9d98ee
管理者かを判断するロジックをVue側に持たせた
ot0m1 Feb 17, 2022
65cfe98
テストを追加
ot0m1 Feb 17, 2022
cc300ee
最新の origin/mian を取り込み
ot0m1 Feb 19, 2022
fe5ba3d
テストの条件を変更
ot0m1 Feb 19, 2022
33b922b
ヘルパーのテストを追加
ot0m1 Feb 22, 2022
fba40bd
テスト名を修正
ot0m1 Feb 22, 2022
6522a73
不要なヘルパーメソッドを削除
ot0m1 Feb 22, 2022
0072d92
不要なヘルパーテストを削除
ot0m1 Feb 22, 2022
634086f
テストのパターンを追加
ot0m1 Feb 25, 2022
8a7fa44
不要なスペースを削除
ot0m1 Feb 28, 2022
f6fc8a7
テストが通るように最新の origin/main を取り込み
ot0m1 Mar 1, 2022
7f133ba
テスト名を変更
ot0m1 Mar 2, 2022
028fd7e
テストが通るように最新の origin/main を取り込み
ot0m1 Mar 2, 2022
57c1b36
適切なアサーションに修正
ot0m1 Mar 3, 2022
e2093ac
間違った assert の引数を修正
ot0m1 Mar 7, 2022
ad279f9
冗長な変数宣言を削除
ot0m1 Mar 8, 2022
4b9677e
テストを分割
ot0m1 Mar 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/helpers/search_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,16 @@ def filtered_message(searchable)
def comment_or_answer?(searchable)
searchable.is_a?(Comment) || searchable.is_a?(Answer)
end

def talk?(searchable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

対応するhelperのテストがあるといいかもです〜

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ヘルパーテストを追加するみたいな考えが全くありませんでした🙏
追加しました。

searchable.instance_of?(User) && Talk.find_by(user_id: searchable.id)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おそらくtalk?メソッドはbooleanを戻り値にしたいと思うんですが&&の右辺まで評価が進むと相談部屋オブジェクトが戻り値になるのでちょっと驚きがある気がします!

こんな感じはどうでしょうか?という提案です。またSQL叩かなくて済むというメリットもあります。

def talk?
  searchable.instance_of?(User) && searchable.talk.present?
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

右辺まで評価が進むと相談部屋オブジェクトが戻り値になるのでちょっと驚きがある

確かにその通りですね🙏 修正しました。


def talk_id(searchable)
Talk.find_by(user_id: searchable.id).id
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Viewに関することというよりはインスタンスに依存するコードなのでモデルに書いた方がいい気がするのですが、自信はないです😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

もしくは、現状talk_idというメソッドを使うところが他になさそうなのでjbuilderの方に直接

if talk?(searchable) && display_talk?(searchable)
  json.talk_id searchable.talk.id
end

と記述してもいいかもしれません。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

talk_id というメソッド名がご指摘いただいたようなコードを読んでいてのひっかかりポイントになっているのかなと考え、メソッド名の変更で対応しました。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ot0m1
返事遅くなりました!

伝え方が悪くて申し訳ないです😅

メソッド名が引っかかっているというよりはhelperメソッドは見た目の部分に関することを記述すると思うのでtalk_idを取得するようなコード(SQLを叩くコード)はここに記述しない方がいいのでは?という疑問から発言しました。

2つ目のコメントはtalk_idを返すメソッドにそこまで汎用性がなさそうなのでjbuiderの方に記述すればメソッドを定義しなくても済むのかなぁと思ってコメントさせてもらいました!

Copy link
Contributor Author

@ot0m1 ot0m1 Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User は talk というカラムを持っていないのでメソッドにして Talk.find_by(user_id: searchable.id).id を実行しないといけないと思い込んでいた(そのため #4147 (comment) みたいなコメントをしている)ので、ご指摘の通りに修正いたしました。

モデルで belongs_to :userhas_one :talk, dependent: :destroy という宣言が行われていたのを把握しておらず、ずれた方向でリファクタリングを進めてしまっておりました🙏


def display_talk?(searchable)
current_user.admin? || current_user.id == searchable.id
end
end
8 changes: 8 additions & 0 deletions app/javascript/searchable.vue
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
| {{ searchable.document_author_login_name }}
|  {{ searchable.model_name_with_i18n }}
| )
a(v-if='canDisplayTalk', :href='talkUrl')
| 相談部屋
</template>
<script>
import dayjs from 'dayjs'
Expand All @@ -61,6 +63,12 @@ export default {
'YYYY年MM月DD日(dd) HH:mm'
)
},
canDisplayTalk() {
return this.searchable.model_name === 'user' && this.searchable.talk_id
},
talkUrl() {
return `/talks/${this.searchable.talk_id}`
},
summary() {
const word = this.word
const wordsPattern = word
Expand Down
3 changes: 3 additions & 0 deletions app/views/api/searchables/_searchable.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@ if comment_or_answer?(searchable)
json.document_author_login_name document.user.login_name
json.document_author_id document.user.id
end
if talk?(searchable) && display_talk?(searchable)
json.talk_id talk_id(searchable)
end
13 changes: 8 additions & 5 deletions test/system/comments_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,14 @@ class CommentsTest < ApplicationSystemTestCase
visit_with_auth "/reports/#{reports(:report1).id}", 'komagata'
find('#comments.loaded', wait: 10)
first(:css, '.thread-comment__created-at').click
# クリップボードを直接読み取る方法がないので、未入力のテキストエリアを経由してクリップボードの値を読み取っている
# また、Ctrl-Vではペーストできなかったので、かわりにShift-Insertをショートカットキーとして使っている
# 参考 https://stackoverflow.com/a/57955123/1058763
find('#js-new-comment').send_keys %i[shift insert]
clip_text = find('#js-new-comment').value
# 参考:https://gist.github.com/ParamagicDev/5fe937ee60695ff1d227f18fe4b1d5c4
cdp_permission = {
origin: page.server_url,
permission: { name: 'clipboard-read' },
setting: 'granted'
}
page.driver.browser.execute_cdp('Browser.setPermission', **cdp_permission)
clip_text = page.evaluate_async_script('navigator.clipboard.readText().then(arguments[0])')
assert_equal current_url + "#comment_#{comments(:comment1).id}", clip_text
end

Expand Down