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

Fix duplicate handling in update questions backend #140

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

SelwynAng
Copy link

Previously, the update question endpoint failed to check for duplicate title in the database, allowing for duplicates titles to be stored in the database.

Changes made

  • Add in check for duplicate questions, return 409 if there are duplicates

@SelwynAng SelwynAng added the bug Something isn't working label Sep 26, 2024
question-service/app/crud/questions.py Outdated Show resolved Hide resolved
question-service/app/crud/questions.py Outdated Show resolved Hide resolved
@jq1836 jq1836 self-requested a review September 26, 2024 08:23
Copy link

@jq1836 jq1836 left a comment

Choose a reason for hiding this comment

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

LGTM! Only a minor nitpick

@@ -39,14 +39,22 @@ async def delete_question(question_id: str):

async def update_question_by_id(question_id: str, question_data: UpdateQuestionModel):
Copy link

Choose a reason for hiding this comment

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

Would be difficult to identify what this function returns. Currently, it can return None, str and QuestionModel which may make it difficult to understand the code

Copy link
Author

Choose a reason for hiding this comment

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

Yep, we need some form error handling for this and other end points too. Instead of returning None/str, should throw error instead

@jq1836 jq1836 merged commit b8c0ae6 into main Sep 26, 2024
1 check passed
@jq1836 jq1836 deleted the fix/bug/update-question-endpoint branch September 26, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants