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(frontend/MkSignup): prevent uncaught errors from interrupted signup #10265

Merged
merged 5 commits into from
Mar 11, 2023

Conversation

saschanaz
Copy link
Member

なにを

  1. onSubmitをasync functionにしてAPIリクエストエラーなどを全てcatch
  2. タイプエラーを修正

なんで

#10262 とは別の原因でローカルでcypressテストが落ちるので

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #10265 (fe9f470) into develop (89393aa) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop   #10265      +/-   ##
===========================================
- Coverage    70.75%   70.74%   -0.01%     
===========================================
  Files          807      807              
  Lines        77290    77290              
  Branches      5383     5384       +1     
===========================================
- Hits         54685    54682       -3     
- Misses       22605    22608       +3     
Impacted Files Coverage Δ
packages/backend/src/core/NotificationService.ts 95.71% <0.00%> (-4.29%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment on lines 61 to 63
<MkCaptcha v-if="instance.enableHcaptcha" ref="hcaptcha" v-model="hCaptchaResponse" class="captcha" provider="hcaptcha" :sitekey="instance.hcaptchaSiteKey"/>
<MkCaptcha v-if="instance.enableRecaptcha" ref="recaptcha" v-model="reCaptchaResponse" class="captcha" provider="recaptcha" :sitekey="instance.recaptchaSiteKey"/>
<MkCaptcha v-if="instance.enableTurnstile" ref="turnstile" v-model="turnstileResponse" class="captcha" provider="turnstile" :sitekey="instance.turnstileSiteKey"/>
<MkCaptcha v-if="instance.enableHcaptcha && instance.hcaptchaSiteKey" ref="hcaptcha" v-model="hCaptchaResponse" class="captcha" provider="hcaptcha" :sitekey="instance.hcaptchaSiteKey"/>
<MkCaptcha v-if="instance.enableRecaptcha && instance.recaptchaSiteKey" ref="recaptcha" v-model="reCaptchaResponse" class="captcha" provider="recaptcha" :sitekey="instance.recaptchaSiteKey"/>
<MkCaptcha v-if="instance.enableTurnstile && instance.turnstileSiteKey" ref="turnstile" v-model="turnstileResponse" class="captcha" provider="turnstile" :sitekey="instance.turnstileSiteKey"/>
Copy link
Member

Choose a reason for hiding this comment

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

管理者が SiteKey を設定し忘れた際にエラー画面が表示されて問題に気付きやすいので、ここは可能ならそのままの方が良さそう

Copy link
Member Author

Choose a reason for hiding this comment

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

あ、でしたら代わりにsitekeyをnullableにしてみます(いまはタイプエラーが出ていますので)

Copy link
Member Author

@saschanaz saschanaz Mar 8, 2023

Choose a reason for hiding this comment

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

でもsitekeyとenableが共にあるのってちょっと謎なのでは…sitekeyだけでいい気がします

Copy link
Member

Choose a reason for hiding this comment

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

例えば

  1. reCAPTCHA から hCaptcha に乗り換える
  2. hCaptcha が難しいので reCAPTCHA に戻ろうかなと考える
  3. enabled が別で存在するならスイッチを切り替えるだけで良いが、そうでない場合は sitekey を入れ直さなくてはならない

みたいなシナリオは想定できるので別に存在するのは非合理的というわけでもない

Copy link
Member Author

Choose a reason for hiding this comment

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

でもpublic APIでフロントエンドに渡すのはやっぱりsitekeyで充分じゃないですか?🤔

Copy link
Member

Choose a reason for hiding this comment

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

あー……それはそうかもです

Copy link
Member

Choose a reason for hiding this comment

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

と思ったけど管理者画面から表示上消えてしまいそう

@tamaina
Copy link
Contributor

tamaina commented Mar 9, 2023

これも fatal: refusing to merge unrelated histories だな

…私のgitがぶっ壊れてる?

@saschanaz
Copy link
Member Author

これも fatal: refusing to merge unrelated histories だな

:soukamo:

@tamaina
Copy link
Contributor

tamaina commented Mar 9, 2023

update branchボタンがあったので押してみた

@syuilo syuilo merged commit 1ea4469 into misskey-dev:develop Mar 11, 2023
@syuilo
Copy link
Member

syuilo commented Mar 11, 2023

🙏🏻

@saschanaz saschanaz deleted the mksignup-fixes branch March 11, 2023 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants