Skip to content
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.

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

chore: add logging, outgoing payment service changes #2419

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

mkurapov
Copy link
Contributor

@mkurapov mkurapov commented Feb 16, 2024

Changes proposed in this pull request

Some minor refactoring around outgoing payments: standardising logging, updating functions & adding tests.

Context

Mostly to improve our logging & error handling + checking off a TODO item in the code.

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Postman collection updated

@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Feb 16, 2024
Copy link

netlify bot commented Feb 16, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit fe32cf0
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65cfa69cc1318d000728febb

@mkurapov mkurapov changed the title Mk/outgoing payments cleanup chore: add logging, outgoing payment service changes Feb 16, 2024
return payment
}

function validateSentAmount(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

standardize the error here

@@ -276,18 +278,14 @@ async function validateGrant(
return false
}

// Lock grant
// TODO: update to use objection once it supports forNoKeyUpdate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using objection now

@@ -3,7 +3,12 @@ import { Logger } from 'pino'
import { ReadContext, CreateContext, ListContext } from '../../../app'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just logging & more gracefully handling 500 errors

Comment on lines -167 to -170
{
...deps,
knex: trx
},
Copy link
Contributor Author

@mkurapov mkurapov Feb 16, 2024

Choose a reason for hiding this comment

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

explicitly passing in trx now instead of "mutating" deps.knex

@mkurapov mkurapov marked this pull request as ready for review February 16, 2024 18:23
)
if (totalSent === undefined) {
throw new Error()
Copy link
Contributor Author

@mkurapov mkurapov Feb 16, 2024

Choose a reason for hiding this comment

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

we have a few of these throw new Error(), so trying to get rid of them slowly

Copy link
Member

@sabineschaller sabineschaller left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM!

@mkurapov mkurapov merged commit ba5d0b3 into main Feb 19, 2024
22 checks passed
@mkurapov mkurapov deleted the mk/outgoing-payments-cleanup branch February 19, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants