-
Notifications
You must be signed in to change notification settings - Fork 0
fix: optimize mongo query for user vote list #19
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
base: release-ulmo
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,4 +2,4 @@ | |
| Openedx forum app. | ||
| """ | ||
|
|
||
| __version__ = "0.4.4" | ||
| __version__ = "0.4.5" | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1159,13 +1159,20 @@ def get_commentables_counts_based_on_type(course_id: str) -> dict[str, Any]: | |||||
| return commentable_counts | ||||||
|
|
||||||
| @classmethod | ||||||
| def get_user_voted_ids(cls, user_id: str, vote: str) -> list[str]: | ||||||
| def get_user_voted_ids( | ||||||
| cls, user_id: str, vote: str, course_id: Optional[str] = None | ||||||
| ) -> list[str]: | ||||||
| """Get the IDs of the posts voted by a user.""" | ||||||
| if vote not in ["up", "down"]: | ||||||
| raise ValueError("Invalid vote type") | ||||||
|
|
||||||
| content_model = Contents() | ||||||
| contents = content_model.get_list() | ||||||
| content_query: dict[str, Any] = {} | ||||||
| if course_id: | ||||||
| content_query["course_id"] = str(course_id) | ||||||
| content_query[f"votes.{vote}"] = {"$in": [user_id, str(user_id)]} | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you help me understand why this is My understanding of MongoDB query syntax is a little hazy, but it seems like we could simplify this:
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me check it in that manner. |
||||||
|
|
||||||
| contents = content_model.get_list(**content_query) | ||||||
| voted_ids = [] | ||||||
| for content in contents: | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can any of this be removed now that the query is more refined? |
||||||
| votes = content["votes"][vote] | ||||||
|
|
@@ -1207,8 +1214,12 @@ def user_to_hash( | |||||
|
|
||||||
| if params.get("complete"): | ||||||
| subscribed_thread_ids = cls.find_subscribed_threads(user["external_id"]) | ||||||
| upvoted_ids = cls.get_user_voted_ids(user["external_id"], "up") | ||||||
| downvoted_ids = cls.get_user_voted_ids(user["external_id"], "down") | ||||||
| upvoted_ids = cls.get_user_voted_ids( | ||||||
| user["external_id"], "up", params.get("course_id") | ||||||
| ) | ||||||
| downvoted_ids = cls.get_user_voted_ids( | ||||||
| user["external_id"], "down", params.get("course_id") | ||||||
| ) | ||||||
| hash_data.update( | ||||||
| { | ||||||
| "subscribed_thread_ids": subscribed_thread_ids, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1132,7 +1132,9 @@ def get_threads( | |
| return threads | ||
|
|
||
| @classmethod | ||
| def get_user_voted_ids(cls, user_id: str, vote: str) -> list[str]: | ||
| def get_user_voted_ids( | ||
| cls, user_id: str, vote: str, course_id: Optional[str] = None | ||
| ) -> list[str]: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a plan or ticket to optimize this by including course ID in the query?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (To be clear, I'm not saying we have to do this. If 2U isn't using the MySQL backend, we could just pass this info along to someone who is.) |
||
| """Get the IDs of the posts voted by a user.""" | ||
| if vote not in ["up", "down"]: | ||
| raise ValueError("Invalid vote type") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -624,7 +624,12 @@ def test_update_user_stats(api_client: APIClient, patched_get_backend: Any) -> N | |
| # Sort the data for expected result (threads, responses, replies) | ||
| expected_result = sorted( | ||
| expected_data.values(), | ||
| key=lambda val: (val["threads"], val["responses"], val["replies"]), | ||
| key=lambda val: ( | ||
| val["threads"], | ||
| val["responses"], | ||
| val["replies"], | ||
| val["username"], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this the unrelated test change? I'm guessing this was a flaky test that you're fixing here. If so, can you ensure this ends up in the squash commit's message?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this was unrelated test-case. Yes, will make sure to add it in squash commit message. |
||
| ), | ||
| reverse=True, | ||
| ) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add test coverage for these changes? (Not sure if this repo is set up with a mock DB for unit tests.)