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: signin の資格情報が足りないだけの場合はエラーにせず200を返すように #14700

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

kakkokari-gtyih
Copy link
Contributor

@kakkokari-gtyih kakkokari-gtyih commented Oct 4, 2024

What

  • /signin -> /signin-flow
  • 資格情報が足りないだけなら200でfinished: falseを返す

Why

Fix #14699

Additional info (optional)

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added packages/frontend Client side specific issue/PR packages/backend Server side specific issue/PR packages/misskey-js labels Oct 4, 2024
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 1.89873% with 155 lines in your changes missing coverage. Please review.

Project coverage is 41.28%. Comparing base (fa06c59) to head (cb426bc).

Files with missing lines Patch % Lines
packages/frontend/src/components/MkSignin.vue 0.00% 123 Missing ⚠️
...ackages/backend/src/server/api/SigninApiService.ts 10.00% 18 Missing ⚠️
...es/frontend/src/components/MkSignupDialog.form.vue 0.00% 8 Missing ⚠️
packages/backend/src/server/api/SigninService.ts 25.00% 3 Missing ⚠️
...ackages/frontend/src/components/MkSignupDialog.vue 0.00% 2 Missing ⚠️
...ackages/backend/src/server/api/ApiServerService.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #14700      +/-   ##
===========================================
+ Coverage    39.67%   41.28%   +1.60%     
===========================================
  Files         1548     1552       +4     
  Lines       194496   200224    +5728     
  Branches      2566     3579    +1013     
===========================================
+ Hits         77174    82668    +5494     
- Misses      116756   116955     +199     
- Partials       566      601      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Oct 4, 2024

このPRによるapi.jsonの差分
差分はありません。
Get diff files from Workflow Page

@anatawa12
Copy link
Member

うーん資格情報が足りない状態でのレスポンスに200は強い違和感を持ちますね

403が違うってのは多分そうなのですが、401は不適当ですし400あたりがよいと私は感じます

@kakkokari-gtyih
Copy link
Contributor Author

うーん資格情報が足りない状態でのレスポンスに200は強い違和感を持ちますね

403が違うってのは多分そうなのですが、401は不適当ですし400あたりがよいと私は感じます

仕様通り想定して飛んできているリクエストに対してエラーを返すのはおかしい

#14699

@kakkokari-gtyih kakkokari-gtyih marked this pull request as ready for review October 4, 2024 11:17
@kakkokari-gtyih
Copy link
Contributor Author

tabun done

@syuilo
Copy link
Member

syuilo commented Oct 4, 2024

ファイル名とかクラス名は変えなくても良い気がするわね

@kakkokari-gtyih
Copy link
Contributor Author

ファイル名とかクラス名は変えなくても良い気がするわね

もどした

@kakkokari-gtyih kakkokari-gtyih added this to the v2024.10.0 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/signin の設計がおかしい可能性がある
3 participants