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

imprv: Upload handler use apiv3 post #8279

Merged
merged 24 commits into from
Dec 8, 2023

Conversation

reiji-h
Copy link
Contributor

@reiji-h reiji-h commented Nov 29, 2023

概要

PageEditor.tsx の uploadHandler 内で添付ファイルの作成に apiv3 を使用するように変更

Task

コメントの削除について

  • editorRef.current.insertText(insertText);
    • codeMirrorEditor?.insertText(insertText); に変更したため削除
  • editorRef.current.terminateUploadingState();
    • react-dropzone に状態の管理を任せて不要になったため削除

apps/app/src/components/PageEditor/PageEditor.tsx Outdated Show resolved Hide resolved
apps/app/src/server/routes/apiv3/attachment.js Outdated Show resolved Hide resolved
Comment on lines 287 to 297
router.post('/add', uploads.single('file'), accessTokenParser, loginRequired, apiV3FormValidator, addActivity, async(req, res) => {
const pageId = req.body.page_id || null;
const pagePath = req.body.path || null;

// check params
if (req.file == null) {
return res.apiv3Err('File error.');
}
if (pageId == null && pagePath == null) {
return res.apiv3Err('Either page_id or path is required.');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

同じくバリデーションを作ってください。そうすればこのあたりの処理もいらなくなる?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

req 直下のバリデーションが無理そうだったので req.file は残しました。
また、pagePath はあったとしても使わず、pageId == null, pagePath == なにかという状況ではファイルのアップロードができないことからそもそも pagePath を入れるのをやめました。

apps/app/src/server/routes/apiv3/attachment.js Outdated Show resolved Hide resolved
apps/app/src/server/routes/apiv3/attachment.js Outdated Show resolved Hide resolved
*/
router.get('/limit', accessTokenParser, loginRequiredStrictly, validator.retrieveFileLimit, apiV3FormValidator, async(req, res) => {
const { fileUploadService } = crowi;
const fileSize = Number(req.query.fileSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

Number() で囲っているけれど、validator.isNumeric() で検証しているので不要だと思う。

/attachment/limit を使っている個所は一か所しかなく number 型が入ってくる

accessToken を使ったときに対応しているのか?
ただ、その場合でも isNumeric() により検証できるようになるはず。

Copy link
Contributor

Choose a reason for hiding this comment

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

クエリパラメータはテキストで渡るので number 型へのキャストが必要

Comment on lines 326 to 328
if (pageId == null) {
formData.append('page_body', codeMirrorEditor?.getDoc() ?? '');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

直接 PR に関係ないですが、この処理は必要でしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

page_body を使用しなかったので、消しました。

*/
router.get('/limit', accessTokenParser, loginRequiredStrictly, validator.retrieveFileLimit, apiV3FormValidator, async(req, res) => {
const { fileUploadService } = crowi;
const fileSize = Number(req.query.fileSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

クエリパラメータはテキストで渡るので number 型へのキャストが必要

* @apiParam {String} path
* @apiParam {File} file
*/
router.post('/add', uploads.single('file'), autoReap, accessTokenParser, loginRequiredStrictly, excludeReadOnlyUser,
Copy link
Member

Choose a reason for hiding this comment

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

router.post('/', でいいと思います

Copy link

reg-suit bot commented Dec 8, 2023

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.

@reiji-h reiji-h merged commit 2592320 into dev/7.0.x Dec 8, 2023
16 of 22 checks passed
@reiji-h reiji-h deleted the imprv/131756-131772-apiv3-file-upload branch December 8, 2023 07:24
@github-actions github-actions bot mentioned this pull request Dec 8, 2023
@yuki-takei yuki-takei mentioned this pull request Mar 26, 2024
@github-actions github-actions bot mentioned this pull request Mar 26, 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
Development

Successfully merging this pull request may close these issues.

3 participants