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
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/app/src/components/PageEditor/PageEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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

overwriteScopesOfDescendants?: boolean
}
export type Save = (
Expand Down
22 changes: 12 additions & 10 deletions apps/app/src/components/SavePageControls.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,32 @@ declare global {
const logger = loggerFactory('growi:SavePageControls');


const SavePageButton = (props: {slackChannels: string, isDeviceLargerThanMd?: boolean}) => {
const SavePageButton = (props: {slackChannels: string, isSlackEnabled?: boolean, isDeviceLargerThanMd?: boolean}) => {

const { t } = useTranslation();
const { data: _isWaitingSaveProcessing } = useWaitingSaveProcessing();
const [isSavePageModalShown, setIsSavePageModalShown] = useState<boolean>(false);

const { slackChannels, isDeviceLargerThanMd } = props;
const { slackChannels, isSlackEnabled, isDeviceLargerThanMd } = props;

const isWaitingSaveProcessing = _isWaitingSaveProcessing === true; // ignore undefined

const save = useCallback(async(): Promise<void> => {
// save
globalEmitter.emit('saveAndReturnToView', { wip: false, slackChannels });
}, [slackChannels]);
globalEmitter.emit('saveAndReturnToView', { wip: false, slackChannels, isSlackEnabled });
}, [isSlackEnabled, slackChannels]);

const saveAndOverwriteScopesOfDescendants = useCallback(() => {
// save
globalEmitter.emit('saveAndReturnToView', { wip: false, overwriteScopesOfDescendants: true, slackChannels });
}, [slackChannels]);
globalEmitter.emit('saveAndReturnToView', {
wip: false, overwriteScopesOfDescendants: true, slackChannels, isSlackEnabled,
});
}, [isSlackEnabled, slackChannels]);

const saveAndMakeWip = useCallback(() => {
// save
globalEmitter.emit('saveAndReturnToView', { wip: true, slackChannels });
}, [slackChannels]);
globalEmitter.emit('saveAndReturnToView', { wip: true, slackChannels, isSlackEnabled });
}, [isSlackEnabled, slackChannels]);

const labelSubmitButton = t('Update');
const labelOverwriteScopes = t('page_edit.overwrite_scopes', { operation: labelSubmitButton });
Expand Down Expand Up @@ -197,11 +199,11 @@ export const SavePageControls = (): JSX.Element | null => {
)
}

<SavePageButton slackChannels={slackChannels} isDeviceLargerThanMd />
<SavePageButton isSlackEnabled={isSlackEnabled} slackChannels={slackChannels} isDeviceLargerThanMd />
</>
) : (
<>
<SavePageButton slackChannels={slackChannels} />
<SavePageButton isSlackEnabled={isSlackEnabled} slackChannels={slackChannels} />
<button
type="button"
className="btn btn-outline-neutral-secondary border-0 fs-5 p-0 ms-1 text-muted"
Expand Down
11 changes: 6 additions & 5 deletions apps/app/src/server/routes/apiv3/page/update-page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
];


async function postAction(req: UpdatePageRequest, res: ApiV3Response, updatedPage: PageDocument) {
async function postAction(req: UpdatePageRequest, res: ApiV3Response, updatedPage: PageDocument, previousRevision: IRevisionHasId | null) {
// Reflect the updates in ydoc
const origin = req.body.origin;
if (origin === Origin.View || origin === undefined) {
Expand Down Expand Up @@ -111,10 +111,10 @@
}

// user notification
const { revisionId, isSlackEnabled, slackChannels } = req.body;
const { isSlackEnabled, slackChannels } = req.body;
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 });

const results = await crowi.userNotificationService.fire(updatedPage, req.user, slackChannels, 'update', option);
results.forEach((result) => {
if (result.status === 'rejected') {
Expand Down Expand Up @@ -159,6 +159,7 @@
}

let updatedPage: PageDocument;
let previousRevision: IRevisionHasId | null;
try {
const {
grant, userRelatedGrantUserGroupIds, overwriteScopesOfDescendants, wip,
Expand All @@ -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() を使ってサニタイズしたものを渡すように改修しました

updatedPage = await crowi.pageService.updatePage(currentPage, body, previousRevision?.body ?? null, req.user, options);
}
catch (err) {
Expand All @@ -183,7 +184,7 @@

res.apiv3(result, 201);

postAction(req, res, updatedPage);
postAction(req, res, updatedPage, previousRevision);
},
];
};
6 changes: 4 additions & 2 deletions apps/app/src/server/service/user-notification/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { IRevisionHasId } from '@growi/core';

import { toArrayFromCsv } from '~/utils/to-array-from-csv';


Expand Down Expand Up @@ -27,11 +29,11 @@ export class UserNotificationService {
* @param {User} user
* @param {string} slackChannelsStr comma separated string. e.g. 'general,channel1,channel2'
* @param {string} mode 'create' or 'update' or 'comment'
* @param {string} previousRevision
* @param {IRevisionHasId} previousRevision
* @param {Comment} comment
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
async fire(page, user, slackChannelsStr, mode, option?: { previousRevision: string }, comment = {}): Promise<PromiseSettledResult<any>[]> {
async fire(page, user, slackChannelsStr, mode, option?: { previousRevision: IRevisionHasId }, comment = {}): Promise<PromiseSettledResult<any>[]> {
const {
appService, slackIntegrationService,
} = this.crowi;
Expand Down
4 changes: 4 additions & 0 deletions apps/app/src/server/util/slack.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.

追加しました


const diff = require('diff');
let diffText = '';

Expand Down
Loading