feat(task.sql): implement transaction#167
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed because the current script can leave the database in an inconsistent state and does not fully satisfy the transactional requirements. Your INSERT INTO Orders is executed before START TRANSACTION, so if inserting the order item or updating Products.WarehouseAmount fails, you could end up with an order without matching items or stock changes; move START TRANSACTION to before the Orders insert so all three operations run atomically and can be rolled back together. When inserting into OrderItems, you currently hardcode OrderID = 1, which will be wrong as soon as the Orders table has any existing rows and also violates the requirement to use the newly created order's ID. Instead, capture the ID of the newly inserted order (for example with SET @order_id = LAST_INSERT_ID(); immediately after the Orders insert) and use @order_id in the OrderItems insert so the item is correctly linked to its order. Make sure the UPDATE Products SET WarehouseAmount = WarehouseAmount - 1 is also inside the same transaction and conceptually tied to the OrderItems row you just inserted. As smaller improvements, avoid using Count as an unquoted column name (or escape it with backticks) and consider checking that WarehouseAmount is sufficient before decrementing, but these are secondary to fixing the transaction boundary and OrderID handling. Once you adjust the transaction scope and order ID usage as described, your solution will align well with the task goals and be much more robust.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| USE ShopDB; | ||
|
|
||
| -- Some data should be created outside the transaction (here) | ||
| -- Create empty order outside transaction |
There was a problem hiding this comment.
Creating the order outside the transaction can leave the database inconsistent if later steps fail. Start the transaction before inserting the order so order creation, order item insert and stock update are atomic.
|
|
||
| -- Some data should be created outside the transaction (here) | ||
| -- Create empty order outside transaction | ||
| INSERT INTO Orders (CustomerID, Date) |
There was a problem hiding this comment.
This INSERT should be executed inside the same transaction that updates OrderItems and Products. Also capture the new order's ID (e.g., with LAST_INSERT_ID() or assign to a user variable) instead of assuming it will be 1.
| -- Some data should be created outside the transaction (here) | ||
| -- Create empty order outside transaction | ||
| INSERT INTO Orders (CustomerID, Date) | ||
| VALUES (1, '2023-01-01'); |
There was a problem hiding this comment.
Hardcoded values are acceptable for the date, but ensure this INSERT is inside the transaction and that you capture the inserted order ID to reference it in OrderItems.
|
|
||
| -- Start the transaction | ||
| START TRANSACTION; | ||
| -- Start the transaction |
There was a problem hiding this comment.
Good to use a transaction, but it must start before the Orders INSERT. Move START TRANSACTION to before creating the Orders row so all related changes are committed or rolled back together.
| START TRANSACTION; | ||
|
|
||
| -- And some data should be created inside the transaction | ||
| -- Add product to the order (OrderID = 1, ProductID = 1, Count = 1) |
There was a problem hiding this comment.
Inserting the order item uses a hardcoded OrderID (1). Instead use the ID of the order you just inserted (e.g., use LAST_INSERT_ID() or store it in a variable) to ensure you link to the correct order.
|
|
||
| -- And some data should be created inside the transaction | ||
| -- Add product to the order (OrderID = 1, ProductID = 1, Count = 1) | ||
| INSERT INTO OrderItems (OrderID, ProductID, Count) |
There was a problem hiding this comment.
Using the column name Count can be problematic because COUNT is a SQL function/reserved word. Either escape it with backticks (Count) or rename the column to avoid ambiguity.
| -- And some data should be created inside the transaction | ||
| -- Add product to the order (OrderID = 1, ProductID = 1, Count = 1) | ||
| INSERT INTO OrderItems (OrderID, ProductID, Count) | ||
| VALUES (1, 1, 1); |
There was a problem hiding this comment.
Values are hardcoded: this makes the script fragile. Replace the hardcoded OrderID with LAST_INSERT_ID() or a previously stored variable referencing the inserted order (e.g., VALUES (LAST_INSERT_ID(), 1, 1)).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed because your current task.sql causes the automated tests to fail: the tests detect that Orders is being modified inside an explicit transaction block, which is not allowed for this task. Inside task.sql, you have an UPDATE Orders ... (and possibly other Orders modifications) between START TRANSACTION/BEGIN and COMMIT/ROLLBACK; this violates the requirement that Orders must be updated, but only outside the explicit transaction. To fix this, move all UPDATE Orders ... statements so they appear after the COMMIT (or before the START TRANSACTION), ensuring there are no Orders modifications between the transaction delimiters, for example:
START TRANSACTION;
-- your other DML here, but not touching Orders
COMMIT;
-- now safely outside the transaction
UPDATE Orders
SET ...
WHERE ...;The rest of your logic around creating orders and order items looks conceptually sound, and you are correctly using a transaction for related operations, so once Orders updates are moved out, the core functional requirement and tests should be satisfied. As a non-blocking improvement, consider making the stock decrement conditional (e.g. UPDATE Products SET WarehouseAmount = WarehouseAmount - 1 WHERE ProductID = 1 AND WarehouseAmount >= 1;) and checking the affected row count to avoid negative inventory in edge cases. Overall, your approach is close; adjusting where Orders is updated and optionally tightening the stock handling will bring it fully in line with the task’s expectations.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| -- Update stock | ||
| UPDATE Products | ||
| SET WarehouseAmount = WarehouseAmount - 1 |
There was a problem hiding this comment.
Before decrementing stock you should ensure there is sufficient WarehouseAmount to avoid negative inventory. Consider making the UPDATE conditional (e.g. add AND WarehouseAmount >= 1 in the WHERE clause) and check the affected rows; if no rows were updated, ROLLBACK the transaction. For stronger concurrency safety, you can SELECT the product row FOR UPDATE, validate the amount, then UPDATE.
b0408e2 to
50fdfd7
Compare
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved and it correctly creates the order outside a transaction, then inserts the order item and updates stock inside a transaction, which matches the intended transaction boundaries. This is an educational approval override so you can keep progressing while being aware of a few small improvement opportunities. There are no critical issues that must be fixed for the task or tests, as the main requirements around order creation, order item insertion, and stock decrement are all satisfied. On the line with UPDATE Products SET WarehouseAmount = WarehouseAmount - 1 WHERE ID = 1; near the end of task.sql, the stock is decremented unconditionally, which could allow WarehouseAmount to become negative if the product is out of stock; as a future improvement, consider adding a condition like WHERE WarehouseAmount >= 1 and checking that a row was actually updated. Also, your INSERT into OrderItems assumes there is a Count column in that table, so double-check that your database creation script defines this column with the expected name and type to avoid runtime errors. The use of LAST_INSERT_ID() to capture the new order ID and the clear separation between Orders modifications (outside the transaction) and OrderItems/Products modifications (inside the transaction) are both well done and show good understanding of transaction boundaries. Overall, this is solid work that meets the project goals, and refining the inventory check and schema alignment will help you build even more robust database logic going forward.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.