Feat/delete question #914
Feat/delete question #914
Conversation
|
@JosueBrenes is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new method, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/admin/taskSteps/quizStep.tsx (3 hunks)
🔇 Additional comments (2)
components/admin/taskSteps/quizStep.tsx (2)
10-10: LGTM: Icon import is properly placedThe FaTrash icon import is correctly added and follows the existing import structure.
154-175: Consider adding safeguards to the delete operationWhile the implementation is technically sound, consider adding the following safety measures:
- Confirmation dialog before deletion
- Prevention of deleting the last question
- Potential undo functionality for accidental deletions
Consider implementing these improvements:
const handleDeleteQuestion = useCallback( (questionIndex: number) => { // Prevent deleting the last question if (steps[index].data.questions.length <= 1) { alert("Cannot delete the last question"); return; } // Add confirmation if (!window.confirm("Are you sure you want to delete this question?")) { return; } const updatedSteps = steps.map((step, i) => { if (i === index && step.type === "Quiz") { const updatedQuestions = step.data.questions.filter( (question: typeof QuizQuestionDefaultInput, qIndex: number) => qIndex !== questionIndex ); return { ...step, data: { ...step.data, questions: updatedQuestions, }, }; } return step; }); setSteps(updatedSteps); }, [steps] );
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
types/backTypes.d.ts (1)
469-472: Consider adding JSDoc comments and validation constraints.
The type definition is well-structured and correctly implements the required fields for question deletion. However, it could benefit from:
- JSDoc comments explaining the purpose and usage
- Validation constraints (e.g., positive numbers) if supported by your validation framework
Example enhancement:
+/**
+ * Type for deleting a specific quiz question
+ * @property {number} id - The unique identifier of the question to delete
+ * @property {number} quiz_id - The identifier of the parent quiz containing the question
+ */
export type DeleteQuizQuestion = {
- id: number;
- quiz_id: number;
+ id: number; // Must be positive
+ quiz_id: number; // Must be positive
};components/admin/taskSteps/quizStep.tsx (1)
Line range hint 13-23: Consider improving type safety and component structure.
- Define proper TypeScript interfaces for the quiz data structure:
interface QuizQuestion {
id: number;
question: string;
options: string[];
correct_answers: number[];
}
interface QuizData {
quiz_id: number;
quiz_name: string;
quiz_desc: string;
quiz_intro: string;
quiz_cta: string;
quiz_help_link: string;
questions: QuizQuestion[];
}
interface StepMap {
type: 'Quiz';
data: QuizData;
}- Consider splitting the component into smaller, more focused components:
- QuizMetadata (for quiz name, description, etc.)
- QuestionList (for managing questions)
- QuestionEditor (for individual questions)
This would improve maintainability and make the code easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- components/admin/taskSteps/quizStep.tsx (3 hunks)
- services/authService.ts (3 hunks)
- types/backTypes.d.ts (1 hunks)
🔇 Additional comments (6)
components/admin/taskSteps/quizStep.tsx (2)
10-10: LGTM: Import statement is correctly placed.
The FaTrash icon import is appropriately added for the delete functionality.
238-250: Previous accessibility concerns still apply.
The current implementation of the delete button still needs accessibility improvements as mentioned in the previous review.
services/authService.ts (4)
18-18: LGTM! Type import follows conventions
The DeleteQuizQuestion type import is properly placed and follows the established naming pattern.
618-618: LGTM! Export follows existing pattern
The deleteQuizQuestion method is properly exported in the AdminService object.
496-513: Consider implementing safeguards for question deletion
As deletion is a destructive operation, consider implementing these safeguards:
- Soft delete pattern instead of hard delete
- Audit logging for tracking who deleted what and when
- Validation to prevent deletion of questions that are part of active quizzes
Let's check if soft delete is used elsewhere in the codebase:
#!/bin/bash
# Look for soft delete patterns
rg "deleted_at|is_deleted|soft.?delete" --type ts
# Check for audit logging patterns
rg "audit|log.*delete|track.*change" --type ts496-513:
Enhance error handling and response validation
While the implementation follows the service's pattern, there are several areas for improvement:
- Add error return value and improve error handling:
const deleteQuizQuestion = async (params: DeleteQuizQuestion) => {
try {
const response = await fetch(
- `${baseurl}/admin/tasks/quiz/question/delete`,
+ `${baseurl}/admin/tasks/quiz/question/delete`,
{
method: "POST",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${localStorage.getItem("token")}`,
},
body: JSON.stringify(params),
}
);
+ const data = await response.json();
+ if (!response.ok) {
+ throw new Error(data.message || 'Failed to delete quiz question');
+ }
- return await response.json();
+ return data;
} catch (err) {
console.log("Error while deleting quiz question", err);
+ return { error: 'Failed to delete quiz question' };
}
};- Consider adding input validation:
if (!params.id || !params.quiz_id) {
return { error: 'Invalid parameters: id and quiz_id are required' };
}Let's verify the error handling pattern across the codebase:
New Delete Question Button
Changes description
feat: Function is created to delete a question.
feat: Added a trash icon using react-icons/fa. Clicking the icon calls and executes the previously created function to delete the question.
Other information (Current output)
2024-10-28.13-01-52.mp4
Summary by CodeRabbit
New Features
UI Enhancements
Type Definitions