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

[FIX] Redundant inherit in balance report #2168

Closed
wants to merge 2 commits into from

Conversation

OpenCode
Copy link
Contributor

Descrizione del problema o della funzionalità:

Travis genera un sacco di commit per i file .pot del modulo account_balance_report

--
Confermo di aver firmato il CLA https://odoo-community.org/page/cla e di aver letto le linee guida su https://odoo-community.org/page/contributing

@rousseldenis
Copy link
Sponsor

Descrizione del problema o della funzionalità:

Travis genera un sacco di commit per i file .pot del modulo account_balance_report

--
Confermo di aver firmato il CLA https://odoo-community.org/page/cla e di aver letto le linee guida su https://odoo-community.org/page/contributing

Ok so try an ocabot merge command

@OpenCode
Copy link
Contributor Author

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 11.0-ocabot-merge-pr-2168-by-OpenCode-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 12, 2021
Signed-off-by OpenCode
@rousseldenis
Copy link
Sponsor

@OpenCode @eLBati If this fixes the 11.0 branch, you need to rebase #2160 in order to have a good merge flow.

@OpenCode
Copy link
Contributor Author

@OpenCode @eLBati If this fixes the 11.0 branch, you need to rebase #2160 in order to have a good merge flow.

Sure :)

@OCA-git-bot
Copy link
Contributor

@OpenCode your merge command was aborted due to failed check(s), which you can inspect on this commit of 11.0-ocabot-merge-pr-2168-by-OpenCode-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@rousseldenis
Copy link
Sponsor

@OpenCode @eLBati This is an issue on the naming you did on the reports...

Odoo already has on account module that report and the same field names:

https://github.com/odoo/odoo/blob/11.0/addons/account/wizard/account_report_trial_balance.py#L8

which inherit that one :

https://github.com/odoo/odoo/blob/11.0/addons/account/wizard/account_report_common.py#L10

For translations, Odoo transforms the model name account.balance.report and account_balance_report to field_account_balance_report_<field_name> : for both !

As the first one is already loaded from account module, it detects that the field reference already exists, so suffixes by the id.
That's the problem you have. On travis side, as the field id in DB is different on each build, the field reference for pot changes also.

My question: Why not having an italian based name for your reports ? l10n.it.account.balance.report for instance.

That will solve:

  • First the travis problem
  • Secondly, and the most important, the issues you can have to effectively translate those problematic fields.

@OpenCode OpenCode force-pushed the 11.0-fix_infinite_travis_build branch from 15d29c9 to 681496e Compare March 13, 2021 11:30
@OpenCode
Copy link
Contributor Author

@OpenCode @eLBati This is an issue on the naming you did on the reports...

Odoo already has on account module that report and the same field names:

https://github.com/odoo/odoo/blob/11.0/addons/account/wizard/account_report_trial_balance.py#L8

which inherit that one :

https://github.com/odoo/odoo/blob/11.0/addons/account/wizard/account_report_common.py#L10

For translations, Odoo transforms the model name account.balance.report and account_balance_report to field_account_balance_report_<field_name> : for both !

As the first one is already loaded from account module, it detects that the field reference already exists, so suffixes by the id.
That's the problem you have. On travis side, as the field id in DB is different on each build, the field reference for pot changes also.

My question: Why not having an italian based name for your reports ? l10n.it.account.balance.report for instance.

That will solve:

* First the travis problem

* Secondly, and the most important, the issues you can have to effectively translate those problematic fields.

I can try to change report name

@rousseldenis
Copy link
Sponsor

I can try to change report name

Ok, that's the best approach.

For travis build, I think this is because a test fails there https://travis-ci.com/github/OCA/l10n-italy/jobs/490679770#L2213

So, ocabot redo the build to try merging (and so the field reference is different next time - comparing each commit for pot on the merge branch).

@OpenCode
Copy link
Contributor Author

I can try to change report name

Ok, that's the best approach.

For travis build, I think this is because a test fails there https://travis-ci.com/github/OCA/l10n-italy/jobs/490679770#L2213

So, ocabot redo the build to try merging (and so the field reference is different next time - comparing each commit for pot on the merge branch).

My last commit changes the report name with a l10nit_ prefix.

This PR was rebased, so Travis can run code on python 3.6.

The error you linked, is fixed (I hope) in another PR: #2160

@OpenCode OpenCode force-pushed the 11.0-fix_infinite_travis_build branch from c664f03 to 4b37e74 Compare March 13, 2021 14:48
@OpenCode
Copy link
Contributor Author

@rousseldenis PR #2160 merged and rebased here. I think now we have a complete situation in this PR, too.

@rousseldenis
Copy link
Sponsor

Situation is stable now:

New export :

image

Current pot builds :

image

@eLBati
Copy link
Member

eLBati commented Mar 14, 2021

@OpenCode @rousseldenis thanks!

@OpenCode Should this also be done for v12?

@OpenCode
Copy link
Contributor Author

@OpenCode @rousseldenis thanks!

@OpenCode Should this also be done for v12?

If we merge this one, we will do this for 12, too.

@OpenCode
Copy link
Contributor Author

@rousseldenis @eLBati I think we can close this PR. Actually, branch 11.0 is working again, so we are sure that this isn't the reason. Are you agree?

@eLBati
Copy link
Member

eLBati commented Mar 31, 2021

OK

@sergiocorato
Copy link
Contributor

Are you agree

Closing as requested.

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

Successfully merging this pull request may close these issues.

None yet

7 participants