-
Notifications
You must be signed in to change notification settings - Fork 71
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
一度公開したお知らせを誰でも公開の状態で保存できるように変更 #4215
一度公開したお知らせを誰でも公開の状態で保存できるように変更 #4215
Conversation
@machida さん ソースコード中のこちらにコメントを入れておきました。要デザイン(ここから)〜(ここまで)の範囲です。今は仮のボタンを割り当てています。PR記載の要件(条件・期待値をまとめた表)につきましても、認識違い等ありましたら、ご指摘ください。 また、初めての依頼となりますので、インプットに不足があるかもしれません。。 よろしくお願いいたします🙇🏻♂️ 追記 |
@aim2bpg |
f4dfc57
to
db214ef
Compare
@aim2bpg テストが落ちる対応がmainに入ったので、最新のmainを取り込みました。 このブランチをいじる際は、 git pull --rebase origin feature/change-anyone-can-republish-announcement をお願いしますー |
@machida さん |
@Saki-htr さん、おつかれさまです。 先日分報で、ド・モルガンの法則を学習されていたようなので、新たな学びが得られるかはわかりませんが、今回の条件分岐の作り込みもみていただければと思いました。 対応難しいようであれば、遠慮なく仰ってください。自分の方は特に急ぎませんので、後でやるでも全然OKですー |
@aim2bpg さん 27日だとちょっと遅いな〜ということであれば、他の方にご依頼いただけましたらありがたいです。 |
@Saki-htr さん |
17dc344
to
71c6160
Compare
コンフリクトが起きてたので、最新のmainからrebaseをしましたー |
@machida さん、ご対応ありがとうございますー🙏 |
@aim2bpg さん |
@Saki-htr さん |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aim2bpg さん
お疲れさまです!大変おまたせして申し訳ありません🙇♀️
動作確認したところ、OKでした🙆♀️ 私の方からはApprove
とさせていただきます〜
一つ気になった所をコメントさせていただきましたが、今回のissueとは直接関係ない所ですので、もしご存知でしたらご回答いただけますと幸いです🙏
description
が非常に丁寧に書かれていて、とてもレビューしやすかったです! また、とても勉強になりました。
以下の点が素晴らしいと思いました👏✨
- 変更理由を、machidaさんからの追加要求も含めて、丁寧に説明してくださっているので、このissueが必要になった背景を理解しやすい
- デシジョンテーブルのおかげで、条件分岐のコードが要件を満たしているかを確認しやすい
- このissueを実装することで、他の動作にバグを作るかもと考えて、表にまとめて確認されている
私も複雑な条件分岐を書く際は、コードを書き始める前に、デシジョンテーブルを書こうと思います!
ご依頼くださってありがとうございました
@@ -39,19 +39,19 @@ | |||
ul.form-actions__items | |||
li.form-actions__item.is-main | |||
= f.submit 'WIP', class: 'a-button is-lg is-primary is-block', id: 'js-shortcut-wip' | |||
- if admin_or_mentor_login? | |||
- if admin_or_mentor_login? && announcement.new_record? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aim2bpg
こちら、デシジョンテーブルのパターン1の要件を満たしていることを確認できました!
コードもネストが無くなり読みやすくなっていて、素晴らしいと思いました👏
今回のissueとは直接関係ないのですが「作成と公開の違い」が気になりました。
komagataさんでログインし、作成と公開の両方を行ってみましたが、どちらのボタンを押しても「公開」を行っているように見えました🤔
なぜ「メンターor管理者ログイン時 かつ お知らせ新規作成時 に作成ボタンが表示」されるのかが少し気になりました。
過去のissueを探しても分からなかったため、もしご存知でしたら教えていただけますと幸いです🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちら、たしかに自分も気になりました😅
- 作成ボタン:新規作成→公開(管理者・メンターのみ)
- 公開ボタン:WIP→公開(管理者・メンターのみ)、公開→(WIP)→公開(ユーザー不問)の2つを兼用
といった解釈ですね。。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aim2bpg
なるほど、そのような使い分け方なら納得です〜
ありがとうございます
end | ||
assert has_button? '公開' | ||
assert_no_text 'お知らせを作成しましたら、WIPで保存し、作成したお知らせのコメントから @mentor へ確認・公開の連絡をお願いします。' | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
デシジョンテーブルでどのようなテストケースが必要か明記してくださっているので、コードレビューがとてもしやすいです!
ありがとうございます🙏✨
6パターン全て確認したところ、OKでした🙆♀️
期待値を反転させて失敗したことも確認&記載してくださっていて素晴らしいと思います👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確認しました、OKですー🙆♂️
Issue #4127
一度公開したお知らせをメンター・管理者以外が編集しても、公開の状態で保存できるように変更しました。
なお、本変更で条件分岐をきちんと作り込まないと、新たなバグを作り込むかもしれないため、デシジョンテーブルを作って条件を導出してみました。(テスト技法のプラクティスの提出物で学んだ内容を活かしました🙌)
変更理由
現在、お知らせはメンター・管理者しか公開ができない。この仕様はこのままでいいが、一度公開したお知らせをメンター・管理者以外が編集した場合、WIPの状態で保存しなくてはならず、公開の状態で保存することができない。
(町田さんからの追加要求対応)
公開されたお知らせの編集ボタンは、メンター・管理者以外には表示されないため、編集画面に入るためにURLを直打ちしていた。URLを直打ちする運用はよくないので、公開されたお知らせにも編集ボタンを表示するように変更したい。
(町田さんからの追加要求対応)
公開されたお知らせに表示する編集ボタンはほとんどの人にとっては不要なリンクなので、目立たないデザインを入れたい。もし、従来の編集ボタンのままだったら、操作ミスが発生する可能性があるのと(お知らせには変更履歴の情報がないので戻せない)、逆になんでお知らせが自分も編集できるようになっていますがバグですか?という質問が殺到しそうなので、公開済みのお知らせのみ目立たないボタンに変更したい。
変更点
変更前(*が変更点)
変更後(*が変更点)
目視確認
動作確認手順
パターン1〜6について、期待値と一致することを確認した。確認結果のエビデンスについては、パターン6を代表で示す。
パターン6 修正前
公開されたお知らせの詳細画面の「内容修正ボタン」が表示されない。
URLの末尾に
/edit
を直打ちして編集画面に入ってみると「公開ボタン」も表示されない。WIPボタンは使えるので、WIPボタン下の公開までの手順は表示される。パターン6 修正後
公開されたお知らせの詳細画面の「内容修正ボタン」が目立たないボタンで表示されている。
注:「内容修正ボタン」は別途、町田さんの方でデザインされるため、仮のボタンで表現。
同お知らせの編集画面の「公開ボタン」が表示される。また、公開済みのため、WIPボタン下の公開までの手順は表示しない。
自動テスト
announcements_test.rb
ファイル単体でテストしてALL-Passを確認済み。(期待値を反転させてのFail確認も済み)