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

Impl/audience #1893

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Impl/audience #1893

wants to merge 3 commits into from

Conversation

ramdos0207
Copy link

チャンネルの未読管理者一覧を取得するAPIを生やしました

resolves #1432

@motoki317
Copy link
Member

実装しちゃったあとでアレなんですが、既存の /subscribers のレスポンスの中に含めるというのは考えました?
内部的にも「通知レベル」という値で管理されてるので分かりやすいし、リクエストも1つその方が少なくなるのでよいかと

@ramdos0207
Copy link
Author

あんまり考えてなかったです...

ただ /subscribersの中に含めることにすると破壊的変更になりそうで困っています。このような場合にはどうすれば良いですか?

  • クライアント側のコードも書き直す
    • @BOT_toki_test /notifycountとかが壊れそうで嫌です
  • 未読管理者も通知受信者も1つのレスポンスに含めて返すエンドポイントを新しく生やし、現行のエンドポイントと共存させる
    • /api/v4を新しく作る?

あたりを考えましたが、どちらが良いのか見当がつきません...

@motoki317
Copy link
Member

motoki317 commented Jul 27, 2023

そうですね、破壊的変更になると思います
普通にユーザーが使う分にはクライアントと同時にバージョン上げればいいですけど、外部クライアントの依存が壊れるのはどうしようもないですね
このエンドポイントを使っている外部クライアントはほぼ無いと考えて互換性を切っちゃうか(どうせ部内SNSなので)、厳密に互換性を保ちたいなら言う通りエンドポイントを分ける or v4などバージョンを上げるかのどちらかですね

正直ここを使っている外部クライアントはほぼ無いと思うので(楽なので)思い切って壊してもいいんじゃないかと思いますが、最終的には現メンテナに任せます

Copy link
Member

@logica0419 logica0419 left a comment

Choose a reason for hiding this comment

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

ありがとう!レビュー遅れてごめんなさい!
実装は完璧ですね、ありがとうございます!
初めてでこれは素晴らしいです。

ただ、これはissueにも書いてあった通りフロントの改修に必要とされているタスクで、フロントエンドがどのような姿になりたいかを無視するわけにはいきません。
なので、一旦フロントエンド(&デザインチーム)に確認を取りたいです。僕が取ります。
すぐに決定できなくて申し訳ない。

router/v3/channels_test.go Outdated Show resolved Hide resolved
@pikachu0310 pikachu0310 self-requested a review April 9, 2024 05:32
@pikachu0310
Copy link
Contributor

再議論されたが、また止まっていそう。役員会/デザイン/@SysAd_Adminに確認を取る必要がある。
https://q.trap.jp/messages/006e829e-7747-4258-98d1-8f93634a710d

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.

チャンネルの未読管理者が取りたい
4 participants