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.

Sign up for GitHub

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
Merged

Fix duplicate handling in update questions backend #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
74d773a
Fix duplicate handling in update questions backend
SelwynAng Sep 26, 2024
efc487f
Update fix
SelwynAng Sep 26, 2024
ffc2749
Resolve merge conflict
SelwynAng Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions question-service/app/crud/questions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

existing_question = await question_collection.find_one({"_id": ObjectId(question_id)})

if existing_question is None:
return None

update_data = question_data.model_dump(exclude_unset=True)

# Check if the new title already exists and belongs to another question
if "title" in update_data and update_data["title"] != existing_question["title"]:
existing_title = await question_collection.find_one({"title": update_data["title"]})
if existing_title and str(existing_title["_id"]) != question_id:
return "duplicate_title"

updated_data = {"$set": question_data.model_dump(exclude_unset=True)}
if not updated_data["$set"]:
if not update_data:
return existing_question
await question_collection.update_one({"_id": ObjectId(question_id)}, updated_data)

await question_collection.update_one({"_id": ObjectId(question_id)}, {"$set": update_data})
return await question_collection.find_one({"_id": ObjectId(question_id)})

async def batch_create_questions(questions: List[CreateQuestionModel]):
Expand All @@ -62,4 +70,4 @@ async def batch_create_questions(questions: List[CreateQuestionModel]):

result = await question_collection.insert_many(new_questions)

return {"message": f"{len(result.inserted_ids)} questions added successfully."}
return {"message": f"{len(result.inserted_ids)} questions added successfully."}
4 changes: 4 additions & 0 deletions question-service/app/routers/questions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,13 @@ async def delete(question_id: str):
@router.put("/{question_id}", response_description="Update question with specified id", response_model=QuestionModel)
async def update_question(question_id: str, question_data: UpdateQuestionModel):
updated_question = await update_question_by_id(question_id, question_data)

if updated_question is None:
raise HTTPException(status_code=404, detail="Question with this id does not exist.")

if updated_question == "duplicate_title":
raise HTTPException(status_code=409, detail="A question with this title already exists.")

return updated_question


Expand Down