Skip to content

Add transaction for creating an order #198

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

Open
TrickyTenshi wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add transaction for creating an order #198

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c6dc76
Add transaction for creating an order
TrickyTenshi Jun 30, 2026
31ba4d0
fix: Moved order creation out of the transaction
TrickyTenshi Jun 30, 2026
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
13 changes: 4 additions & 9 deletions task.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
-- Use our database
USE ShopDB;

-- Some data should be created outside the transaction (here)

-- Start the transaction
USE ShopDB;
INSERT INTO Orders (CustomerID, Date) VALUES (1, '2023-01-01');

@mateacademy-ai-mentor mateacademy-ai-mentor Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The INSERT INTO Orders statement is executed before START TRANSACTION, so creating the order is not part of the atomic transaction with the order item insertion and warehouse update. According to the business rules and checklist item #17, consider moving this insert inside the transaction so that order creation, OrderItems, and product stock updates either all succeed or all roll back together.

@TrickyTenshi TrickyTenshi Jul 1, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There's a conflict here, and I'd like to clarify the correct approach.
I originally placed INSERT INTO Orders outside the transaction on purpose, because the automated test in test.sh (GitHub Actions) requires it. There's an explicit check:

insertIntoOrders=$(echo $afterTransactionStart | grep "INSERT INTO Orders" || true)
if [ -z "$insertIntoOrders" ]; then
echo "checked if Orders are updated outside of the transaction - ok"
else
echo "error: Orders are updated inside of the transaction"
exit 1
fi

This test looks for INSERT INTO Orders after START TRANSACTION and fails with exit 1 if it finds it. It requires order creation to be outside the transaction.
Your review asks for the opposite — to move INSERT INTO Orders inside the transaction. If I do that, the CI test will fail.

START TRANSACTION;

-- And some data should be created inside the transaction

INSERT INTO OrderItems (ProductID, Count, OrderID) VALUES (1, 1, LAST_INSERT_ID());

@mateacademy-ai-mentor mateacademy-ai-mentor Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LAST_INSERT_ID() here depends on the previous insert into Orders, which currently happens outside the transaction. After moving the Orders insert into the transaction, this will correctly link the OrderItems row to the newly created order as part of the same atomic operation.

UPDATE Products SET WarehouseAmount = WarehouseAmount - 1 WHERE ID = 1;
COMMIT;
Loading