-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[API] Statistics #15598
[API] Statistics #15598
Conversation
6e0f70a
to
f5d7f65
Compare
Bunnyshell Preview Environment deletedAvailable commands:
|
f5d7f65
to
bdf0ddd
Compare
src/Sylius/Bundle/ApiBundle/Controller/GetSalesStatisticsAction.php
Outdated
Show resolved
Hide resolved
bdf0ddd
to
5aca55d
Compare
5aca55d
to
19a8d1c
Compare
"period": "2023-12-01 00:00:00", | ||
"total": 19500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/Sylius/Bundle/ApiBundle/Controller/GetSalesStatisticsAction.php
Outdated
Show resolved
Hide resolved
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the switch
statement:
$interval = $salesPeriod->getInterval();
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modifyQueryBuilderForHandlingYear
can be replaced by any other name. Just an example, nothing better came to my mind.
80365bc
to
f273e8c
Compare
src/Sylius/Bundle/ApiBundle/Controller/GetSalesStatisticsAction.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My biggest concerns are:
- Feature named
SalesStatistics
instead of more general purposeStatistics
:- while already contains "customer created within period", which could have just registered.
- which limits/confuses re-usability for end-users.
- Ages-olds issue with orders count, total sales, average order value only include
paid
orders and doesn't include refunded, partial refunded. - 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) - 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.
And I choose "United States" channel | ||
Then I should see 2 new orders | ||
And I should see 8 new customers | ||
And there should be total sales of "$459.00" | ||
And the average order value should be "$229.50" | ||
|
||
@ui | ||
@api @ui | ||
Scenario: Changing channel in administration dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This and previous scenario have the same name. Could they be renamed?
Scenario: Changing channel in administration dashboard
Scenario: Changing channel in administration dashboard
- Cases look identical, only different is base channel and selected channel. Are both of them needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
And there should be total sales of "$5,241.00" | ||
And the average order value should be "$1,310.25" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I should see 4 new orders | ||
And I should see 8 new customers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it checks order summary
and not last customers/orders widget
then phrasing can be adjusted to
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)
src/Sylius/Component/Core/Sales/ValueObject/SalesStatistics.php
Outdated
Show resolved
Hide resolved
src/Sylius/Component/Core/Sales/ValueObject/SalesStatistics.php
Outdated
Show resolved
Hide resolved
src/Sylius/Component/Core/Sales/ValueObject/SalesStatistics.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/Controller/GetSalesStatisticsAction.php
Outdated
Show resolved
Hide resolved
With no connection to others source codes and only focusing on ValueObjects design,
|
@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 |
837e6f0
to
b749e28
Compare
b749e28
to
73df243
Compare
8840f9f
to
a119ffe
Compare
a119ffe
to
1e196e2
Compare
/bns:start |
$result = ['message' => $exception->getMessage()]; | ||
|
||
if ($exception instanceof ChannelNotFoundException) { | ||
$status = Response::HTTP_NOT_FOUND; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting this status is not needed as ChannelNotFoundException
extends NotFoundHttpException
$dataKeys = array_keys($data); | ||
|
||
foreach ($requiredFields as $field) { | ||
Assert::inArray($field, $dataKeys, "The data must contain the $field key."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert::inArray($field, $dataKeys, "The data must contain the $field key."); | |
Assert::inArray($field, $dataKeys, sprintf('The data must contain the %s key.', $field)); |
Thanks, Rafał! 🎉 |
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.