Skip to content

Conversation

andreparames
Copy link

@andreparames andreparames commented Oct 12, 2025

Sign of transaction amount needs to be flipped for Cetelem Black credit cards.

Copy link

netlify bot commented Oct 12, 2025

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 3b649e4
🔍 Latest deploy log https://app.netlify.com/projects/actualbudget/deploys/68ee1fc5e1d5a300082ffd34
😎 Deploy Preview https://deploy-preview-5914.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

The amount coming from GoCardless has the opposite sign of what we expect.
@andreparames andreparames marked this pull request as ready for review October 12, 2025 18:31
Copy link
Contributor

coderabbitai bot commented Oct 12, 2025

Walkthrough

Adds a new bank integration module at packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js. The module spreads the existing Fallback bank, registers institutionIds with ['CETELEM_CETMPTP1XXX'], and overrides normalizeTransaction to create a shallow copy editedTrans, negate transaction.transactionAmount.amount (numeric negation, converted to string, currency preserved), then calls and returns Fallback.normalizeTransaction with editedTrans as the referenced transaction.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • matt-fidd

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly captures the primary change by indicating a fix for Cetelem bank transactions, aligning with the pull request’s goal of flipping transaction amount signs for Cetelem accounts.
Description Check ✅ Passed The description clearly states that the sign of the transaction amount needs to be flipped for Cetelem Black credit cards, which directly matches the implemented code change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c554a5 and 3b649e4.

📒 Files selected for processing (1)
  • packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: build (ubuntu-22.04)
  • GitHub Check: build (macos-latest)
  • GitHub Check: build (windows-latest)
  • GitHub Check: test
  • GitHub Check: Functional Desktop App
  • GitHub Check: Wait for Netlify build to finish

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3715f16 and 2c554a5.

⛔ Files ignored due to path filters (1)
  • upcoming-release-notes/5914.md is excluded by !**/*.md
📒 Files selected for processing (1)
  • packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)

Use TypeScript for all code

Files:

  • packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js
🔇 Additional comments (1)
packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js (1)

12-22: No changes needed—sign flip and mutation follow existing patterns Using parseFloat for a simple unary negation of fixed two-decimal strings won’t incur rounding issues here, and in-place mutation of transaction is how all other bank integrations implement normalizeTransaction.

Likely an incorrect or invalid review comment.

Copy link
Contributor

coderabbitai bot commented Oct 12, 2025

Walkthrough

Adds a new GoCardless bank adapter module for Cetelem (CETELEM_CETMPTP1XXX). The adapter extends the existing Fallback implementation, sets institutionIds to ["CETELEM_CETMPTP1XXX"], and overrides normalizeTransaction. In normalizeTransaction, it creates a shallow copy of the input transaction, flips the sign of transaction.transactionAmount.amount (preserving currency), and delegates normalization to Fallback.normalizeTransaction(transaction, booked, editedTrans). The module exports the composed IBank-implementing object as the default export.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • matt-fidd

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly summarizes the main purpose of the pull request by indicating a fix to Cetelem bank transactions and uses the “enhancement” prefix to denote the type of change, making it directly related to the changeset. Despite its clarity, the word “transations” is misspelled, which could hinder readability. Overall, the title captures the primary focus of the update without extraneous detail.
Description Check ✅ Passed The description directly states that the sign of the transaction amount needs to be flipped for Cetelem Black credit cards, which aligns precisely with the implemented change and offers clear, relevant context.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3715f16 and 2c554a5.

⛔ Files ignored due to path filters (1)
  • upcoming-release-notes/5914.md is excluded by !**/*.md
📒 Files selected for processing (1)
  • packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)

Use TypeScript for all code

Files:

  • packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js
🔇 Additional comments (1)
packages/sync-server/src/app-gocardless/banks/cetelem_cetmptp1xxx.js (1)

12-22: No changes needed: mutating core fields on transaction and using an unmodified copy as editedTrans aligns with Fallback.normalizeTransaction(transaction, booked, editedTrans) signature and existing adapters.

Copy link
Member

@matt-fidd matt-fidd left a comment

Choose a reason for hiding this comment

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

Could you change this to spread the old object in? it's a little more future proof

Change based on code review suggestion

Co-authored-by: Matt Fiddaman <[email protected]>
@andreparames
Copy link
Author

Could you change this to spread the old object in? it's a little more future proof

Absolutely! And thanks for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants