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.

Sign up for GitHub

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

管理者の場合、ユーザー情報変更ページでコース変更できるようにした #4596

Merged
merged 6 commits into from
Apr 17, 2022
Merged

管理者の場合、ユーザー情報変更ページでコース変更できるようにした #4596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2cc338f
ユーザー情報変更ページの管理者が変更できる項目に、コースを追加
Saki-htr Apr 7, 2022
41f5e07
不要な{}を削除
Saki-htr Apr 9, 2022
e552597
管理者が登録情報変更ページで、コース変更できることを確認するテストを追加
Saki-htr Apr 10, 2022
1a89100
findよりreloadを使ったほうが、シンプルかつuserのデータを再読み込みしているとすぐに理解できるため、変更
Saki-htr Apr 12, 2022
935a07a
Choices-jsを使って、インクリメンタルサーチできる仕様にする際に必要になるため、{}を再追加
Saki-htr Apr 12, 2022
16b7905
管理者以外の一般ユーザーは、ユーザーのコースを変更できないテストを追加
Saki-htr Apr 17, 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
1 change: 1 addition & 0 deletions app/views/users/_form.html.slim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
- if admin_login?
.form__items
h3.form__items-title 以下管理者のみ操作ができます
= render 'users/form/course', f: f
= render 'users/form/job_seeking', f: f
= render 'users/form/company', f: f
= render 'users/form/retire', f: f
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/form/_course.html.slim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.form-item.is-hidden
.form-item
= f.label :course_id, class: 'a-form-label is-required'
.a-button.is-md.is-secondary.is-select.is-block
= f.collection_select :course_id, Course.order(:created_at), :id, :title, {}
Copy link
Contributor

Choose a reason for hiding this comment

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

{}は削除しても動作に問題がなかったため、不要と考え削除しました。

余談なんですが、以前レビューさせてもらった、#4385のapp/views/users/form/_company.html.slim で{}がないと、その後ろに記述されるHTML属性が効かなかったので、今後、Choices-jsを使って、インクリメンタルサーチできる仕様にする可能性ありそうな気がしたので、残しておいてもいいのかな〜って一瞬思ったのですが、現状なくても動くので問題ないと思います…！

参考にしたサイト

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{}は、Choices-jsを使って、インクリメンタルサーチできるようにする際に必要なのですね〜
勉強になります。ありがとうございます！

  • 今は問題なく動いているので、削除しておくか
  • 今後、Choices-jsを使って、インクリメンタルサーチできる仕様にする可能性があるので削除しないでおくか

で非常に迷ったのですが、今後に備えて残しておくことにしました。
61e71e5のコミットで修正しましたので、ご確認をお願いいたします🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

戻して頂いたんですね…！ありがとうございます🙏

{}は、Choices-jsを使って、インクリメンタルサーチできるようにする際に必要なのですね〜

Choices-jsに限らず、javascriptとかで何かイベントを発火させたい時に、記述の順番として
HTML属性={} or イベント属性={}の指定の前にはオプション={}が必要そうな感じがしました…！
もしかしたら、なくても内容によっては動くかもしれませんが😅

参考にしたサイトより↓

f.collection_select(メソッド名, 要素の配列, value属性の項目, テキストの項目, オプション={}, HTML属性={} or イベント属性={})


私が以前レビューさせてもらったissueの場合だと、company-select.jsの4行目のgetElementById('js-company-select')を _company.html.slimの= f.collection_select に指定した時に{}を外すと動かなかったんですよね〜。

bootcamp/app/javascript/company-select.js

Lines 3 to 15 in a55ba9f

document.addEventListener('DOMContentLoaded', () => {
const element = document.getElementById('js-company-select')
if (element) {
return new Choices(element, {
removeItemButton: true,
allowHTML: true,
searchResultLimit: 10,
searchPlaceholderValue: '検索ワード',
noResultsText: '一致する情報は見つかりません',
itemSelectText: '選択'
})
}
})

bootcamp/app/views/users/form/_company.html.slim

Line 4 in a55ba9f

= f.collection_select :company_id, all_companies_with_empty, :id, :name, {}, { id: 'js-company-select' }

Expand Down
17 changes: 17 additions & 0 deletions test/system/admin/users_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,4 +216,21 @@ class Admin::UsersTest < ApplicationSystemTestCase
assert has_checked_field?('user_trainee', visible: false)
assert has_field?('user_training_ends_on', with: '')
end

test 'admin can change user course' do
Copy link
Member

Choose a reason for hiding this comment

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

逆に一般ユーザーで変更できると困るので、「一般ユーザーは変更できない」というテストもあるとありがたいです〜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata さん
仰る通り、「一般ユーザーは変更できない」テストも追加したほうが良いと思いましたので、追加いたしました。
16b7905で変更しております。

テスト名ですが、'not admin cannot change user course' ですと、2つ否定が入っていて分かりづらいと思いましたので、 'general user cannot change user course' という名前にしました。
よろしくお願いいたします🙏

user = users(:kensyu)
visit_with_auth "/admin/users/#{user.id}/edit", 'machida'
within 'form[name=user]' do
select 'iOSプログラマー', from: 'user[course_id]'
end
click_on '更新する'
assert_equal 'iOSプログラマー', user.reload.course.title
end

test 'general user cannot change user course' do
user = users(:kensyu)
visit_with_auth "/admin/users/#{user.id}/edit", 'kimura'
assert_current_path('/')
assert_text '管理者としてログインしてください'
end
end