Easy-digital-downloads: 3.0 - Migration: Tax rates on orders are rounded incorrectly, empty tax rates being migrated into order adjustments.

[cklosowski]

A few things to fix for migrating tax rates:
1) While the tax rates themselves are migrated correctly, the order adjustment tax rates are being rounded incorrectly. I migrated a tax rate of 6.8% but on the order adjustment it was 7%.
2) When no tax rate applies to an order, we still have a 0% tax rate added into the order adjustments. I would state that unless a tax rate is applied to an order, we shouldn't be migrating or inserting one.
3) @mintplugins suspects that the 0 rates are always applying even though a tax rate for an order was specified as it's looking at the use_taxes setting. Need to investigate this further.

[cklosowski]

Found the reason issue 1 happens:
The order adjustments table has a sanitization callback set which is edd_sanitize_amount which, then runs the following before returning:

    /**
     * Filter number of decimals to use for prices
     *
     * @since unknown
     *
     * @param int $number Number of decimals
     * @param int|string $amount Price
     */
    $decimals = apply_filters( 'edd_sanitize_amount_decimals', 2, $amount );
    $amount   = number_format( (float) $amount, $decimals, '.', '' );

The problem here is that a value of 0.068 for 6.8% is rounded to 0.07 since we're rounding based off the default of 2 decimal places.

[cklosowski]

@JJJ While i'm sure the sanitize amount was in place for things like discounts codes and fees for the adjustments, so they _should_ follow the sanitize amount path....tax rates pose a different problem. I wonder if there is a way we can bypass that for tax rates only.

[cklosowski]

Right now the only way I see around this is by doing something in the Base Query class while we're doing validate_item around this:

            $callback = $this->get_column_field( array( 'name' => $key ), 'validate' );

            // Attempt to validate
            if ( ! empty( $callback ) && is_callable( $callback ) ) {
                $validated = call_user_func( $callback, $value );

This is where we take the sanitization callback in validate, and with that switching it off in the event that we're in a tax_rate entry. In which case we'd want to have that live within the order_adjustments method i'm guessing.

[JJJ]

Are the values correct in the database, and it’s only the math that is wrong (because of rounding?) Or are the values incorrect in the database, causing the math to be wrong?

[cklosowski]

@jjj the values are wrong in the Database.
In the order adjustments table, the tax rate row has an amount of 0.070000000, instead of 0.068000000

[cklosowski]

So at this point I have a few things we could do, but I think we need to support a type set for the validation callbacks if possible.

A thought I had was:

        // amount
        array(
            'name'       => 'amount',
            'type'       => 'decimal',
            'length'     => '18,9',
            'default'    => '0',
            'validate'   => array( 'tax_rate' => '<suitable function>`, 'default' => 'edd_sanitize_amount' ),
            'searchable' => true
        ),

then we could use the 'type' column to determine what validation function to use...

I'm not in love with it but since we're storing both dollar values and percentage rates here, we should find a way to support both.

We could then, in the base query class validate_item method, determine if we have a multi-type support for the column.

[JJJ]

Let me think on that approach a bit. I can see what you’re working towards.

I think it’s also just a bug that needs fixing. We probably shouldn’t be rounding ANY values getting saved into the database the way we are, but instead simply confirming that they are sanitized/formatted correctly. Anytime the application needs to enforce some kind of rounding, I don’t think that should be up to the database schema to enforce.

[cklosowski]

Thanks @JJJ let me know what you think when you have a second to mull it over.

[cklosowski]

Been thinking on this quite a bit, I think @jjj is right that we shouldn't be sanitizing this on the way into the DB, that sanitization should happen before it hits the DB.

We'll just have to verify the data prior to being sent to the DB class.

[cklosowski]

So talked with @mintplugins today and I think we've settled that we'll build a function that simply returns an 18,9 compliant value, and uses that as the column sanitization and therefore still requires it be in a format, but doesn't restrict it's type to greedily.

[cklosowski]

If we simply have a function that does something like:

number_format( $value, 9, '.', '') this will ensure that the value is dropped into the DB in the proper format with 0 padding until the 9th decimal. Keep in mind this is a raw DB value for decimal, not locale specific, that is done on the way out of the DB, so the . as a decimal separator is fine to sanitize on the way into the DB, no matter what thousands and decimal separator is used in the EDD Settings.

[JJJ]

I started a fix for this late last night in BerlinDB proper. Should have a PR today.

[cklosowski]

@JJJ awesome thank you, let me know when you push something up and I'll take a look at it.

[JJJ]

@cklosowski See #7380 above. 🤞