-
-
Notifications
You must be signed in to change notification settings - Fork 534
[10.0][IMP] migrated sale_require_login and dependency #170
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
[10.0][IMP] migrated sale_require_login and dependency #170
Conversation
|
Hey @ovnicraft, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
52ea12d to
75950e1
Compare
|
There's already a PR for this module. Please review #138 and maybe propose any change to that branch. |
|
@pedrobaeza the migration hasta latest commit Nov/2016 this is an issue :(. So if there is no response about this, OCA has any definition about close the abandon PR and continues with another proposed by community ? |
|
Well, not exactly a procedure, but when someone doesn't respond in the PR, usual steps is to take over his/her work and create another PR. Continuing from the previous PR work or start from the scratch will depend on the complexity of the changes. Anyway, I see that this module depends on another one, so you should first migrate that one also, isn't it? |
|
@pedrobaeza I migrate both in this PR, so i review their changes and has the same changes (module works without major changes). Open a new PR for dependency module? |
|
Let's see if tomorrow there's an answer, and if not, please open first a PR for the dependency, and then we reuse this one for the website_sale_require_login. |
|
Ok 👍 |
|
@pedrobaeza I would like you to take this instead of mine #138, since @ovnicraft has fixed what was not right and so much time has elapsed . Sorry I'm messing up |
Module from 9.0 is working ok, migrate to 10.0 without major changes