Skip to content

[10.0] account - #843

Merged
hbrunn merged 11 commits into
OCA:10.0from
NL66278:10.0-account
Jun 10, 2017
Merged

[10.0] account#843
hbrunn merged 11 commits into
OCA:10.0from
NL66278:10.0-account

Conversation

@NL66278

@NL66278 NL66278 commented May 29, 2017

Copy link
Copy Markdown

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@pedrobaeza

Copy link
Copy Markdown
Member

@hbrunn @StefanRijnhart I have rebased this PR on latest 10.0 branch and we continue having the same error installing l10n_generic_coa.

@NL66278 are you going to continue working on this or should I take over the work?

@NL66278

NL66278 commented Jun 7, 2017

Copy link
Copy Markdown
Author

@pedrobaeza I will be working on this on friday.

@pedrobaeza

Copy link
Copy Markdown
Member

OK, let me know any progress or question

@NL66278

NL66278 commented Jun 9, 2017

Copy link
Copy Markdown
Author

Now fails on l10n_be:
2017-06-09 11:56:13,326 41064 INFO openupgrade odoo.sql_db: bad query: INSERT INTO "account_invoice_line" ("id", "account_id", "layout_category_sequence", "invoice_id", "price_unit", "sequence", "discount", "product_id", "quantity", "create_uid", "write_uid", "create_date", "write_date") VALUES(nextval('account_invoice_line_id_seq'), NULL, 0, 8, '642.00', 10, '0.00', 163, '5.000', 1, 1, (now() at time zone 'UTC'), (now() at time zone 'UTC')) RETURNING id

2017-06-09 11:56:13,326 41064 ERROR openupgrade odoo.tools.yaml_import: null value in column "account_id" violates not-null constraint

DETAIL: Failing row contains (19, null, 2017-06-09 11:56:10.618323, 10, 642.00, null, 1, null, null, null, 1, null, null, null, 0.00, 2017-06-09 11:56:10.618323, null, null, 163, 8, 5.000, null, null, null, null, null, null, null, null, 0, null).

@pedrobaeza

Copy link
Copy Markdown
Member

That's the same problem as it was happening with l10n_generic_coa, complaining about an account_id null value. I suppose that this is because now l10n_be is installed instead of the generic one, but the problem stays the same, and can be because there's something to do in account migration itself.

@NL66278

NL66278 commented Jun 9, 2017

Copy link
Copy Markdown
Author

@pedrobaeza Something incredible happens in l10n_be, They are loading demo data from the account module in the manifest file. Talk about bad practice.... I think the account.invoice specified in the yaml file will not be created (because already existing), and then the child account.invoice lines have nothing to point to.

@pedrobaeza

Copy link
Copy Markdown
Member

All localization modules do that, but I think it's not loaded twice. Please check if in account that file is loaded, which I guess it isn't

@NL66278

NL66278 commented Jun 9, 2017

Copy link
Copy Markdown
Author

@pedrobaeza Indeed the problem is different, account_id on an invoice line is now required, but is not set. I still think it bad practice to load demo data from another module, but that is not what is causing the crash.

@NL66278

NL66278 commented Jun 9, 2017

Copy link
Copy Markdown
Author

@pedrobaeza I have not really a good idea on how to solve this. Seems best to patch the yaml file to also create an account and then refer to that account in the invoice lines. Of course the yaml file would then be different from what we have in standard Odoo. Or would you see another solution?

@NL66278

NL66278 commented Jun 9, 2017

Copy link
Copy Markdown
Author

Some Travis builds fail, because this bug: travis-ci/travis-ci#7459

When we reset branch to $TRAVIS_COMMIT, this env var may contain an outdated commit. If commit still exists, there will be no error and you will wonder why new features do not work. If it no longer exists, everything later on wil bomb, because you will still be on 9.0.

@NL66278 NL66278 changed the title [WIP][10.0] account [10.0] account Jun 9, 2017
@NL66278

NL66278 commented Jun 9, 2017

Copy link
Copy Markdown
Author

And finally it is green. Now please review the real functionality. And yes I have no authority to put a review label on the PR myself.

@NL66278

NL66278 commented Jun 9, 2017

Copy link
Copy Markdown
Author

@StefanRijnhart @hbrunn Had to patch demo data to get this green. I wonder what you think about this, could there have been a better solution?

@StefanRijnhart

Copy link
Copy Markdown
Member

Well, the fix looks harmless. If it stops us worrying about l10n_be insanity, well done!

@hbrunn
hbrunn merged commit 661c77a into OCA:10.0 Jun 10, 2017
@legalsylvain legalsylvain added this to the 10.0 milestone May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants