Repository navigation
fix: report endorsed status for threads in MySQL thread lists - #292
taimoor-ahmed-1 merged 1 commit into
Conversation
MySQLBackend.get_endorsed() returns a dict keyed by the thread id as a string, but threads_presentor() looked it up with the integer primary key, so every thread in a list was reported as unendorsed. Answered questions therefore lost their "answered" state in the discussions sidebar after a page reload, while the single-thread endpoint (which does not go through threads_presentor) kept reporting them correctly. Look the thread up by str(thread.pk), matching the read state and abuse flag lookups next to it.
|
Thanks for the pull request, @AhtishamShahid! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused fix aligns ID types and is adequately covered by a regression test.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes MySQL thread lists to preserve answered/endorsed status after reload.
Changes:
- Uses string thread IDs for endorsed-status lookup.
- Adds regression coverage for answered and unanswered threads.
| File | Description |
|---|---|
src/forum/backends/mysql/api.py |
Corrects endorsed-status lookup key type. |
tests/test_backends/test_mysql/test_api.py |
Adds regression test for endorsement presentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Hi , @taimoor-ahmed-1 can you please merge this PR? I don't have write access to this repo. Thanks. |
Description
With the MySQL backend, a question with an endorsed ("answered") response is always listed as unanswered.
MySQLBackend.get_endorsed()returns a dict keyed bystr(thread_id):threads_presentor()looked that dict up with the integer primary key, so the lookup always missed and every thread came back withendorsed: False:This PR looks the thread up by
str(thread.pk), which matches theread_statesandthreads_flaggedlookups next to it. The MongoDB backend is not affected because it uses string ids on both sides.User-facing impact
In the discussions MFE, marking a response as the answer shows the green "answered" checkmark in the post list. That's because the MFE re-fetches the single thread, which does not go through
threads_presentor. After a page reload, the list is rebuilt from the thread-list endpoint, which returnedhas_endorsed: false, so the checkmark disappeared. Course staff then had no way to see at a glance which questions still needed an answer.Testing
test_threads_presentor_includes_endorsed_status. It fails onmasterand passes with this change.pytest --ignore=tests/e2e: 203 passed).pylintandmypyare clean.isortis clean on the changed files.Manual testing instructions
forum_v2.enable_mysql_backendfor a course.GET /api/discussion/v1/threads/?course_id=...returns"has_endorsed": falsefor the thread."has_endorsed": true.Merge checklist:
Check off if complete or not applicable: