Make IP address optional when reporting transactions #175
Conversation
ugexe
force-pushed
the
nlogan/no-ip-report
branch
3 times, most recently
from
bce855a to
033e93c
Compare
ugexe
force-pushed
the
nlogan/no-ip-report
branch
from
033e93c to
a0f49df
Compare
CHANGELOG.md
Outdated
| parameters must be supplied: `ip_address`, `maxmind_id`, `minfraud_id`, | ||
| `transaction_id`. |
There was a problem hiding this comment.
| parameters must be supplied: `ip_address`, `maxmind_id`, `minfraud_id`, | |
| `transaction_id`. | |
| parameters must be supplied: `ipAddress`, `maxmindId`, `minfraudId`, | |
| `transactionId`. |
README.md
Outdated
| called with invalid input data or when the required fields are missing. The | ||
| required fields are `tag` and one or more of the following: `ip_address`, | ||
| `maxmind_id`, `minfraud_id`, or `transaction_id`. |
There was a problem hiding this comment.
| called with invalid input data or when the required fields are missing. The | |
| required fields are `tag` and one or more of the following: `ip_address`, | |
| `maxmind_id`, `minfraud_id`, or `transaction_id`. | |
| called with invalid input data or when the required arguments are null. The | |
| required arguments are `tag` and one or more of the following: `ipAddress`, | |
| `maxmindId`, `minfraudId`, or `transactionId`. |
This one is my fault. I should have updated this when I switched to named arguments as the preferred method.
| * order. This should be passed as a string like | ||
| * "44.55.66.77" or "2001:db8::2:1". | ||
| * "44.55.66.77" or "2001:db8::2:1". This field is not |
There was a problem hiding this comment.
Also on me, but these "field" uses probably should all be "argument". It might be better for us to just do that in a separate PR though as I suspect it is endemic.
src/MinFraud/ReportTransaction.php
Outdated
| * minFraud. This field is not required if you provide at | ||
| * least one of the transaction's ipAddress, maxmindId, or | ||
| * minfraudId. You are encouraged to provide it or the | ||
| * transaction's maxmind_id or minfraud_id. |
There was a problem hiding this comment.
| * transaction's maxmind_id or minfraud_id. | |
| * transaction's maxmindId or minfraudId. |
I suppose we could put backticks around all of these, but that is preexisting.
src/MinFraud/ReportTransaction.php
Outdated
| * the request was made to one of these services. | ||
| * not required if you provide at least one of the transaction's | ||
| * ipAddress, maxmindId, or transactionId. You are encouraged to | ||
| * provide it the request was made to one of these services. |
There was a problem hiding this comment.
| * provide it the request was made to one of these services. | |
| * provide it if the request was made to one of these services. |
src/MinFraud/ReportTransaction.php
Outdated
| @@ -116,18 +121,27 @@ public function report( | |||
| $this->verifyEmpty($values); | |||
| } | |||
|
|
|||
| if ($ipAddress === null | |||
| && $minfraudId === null | |||
There was a problem hiding this comment.
Given that we are not validating that this is a valid UUID, maybe w should at least check that it is not the mepty string.
There was a problem hiding this comment.
$minfraudId is validated as a UUID a little later in the validation process
minfraud-api-php/src/MinFraud/ReportTransaction.php
Lines 153 to 162 in a0f49df
That being said, $maxmindId is also validated to ensure it is 8 characters so I don't think we need the check for $maxmindId === ''. It might also make sense to put this "one of these fields" check after the aforementioned individual field validation has occurred.
There was a problem hiding this comment.
Actually we might still need to check for the empty string, since the validation calls to maybeThrowInvalidInputException won't throw an exception if validateInput is false.
In most situations the individual field validation ensures these fields aren't empty strings. However when validation is disabled it is possible for empty strings to reach this conditional.
No description provided.