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

[12.0][MIG] l10n_nl_tax_statement_icp #217

Merged
merged 6 commits into from
May 8, 2020

Conversation

astirpe
Copy link
Member

@astirpe astirpe commented Jan 22, 2019

@astirpe astirpe force-pushed the 12_mig_l10n_nl_tax_statement_icp branch 2 times, most recently from 3d8843f to 05659b0 Compare January 28, 2019 12:34
@CasVissers-360ERP
Copy link

CasVissers-360ERP commented Mar 8, 2019

@astirpe
Why do you hide the ICP statement when the tax statement is in draft?
attrs="{'invisible':[('state','=','draft')]}">

There is an update button available?

@astirpe
Copy link
Member Author

astirpe commented Mar 8, 2019

@CasVissers
I don't remember anymore why did I make it this way, probably for simplicity or performance reasons.
Does it work fine if removing attrs="{'invisible':[('state','=','draft')]}"? Otherwise I need to find some time to handle that case correctly.

@hbrunn hbrunn added this to the 12.0 milestone Mar 18, 2019
@CasVissers-360ERP
Copy link

@astirpe It's because you only calculate the ICP when doing @api.multi def post(self):. (self._compute_icp_lines())
The main reason why I don't like this concept is that VAT's for partners might be missing. A user then books, has to reset to draft and then recalculate the ICP statement. Seems illogical to me.

There are multiple improvements possible, ie:

  • recalculate button
  • check if all partner's have a VAT before booking. (like this one most)

Anyhow. It's an improvement the module is functionally OK. Also did a small technical review.

@astirpe astirpe force-pushed the 12_mig_l10n_nl_tax_statement_icp branch from 12bfa45 to 305450e Compare November 11, 2019 14:04
@OCA-git-bot

This comment has been minimized.

@OCA-git-bot

This comment has been minimized.

@StefanRijnhart
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@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). 🤖

@OCA-git-bot OCA-git-bot merged commit 511dd8d into OCA:12.0 May 8, 2020
@OCA-git-bot
Copy link
Contributor

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

@astirpe astirpe deleted the 12_mig_l10n_nl_tax_statement_icp branch May 8, 2020 13:48
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

6 participants