[API] Statistics

[Rafikooo]

Q	A
Branch?	1.13
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
License	MIT

This data response format is designed to facilitate chart generation. The following example is specifically for the annual sales summary with monthly intervals; other configurations will be added in future PRs.

This data format is intended for the Chart.js library, but it should also be easy to integrate with Google Charts.
It has built-in support for handling multiple datasets.

{
	"salesChart": {
		"labels": [
			"2023-01-01",
			"2023-02-01",
			"2023-03-01",
			"2023-04-01",
			"2023-05-01",
			"2023-06-01",
			"2023-07-01",
			"2023-08-01",
			"2023-09-01",
			"2023-10-01",
			"2023-11-01",
			"2023-12-01"
		],
		"datasets": {
			"sales": [
				97043,
				17740,
				125045,
				54611,
				85021,
				78858,
				104452,
				45415,
				50234,
				29463,
				0,
				0
			] // If necessary, additional datasets could be included
		}
	},
	"businessActivitySummary": {
		"totalSales": 687882,
		"newOrdersCount": 14,
		"newCustomersCount": 21,
		"averageOrderValue": 49134
	},
	"intervalType": "month"
}

[github-actions]

Bunnyshell Preview Environment deleted

Available commands:

• /bns:deploy to redeploy the environment

[NoResponseMate]

Is this date intentional or will we get failing tests when the month changes?
Also would be nice to have at least one order in any other month.

[Rafikooo]

Honestly, I'm not sure. My plan was to test these various scenarios in a separate PR and add the possibility for the client to set the period and interval.

[jakubtobiasz]

Before the switch statement:

$interval = $salesPeriod->getInterval();

Suggested change

	case 'year':
	$queryBuilder
	->addSelect('YEAR(o.checkoutCompletedAt) as year')
	->groupBy('year')
	->andWhere('YEAR(o.checkoutCompletedAt) >= :startYear AND YEAR(o.checkoutCompletedAt) <= :endYear')
	->setParameter('startYear', $salesPeriod->getStartDate()->format('Y'))
	->setParameter('endYear', $salesPeriod->getEndDate()->format('Y'))
	;
	$dateFormatter = static function (\DateTimeInterface $date): string {
	return $date->format('Y');
	};
	$resultFormatter = static function (array $data): string {
	return (string) $data['year'];
	};
	break;
	case 'year':
	$this->modifyQueryBuilderForHandlingYear($queryBuilder)
	break;

And after the switch statement

$dateFormatter = $this->getDateFormatterFor($interval);
$resultFormatter = $this->getResultFormatterFor($interval);

It'd improve readability IMO.

[jakubtobiasz]

modifyQueryBuilderForHandlingYear can be replaced by any other name. Just an example, nothing better came to my mind.

[diimpp]

My biggest concerns are:

1. Feature named SalesStatistics instead of more general purpose Statistics:
   • while already contains "customer created within period", which could have just registered.
   • which limits/confuses re-usability for end-users.
2. Ages-olds issue with orders count, total sales, average order value only include paid orders and doesn't include refunded, partial refunded.
3. Placement of the new sales statistics in the core, which immediatly makes it part of BC promise, while I've a feeling it should be polished more. For example it can be placed to ApiBundle and moved to Core at 2.0. (Plus current DashboardStatistics needs to be ported to this new usage as well)
4. Extensibility needs to be checked, e.g. how many classes I will need to extend and override to add refunds total, coupon usage for period, new products created for the period or products sold for the period.

"Request changes" is due comments.

[diimpp]

1. This and previous scenario have the same name. Could they be renamed?

Scenario: Changing channel in administration dashboard
Scenario: Changing channel in administration dashboard

2. Cases look identical, only different is base channel and selected channel. Are both of them needed?

[Rafikooo]

Even though these scenarios may seem similar, I found them valuable because one consists only of fulfilled orders and the other is a mix of fulfilled and placed orders, which affects the final summary. I modified the scenario names to accurately reflect that fact

[diimpp]

Total sales and average order value depend on selected period, should it be indicated somehow in the cases?

[diimpp]

I assume it checks order summary

and not last customers/orders widget

then phrasing can be adjusted to

Suggested change

	Then I should see 4 new orders
	And I should see 8 new customers
	Then I should see 4 paid orders
	And I should see 8 customers

P.S. should it indicate period as well? (Year/Month/2 weeks)

[diimpp]

With no connection to others source codes and only focusing on ValueObjects design,

• SalesInPeriod should be renamed to SalesPoint/SalesRecord/SalesRow, but ultimately I would suggest to go from sales VO to some basic DataTable for charts render. (Not some specific library, but a general data structure idea), which will allow to re-use this for more complex charts for end-users.
  • https://developers.google.com/chart/interactive/docs/reference#DataTable
  • https://github.com/kevinkhill/lavacharts/tree/3.1/src/DataTables
  • https://github.com/tmilos/google-charts-php
  • https://www.chartjs.org/docs/latest/general/data-structures.html
• SalesPeriod should be renamed to Period.
• SalesSummary should contain Period
• SalesStatistics should be renamed to Statistics and would either not needed anymore after those changes, or contain SalesSummary,SalesDataTable,OtherEndUserSummary and possibly syncing/enforcing same Period of all child VOs.
• ValueObject as part of the namespace is usually not used in sylius/sylius "components" and it can be just ApiBundle\Statistics\Period

[Rafikooo]

@diimpp, thank you for the valuable review. I'll strive to implement most of your suggestions. However, I want to clarify that this task isn't among our top priorities at the moment. We have agreed to focus on enabling retrieval within a yearly period, and closer to the release of 'Sylius 2.0', we will incorporate the requested period and interval management. Afterwards, we will deprecate the current code utilized by the UI and change the JS logic to use the new one

[jakubtobiasz]

/bns:start

[GSadee]

Setting this status is not needed as ChannelNotFoundException extends NotFoundHttpException

[GSadee]

Suggested change

	Assert::inArray($field, $dataKeys, "The data must contain the $field key.");
	Assert::inArray($field, $dataKeys, sprintf('The data must contain the %s key.', $field));

[GSadee]

Thanks, Rafał! 🎉