-
Notifications
You must be signed in to change notification settings - Fork 10
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
Make IP address optional when reporting transactions #1326
Conversation
src/request/transaction-report.ts
Outdated
* The IP address of the customer placing the order. | ||
* The IP address of the customer placing the order. This field is not | ||
* required if you provide at least one of the transaction's minfraud_id, | ||
* maxmind_id, or transaction_id. You are encouraged to provide it, if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the casung should match the names in this interface (which also match the names used elsewhere in this package).
CHANGELOG.md
Outdated
|
||
* Updated `TransactionReport` to make the `ipAddress` parameter optional. Now | ||
the `tag` and at least one of the following paramters must be supplied: | ||
`ipAddress`, `maxmindId`, `minfraudID`, `transactionID`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`ipAddress`, `maxmindId`, `minfraudID`, `transactionID`. | |
`ipAddress`, `maxmindId`, `minfraudId`, `transactionId`. |
@@ -50,7 +54,7 @@ export default class TransactionReport { | |||
/** @inheritDoc TransactionReportProps.chargebackCode */ | |||
public chargebackCode?: string; | |||
/** @inheritDoc TransactionReportProps.ipAddress */ | |||
public ipAddress: string; | |||
public ipAddress?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this seems like a breaking change too. We should probably do a major version bump.
No description provided.