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

Scrapbox: 指定したページが更新されても通知が来ないようにする #229

Draft
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

pizzacat83
Copy link
Member

@pizzacat83 pizzacat83 commented Jan 15, 2020

writeupなどネタバレを含むページの更新通知が #_scrapbox に流れてくるのがつらいので,指定したページは更新されても通知が来ないようにする

実装

記事が更新されるとScrapboxから指定したURLにWebhook向け形式のJSONが送られてくるので,送信先をSlackではなくTSGサーバーに設定してScrapboxから更新通知を受け取る。
TSGサーバーは更新されたページそれぞれが「ミュートしてほしいページ」かどうか判定し,そうでないもののみ更新通知を #_scrapbox に投稿する。

論点

Scrapbox通知をいじるのは割とTSGersに影響があると思うので,Slackbot民に限らず広く意見を募集します。

1. ミュートしたいページをどう指定するか

2. ミュートされたページの更新はどのように通知されるべきか

更新の内容は見たくなくても,更新されたという情報は欲しい場合がある

3. 誰が実装するか

4. ミュートタグのガイドライン

誰でもつけ外ししていいものなのか,それとも。

-1: 他に論点はないか

@pizzacat83
Copy link
Member Author

以下pizzacat83の構想です。

1.

  • ミュートしたいページのどこかに ###ミュート と書く
    • # が3つあるのは他の記事との衝突を避けるため
  • サーバーは更新されたページのAPIを叩いてこのタグを含むかどうか調べる

2.

  • 本文: 「この記事の更新通知はミュートされています。」
  • それ以外 (タイトルなど): そのまま

3.

私がやってもいいですが, @Szkieletor37 さんがやりたいとかだったらお譲りします。

@Szkieletor37
Copy link
Contributor

1,2 pizzacat氏の案に賛成します

3, 体調がアなため @pizzacat83 氏がやってくれるならありがたい

4, ミュートタグを個々の判断で勝手に入れてもいいのか、ガイドラインを決めて運用するのかを決める必要があると思います

@pizzacat83
Copy link
Member Author

4,論点に追加しました(ついでに4のindexがどんどんずれていくことに気がついたので-1にしました)

4はTSGerの良心に任せればまあええんちゃう〜と思っています。pizzacat83の2. なら更新自体の通知は来るため知らないうちにミュートということは起きないので,ミュートすべきかで人々の判断基準がずれていたらその時議論してくれればいいんじゃないかな〜(未来に丸投げ)

@cookie-s
Copy link
Member

cookie-s commented Jan 15, 2020

え〜実装方針案ふくめ構想もとてもよさそう〜楽しそう〜〜

1, む、###ミュートって含めるとどうなるんですか

4, TSGerはみんな良心の塊だからね!大丈夫なんちゃう!まぁでも「ガイドラインは今はありません、勝手にやってください、議論の対象になることはありますが咎められることはありません。」というガイドラインを書くべき説がある?

@pizzacat83

This comment has been minimized.

@pizzacat83
Copy link
Member Author

だいたい実装し終わったので,テストを書きます

@codecov-io
Copy link

codecov-io commented Jan 15, 2020

Codecov Report

Merging #229 into master will increase coverage by 0.68%.
The diff coverage is 90.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   27.58%   28.26%   +0.68%     
==========================================
  Files          84       87       +3     
  Lines        6957     7029      +72     
  Branches     1343     1359      +16     
==========================================
+ Hits         1919     1987      +68     
- Misses       4393     4395       +2     
- Partials      645      647       +2
Impacted Files Coverage Δ
index.js 0% <ø> (ø) ⬆️
welcome/index.ts 0% <0%> (ø) ⬆️
scrapbox/mute.ts 100% <100%> (ø)
lib/fastify.ts 100% <100%> (ø)
scrapbox/index.ts 79.31% <87.5%> (-4.57%) ⬇️
lib/scrapbox.ts 90.47% <90.47%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb5f4a3...c3f17a0. Read the comment docs.

@pizzacat83

This comment has been minimized.

@pizzacat83 pizzacat83 self-assigned this Jan 15, 2020
@pizzacat83
Copy link
Member Author

pizzacat83 commented Jan 19, 2020

うおお 結構時間かかってしまった:ojigineko-superfast:
テストは通っていて,手元でも動作確認してます。

2.

本文を上書きするのに加えて,画像とサムネを消すようにしました。

既存のコードの変更点

元からあったunfurlのコードと共通化できそうな部分は共通化しました。あと手元で動かすのが辛かったのでScrapboxのProject名は直書きせず環境変数を使うようにしました。

Merge時に必要な処理

  • サーバーに以下の環境変数を追加
    • CHANNEL_SCRAPBOX: Cなんとか
    • SCRAPBOX_PROJECT_NAME: tsg
  • Scrapboxの通知用URLをSlackのWebhookから https://TSGサーバーのURL/hooks/scrapbox に変更
    • これはOwnerでないとできないっぽいのでよろしくお願いします

@pizzacat83 pizzacat83 marked this pull request as ready for review January 19, 2020 04:49
@hideo54
Copy link
Member

hideo54 commented Jan 19, 2020

今更読んだんですけどめちゃくちゃ良さそう!!!! 今出てる案に全面的に同意です!

一つ提案なんですが、Slack 通知の時、アイコンを更新者のアイコンにしませんか?

Copy link
Member

@hakatashi hakatashi left a comment

Choose a reason for hiding this comment

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

実装ありがとう!大まかな仕様はよさそう

scrapbox/index.ts Outdated Show resolved Hide resolved
scrapbox/index.ts Outdated Show resolved Hide resolved
scrapbox/index.ts Outdated Show resolved Hide resolved
scrapbox/index.test.ts Outdated Show resolved Hide resolved
scrapbox/index.test.ts Outdated Show resolved Hide resolved
scrapbox/index.test.ts Outdated Show resolved Hide resolved
scrapbox/index.ts Outdated Show resolved Hide resolved
Comment on lines 225 to 226
expect(attachments_res.length).toBe(
sum(separatedAttachments.map((a, i) => isMuted[i] ? 1 : a.length)),
Copy link
Member Author

Choose a reason for hiding this comment

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

attachments_res のテストが配列長だけで雑だなあとは思っている

scrapbox/index.test.ts Outdated Show resolved Hide resolved
lib/scrapbox.ts Outdated Show resolved Hide resolved
@pizzacat83
Copy link
Member Author

またしても時間がかかってしまった:ojigineko-superfast:
手元で動作確認済みです。

前回レビューからの変更点

  • ご指摘をいろいろ修正
  • エラーを握りつぶさないテスト用fastifyコンストラクタを lib/fastify.ts に配置
  • ScrapboxのAPIラッパを lib/scrapbox に作成
  • Scrapbox APIを使っていたScrapboxリンク展開とwelcomeがaxios直叩きから lib/scrapbox を使うように
  • scrapbox/index.ts(元からあったリンク展開) と scrapbox/mute.ts (今回実装した通知ミュート) に分割
    • ファイルが大きくなってきたため
    • npm run dev --only で片方だけ起動できるように
      • Private 個人プロジェクトで通知テストをしているときに他の部員がURL展開で個人プロジェクトを覗けてしまう
    • Scrapbox周りのutilsを lib/ に移動したので共通して使うものが全て外部からのimportになったため
  • ミュートの仕様がかなり変わった (各記事APIコール -> ##ミュート のAPIコール, 画像大量崩壊notificationへの対応) ため,scrapbox/mute.ts とそのテストを大幅に再実装

前回レビュー時点での既存のコードの変更点

  • ScrapboxのProject名は直書きせず環境変数を使う
    • 手元で動かすときに個人Scrapboxが使えるように

Merge時に必要な処理

  • サーバーに以下の環境変数を追加
    • CHANNEL_SCRAPBOX: Cなんとか
    • SCRAPBOX_PROJECT_NAME: tsg
  • Scrapboxの通知用URLをSlackのWebhookから https://TSGサーバーのURL/hooks/scrapbox に変更
    • これはOwnerでないとできないっぽいのでよろしくお願いします

ほぼ0からの再レビューって感じだと思いますがよろしくお願いします:ojigineko:

Copy link
Member

@hakatashi hakatashi left a comment

Choose a reason for hiding this comment

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

鬼のように実装しててやべーーーーー開発力の塊???????????

いろいろコメントしたけど、大きな問題は無いと思います!

lib/fastify.ts Outdated Show resolved Hide resolved
* 単体テストでexpectの失敗などによる例外をJestが検知することができない
* 発生した例外は全て再送出するように設定
*/
fastify.setErrorHandler((err) => { throw err; });
Copy link
Member

Choose a reason for hiding this comment

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

[question] うーむ⋯⋯⋯⋯ fastify, ルートハンドラがthrowした場合はちゃんとinjectの結果をrejectしてくれると思ってたけど、どうだろう⋯⋯? (実際にこの行をコメントアウトしてもfastify.test.tsのテストが通る)

Copy link
Member Author

Choose a reason for hiding this comment

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

びっくりしているんですが,上にある throw Error をコメントアウトしても通っていて,少なくともテストコードはバグっています:ojigineko-superfast:

Copy link
Member Author

Choose a reason for hiding this comment

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

え〜この行無くても動くな どうして昔握りつぶされてInternal Server Errorしてたんだ??????

scrapbox/index.test.ts Outdated Show resolved Hide resolved
scrapbox/mute.test.ts Outdated Show resolved Hide resolved
scrapbox/mute.test.ts Outdated Show resolved Hide resolved
});

describe('methods', () => {
let page: Page | null = null;
Copy link
Member

Choose a reason for hiding this comment

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

[question] strictNullChecksが入ってないので現状意味のない指定だと思いますが、大丈夫ですか

Copy link
Member Author

Choose a reason for hiding this comment

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

将来誰かがstrictNullChecksをしたくなった時用に lib/scrapbox.* はstrictNullChecksでも通るコードになってるんですが,どこかにその旨書いた方がいいですかね

Copy link
Member

Choose a reason for hiding this comment

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

なるほど、まあ意図があるなら大丈夫だけど、現状のプロジェクトの設定では検証できない制約をソースコードに課すのはあまり良くない気もする

welcome/index.ts Outdated Show resolved Hide resolved
@pizzacat83
Copy link
Member Author

む 前回テスト時からScrapbox通知の形式が変わった気がする 調査します

@cookie-s
Copy link
Member

cookie-s commented Nov 7, 2020

これどうしたらいいですか、approveすればいいですか、すればいいならします

@pizzacat83
Copy link
Member Author

ぐえ 私がレビューに対して修正をしていないという認識です…

@pizzacat83
Copy link
Member Author

無言で放置しておくのはよくないという気持ちになったので書いておきます

たぶん私はこのPRに手をつけません (100%とはいえないが)

タスクの残り:

  • PRのコメントへの対処
  • だいぶ時間が経ったので現在のScrapboxの通知フォーマットでも正常に動作するか確認する

@pizzacat83 pizzacat83 marked this pull request as draft January 20, 2021 13:08
@hideo54
Copy link
Member

hideo54 commented Jan 20, 2021

そこそこのモチベーションあるので気が向いたら引き継ぎます。動き始める時はここで宣言しますね。(それまでに誰か開発したい人が現れたら譲ります)

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.

6 participants