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

Promotions #17

Merged
merged 1 commit into from
Feb 27, 2013
Merged

Promotions #17

merged 1 commit into from
Feb 27, 2013

Conversation

Given I am logged in as administrator
And the following promotion rules are defined:
| type | configuration |
| order_total | {"amount":5000, "equal":true} |
Copy link
Contributor

Choose a reason for hiding this comment

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

What {"amount":5000, "equal":true} mean ? Will that promotion be included when order total amount will be equal 5000 ?
Maybe then we should use something like:

| type        | amount    | is_equal |
| order_total | 5000      | true     |

instead of json ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is promotion rule configuration. Promotion actions will be applied only if rules are satisfied. In this case if order amount is greater then 5000.

But yes, I like the idea to use table instead of json, but then we must split this into 2 tables:

| type        | amount    | is_equal |
| order_total | 5000      | true     |

and

| type       | count | is_equal |
| item_count | 10    | true     |

Copy link
Member

Choose a reason for hiding this comment

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

Yep, we should remove the json for sure. The case is that different rules can have many different parameters. So columns are not best option too... I think we could parse a simpler format like Amount: 500, Foo: bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pjedrzejewski Why not spliting and have table per rule/action, and add to config everything that is not type?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

pjedrzejewski pushed a commit that referenced this pull request Feb 27, 2013
@pjedrzejewski pjedrzejewski merged commit 8fbf4a8 into Sylius:master Feb 27, 2013
@pjedrzejewski
Copy link
Member

👍 👍 👍

pjedrzejewski pushed a commit that referenced this pull request Aug 26, 2013
pjedrzejewski pushed a commit that referenced this pull request Aug 26, 2013
Small cleanup of cart functionality, re-use existing collection code fro...
pjedrzejewski pushed a commit that referenced this pull request Aug 26, 2013
pjedrzejewski pushed a commit that referenced this pull request Aug 27, 2013
pjedrzejewski pushed a commit that referenced this pull request Aug 27, 2013
Add ::matchAll to ZoneMatcher & ZoneMatcherInterface
pjedrzejewski pushed a commit that referenced this pull request Aug 27, 2013
pjedrzejewski pushed a commit that referenced this pull request Aug 27, 2013
Init polish translation
pjedrzejewski pushed a commit that referenced this pull request Aug 27, 2013
[WIP] Symfony 2.3 upgrade
pjedrzejewski pushed a commit that referenced this pull request Aug 29, 2013
pjedrzejewski pushed a commit that referenced this pull request Aug 29, 2013
Small cleanup of cart functionality, re-use existing collection code fro...
pjedrzejewski pushed a commit that referenced this pull request Aug 29, 2013
pjedrzejewski pushed a commit that referenced this pull request Aug 29, 2013
Init polish translation
pjedrzejewski pushed a commit that referenced this pull request Aug 29, 2013
[WIP] Symfony 2.3 upgrade
pjedrzejewski pushed a commit that referenced this pull request Aug 29, 2013
pjedrzejewski pushed a commit that referenced this pull request Aug 29, 2013
pjedrzejewski pushed a commit that referenced this pull request Aug 29, 2013
Add ::matchAll to ZoneMatcher & ZoneMatcherInterface
lchrusciel referenced this pull request in lchrusciel/Sylius Dec 15, 2014
pamil referenced this pull request in pamil/Sylius Mar 21, 2016
CoderMaggie pushed a commit to CoderMaggie/Sylius that referenced this pull request Jun 1, 2016
…ntegration

[CJMAX-18] Account order details page integration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants