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

custom_profile_fields: Improve user validation efficiency. #29979

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ngadou
Copy link

@ngadou ngadou commented May 7, 2024

Refactor user validation in check_valid_user_ids function for custom profile fields in zerver/models/custom_profile_fields.py. Instead of individual queries within a loop to fetch user profiles, utilize a bulk fetch query with UserProfile.objects.filter() to retrieve user profiles efficiently. This change enhances performance, particularly for scenarios with multiple user IDs, ensuring a smoother user validation process.

The previous implementation iterated over each user ID to retrieve user profiles, resulting in potential performance overhead for large sets of user IDs. This refactor addresses the inefficiency by fetching user profiles in bulk, significantly reducing database queries and improving overall validation speed.

Fixes:
N/A

Screenshots and screen captures:
I am a new contributor and I am working on #29921 (PR #29977)

While Navigating the models, I fell on this note in the zerver/models/custom_profile_fields.py file

image

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

Refactor user validation in check_valid_user_ids function for custom profile fields in zerver/models/custom_profile_fields.py. Instead of individual queries within a loop to fetch user profiles, utilize a bulk fetch query with UserProfile.objects.filter() to retrieve user profiles efficiently. This change enhances performance, particularly for scenarios with multiple user IDs, ensuring a smoother user validation process.

The previous implementation iterated over each user ID to retrieve user profiles, resulting in potential performance overhead for large sets of user IDs. This refactor addresses the inefficiency by fetching user profiles in bulk, significantly reducing database queries and improving overall validation speed.
@alya alya added the integration review Added by maintainers when a PR may be ready for integration. label May 7, 2024
@alya
Copy link
Contributor

alya commented May 7, 2024

@timabbott Do you want to take a look at this directly?

@timabbott
Copy link
Sponsor Member

@ngadou did you intend to push changes in addition to marking my comment as resolved?

Check out our GitHub guide for advice on how to manage a PR.

@ngadou
Copy link
Author

ngadou commented May 10, 2024

@ngadou did you intend to push changes in addition to marking my comment as resolved?

Check out our GitHub guide for advice on how to manage a PR.

Your feedback wasn't clear could you please state how the access_user_common instead of the is_bot

@timabbott
Copy link
Sponsor Member

What do you mean? git grep access_user_common.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when a PR may be ready for integration. size: M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants