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): ノート投稿に関する実績が投稿したアカウントで解除されるように #13511

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

Conversation

kakkokari-gtyih
Copy link
Contributor

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

What

Why

Fix #13361

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 the packages/frontend Client side specific issue/PR label Mar 4, 2024
@kakkokari-gtyih kakkokari-gtyih marked this pull request as draft March 4, 2024 05:53
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 56.75676% with 16 lines in your changes missing coverage. Please review.

Project coverage is 19.16%. Comparing base (6718a54) to head (31ec1ee).

Files with missing lines Patch % Lines
packages/frontend/src/scripts/achievements.ts 23.80% 16 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #13511       +/-   ##
============================================
- Coverage    39.47%   19.16%   -20.32%     
============================================
  Files         1559      726      -833     
  Lines       196948   103499    -93449     
  Branches      3563      996     -2567     
============================================
- Hits         77750    19833    -57917     
+ Misses      118592    83111    -35481     
+ Partials       606      555       -51     

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

Comment on lines 500 to 501
// バックエンドにも実績を獲ったかどうかのチェックがあるのでtoken指定時は常に実績獲得を送信する
if (!token && claimedAchievements.includes(type)) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

自アカウントの場合は獲得されたかどうかがすぐに分かるけどtoken指定の場合はそうは行かないので、token指定時は実績がすでに獲得されているかどうかの判定を行わずすべて送信する仕様にした(バックエンド側でも獲得されているかのチェックが入るので問題なさそう)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// バックエンドにも実績を獲ったかどうかのチェックがあるのでtoken指定時は常に実績獲得を送信する
if (!token && claimedAchievements.includes(type)) return;
// バックエンドにも実績を獲ったかどうかのチェックがあるのでtoken指定時は常に実績獲得を送信する
if ((!token || token === $i.token) && claimedAchievements.includes(type)) return;

でもいいかもしれない

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

Changelogを入れたところが違うのでautomated releaseのあれを発動するなりしないといけない

@kakkokari-gtyih
Copy link
Contributor Author

c解消

@kakkokari-gtyih
Copy link
Contributor Author

解消

Comment on lines +492 to +495
const claimingQueue = new Set<{
name: typeof ACHIEVEMENT_TYPES[number];
token?: string;
}>();
Copy link
Member

Choose a reason for hiding this comment

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

SetにObject入れても期待した結果にならないわね(別のものとして扱われる)

Copy link
Contributor Author

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.

mapにするか

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
Development

Successfully merging this pull request may close these issues.

初回投稿実績がノートしていないのに取得できる
2 participants