Add edit and delete actions for sales entries#78
Conversation
|
@Dipti1516 is attempting to deploy a commit to the participationcorner2025-8967's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Codesense Ai: This PR is too large to review automatically. A human maintainer will take a look! |
|
Hey @Dipti1516! Welcome to the project, and thank you for taking on this issue! Your implementation of the Edit and Delete flows is really well thought out, and I especially love that you remembered to roll the inventory item status back to There are just a few things we need to polish up before we can merge this: 1. Revert 2. Critical Security Fix (IDOR Prevention) await prisma.sale.update({
where: {
id: saleId,
userId: user.id // <-- Add this!
},
// ...Make sure to add 3. Cleanup Push those quick fixes and we will get this merged. Really amazing work on the logic so far! |
📝 SummaryThe provided pull request diff introduces several changes to the sales log feature, including the addition of a delete sale modal, editing functionality for existing sales, and improvements to the sales table and log sale modal components. The changes also update the loader and action functions to handle the new functionality. 📂 Files Changed
🎭 Code PoemChanges abound in this code so fine, 🚨 Bugs & Architectural Violations* The `DeleteSaleModal` component does not include any accessibility attributes, such as `aria-hidden` or `role`. This should be added to ensure the component is accessible to all users. * The `LogSaleModal` component uses the `disabled` attribute on the select input when editing a sale. However, this attribute is not properly handled for screen readers. Consider using a more accessible approach, such as using a `readonly` attribute or a separate button to toggle the select input. * The `SalesTable` component uses the `IconEdit` and `IconTrash` components from `@tabler/icons-react`, but does not include any accessibility attributes. This should be added to ensure the icons are accessible to all users. * The `loader` function in `app/routes/sales-log.tsx` does not handle any potential errors that may occur when fetching the sales data. Consider adding error handling to ensure the application remains stable in case of errors. Looks solid otherwise!💡 Suggestions & Best Practices* Consider adding a loading indicator to the `SalesTable` component to indicate when the data is being fetched. * The `LogSaleModal` component uses a lot of inline styles. Consider moving these styles to a separate CSS file to improve maintainability and readability. * The `DeleteSaleModal` component does not include any confirmation message before deleting a sale. Consider adding a confirmation message to ensure the user intends to delete the sale. * The `loader` function in `app/routes/sales-log.tsx` fetches the sales data and inventory data separately. Consider combining these into a single query to improve performance. * The `action` function in `app/routes/sales-log.tsx` handles the edit and delete functionality. Consider breaking this out into separate functions to improve readability and maintainability. |
📝 SummaryThe provided pull request diff introduces several changes to the sales log feature, including the addition of a delete sale modal, editing functionality for existing sales, and improvements to the sales table and log sale modal components. The changes also update the loader and action functions to handle the new functionality. 📂 Files Changed
🎭 Code PoemChanges abound, with features so fine, 🚨 Bugs & Architectural ViolationsThe provided code seems to follow the critical codebase architecture and rules. However, there are a few potential issues: * The `DeleteSaleModal` component does not include any accessibility attributes, such as `aria-hidden` or `role`. * The `LogSaleModal` component uses `useState` to store the sale price, selected item ID, and sale date. However, it does not handle cases where the sale data is not available or is invalid. * The `SalesTable` component uses `onClick` events to handle the edit and delete buttons. However, it does not provide any feedback to the user when the buttons are clicked. * The `loader` function returns a serialized version of the sales data, but it does not handle cases where the data is not available or is invalid. Looks solid, but these potential issues should be addressed.💡 Suggestions & Best Practices* Consider adding accessibility attributes to the `DeleteSaleModal` component. * Add error handling to the `LogSaleModal` component to handle cases where the sale data is not available or is invalid. * Provide feedback to the user when the edit and delete buttons are clicked in the `SalesTable` component. * Consider adding validation to the `loader` function to handle cases where the data is not available or is invalid. * Use a more robust way to handle the sale data, such as using a state management library or a more robust data storage solution. * Consider adding a loading indicator to the `SalesTable` component to handle cases where the data is being loaded. * Use a more consistent naming convention throughout the codebase. |
|
Hii @rushikesh-bobade , Thanks for the review! I've addressed all the requested changes, including reverting package-lock.json, adding ownership checks to the edit and delete actions, and removing the leftover console.log. I'd appreciate another review when you have time. Thanks! |
|
@Dipti1516 Awesome job, the IDOR security fixes look perfect! Thank you! You probably noticed the branch has merge conflicts now. This is just because we merged another PR recently that added To fix it, just run Let me know if you run into any Git trouble and I can help you out! |
|
Codesense Ai: This PR is too large to review automatically. A human maintainer will take a look! |
|
Hi @Dipti1516, Thanks for resolving those conflicts! The IDOR security fixes look perfect, and the CI typecheck passed successfully. However, I did a deep code review and noticed two critical bugs that accidentally slipped in while you were resolving the Git merge conflicts in VS Code: 1. Missing UI InputsIn 2. Missing Backend Update FieldsIn Once you restore those inputs and add the missing backend fields, we are completely good to merge! Let me know if you need any help finding where they go! |
|
Codesense Ai: This PR is too large to review automatically. A human maintainer will take a look! |
2 participants
Description
This PR adds edit and delete functionality for sales entries in the Sales Log.
Changes made
IN_STOCKwhen a sale is deletedRelated Issues
Fixes #28
Type of Change
Checklist:
Screenshots