Fix UnboundLocalError in ClassroomNotificationsViewset.list when filter_queryset raises DatabaseError#14345
Conversation
…er_queryset raises DatabaseError
github-actions
Bot
added
DEV: backend
|
👋 Thanks for contributing! We will assign a reviewer within the next two weeks. In the meantime, please ensure that:
We'll be in touch! 😊 |
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
I asked you to include a regression test in this, please include.
|
I am Sorry for that thankyou for pointing out .I will add a regression test and update the PR . |
|
I’ve added a regression test and updated the PR. Please review and suggest changes if necessary , thanks again for pointing out the mistake. |
|
|
||
|
|
||
| def test_database_error_does_not_crash(self): | ||
| from unittest.mock import patch |
There was a problem hiding this comment.
Not sure why these imports are inlined like this - there's no reason to defer their import.
This is a common pattern for LLM generated Python code, but I don't see any disclosure of your usage of LLM tools.
There was a problem hiding this comment.
Thanks for pointing out that @rtibbles
yes you are 100% right that there is no need to defer that import sorry for that i will move the import to top and update pr accordingly
the inline import was really not intentional i just by mistake overlooked while writing the test . actually i sometimes write quick test this way and then clean them up afterwards but unfortunately missed moving up in this case.
i would like to clear about LLm comment while learning and exploring the codebase i do occasionally use llm tool for guidance but for code i generally write myself else would have mentioned in the specific section . i am trying to improve my coding style .
thanks again for review i will push the fix in few minute
|
i have moved import to top and pushed the update . Please let me know if anything else needs any adjustment . |
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
As long as all checks pass this is good to go! Thank you.
Build Artifacts
|
|
All checks are passing now . Thanks for the review. |
|
Thank you! |
TheGreatPratyush
deleted the
fix-classroom-notification-unboundlocalerror
branch
…er_queryset raises DatabaseError (learningequality#14345) * Fix UnboundLocalError in ClassroomNotificationsViewset.list when filter_queryset raises DatabaseError * Add regression test for ClassroomNotificationsViewset DatabaseError handling * Move unittest.mock import to top-level as suggested in review * [pre-commit.ci lite] apply automatic fixes * Fix DatabaseError handling in ClassroomNotificationsViewset.list for postgres CI --------- Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
2 participants
Summary
This PR fixes an issue in
ClassroomNotificationsViewset.listwherequerysetcan be referenced before it was assigned.If
filter_queryset()raises aDatabaseErrororOperationalError,repair_sqlite_db()is triggered, butquerysetis not reassigned before it is used later in the method, which results in an error :UnboundLocalError: local variable 'queryset' referenced before assignmentThis change ensures that
querysetis always defined by assigning an empty queryset after the database repair:queryset = self.get_queryset().none()
With this fix, the
/coach/api/notifications/endpoint no longer crasheswith a 500 error and instead returns a valid response.
References
Fixes #14322
Reviewer guidance
DatabaseErrorinsidefilter_queryset()./coach/api/notifications/endpoint no longer raisesUnboundLocalError.