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

fix: Order type restriction conflictive code #18086

Closed
wants to merge 1 commit into from
Closed

fix: Order type restriction conflictive code #18086

wants to merge 1 commit into from

Conversation

Don-Leopardo
Copy link
Contributor

I was experimenting and added a new order_type via property setter, but when I saved my new order with my brand new order_type, it turns out that there is a hardcoded validation in the code. I was surprised, should it be there? Does a reason for it exists?
If there is no reason for it to exists then this PR can be merged to permit the users to create their own order_type.
If there is a valid reason, I think that we should discuss it, because is probable that there exists a better way to solve it. This is not only bad for the users that can't add a new order_type, it's bad because is code that should be maintained. For example, if tomorrow we decide to implement a new order_type, we should change this code too, and that's something more to remember and, because of that, bad.
Please, share your ideas.

@rmehta
Copy link
Member

rmehta commented Jun 27, 2019

Breaking changes can only go in develop branch.

I think there are some other places this has been hardcoded. If it needs to be removed, then we should remove it from other places too.

@rmehta rmehta closed this Jun 27, 2019
@Don-Leopardo
Copy link
Contributor Author

@rmehta Is this a breaking change or is it that we can't decide if it is a useless code because we don't know why is it there?

@rmehta
Copy link
Member

rmehta commented Jun 27, 2019 via email

@Don-Leopardo
Copy link
Contributor Author

To develop: #18096

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

2 participants