Flowframe / laravel-trend

Generate trends for your models. Easily generate charts or reports.
MIT License
671 stars 70 forks source link

sum() did not return right value. #14

Closed aldesrahim closed 1 year ago

aldesrahim commented 2 years ago

I've a datetime column named date, I want to get sum of total using perDay and perMonth, but it seems not giving the right value.

Please take a look at this specific line of code: https://github.com/Flowframe/laravel-trend/blob/dd00920b628e6e9655aeb6c59921248ee8ff0937/src/Trend.php#L94

Imo, it should be like this: ->groupByRaw($this->getSqlDate())

because while using perMonth or perDay, it no grouping rows by the formatted column like date_format(date, '%Y-%m') instead it group using column like date.

In this case, rows with same same day or month not summing up the total.

Edit: Here is the query of given example above using perDay method:

select 
date_format(date, '%Y-%m-%d') as date,
sum(total) as aggregate
from `sales` where `date` between '2022-05-23 00:00:00' and '2022-05-30 23:59:59' group by `date` order by `date` asc

if I have 2 records within the same day but different hour, it will return 2 record and not summed up the total.

Info: Laravel 9, MySQL, Apache

I am sorry for my bad English. Hope you all can understand what I am trying to say. Thank you !

haheap commented 2 years ago

I haven’t tested it, but I suspect it’s related to #8 and issues when the table has a field named ‘date’. I’ve submitted a PR awaiting approval: https://github.com/Flowframe/laravel-trend/pull/13

aldesrahim commented 2 years ago

Ah, I have noticed that. Maybe it because I have a column named date so when the query builder trying to group by 'date' it refers to my column named date instead of the alias.

magarrent commented 1 year ago

Hey there! This issue was solved? I think the sum() function is not working:

image

image

image

raymon-roos commented 1 year ago

Hello. I have encountered the same problem.

PR #28 allows us the change the alias used for aggregating. However, when I set the alias as such:

        $trend->dateAlias = 'aggregation_date';
        return $trend->dateColumn('date')
            ->between(
                start: now()->sub($this->filter, 1),
                end: now(),
            )

I get an errorException Undefined property: stdClass::$date

It seems to me like a field might have been overlooked when the dateAlias feature got introduced. The aggregate() method passes its values through the mapValuesToDates() method before returning, which does the following:

        $values = $values->map(fn ($value) => new TrendValue(
            date: $value->date,
            aggregate: $value->aggregate,
        ));

It looks like this method presumes the presence of a date property in the values collection, even though the alias might have changed.

Am I using dateAlias incorrectly, or is this indeed unintended?

This could be fixed - if a fix is in order - like so:

    date: $value->{$this->dateAlias},

I'm sorry if I am completely mistaken or this is not the right place for mentioning it. I hope it is useful in some way.

Thank you for your work.