Skip to content

bug: fix createLikes notification logic and avoid mutating updateTransactionById edits #129

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
devin-ai-integration wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

bug: fix createLikes notification logic and avoid mutating updateTransactionById edits #129

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e115bc
bug: fix createLikes condition and avoid mutating updateTransactionBy…
devin-ai-integration[bot] Jun 30, 2026
3d096c4
bug: fix identical createComments notification condition
devin-ai-integration[bot] 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
20 changes: 10 additions & 10 deletions backend/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,11 +581,13 @@ export const updateTransactionById = (transactionId: string, edits: Partial<Tran
const sender = getUserById(senderId);
const receiver = getUserById(receiverId);

const finalEdits = { ...edits };

@devin-ai-integration devin-ai-integration Bot Jun 30, 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.

📝 Info: Shallow copy of edits prevents mutation of caller's object

The change at backend/database.ts:584 creates finalEdits = { ...edits } so that when finalEdits.status is set to TransactionStatus.complete on line 590, the original edits object passed by the caller is not mutated. This matters because updateTransactionById is called from backend/transaction-routes.ts:179 with req.body, and mutating req.body could cause subtle issues if it's referenced again later. The fix is correct and well-scoped.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


// if payment, debit sender's balance for payment amount
if (isRequestTransaction(transaction)) {
debitPayAppBalance(receiver, transaction);
creditPayAppBalance(sender, transaction);
edits.status = TransactionStatus.complete;
finalEdits.status = TransactionStatus.complete;

createPaymentNotification(
transaction.senderId,
Expand All @@ -594,7 +596,7 @@ export const updateTransactionById = (transactionId: string, edits: Partial<Tran
);
}

db.get(TRANSACTION_TABLE).find(transaction).assign(edits).write();
db.get(TRANSACTION_TABLE).find(transaction).assign(finalEdits).write();
};

// Likes
Expand Down Expand Up @@ -624,14 +626,13 @@ export const createLikes = (userId: string, transactionId: string) => {

const like = createLike(userId, transactionId);

/* istanbul ignore next */
if (userId !== senderId || userId !== receiverId) {
if (userId !== senderId && userId !== receiverId) {
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
createLikeNotification(senderId, transactionId, like.id);
createLikeNotification(receiverId, transactionId, like.id);
} else if (userId === senderId) {
createLikeNotification(senderId, transactionId, like.id);
} else {
createLikeNotification(receiverId, transactionId, like.id);
} else {
createLikeNotification(senderId, transactionId, like.id);
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
}
Comment on lines +629 to 636

@devin-ai-integration devin-ai-integration Bot Jun 30, 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.

📝 Info: Original createLikes condition was always true, making branches unreachable

The old condition userId !== senderId || userId !== receiverId at backend/database.ts:628 (LEFT) was a tautology whenever senderId !== receiverId (which is always true since a user cannot transact with themselves). This means the old code always entered the first branch, sending notifications to both parties regardless of who liked the transaction. The fix to && correctly makes the condition false when the liker is either the sender or receiver, allowing the sub-branches to execute. The swapped notification targets in lines 632-635 are also correct: when the liker is the sender, only the receiver should be notified, and vice versa.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +629 to 636

@devin-ai-integration devin-ai-integration Bot Jun 30, 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.

🚩 Condition fix changes notification behavior for all like/comment actions

The old condition userId !== senderId || userId !== receiverId was always true when sender ≠ receiver (which is the normal case for transactions), meaning both sender and receiver were always notified — even when one of them was the person performing the action. The else if / else branches were dead code (confirmed by /* istanbul ignore next */ markers that were removed).

The new && condition means the first branch only fires for third-party users. When the liker/commenter is the sender or receiver, only the OTHER party is now notified. This is a behavioral change: previously both parties always got notifications; now participants don't get self-notifications.

This is almost certainly the intended fix, but it's worth confirming there are no E2E tests or frontend logic that depend on self-notifications being generated (e.g., notification count assertions in Cypress tests at cypress/tests/).

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jun 30, 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.

Confirmed intended — this matches the behavior requested in the task. Checked for dependencies:

  • CI (.github/workflows/main.yml) only runs yarn test:unit:ci (vitest); the Cypress E2E suite isn't run in CI. All 44 unit tests pass, including likes.test.ts, comments.test.ts, and notifications.test.ts.
  • The E2E notification specs (cypress/tests/ui/notifications.spec.ts) assert on the other party's notification counts (third-party liker → both notified; participant liker → other party notified), not on self-notifications, so they're consistent with this change.

Comment on lines +629 to 636

@devin-ai-integration devin-ai-integration Bot Jun 30, 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.

📝 Info: Edge case: sender equals receiver is not guarded

If a transaction somehow has senderId === receiverId, the first branch (userId !== senderId && userId !== receiverId) would be false when the user is that person, falling to else if (userId === senderId) which would call createLikeNotification(receiverId, ...) — effectively a self-notification. This is the same behavior as before the change, so it's not a regression. In practice, createTransaction at backend/database.ts:528 takes separate userId and receiverId, so this edge case shouldn't occur in normal operation.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jun 30, 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.

Agreed — not a regression (same behavior as before for the senderId === receiverId case), and transactions are always created with distinct sender/receiver, so it won't occur in practice. Leaving as-is to keep the change scoped.

};

Expand Down Expand Up @@ -671,14 +672,13 @@ export const createComments = (userId: string, transactionId: string, content: s

const comment = createComment(userId, transactionId, content);

/* istanbul ignore next */
if (userId !== senderId || userId !== receiverId) {
if (userId !== senderId && userId !== receiverId) {
createCommentNotification(senderId, transactionId, comment.id);
createCommentNotification(receiverId, transactionId, comment.id);
} else if (userId === senderId) {
createCommentNotification(senderId, transactionId, comment.id);
} else {
createCommentNotification(receiverId, transactionId, comment.id);
} else {
createCommentNotification(senderId, transactionId, comment.id);
}
};

Expand Down