-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
[MIG] website_salesuggest_create_account: basic mig from v10 to v11 #225
[MIG] website_salesuggest_create_account: basic mig from v10 to v11 #225
Conversation
Example of module which requires such refactoring: https://github.com/it-projects-llc/website-addons/tree/10.0/website_sale_checkout_store [FIX] condition to show normal checkout button was wrong in website_sale_suggest_create_account I was equal to (user_authenticated or not signup_allowed and can_checkout) while it has to be (user_authenticated or not signup_allowed) and can_checkout
38596c2
to
1a5f4c6
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.
Update version. The rest updated code seems ok (I didn't try on runbot)
@@ -4,11 +4,12 @@ | |||
{ | |||
"name": "Suggest to create user account when buying", | |||
"summary": "Suggest users to create an account when buying in the website", | |||
"version": "10.0.1.1.0", | |||
"version": "11.0.0.0.0", |
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.
It should be 11.0.1.0.0
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.
This comment is still unattended
@pedrobaeza @yelizariev all comments done. |
@pedrobaeza I think this is ready to review |
"category": "Website", | ||
"website": "https://www.tecnativa.com", | ||
"author": "Tecnativa, " | ||
"LasLabs, " | ||
"Vauxoo, " |
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.
Changes done doesn't deserve co-authorship IMO. If you would add tests...
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.
I'm not sure what is the OCA criteria to add a co-authorship.
Could you send us the guideline about this matter, please?
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.
@moylop260 There is no guideline yet and today this is open to interpretation.
My opinion is:
- Migration, bugfix, translation, documentation --> Contributors
- Development, Refactoring, Co-development, Tests development --> Autors
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.
We can put in the middle migrations that require serious efforts for making it work, but this one is not clearly one. That's why I say that if you add tests, then authorship won't be discussed.
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.
Squash migration commits to merge please
@yajo |
1e02317
to
a9f10ab
Compare
By "migration commits" I mean commits related to v10-v11 migration, not older history. Sorry, I didn't explain myself 😅 |
a9f10ab
to
e8ae0b7
Compare
@yajo done |
No description provided.