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.

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

[Admin] Add single channel dashboard #5086

Merged

Conversation

pjedrzejewski
Copy link
Member

@pjedrzejewski pjedrzejewski commented May 22, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Related tickets -
License MIT

Todo:

  • Reduce duplication in the 2 methods of Setup\OrderContext

Future improvements (got the template - see first commit):

  • Handle multiple channels
  • Add date range selector

@pjedrzejewski pjedrzejewski added New Feature Admin AdminBundle related issues and PRs. labels May 22, 2016
@pjedrzejewski
Copy link
Member Author

zrzut ekranu 2016-05-22 o 23 35 30

And then 2 more customers have placed 2 orders for total of "€459.00"
When I open administration dashboard
Then I should see 6 new orders
And I should see 6 new customers
Copy link
Contributor

Choose a reason for hiding this comment

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

3 customers + 2 customers != 6 customers (I know there's also an admin, but it seems werid)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but that's the flaw in our entire app, so I have no other way currently. :)

@pamil
Copy link
Contributor

pamil commented May 23, 2016

The context of "new" customers and orders seems weird to me, as there's no such thing in code right now.

@pjedrzejewski
Copy link
Member Author

@pamil Yeah, I noticed that too, but I think it is not worth removing it, as I will add per channel / date filtering for this dashboard, so both provider interface and the logic will change. :)

*
* @return int
*/
private function getPriceFromString($price)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this method? It is context, so why can't we use a lexical context?

@pjedrzejewski pjedrzejewski force-pushed the admin/dashboard-prototype branch 2 times, most recently from b45a33a to 084228d Compare May 24, 2016 09:36
@michalmarcinkowski michalmarcinkowski merged commit 73d66c7 into Sylius:master May 24, 2016
@michalmarcinkowski
Copy link
Contributor

Looks nice! Please apply @lchrusciel comments in a separate PR 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants