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] [12.0] l10n_nl_partner_name #251

Merged
merged 19 commits into from Jul 29, 2020

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Jan 12, 2020

No description provided.

hbrunn and others added 17 commits November 17, 2019 18:07
from 8.0 to 10.0

[MIG] Migrates l10n_nl_partner_salutation from 8.0 to 10.0

Adding a commiter, changing the format of the licence comment, removing deprecated Char init variable

[FIX] Sort version of licencing, removing VIM comments, changing __openerp__ to __manifest__

[FIX] Adding tests to increase code coverage

[FIX] Replaces api.one with api.multi and inserting api.model

[FIX] Adds newlines on the xmls, adds licence on manifest, changes name of compute methods

[FIX] Creates the res_partner_title_sir in the module instead on the base module
* [DEL] obsolete manifest

* [IMP] allow to fill in names in a more natural way
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: l10n-netherlands-11.0/l10n-netherlands-11.0-l10n_nl_partner_name
Translate-URL: https://translation.odoo-community.org/projects/l10n-netherlands-11-0/l10n-netherlands-11-0-l10n_nl_partner_name/
@hbrunn hbrunn added this to the 12.0 milestone Jan 12, 2020
Copy link

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

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

LGTM, functional review.

@CasVissers-360ERP
Copy link

@hbrunn
One small point of feedback, see video:
https://drive.google.com/file/d/1w9lYJni3gYfwGAV_Id_AYOUsoBGlFm3O/view

Might be an issue of partner_firstname?

@hbrunn
Copy link
Member Author

hbrunn commented Jan 22, 2020

I don't really understand what I see. When you reset the email address, the contact form changes? This looks to me like there's some form that needs the correct conditional invisibility

@hbrunn
Copy link
Member Author

hbrunn commented Jan 22, 2020

we'll need a fix for l10n_nl_tax_statement anyways before we can start merging in 12 again

@astirpe
Copy link
Member

astirpe commented Jan 22, 2020

I don't have time to investigate why tests of l10n_nl_tax_statement suddenly fail.
For me the failing tests can be temporarily commented out, as done in a40dc59.
I'll try to fix them when I have enough time.

@CasVissers-360ERP
Copy link

I don't really understand what I see. When you reset the email address, the contact form changes? This looks to me like there's some form that needs the correct conditional invisibility

When a partner has no e-mail set, and you want to send out a sale order or invoice. Odoo prompts for an e-mail. In the view first_name and last_name is shown although the contact of the object is a company.

@hbrunn
Copy link
Member Author

hbrunn commented Jan 22, 2020

@CasVissers thanks, this should be fixed now

@astirpe anybody else can dig into this too, so I don't see you as the one who is solely responsible to fix this. 'Fixing' tests by deactivating them is something I only want to do in exceptional cases.

Probably the issue is the new year, so we might have to make some of the dates involved in the tests relative to the current date.

Copy link

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

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

Thnx, fixes the issue.

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.

👍 LGTM

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@hbrunn
Copy link
Member Author

hbrunn commented Jul 29, 2020

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-251-by-hbrunn-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 29, 2020
Signed-off-by hbrunn
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 12.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 12.0-ocabot-merge-pr-251-by-hbrunn-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4698d99 into OCA:12.0 Jul 29, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 279b3e0. Thanks a lot for contributing to OCA. ❤️

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

8 participants