-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(types,utils): added promotion create with rules and application target rules #5957
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Ignored Deployments
|
🦋 Changeset detectedLatest commit: eb6be8e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ffdf94e
to
20166a4
Compare
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.
Strong work @riqwan. Have added a couple of comments and todos.
On the model ID prefix, we should probably (re)establish a convention:
- Single words i.e. no
-
or_
- Keep them short
- Include some "ref" to module (if the concept is likely to live elsewhere) e.g.
pro
orpric
. For readability purposes
This might need to be applied to the pricing module too, if we agree.
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.
LGTM, very nice work :) I like what the promotion engine will become ❤️
@olivermrbl good to merge this one? |
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.
LGTM 💪
what:
Adds support for creating prommotions along with rules for promotion and rules for application
RESOLVES CORE-1593
RESOLVES CORE-1594