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

コースに表示、非表示の設定ができるようにしたい #3093

Merged
merged 18 commits into from
Sep 25, 2021

Conversation

udaikue
Copy link
Contributor

@udaikue udaikue commented Aug 5, 2021

issue #2812

概要

コースに表示・非表示を設定するようにし、管理者は編集画面で編集できるようにした。

変更後

  • coursesテーブルに表示・非表示を表すカラムを追加した

  • コース一覧でコースの表示・非表示を確認できるようにした(管理者のみ)
    スクリーンショット 2021-09-11 10 09 46

  • コース編集画面でコースの表示・非表示を編集できるようにした(管理者のみ)
    スクリーンショット 2021-09-11 10 12 21

  • 管理者以外の人は非表示のコースは閲覧できないようにした
    スクリーンショット 2021-09-11 10 13 47

@udaikue udaikue self-assigned this Aug 5, 2021
@udaikue udaikue force-pushed the feature/users-can-select-a-course branch 2 times, most recently from 8bc763d to 2a59177 Compare August 13, 2021 08:36
@udaikue
Copy link
Contributor Author

udaikue commented Aug 13, 2021

@machida こちらレビュー前にデザインをお願いできますでしょうか🙏

@machida
Copy link
Member

machida commented Aug 14, 2021

@udaikue cc @komagata
セレクトタグだと、ユーザーはどんな選択肢があるかわからないのと、それぞれの選択肢の内容の説明が見れないので、選択をすることが困難です。なので、ラジオボタンで表示して、コース名だけでなく、説明も表示するようにしたいですー

@udaikue
Copy link
Contributor Author

udaikue commented Aug 14, 2021

@machida 承知しましたー。修正します💪

@udaikue udaikue force-pushed the feature/users-can-select-a-course branch from 03db008 to b2ebdce Compare August 16, 2021 09:06
@machida
Copy link
Member

machida commented Aug 18, 2021

@udaikue (cc @komagata )
会員登録にコース選択を表示しなくなりそうなので、一旦会員登録のページはストップでお願いしますー

@machida machida changed the title 登録時にユーザーがコースを選択できるようにする コースに表示、非表示の設定ができるようにしたい Aug 18, 2021
@udaikue
Copy link
Contributor Author

udaikue commented Aug 18, 2021

@machida 承知しましたー!表示・非表示の設定ができるようにするのみですね。

@machida
Copy link
Member

machida commented Aug 18, 2021

@udaikue
コース一覧ページでは、非表示のコースは admin だけが見れるように修正をお願いします🙏

@udaikue udaikue force-pushed the feature/users-can-select-a-course branch 2 times, most recently from c85edf6 to 7b43e40 Compare August 21, 2021 13:36
@udaikue
Copy link
Contributor Author

udaikue commented Aug 22, 2021

@machida 修正しましたので、再度ご確認をお願いします🙏

@machida
Copy link
Member

machida commented Aug 25, 2021

@udaikue 確認しました!ありがとうございます。デザインも入れましたー

@machida machida removed their assignment Aug 25, 2021
@udaikue
Copy link
Contributor Author

udaikue commented Aug 26, 2021

@machida コースのタイトルが消える問題、修正をお願いします〜🧎

@machida
Copy link
Member

machida commented Sep 8, 2021

@udaikue 修正しましたー コンフリクト解消したので、このPRをいじるときは、

git pull --rebase origin feature/users-can-select-a-course

をやってから作業をお願いします。

@machida machida removed their assignment Sep 8, 2021
@machida
Copy link
Member

machida commented Sep 8, 2021

@udaikue おっと、DOM構造が変わったせいかテストが落ちてしまいました。こちら修正をお願いします🙏

@udaikue
Copy link
Contributor Author

udaikue commented Sep 11, 2021

@machida ありがとうございました!

@makiichikawa お手すきのときにレビューをお願いします〜🙏

@udaikue udaikue marked this pull request as ready for review September 11, 2021 02:44
Comment on lines 7 to 14
- if admin_login? && !course.open_course
.courses-item__title-icon.is-closed
| 非表示
span.courses-item__title-label
= course.title
- else
span.courses-item__title-label
= course.title
Copy link
Contributor

Choose a reason for hiding this comment

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

span が両方にあるので、if文の外に書いた方がシンプルかなと思いました。

Suggested change
- if admin_login? && !course.open_course
.courses-item__title-icon.is-closed
| 非表示
span.courses-item__title-label
= course.title
- else
span.courses-item__title-label
= course.title
- if admin_login? && !course.open_course
.courses-item__title-icon.is-closed
| 非表示
span.courses-item__title-label
= course.title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sinsoku ありがとうございます!その通りでした!修正しました✨

Copy link
Contributor

@makiichikawa makiichikawa left a comment

Choose a reason for hiding this comment

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

@udaikue
確認しました、問題ないと思います!

@udaikue
Copy link
Contributor Author

udaikue commented Sep 11, 2021

@makiichikawa 早々に確認していただきありがとうございます〜✨

@komagata こちらレビューをお願いいたします!

@@ -0,0 +1,5 @@
class AddOpenCourseToCourses < ActiveRecord::Migration[6.1]
def change
add_column :courses, :open_course, :boolean, default: false, null: false
Copy link
Member

Choose a reason for hiding this comment

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

クラス名は名詞(主語)でメソッド名が動詞になり、

主語#動詞

の形になるので、Course#open_courseだと「頭痛が痛い」のように意味が重複してしまいます。
他の名前を考えてみてください〜

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 そうですね...😅名前を修正しました。

再度ご確認お願いします!

Copy link
Member

Choose a reason for hiding this comment

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

こちら修正されていないみたいです〜

@udaikue udaikue force-pushed the feature/users-can-select-a-course branch from 26baee1 to cc99e9d Compare September 23, 2021 01:54
@udaikue
Copy link
Contributor Author

udaikue commented Sep 23, 2021

@komagata 修正しましたので、再度ご確認お願いします🙏

@@ -0,0 +1,5 @@
class AddIsOpenedToCourses < ActiveRecord::Migration[6.1]
def change
add_column :courses, :is_opened, :boolean, default: false, null: false
Copy link
Member

Choose a reason for hiding this comment

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

is_xxxxはrubyだとあまり使わないのでopenとかが良いかもです〜

@udaikue udaikue force-pushed the feature/users-can-select-a-course branch 2 times, most recently from 4f1ca7e to e8b10a2 Compare September 23, 2021 06:10
@udaikue udaikue force-pushed the feature/users-can-select-a-course branch from 4199f3a to 0ab7297 Compare September 23, 2021 11:52
@udaikue
Copy link
Contributor Author

udaikue commented Sep 23, 2021

@komagata そうなんですね、openに変更しました。ご確認お願いします!

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

openってメソッドが一般的にたくさんあるのでどっかでその名前とぶつかりそうでちょっと怖い気がしますが、ま〜大丈夫だと思うのでOKです〜!

@komagata komagata merged commit 29fdb50 into main Sep 25, 2021
@komagata komagata deleted the feature/users-can-select-a-course branch September 25, 2021 13:20
@github-actions github-actions bot mentioned this pull request Sep 25, 2021
14 tasks
@sinsoku
Copy link
Contributor

sinsoku commented Sep 26, 2021

@komagata @udaikue
Kernel.#open と被るため、移行が簡単であれば別の名前にした方が良かったりしません?
Rails 6.0では直っていますが、過去にRails本体でも rails/rails#34098 のようなIssueがあったこともあり、少し怖いなと思いました。
コースの表示・非表示なら publishedenabled のような名前に変えるのはどうかなと。

@komagata
Copy link
Member

@udaikue @sinsoku さんがおっしゃってるのを見ると確かに怖いので別のIssueで修正できたらありがたいです〜

@udaikue
Copy link
Contributor Author

udaikue commented Sep 28, 2021

@komagata (CC: @sinsoku )

#3326でIssueを作成してみましたのでご確認いただければと思います〜🧎

@komagata
Copy link
Member

@udaikue ありがとうござます!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants