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

Minimal instalment value setting #477

Merged
merged 20 commits into from Apr 25, 2017
Merged

Conversation

gpressutto5
Copy link
Contributor

Closes #470

@cezarsmpio
Copy link
Contributor

@gpressutto5 Please, don't forget talk to Larissa about the tooltip description ;)

Copy link
Contributor

@cezarsmpio cezarsmpio left a comment

Choose a reason for hiding this comment

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

All texts must be send to Larissa's review.

@@ -326,15 +326,22 @@ public function process_payment($order_id)
public function fetch_acquirer_max_installments_for_price($price, $country = null) {
$max_instalments = WC_EBANX_Constants::MAX_INSTALMENTS;
$country = $country ?: WC()->customer->get_country();
$currency_code = get_woocommerce_currency();
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a property on gateway root $this->merchant_currency = strtoupper(get_woocommerce_currency());. Please, use it rather than that.

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 didn't want to use strtolower on a strtoupper. I need it to be lowercase. Do you think that's ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so use the strtolower function just to ensure that code will work.

),
)
);
$currency_code = get_woocommerce_currency();
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the $this->merchant_currency = strtoupper(get_woocommerce_currency());.

'interest_rates_enabled' => 'no',
'min_instalment_value_brl' => '20',
'min_instalment_value_usd' => '20',
'min_instalment_value_eur' => '20',
Copy link
Contributor

Choose a reason for hiding this comment

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

20 dollars and 20 euros are not restricted.
Those could default to 0 as far as we know.

@@ -286,7 +286,7 @@ public function init_form_fields() {
'step' => '0.01'
),
'desc_tip' => true,
'description' => __('The minimum instalment value supported by EBANX is $20 BRL and $100 MXN. If you put a value lower than these the value used will be the EBANX\'s ones', 'woocommerce-gateway-ebanx')
'description' => __('The minimum instalment value supported by EBANX is $20 BRL and $100 MXN. If you insert a lower value the EBANX pattern ones will be used.', 'woocommerce-gateway-ebanx')
Copy link
Contributor

Choose a reason for hiding this comment

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

Pattern = padrão de repetição; Standard = que vem de série; Default = de facto, comum, já esperado.

PS: Falar com a Lari

@cezarsmpio
Copy link
Contributor

cezarsmpio commented Apr 19, 2017

@gpressutto5 We need to do some few changes:

Texts

  • The values on tooltip must be BRL 20 and MXN 100, not $20 BRL and $100 MXN.
  • Change the tooltip text to imperative format. Please, talk to Larissa to help you ;)
  • Try to change the label to be smaller.
  • The label is wrong, is istalment and the correct is instalment.
  • Some labels are installments with double L, the right is instalment.

Code

  • We need to consider the interest rate too, not value only.

'type' => 'number',
'class' => 'ebanx-payments-option',
'placeholder' => sprintf(__('The default is %d', 'woocommerce-gateway-ebanx'),
$currency_code === WC_EBANX_Constants::CURRENCY_CODE_MXN ? WC_EBANX_Constants::ACQUIRER_MIN_INSTALMENT_VALUE_MXN : WC_EBANX_Constants::ACQUIRER_MIN_INSTALMENT_VALUE_BRL),
Copy link
Contributor

Choose a reason for hiding this comment

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

When currency is COP it will display 20 nonetheless and that's not what we want, is it?
Abstracting this to a method instead of using ternary will be better for setting the default message on this field in a more dynamic way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the currency is COP it will display nothing.

'placeholder' => sprintf(__('The default is %d', 'woocommerce-gateway-ebanx'),
$currency_code === WC_EBANX_Constants::CURRENCY_CODE_MXN ? WC_EBANX_Constants::ACQUIRER_MIN_INSTALMENT_VALUE_MXN : WC_EBANX_Constants::ACQUIRER_MIN_INSTALMENT_VALUE_BRL),
'custom_attributes' => array(
'min' => $currency_code === WC_EBANX_Constants::CURRENCY_CODE_MXN ? WC_EBANX_Constants::ACQUIRER_MIN_INSTALMENT_VALUE_MXN : ( $currency_code === WC_EBANX_Constants::CURRENCY_CODE_MXN ? WC_EBANX_Constants::ACQUIRER_MIN_INSTALMENT_VALUE_BRL : 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

This ternary is very similar to the other one, abstract away bro

$currency_code = strtolower($this->merchant_currency);
}
if ( ! in_array(strtoupper($currency_code), WC_EBANX_Constants::$CREDIT_CARD_CURRENCIES) ) {
throw new InvalidArgumentException("The provided country doesn't accept Credit Card payment", 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Error message doesn't match the parameter type


switch ($currency_code) {
case WC_EBANX_Constants::CURRENCY_CODE_BRL:
$min_instalmente_value = WC_EBANX_Constants::ACQUIRER_MIN_INSTALMENT_VALUE_BRL;
Copy link
Contributor

Choose a reason for hiding this comment

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

return directly instead of declaring a var here, would look cleaner

@gpressutto5 gpressutto5 merged commit 9d5ff37 into develop Apr 25, 2017
@gpressutto5 gpressutto5 deleted the feature/set-min-instalment-value branch April 25, 2017 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants