Power-Components / livewire-powergrid

⚡ PowerGrid generates modern, powerful and easy-to-customize data tables using Laravel Livewire.
https://livewire-powergrid.com
MIT License
1.41k stars 205 forks source link

Fix: cast value to a decimal #1520

Closed wandesnet closed 2 months ago

wandesnet commented 2 months ago

⚡ PowerGrid - Pull Request

Welcome and thank you for your interest in contributing to our project!. You must use this template to submit a Pull Request or it will not be accepted.


Motivation

Description

This Pull Request adds...

Related Issue(s): #_____.

Documentation

This PR requires Documentation update?

This Pr fixes a bug, let's understand the problem. Let's say we have an Orders table with the amount field of decimal type in which NULL VALUE is allowed.

In the Order model, define the cast, e.g.:

 protected $casts = [
        'amount' => 'decimal:2',
    ];

Component PowerGrid


 public function datasource(): Builder
    {
        return Order::query();
    }

    public function fields(): PowerGridFields
    {
        return PowerGrid::fields()
            ->add('amount')
            ->add('amount_formatted', fn (Order $model) => formatCurrency($model->amount));

    }

When I access the page, a cast error appears.

image

Reason for the error, this transforms all values into strings, when the amount field is null in the table, the code below transforms the value of the amount field into an empty string, ""

     e(strval(data_get($model, $fieldName))

this ends up resulting in an error because it is trying to convert an empty string to decimal

Illuminate\Database\Eloquent\Concerns\HasAttributes.php

 protected function asDecimal($value, $decimals)
    {

        try {
            return (string) BigDecimal::of($value)->toScale($decimals, RoundingMode::HALF_UP);
        } catch (BrickMathException $e) {
            throw new MathException('Unable to cast value to a decimal.', previous: $e);
        }
    }

There are three ways to solve the problem.

  1. accept this PR.
  2. send a PR to Laravel, inserting a check, that is, checking whether the $value is a number in the asDecimal() method.
  3. add a callable to the field, forcing it to return the original value from the table, e.g.:
 public function fields(): PowerGridFields
    {
        return PowerGrid::fields()
            ->add('amount', fn (Order $model) => $model->amount)
            ->add('amount_formatted', fn (Order $model) => formatCurrency($model->amount));

    }
luanfreitasdev commented 2 months ago

Hello @wandesnet, thank you for submitting this PR!

Can you add any tests?

dansysanalyst commented 2 months ago

In this specific case, shouldn't the user be checking for null in the custom column?

    public function fields(): PowerGridFields
    {
        return PowerGrid::fields()
            ->add('amount')
            ->add('amount_formatted', fn (Order $model) => is_null($model->amount) ? 0 : formatCurrency($model->amount));
    }

In my experience, when dealing with currency amounts, humans don't say "your bill is null dollars". Displaying "$0.00" is often the same as displaying " ".

Regarding the PR, I am not sure if we should remove the e() helper here.

Alternatively, could we just provide an empty string as the default value in data_get?

this->fields[$fieldName] = $closure ?? fn ($model) => e(strval(data_get($model, $fieldName, '')));
wandesnet commented 2 months ago

In this specific case, shouldn't the user be checking for null in the custom column?

    public function fields(): PowerGridFields
    {
        return PowerGrid::fields()
            ->add('amount')
            ->add('amount_formatted', fn (Order $model) => is_null($model->amount) ? 0 : formatCurrency($model->amount));
    }

In this specific case, the error occurs regardless of whether it is a custom column.

In my experience, when dealing with currency amounts, humans don't say "your bill is null dollars". Displaying "$0.00" is often the same as displaying " ".

Regarding the PR, I am not sure if we should remove the e() helper here.

In my opinion, you should not change the values to string, that is, you should always keep the original value/type saved in the table.

Alternatively, could we just provide an empty string as the default value in data_get?

this->fields[$fieldName] = $closure ?? fn ($model) => e(strval(data_get($model, $fieldName, '')));

This way, the error will persist because it continues to return an empty string if the amount field is null, why not keep the original value/type saved in the table? Is there a reason for this?

dansysanalyst commented 2 months ago

I see the point, as it is a cast.

For me, it is more about not removing the e() helper. However, tests must be written here.

So perhaps something like:

if is_callable($closure) {
  $this->fields[$fieldName] = $closure;
   return;
}

$data =  fn ($model) => data_get($model, $fieldName);

if (is_string($data) {
     $data = e($data);
}

$this->fields[$fieldName] = $data

(writing code on the go, from my head and untested)

wandesnet commented 2 months ago

Got it, we can keep e() when it's a string

dansysanalyst commented 2 months ago

👍 L.G.T.M

Thank you very much @wandesnet