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

fix: Slack notification not sent on page update #8841

Merged

Conversation

miya
Copy link
Member

@miya miya commented May 24, 2024

Tasks

#146960 [v7] slack 連携した slack に対して wiki からの通知が飛ばない件の修正
#146980 修正

@miya miya self-assigned this May 24, 2024
Copy link

changeset-bot bot commented May 24, 2024

⚠️ No Changeset found

Latest commit: 1ffb817

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -168,7 +169,7 @@
options.grant = grant;
options.userRelatedGrantUserGroupIds = userRelatedGrantUserGroupIds;
}
const previousRevision = await Revision.findById(revisionId);
previousRevision = await Revision.findById<IRevisionHasId>(revisionId);

Check failure

Code scanning / CodeQL

Database query built from user-controlled sources High

This query object depends on a user-provided value.
Copy link
Member

Choose a reason for hiding this comment

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

この failure はちゃんと消えるような実装をしてください

Copy link
Member Author

Choose a reason for hiding this comment

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

xss.process() を使ってサニタイズしたものを渡すように改修しました

@@ -68,6 +68,7 @@ declare global {
export type SaveOptions = {
wip: boolean,
slackChannels: string,
isSlackEnabled: boolean,
Copy link
Member Author

Choose a reason for hiding this comment

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

  • スイッチ (下記の画像) の ON/OFF によって isSlackEnabled が書き換わる
  • PUT /_api/v3/page のリクエストボディに isSlackEnabled を含めるように改修
スクリーンショット 2024-05-27 12 58 39

if (isSlackEnabled) {
try {
const option = revisionId != null ? { previousRevision: revisionId } : undefined;
const option = previousRevision != null ? { previousRevision } : undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

crowi.userNotificationService.fire() の第5引数には IRevisionHasId 相当の値のオブジェクトを渡すように改修 (v6踏襲)

参考

const results = await userNotificationService.fire(page, req.user, slackChannels, 'update', { previousRevision });

@miya miya requested a review from yuki-takei May 27, 2024 09:21
@@ -25,6 +25,10 @@ const prepareAttachmentTextForCreate = function(page, siteUrl) {
};

const prepareAttachmentTextForUpdate = function(page, siteUrl, previousRevision) {
if (previousRevision == null) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

少しでも type safe に近づくように、jsdoc で previousRevision の型だけでも明示するようにしてください

Copy link
Member Author

Choose a reason for hiding this comment

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

追加しました

@@ -168,7 +169,7 @@
options.grant = grant;
options.userRelatedGrantUserGroupIds = userRelatedGrantUserGroupIds;
}
const previousRevision = await Revision.findById(revisionId);
previousRevision = await Revision.findById<IRevisionHasId>(revisionId);
Copy link
Member

Choose a reason for hiding this comment

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

この failure はちゃんと消えるような実装をしてください

Copy link

reg-suit bot commented May 28, 2024

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴

🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

@miya miya requested a review from yuki-takei May 28, 2024 01:18
@yuki-takei yuki-takei merged commit 869e2fa into master May 28, 2024
20 of 24 checks passed
@yuki-takei yuki-takei deleted the fix/146980-slack-notification-not-sent-on-page-update branch May 28, 2024 20:22
@github-actions github-actions bot mentioned this pull request May 28, 2024
@yuki-takei yuki-takei mentioned this pull request May 30, 2024
@github-actions github-actions bot mentioned this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants