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

🐛 get suspended users #371

Merged
merged 1 commit into from
Oct 4, 2024
Merged

🐛 get suspended users #371

merged 1 commit into from
Oct 4, 2024

Conversation

ras0q
Copy link
Member

@ras0q ras0q commented Oct 3, 2024

User description

fix #339


PR Type

bug_fix


Description

  • 修正されたバグにより、ユーザーの取得時に停止されたユーザーも含まれるようになりました。
  • getUsers API 呼び出しに includeSuspended パラメータを追加しました。

Changes walkthrough 📝

Relevant files
Bug fix
user.ts
Include suspended users in user fetch API call                     

src/store/user.ts

  • Added a parameter includeSuspended to the getUsers API call.
  • Ensures suspended users are included in the fetched user list.
  • +2/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @ras0q ras0q requested a review from Pugma October 3, 2024 12:58
    Copy link
    Contributor

    github-actions bot commented Oct 3, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Hardcoded Value
    includeSuspended パラメータが常に true に設定されています。これにより、APIが常に停止されたユーザーを含めるようになります。この値を動的に設定できるようにするか、設定可能なオプションを提供することを検討してください。

    Copy link
    Contributor

    github-actions bot commented Oct 3, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    関数の柔軟性とテスト容易性を向上させるために、ハードコードされた変数を引数に置き換えます。

    includeSuspended 変数を fetchUsers 関数の引数として渡すことを検討してください。これにより、関数の再利用性とテストのしやすさが向上します。

    src/store/user.ts [14-15]

    -const includeSuspended = true
     const res = await apis.getUsers(includeSuspended)
    Suggestion importance[1-10]: 8

    Why: The suggestion to pass includeSuspended as an argument to the fetchUsers function enhances the function's reusability and testability by allowing different values to be passed in different contexts. This change improves the flexibility of the code without altering its current functionality.

    8

    Copy link
    Collaborator

    @Pugma Pugma left a comment

    Choose a reason for hiding this comment

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

    凍結済みを含めて全ユーザーを受け取ったあと、それを表示するかはブラウザ上でトグルなどを用いて選択できるようにできれば UX も良さそうですが、とりあえず API を叩く部分のロジックとしては今の状態で大丈夫だと思います
    ありがとうございます!

    @ras0q
    Copy link
    Member Author

    ras0q commented Oct 4, 2024

    apiのレスポンスには凍結情報が含まれてないのでそこは注意ですね
    現役だけの取得が必要になれば適宜includeSuspendedを変更できるようにすると良さそうです

    @ras0q ras0q merged commit 1a99f62 into main Oct 4, 2024
    10 checks passed
    @ras0q ras0q deleted the fix/get-suspended-users branch October 4, 2024 05:49
    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.

    usersの取得時に凍結済みユーザーも取得したい
    2 participants