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

Avs validation #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

senthilkumar-muthusamy
Copy link

  1. Added admin settings for AVS CVN decline conditions
  2. Updated the front end code to perform reversal based on AVS and CVN result codes

if (property_exists($response, 'cvvResultCode')) {
$details['cvv_response_code'] = $response->cvvResultCode;
if (property_exists($response, 'cavvResponseCode')) {
$details['cvv_response_code'] = $response->cavvResponseCode;
Copy link
Contributor

Choose a reason for hiding this comment

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

cvv and cavv represent two separate ideas. We need cvv in this case.

Choose a reason for hiding this comment

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

updated the response code and message

'label' => 'AVS and CVV2 failed'
),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be using the individual AVS result codes instead of the gateway's general response code.

Choose a reason for hiding this comment

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

@slogsdon from the available transit documents which you already shared, I found only these 2 result codes related to AVS/CVV, is there any other document available for the result codes?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're performing the comparison against the $response->avsResponseCode and $response->cvnResponseCode response properties, which is correct, but the codes in these properties align with the standard AVS/CVV codes (i.e. what I listed in the requirements doc and what you're using in other PRs, WooCommerce as an example)

Choose a reason for hiding this comment

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

ok.. updated the result codes

'label' => 'AVS and CVV2 failed'
)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be using the individual CVV result codes instead of the gateway's general response code.

Choose a reason for hiding this comment

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

ok.. updated the result codes


if ($response->responseCode !== '00' || $response->responseMessage === 'Partially Approved') {
if ($response->responseCode !== '00') {

Choose a reason for hiding this comment

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

I think that the code inside the if statement will never be run because response code different than '00' throw an exception. An exception is thrown in checkResponse function (https://github.com/hps/php-sdk/blob/962f6826c5938ec9b7c13dbad911970c019057da/src/Gateways/TransITConnector.php#L573).

Choose a reason for hiding this comment

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

@slogsdon shall we update the PHP SDK to allow the AVS failure code and handle this in plugin side

<avs_decline_codes translate="label">
<label>AVS Decline Condition</label>
<frontend_type>multiselect</frontend_type>
<source_model>hps_transit/source_avsresultcodes</source_model>

Choose a reason for hiding this comment

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

Should be:
<source_model>hps_transit/source_avsResultCodes</source_model>

Choose a reason for hiding this comment

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

updated the model name

<cvv_decline_codes translate="label">
<label>CVV Decline Condition</label>
<frontend_type>multiselect</frontend_type>
<source_model>hps_transit/source_cvvresultcodes</source_model>

Choose a reason for hiding this comment

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

Should be:
<source_model>hps_transit/source_cvvResultCodes</source_model>

Choose a reason for hiding this comment

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

updated the model name

Comment on lines 176 to 177
$declinedAvsCodes = Mage::getStoreConfig('payment/hps_transit/avs_decline_codes');
$declinedCvnCodes = Mage::getStoreConfig('payment/hps_transit/cvv_decline_codes');

Choose a reason for hiding this comment

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

Should be:

$declinedAvsCodes = explode(',', Mage::getStoreConfig('payment/hps_transit/avs_decline_codes'));
$declinedCvnCodes = explode(',', Mage::getStoreConfig('payment/hps_transit/cvv_decline_codes'));

Choose a reason for hiding this comment

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

thanks.. I updated the code with explode function

Copy link

@przemyslaw-p przemyslaw-p left a comment

Choose a reason for hiding this comment

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

The getStoreConfig will return a comma separated string value.

'label' => 'AVS and CVV2 failed'
),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You're performing the comparison against the $response->avsResponseCode and $response->cvnResponseCode response properties, which is correct, but the codes in these properties align with the standard AVS/CVV codes (i.e. what I listed in the requirements doc and what you're using in other PRs, WooCommerce as an example)

@scarman01 scarman01 requested review from MSmedal and removed request for slogsdon April 6, 2023 12:06
Copy link

@MSmedal MSmedal left a comment

Choose a reason for hiding this comment

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

Has this extension been tested? The checkout page seems to get stuck on the shipping step; the payment step cannot be reached

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants