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

Add Transaction Reporting functionality #88

Merged
merged 11 commits into from May 15, 2020
Merged

Conversation

PatrickCronin
Copy link
Contributor

In order to encourage more usage of the minFraud Report Transaction API, this PR adds a method to this client library that simplifies transaction reporting.

@PatrickCronin PatrickCronin force-pushed the pcronin/report-txns branch 5 times, most recently from 4c36b0a to 151c8d1 Compare May 12, 2020 13:59
Copy link
Member

@oschwald oschwald left a comment

Choose a reason for hiding this comment

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

Looks great. I had a few comments. Also, should the README be updated?

src/MinFraud.php Show resolved Hide resolved
src/MinFraud/Validation/Rules/TransactionReport.php Outdated Show resolved Hide resolved
src/ReportTransaction.php Outdated Show resolved Hide resolved
src/ReportTransaction.php Outdated Show resolved Hide resolved
tests/MaxMind/Test/ReportTransactionTest.php Outdated Show resolved Hide resolved
tests/MaxMind/Test/ReportTransactionTest.php Outdated Show resolved Hide resolved
Copy link
Member

@oschwald oschwald left a comment

Choose a reason for hiding this comment

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

Almost there.

README.md Outdated
@@ -77,7 +77,13 @@ Page](https://maxmind.github.io/minfraud-api-php/) under the "API" tab.

## Usage ##

To use this API, create a new `\MaxMind\MinFraud` object. The constructor
This library provides access to both the [minFraud2 (Score, Insights and
Copy link
Member

Choose a reason for hiding this comment

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

"minFraud2" is an internal name. We should just use "minFraud" in public documentation. The same applies below.

CHANGELOG.md Show resolved Hide resolved
README.md Outdated
MaxMind encourages the use of this API as data received through this channel is
used to continually improve the accuracy of our fraud detection algorithms.

To use the Report Transactions API, create a new `\MaxMind\ReportTransaction`
Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice this before, but I wonder if \MaxMind\MinFraud\ReportTransaction would be a better name. It is a minFraud API, just not one of our scoring APIs.

Similarly, \MaxMind\ServiceClients could lead to accidental name collisions with our other client APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made this change.

README.md Show resolved Hide resolved
* @dataProvider services
*
* @param mixed $class
* @param mixed $service
*/
public function testMissingIpAddress($class, $service)
{
$this->expectException(InvalidInputException::class);
$this->expectExceptionMessage('Must have keys');
Copy link
Member

Choose a reason for hiding this comment

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

Nice. I wasn't saying you had to do these ones as part of the story, but it is good that we won't have to deal with them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, no worries. It was just a couple regexp finds/replaces.

Copy link
Member

@oschwald oschwald left a comment

Choose a reason for hiding this comment

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

Hopefully last change!

@@ -44,7 +44,7 @@ fi
php composer.phar self-update
php composer.phar update --no-dev

perl -pi -e "s/(?<=const VERSION = ').+?(?=';)/$tag/g" src/MinFraud.php
perl -pi -e "s/(?<=const VERSION = ').+?(?=';)/$tag/g" src/ServiceClient.php
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be updated now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks -- great catch! I've rebased this in with the initial abstraction commit.

@oschwald oschwald merged commit 2f6fc3c into master May 15, 2020
@oschwald oschwald deleted the pcronin/report-txns branch May 15, 2020 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants