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

[10.0] [MIG] l10n_de_steuernummer #23

Merged
merged 2 commits into from
Aug 17, 2017

Conversation

pedrobaeza
Copy link
Member

Following #17, this is the migration to 10.0 of the module still in PR. The discussion ended that this can be useful and should be merged, but it has not done it yet. We can move forward to this version now, as the customer is moving to this version.

@pedrobaeza
Copy link
Member Author

cc @Tecnativa

@coveralls
Copy link

coveralls commented Jun 22, 2017

Coverage Status

Coverage remained the same at 92.857% when pulling 5aab698 on Tecnativa:10.0-l10n_de_steuernummer into 9bcb36d on OCA:10.0.

@pedrobaeza
Copy link
Member Author

@chienandalu @cubells please review

Copy link
Sponsor Member

@cubells cubells left a comment

Choose a reason for hiding this comment

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

Minor changes. Typos.

if getattr(vatnumber, 'check_vat_de', None):
res = vatnumber.check_vat_de(vat)
if not res:
# This vat is not a Steue-IdNr., so if it is 10 or 11 digit
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This vat is not an Steue-IdNr.

Copy link
Member

@chienandalu chienandalu Aug 16, 2017

Choose a reason for hiding this comment

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

@cubells I think that's not correct. Following grammar rules(https://rayli.net/blog/life/when-to-use-a-or-an/#First_why_the_confusion) a word like Steuer doesn't begin with a vowel sound (although we spanish speakers tend to pronounce such words more like 'es...', than the proper 'ss').

Copy link
Member

Choose a reason for hiding this comment

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

Anyway s/Steue-IdNr/Steuer-IdNr

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@chienandalu
I'm not an expert english speaker, sure.

So according to you, I made "a stupid comment" or "an stupid comment"?
:)

Copy link
Member

Choose a reason for hiding this comment

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

@cubells Just an unfortunate observation 😄

res = vatnumber.check_vat_de(vat)
if not res:
# This vat is not a Steue-IdNr., so if it is 10 or 11 digit
# length then we can assume that this is a old Steuernummer
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

that this is an old Steuernummer


#. Go to Contacts.
#. Create a new record.
#. Put a SteuerNummer in the VAT field preceded by 'DE'.
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Put an SteuerNummer in the VAT

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

@pedrobaeza Runbot doesn't build. Rebase maybe?

@pedrobaeza
Copy link
Member Author

The problem is an outdated .travis.yml. You can update it in a separate PR.

@chienandalu
Copy link
Member

@pedrobaeza Here you are #24

@pedrobaeza
Copy link
Member Author

Please rebase and fix the comments that should be fixed (if any pending)

@OCA OCA deleted a comment from coveralls Aug 17, 2017
@OCA OCA deleted a comment from coveralls Aug 17, 2017
@pedrobaeza
Copy link
Member Author

@chienandalu @cubells give then your final blessing for merging.

@chienandalu
Copy link
Member

@pedrobaeza It still skips building runbot 😕

Copy link
Sponsor Member

@cubells cubells left a comment

Choose a reason for hiding this comment

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

👍
LGTM

@pedrobaeza
Copy link
Member Author

But it's because you haven't rebased properly. Check the .travis.yml in the branch: https://github.com/Tecnativa/l10n-germany/blob/10.0-l10n_de_steuernummer/.travis.yml

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

@pedrobaeza Yes, I didn't update remotes.

Tested on runbot

@pedrobaeza pedrobaeza merged commit 692e011 into OCA:10.0 Aug 17, 2017
@pedrobaeza pedrobaeza deleted the 10.0-l10n_de_steuernummer branch August 17, 2017 08:43
@pedrobaeza pedrobaeza mentioned this pull request Sep 13, 2017
3 tasks
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.

None yet

5 participants