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

Jump to bottom

日報フォロー機能のコメント通知を切り替えられるようにする #3227

Merged
merged 25 commits into from
Sep 30, 2021
Merged

日報フォロー機能のコメント通知を切り替えられるようにする #3227

merged 25 commits into from
Sep 30, 2021
+425 −193

Conversation

yana-gi
Copy link
Contributor

@yana-gi yana-gi commented Sep 5, 2021 •
edited
Loading

ref: #3007

概要

フォローするユーザーごとに日報を自動Watchにするか・しないかを選択できるようにした

修正前

デフォルトでフォローしたユーザーの日報が自動でWatchされる
修正前の仕様は #2015 を参照

修正後

自動でWatchしない設定を追加し、フォロー時に選択できるようにした

通知がくるタイミング

通知の種類 \ フォローの種類 コメントあり コメントなし フォローしない
フォロー中のユーザーが
日報を投稿した時		 ○ ○ -
フォロー中のユーザーの
日報にコメントが来た時		 ○ × -

やったこと

  • followingテーブルにwatchを追加
  • フォローがwatch=falseの場合、自動Watchをしないようにした
  • フォローボタンを変更
    • フォロー時に コメントあり/コメントなし (自動Watchあり/なし)をアコーディオン形式で選択できるようにした
    • コメントあり/コメントなしの説明を追加した
    • フォロー状態によってフォローボタンの表記が変わるようにした
  • ユーザー一覧のフォロー中タブの階層を一段階上にした
  • フォロー一覧ページで 全て/コメントあり/コメントなし 一覧を指定できるようにした

スクリーンショット

ユーザー個別ページ

フォローしない状態

変更前 変更後
貼り付けた画像_2021_09_21_17_10 貼り付けた画像_2021_09_21_17_06

フォロー中の状態

変更前 変更後
貼り付けた画像_2021_09_21_17_12 貼り付けた画像_2021_09_21_17_06

フォロー中一覧ページ

変更前 変更後
貼り付けた画像_2021_09_21_17_13 貼り付けた画像_2021_09_21_17_07

タスク一覧

タスク一覧
  • DB
    • カラム追加
  • コントローラー・モデル
    • 新しくフォローできるようにする(create)
    • コメントあり/なしを切り替えできるようにする(update)
    • index一覧(一覧)
  • ビュー・Vue.js
    • 新しくフォローできるようにする(create)
    • コメントあり/なしを切り替えできるようにする(update)
    • index一覧でコメントあり/なしを切り替える
    • 「フォロー中」の階層を1段階あげる
    • フォローボタンを「フォロー中」ではなく「コメントあり/なし」にする
  • テスト作成
    • モデルテスト
      • 既にあるテストを通るようにする
      • following?
      • change_watching
      • auto_watching?
      • unfollow
      • following_list
  • システムテスト
    • フォロー
      • コメントあり
      • コメントなし
    • フォロー解除
    • フォロー一覧
      • 全件
      • コメントあり
      • コメントなし
    • フォローしたユーザーが日報投稿の通知を受け取る
    • フォローしたユーザーがコメント投稿の通知を受け取る
  • テストデータ修正
    • watch=false のデータ追加

@yana-gi yana-gi force-pushed the feature/enable_choose_type_of_follow branch 4 times, most recently from 34fcc80 to 330a49d Compare September 13, 2021 09:32
@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 13, 2021

@machida
こちらテスト作成前ですが、デザインを入れていただくことは可能でしょうか。
現行の機能で参考できるものがなくHTML部分も仮実装になってしまっているため、この段階でデザインを入れて頂けると助かります！
デザイン待ちの間にもテスト作成などの作業はあるので、急ぎではありません〜

following.vueに仮でCSSを入れていますが、すべて削除してしまって構いません。
CSSで参考にした記事 : https://qiita.com/zackey2/items/55a004bcc5198d25f500

デザインを入れてほしいファイル

  • app/javascript/following.vue
  • app/views/users/_followings_nav.html.slim

@yana-gi yana-gi force-pushed the feature/enable_choose_type_of_follow branch from 330a49d to 88e888d Compare September 14, 2021 08:08
@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 14, 2021

@machida
本日のミーティングで指摘いただいた点修正しました。

  • フォローボタンの表示を変更
  • usersの「フォロー中」の階層を1段階あげた

usersの「フォロー中」の階層を1段階あげた件ですが、「フォロー中」タブのデザイン(アクティブ状態)は未対応のため、machidaさんの方で対応頂けないでしょうか。

上のコメントの件と合わせて対応お願いします🙏

@machida
Copy link
Member

machida commented Sep 17, 2021

@yana-gi すいません、遅くなりました🙇‍♂️ デザイン入れましたー

@machida machida force-pushed the feature/enable_choose_type_of_follow branch from 18e9363 to 34054e7 Compare September 17, 2021 03:28
@machida
Copy link
Member

machida commented Sep 17, 2021

@yana-gi 最新のmainを取り込んだので、

$ git pull --rebase origin feature/enable_choose_type_of_follow

をやってから作業の続きお願いしますー

@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 18, 2021

@machida
ありがとうございますー！確認します！

@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 19, 2021

@machida
デザイン確認しました！
手元で確認したところ何点か確認していただきたい点があります〜
確認中でもシステムテストは着手できるので、お手隙の際に対応頂ければと思います！

相談したいこと

説明文の表示

入れていただいたデザインでは説明文が選択中のステータスでは非表示になっているようです。
image
個人的には選択中でも説明文を表示した方がステータスの違いを比較しやすいと感じています。
GitHubのWatch機能もそのような仕様になっているようです。
image
この件に関してmachidaさんの意見をお伺いしたいです！

修正を依頼したいこと

タブのアクティブ状態

/users?target=followings表示時のタブが「全て」がアクティブ状態になっています。
「フォロー中」タブをアクティブ状態にして頂きたいです。
image
/users?target=followingsを/users/followingsにする必要があればおっしゃってください〜！

通知の表示が崩れている

通知ベルのバッヂとアイコンの大きさが崩れているようです。
image

ご確認お願いします🙏

@yana-gi yana-gi force-pushed the feature/enable_choose_type_of_follow branch 2 times, most recently from 4c11fb6 to c7dca88 Compare September 21, 2021 07:59
@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 21, 2021

📝MTGメモ
デザインの修正依頼はレビューが完了してから。
レビューが終わったらmachidaさんにメンションする。

@yana-gi yana-gi marked this pull request as ready for review September 21, 2021 09:07
@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 21, 2021

@konaga-k
こちらレビューお願いできますでしょうか〜

現在テストが通っていませんが、自分の変更内容とは別の問題で落ちていることを確認済です。
解決次第テストを再実行する予定です。

@konaga-k
Copy link
Contributor

明日以降確認します〜

konaga-k
konaga-k suggested changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@konaga-k konaga-k left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@yana-gi
結局今日レビューしました

これかなり大変なissueですね…
お疲れ様です😭

  • 動作確認はOKです
  • コードレビューしたので対応もしくはコメントお願いします。IMOは個人的な意見なので、合わなかったら対応しなくても構わないです。色々書いてますが、致命的におかしなところはないと思います
  • Railsのコードはひと通り見ましたが、Vue.jsは不慣れなためあんまりちゃんと見れてないです💦

app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb
end

def change_watching(other_user, watch)
following_row = Following.find_by(follower_id: self, followed_id: other_user)
Copy link
Contributor

@konaga-k konaga-k Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

【感想】
なんで following じゃなくて following_row なんだと思ったんですが、 following が has_many で使われているからですね…
今更 has_many の方を変えるのは大変なので、ここは仕方ない感じですね
（そのうち has_many の方の following という名前が負債になる気がする…）

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue登録ありがとうございます！
自分も following_row の命名を考える際にfollowingの命名が気になっていたのでありがたいです🙏

app/controllers/api/users_controller.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/views/users/_followings_nav.html.slim Outdated Show resolved Hide resolved
db/migrate/20210905061726_add_watch_to_followings.rb Show resolved Hide resolved
@konaga-k konaga-k mentioned this pull request Sep 22, 2021
Closed
@yana-gi yana-gi force-pushed the feature/enable_choose_type_of_follow branch from c7dca88 to 10c2f28 Compare September 22, 2021 06:47
@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 22, 2021

@konaga-k
レビューありがとうございます！
細かく見ていただいてとても助かります😄

指摘いただいた点を修正しました。
migrationの件はkomagataさんに確認次第対応する予定です〜

@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 22, 2021

@konaga-k
migrationの件は対応不要とのことでした。
修正点の確認お願いします〜🙏

konaga-k
konaga-k approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@konaga-k konaga-k left a comment

Choose a reason for hiding this comment

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

@yana-gi
OKです〜👌

@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 22, 2021

@konaga-k
レビューありがとうございました〜！

@komagata
こちらレビューお願いします。
レビュー後にmachidaさんにデザインを修正してもらうため一旦マージしないで頂きたいです🙏

komagata
komagata approved these changes Sep 23, 2021
View reviewed changes
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.

確認しました、OKですー🙆‍♂️

yana-gi and others added 17 commits September 27, 2021 13:26
@yana-gi @machida
文言・デザイン仮追加
d23dae6
@yana-gi @machida
フォローボタンの表示を変更
665f94e
@yana-gi @machida
usersの「フォロー中」の階層を1段階あげた
ef101c8
@machida
フォローボタンのデザイン、タブ周りの色調整
7fb7975
@yana-gi @machida
モデルテスト追加
df2fb34
@yana-gi @machida
システムテスト追加
455966a
@yana-gi @machida
テストデータ追加
15f0768
@yana-gi @machida
エラー発生時の文言を修正
3b2d9a7
@yana-gi @machida
lint修正
9cc96eb
@yana-gi @machida
User#followにキーワード引数を設定した
bb32343
@yana-gi @machida
User#change_watching の更新処理を update! から update に変更
3f3f97f
@yana-gi @machida
User#following_list の処理変更
774512e 
返ってくる値をidではなくuserインスタンスの配列にするため
@yana-gi @machida
User#auto_watching? の名前を watching? に変更
329e103
@yana-gi @machida
boolean属性の指定に?を付与
fa8a40a
@yana-gi @machida
不要なファイルを削除
4546e9e
@yana-gi @machida
User#follow の処理を変更
ca9b433 
2クエリ発行していたところを1クエリにした
@machida
フォローの説明が消えてしまっていたのを修正、通知のデザインファイルを消してしまっていたのを戻した
c18e8c2
@machida machida force-pushed the feature/enable_choose_type_of_follow branch from 10c2f28 to c18e8c2 Compare September 27, 2021 04:26
@machida
Copy link
Member

machida commented Sep 27, 2021

@yana-gi
すいません、説明文が消えてしまうところ、勘違いしてました。修正しましたー。
通知のデザイン崩れもありがとうございます！！思いっきりファイルを消してしまってました。。。（危なかった、、、🙏）
コンフリクトを解消したので、

git pull --rebase origin feature/enable_choose_type_of_follow

をお願いしますー

一点気になったのですが、フォロー状態を切り替えるとき、赤い枠で囲った部分をクリックしたら、detail タグの open 属性が削除されて detail が close になると便利かと思いました。

貼り付けた画像_2021_09_27_13_37

もし、こちら手間がかかりそうでしたら、これでリリースして別 Issue で対応でも大丈夫ですー

@machida machida removed their assignment Sep 27, 2021
@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 28, 2021

@machida
修正ありがとうございます！
通知のデザイン崩れが修正されていることを確認しました🙆‍♂️

detailsのopen属性

一点気になったのですが、フォロー状態を切り替えるとき、赤い枠で囲った部分をクリックしたら、detail タグの open 属性が削除されて detail が close になると便利かと思いました。
もし、こちら手間がかかりそうでしたら、これでリリースして別 Issue で対応でも大丈夫ですー

こちら少し試してみたのですが、すぐには解決できなさそうなので別issueとして対応させていただければと思います。

アクティブ状態

タブのアクティブ状態について再度確認させてください。

/users?target=followings表示時のタブが「全て」がアクティブ状態になっています。
image
/users?target=followingsの場合は「フォロー中」タブのみを
/usersの場合は「全て」タブのみを
アクティブな状態に修正していただくことは可能でしょうか。

こちらで見落としているところがあればすみません。今一度確認お願いします！

何度もお手数ですがご確認お願いします🙏

@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 28, 2021

@machida
ミーティングで言ったようにタブの修正を行いました。
変更内容に問題がなければマージお願いします🙏

@machida machida self-assigned this Sep 29, 2021
@machida
Copy link
Member

machida commented Sep 30, 2021

@yana-gi お待たせしました！！確認しました。
OKですー
マージしますー🎉

@machida machida merged commit e4aaa14 into main Sep 30, 2021
@machida machida deleted the feature/enable_choose_type_of_follow branch September 30, 2021 02:38
@github-actions github-actions bot mentioned this pull request Sep 30, 2021
Merged
14 tasks
@yana-gi
Copy link
Contributor Author

yana-gi commented Sep 30, 2021

@machida
ありがとうございますー！

@yana-gi yana-gi mentioned this pull request Oct 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konaga-k konaga-k konaga-k approved these changes

@komagata komagata komagata approved these changes

Assignees

@machida machida

@yana-gi yana-gi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yana-gi @machida @konaga-k @komagata