-
Notifications
You must be signed in to change notification settings - Fork 235
feat: audit log bulk export [prototype] #10360
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
base: master
Are you sure you want to change the base?
Conversation
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.
全言語対応する後続タスクは作ってある?
| import next from 'next'; | ||
|
|
||
| import instanciateAuditLogBulkExportJobCronService from '~/features/audit-log-export/server/service/audit-log-bulk-export-job-cron'; | ||
| import '~/features/audit-log-export/server/models/audit-log-bulk-export-job'; |
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.
models の import は違和感がある
他のモデル群の取り扱いに倣ってほしい
| 'env:useOnlyEnvVars:app:isAuditLogExportEnabled': defineConfig<boolean>({ | ||
| envVarName: 'AUDIT_LOG_EXPORT_ENABLED_USES_ONLY_ENV_VARS', | ||
| defaultValue: false, | ||
| }), |
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.
audit log のエクスポートは管理者以外は使わない機能なので、並列数や cron 頻度をカスタマイズできるようにする必要はない (ヘビーウェイとな使い方は想定されてないという建て付けでよい)
そのため、これらの設定値はサービス層でハードコードしてよい
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.
core package を改修するので、
/資料/開発ガイドライン/ADR - Architecture Decision Record/[CI] changesets によるパッケージリリース
を参考に .changeset ディレクトリに changeset ファイルを作ってほしい
(これがなぜ必要なのか、どう作ればいいのかわからなかったら口頭で武井に聞きに来て)
|
|
||
| export const parseSnapshot = (snapshot: string): IAuditLogExportJobSnapshot => { | ||
| try { | ||
| return JSON.parse(snapshot); |
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.
type guard を導入し、IAuditLogExportJobSnapshot 型であることを保証して
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.
コード内コメントはすべて英語で
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.
サービス層に対してはインテグレーションテストが必要
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.
このファイル魔改造されてる気がする
mousedown イベントハンドラや stopPropagation() が必要になるケースは稀なので、なにか変なことをやっていないか要チェック
|
|
||
| if (res.status === 204) { | ||
| toastSuccess(t('audit_log_export.export_started')); | ||
| toastSuccess(t('audit_log_export.notification_message')); |
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.
toastr 二つもいらないかな
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.
コンポーネントの実体が Modal なのに、名前が Button なのは名が体を表していない
また、このモーダルの仕様は固まっているのだろうか?
ユーザー・期間の選択部分の使い勝手をもうちょっと拡張する余地もありそう
そうするとこのコンポーネントに全て詰め込むと fat component になってしまうので、モジュールは分割すべきかもしれない
後続ストーリーで改善すべき
このPRはプロトタイプ実装なのでMergeを目的とないためDraftとしています
方針の確認や認識のすり合わせ、改善点のフィードバックをいただければと思います。
動作の様子
2025-09-30.18.28.11.mov
内部仕様 -> https://dev.growi.org/68b96f7fcde220ddd8dad822
外部仕様 -> https://dev.growi.org/68b694cdabc93bd7957d265c