Skip to content

Conversation

@Klakurka
Copy link
Member

Related to #765

Description

Added check to make sure we aren't firing off payment triggers with invalid JSON.

Test plan

  1. Createa a new payment trigger. Go into the db and update the JSON manually to be invalid since it can't be done through the UI.
  2. Fire the payment trigger. It should no longer throw an error like it does in prod - still logged though.

@Klakurka Klakurka added this to the Phase 3 milestone Jul 28, 2025
@Klakurka Klakurka requested a review from chedieck July 28, 2025 20:00
@Klakurka Klakurka self-assigned this Jul 28, 2025
@Klakurka Klakurka added bug Something isn't working enhancement (behind the scenes) Stuff that users won't see labels Jul 28, 2025
@Klakurka Klakurka linked an issue Aug 3, 2025 that may be closed by this pull request
@Klakurka Klakurka requested a review from Copilot August 7, 2025 17:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds JSON validation to payment triggers to prevent network requests when the trigger's JSON data is invalid. The change addresses issue #765 by implementing early JSON validation that logs errors without attempting external HTTP calls.

  • Added try-catch wrapper around JSON parsing in postDataForTrigger function
  • Implemented early return with error logging when JSON validation fails
  • Added comprehensive test coverage for the new validation behavior

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
services/triggerService.ts Added JSON validation with early return and error logging before network requests
tests/unittests/triggerService.test.ts Added unit tests demonstrating JSON validation functionality
tests/integration-tests/triggerJsonValidation.test.ts Added integration tests covering various JSON validation scenarios

Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

just a minor thing

@Klakurka Klakurka requested a review from chedieck August 9, 2025 02:08
chedieck
chedieck previously approved these changes Aug 18, 2025
Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

I added a console error, since this situation should never happen unless we manual edit the db, and fixed linting issues (check if you have ./git/hooks/pre-commit in place, if not copy it there from .githooks/).

Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

I fixed some linting issues and some stuff that was erroring due to changes in master.

In any case, I looked further and didn't undestand the reason for the tests included in the tests/integration-tests/triggerJsonValidation.test.ts to be in a separate file from the other trigger tests — I don't understand why they are included in integration-tests/ since, AFAIK, they are not testing any integration.

Copy link
Member Author

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Right, thanks.

I consolidated those tests, cleaned it up a bit.

Have a look at the lint-staged changes I had to make to be able to build - this is an issue for me that I had to fix in another WIP branch.

@Klakurka Klakurka requested a review from chedieck August 18, 2025 16:58
Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

Just a typo mistake.

Have a look at the lint-staged changes I had to make to be able to build - this is an issue for me that I had to fix in another WIP branch.

I don't see it, did you commit it in this branch?

@chedieck chedieck merged commit 4c33fb0 into master Aug 25, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement (behind the scenes) Stuff that users won't see

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't try to fire a payment trigger with invalid JSON

3 participants