Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Admin] Feed dashboard graph with real data #11082

Merged
merged 11 commits into from Feb 3, 2020
Merged

[Admin] Feed dashboard graph with real data #11082

merged 11 commits into from Feb 3, 2020

Conversation

Zales0123
Copy link
Member

@Zales0123 Zales0123 commented Jan 30, 2020 •
edited

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets related to #11050
License MIT

Known problems:

  • no currency in the graph (where it should be displayed? Tooltip? On the Y-axis? cc @CoderMaggie)
  • data is not consistent with the "SALES" statistics above - however the latter should be fixed, as it takes in the account if the order is shipped or not (it shouldn't)
  • if there is 0 sale in a month it won't be displayed (rare, but possible)

Zrzut ekranu 2020-01-30 o 13 21 13

@Zales0123 Zales0123 added the Feature New feature proposals. label Jan 30, 2020
@Zales0123 Zales0123 requested a review from a team as a code owner January 30, 2020 12:37
@CoderMaggie
Copy link
Member

Response to problems:
Currency: Shall we display the symbol with the amount? I mean $100, $200, $300 on the Y axis? If not then maybe currency code above the Y axis?
Consistency with the sales count: ok, we should do it separately :)
Months with no sales: current solution makes me cry 😢 if no sales in a month then the month should still be displayed, but it should have no bar 💃

@Zales0123
Copy link
Member Author

Zales0123 commented Jan 30, 2020 •
edited

@CoderMaggie 1st and 3rd problem resolved:

Zrzut ekranu 2020-01-30 o 16 15 01

lchrusciel
lchrusciel reviewed Jan 31, 2020
View reviewed changes
src/Sylius/Bundle/AdminBundle/Controller/DashboardController.php Show resolved Hide resolved
src/Sylius/Component/Core/Dashboard/SalesDataProvider.php
$channelId = $channel->getId();

$query = $this->entityManager->getConnection()->query(
"SELECT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this implement a DQL here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be for it to leave it as it is now, because it is an experimental feature

@GSadee GSadee dismissed pamil’s stale review February 3, 2020 06:35

The requested changes have been applied :)

@GSadee GSadee merged commit a40beca into Sylius:master Feb 3, 2020
@GSadee
Copy link
Member

GSadee commented Feb 3, 2020

Thanks, Mateusz! 🎉

@Zales0123
Copy link
Member Author

And Grzegorz 🎉 😄

@Zales0123 Zales0123 deleted the graph-of-sales-on-dashboard branch February 3, 2020 10:01
lchrusciel
lchrusciel reviewed Apr 10, 2020
View reviewed changes
src/Sylius/Component/Core/Dashboard/SalesDataProvider.php
DATE_FORMAT(checkout_completed_at, '%m.%y') AS \"date\",
SUM(total) as \"total\"
FROM sylius_order
WHERE (channel_id = $channelId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To support string and int ids:

Suggested change
WHERE (channel_id = $channelId)
WHERE (channel_id = "$channelId")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lchrusciel lchrusciel lchrusciel left review comments

@GSadee GSadee GSadee approved these changes

@AdamKasp AdamKasp AdamKasp approved these changes

@pamil pamil pamil left review comments

Assignees
No one assigned
Labels
Feature New feature proposals.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Zales0123 @CoderMaggie @GSadee @pamil @lchrusciel @AdamKasp