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

[MIG] 11.0 mig partner company type #660

Merged
merged 8 commits into from
Feb 16, 2019

Conversation

daramousk
Copy link
Member

No description provided.

@oca-clabot
Copy link

Hey @daramousk, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@pedrobaeza pedrobaeza added this to the 11.0 milestone Nov 21, 2018
Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

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

Improve Code

'version': '11.0.1.0.0',
'license': 'AGPL-3',
'author': 'ACSONE SA/NV,Odoo Community Association (OCA)',
'website': 'https://acsone.eu',
Copy link
Member

Choose a reason for hiding this comment

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

@daramousk website should be this

@@ -0,0 +1,5 @@
# Copyright 2014-2015 Grupo ESOC <www.grupoesoc.es>
Copy link
Member

Choose a reason for hiding this comment

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

@daramousk remove Copyright from all __init__ file

"""Partner with birth date in date format."""
_inherit = "res.partner"

birthdate_date = fields.Date("Birthdate")
Copy link
Member

Choose a reason for hiding this comment

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

IMO here change birthdate_date to birthdate. For more worth reading.

@daramousk daramousk force-pushed the 11.0-mig-partner_company_type branch 2 times, most recently from 8437d6c to f81c62e Compare November 22, 2018 13:05
@daramousk
Copy link
Member Author

@hbrunn The tests do not actually fail but they are marked as a failure. See assertRaises. Also, I am not sure what to do with the CLA not being signed.

@hbrunn
Copy link
Member

hbrunn commented Nov 29, 2018

wrap it in https://github.com/OCA/OCB/blob/11.0/odoo/tools/misc.py#L783

The mail address is @yajo, so no problem. Do we have a possibility to declare aliases of people for the CLA bot?

@pedrobaeza
Copy link
Member

Uhm, that's a problem... He worked for other company and makes with that email address the commits, but no he works for us and uses other email address, but the GitHub login is still the same... @yajo, do you still have the other email address registered here in GitHub?

@hbrunn
Copy link
Member

hbrunn commented Nov 29, 2018

I think the alias makes sense. @StefanRijnhart had changed commit addresses too, don't you run in the same problem?

@StefanRijnhart
Copy link
Member

StefanRijnhart commented Nov 29, 2018

I did not mix addresses so I did not have this problem.

BTW in the Netherlands it is not unusual that the employer owns the copyright, so in that case there needs to be a new CLA granted by this company. Not relevant I believe.

@hbrunn
Copy link
Member

hbrunn commented Nov 29, 2018

but why does it work when I pull stuff that <redactedoldeviladdress> committed?

@StefanRijnhart
Copy link
Member

Seems like Github adds the login in the commit metadata https://github.com/gurneyalex/ak-oca-clabot/blob/master/clabot.py#L371

@hbrunn
Copy link
Member

hbrunn commented Nov 29, 2018

interesting, cool and a little bit scary wanting to look through the metadata in our source trees

@StefanRijnhart
Copy link
Member

Pedro found an instance of this error with my old email though: OCA/account-financial-tools#738 (comment)

@yajo
Copy link
Member

yajo commented Nov 30, 2018

I do not own that old address anymore, but my CLA is OK. I think we can just ignore the CLA message.

@daramousk daramousk force-pushed the 11.0-mig-partner_company_type branch from f81c62e to 6ed0116 Compare November 30, 2018 09:53
Copy link
Contributor

@lfreeke lfreeke left a comment

Choose a reason for hiding this comment

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

👍 (functional test)

@@ -0,0 +1 @@
from . import models
Copy link
Contributor

Choose a reason for hiding this comment

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

init.py files still by convention have the License line, only the copyright lines were dropped

Copy link
Contributor

@NL66278 NL66278 left a comment

Choose a reason for hiding this comment

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

Small changes would be nice. See other comments. After that, good to go.

@emagdalenaC2i emagdalenaC2i mentioned this pull request Dec 28, 2018
53 tasks
@NL66278
Copy link
Contributor

NL66278 commented Jan 17, 2019

@daramousk Hi George, are you going to take up the review comments on this one?

Copy link
Contributor

@NL66278 NL66278 left a comment

Choose a reason for hiding this comment

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

👍 One additional comment todo. And needs rebase to squash Transifex en fixup! comits. For the rest: LGTM and tested. So in principle approve.

@NL66278
Copy link
Contributor

NL66278 commented Jan 21, 2019

@daramousk And it helps if you remove the WIP.

@daramousk daramousk force-pushed the 11.0-mig-partner_company_type branch from 98b21eb to f16a826 Compare January 21, 2019 09:35
@daramousk daramousk changed the title [WIP] 11.0 mig partner company type [MIG] 11.0 mig partner company type Jan 21, 2019
@daramousk
Copy link
Member Author

@lfreeke Need your approval here as well

Copy link
Contributor

@lfreeke lfreeke left a comment

Choose a reason for hiding this comment

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

(functional test) 👍

@Nikul-Chaudhary
Copy link
Member

@daramousk Travis 🔴

@NL66278
Copy link
Contributor

NL66278 commented Jan 24, 2019

Travis error is in base_location_nuts. Nothing to do with this PR.

@NL66278
Copy link
Contributor

NL66278 commented Jan 24, 2019

Another example of why unit tests should not depend on the availability of external services...

@NL66278 NL66278 merged commit b3f8332 into OCA:11.0 Feb 16, 2019
@pedrobaeza
Copy link
Member

@NL66278 if you merge a migration PR, please remember to check the mark in the migration issue for marking the module migration as completed. I have already done it for the modules you have merged today.

@NL66278
Copy link
Contributor

NL66278 commented Feb 18, 2019

@pedrobaeza Thanks. I will try to remember it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.