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.

Sign up for GitHub

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

Jump to bottom

[13.0][MIG] l10n_nl_tax_statement #243

Merged
merged 37 commits into from
Jan 22, 2021
Merged

[13.0][MIG] l10n_nl_tax_statement #243

merged 37 commits into from
Jan 22, 2021

Conversation

astirpe
Copy link
Member

@astirpe astirpe commented Oct 10, 2019 •
edited
Loading

@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch 2 times, most recently from 93e12ea to fe644cf Compare October 10, 2019 08:36
@astirpe astirpe changed the title [13.0][MIG] l10n_nl_tax_statement (WIP) [13.0][MIG] l10n_nl_tax_statement Jan 16, 2020
CasVissers-360ERP
CasVissers-360ERP suggested changes Feb 26, 2020
View reviewed changes
l10n_nl_tax_statement/README.rst Outdated Show resolved Hide resolved
l10n_nl_tax_statement/README.rst Outdated Show resolved Hide resolved
l10n_nl_tax_statement/README.rst Outdated Show resolved Hide resolved
l10n_nl_tax_statement/views/l10n_nl_vat_statement_view.xml Outdated Show resolved Hide resolved
l10n_nl_tax_statement/views/l10n_nl_vat_statement_view.xml Outdated Show resolved Hide resolved
l10n_nl_tax_statement/views/l10n_nl_vat_statement_view.xml Outdated Show resolved Hide resolved
l10n_nl_tax_statement/models/l10n_nl_vat_statement_line.py Show resolved Hide resolved
l10n_nl_tax_statement/models/l10n_nl_vat_statement_line.py Show resolved Hide resolved
@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch 2 times, most recently from 2c96792 to d2ee9c9 Compare February 26, 2020 11:20
CasVissers-360ERP
CasVissers-360ERP approved these changes Feb 26, 2020
View reviewed changes
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.

Functional review, module is working correctly. Some nice2haves but nothing that should stop it from being merged.

@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch from 2e224ca to 95558ba Compare March 23, 2020 09:00
@CasVissers-360ERP
Copy link

CasVissers-360ERP commented Mar 25, 2020 •
edited
Loading

@astirpe
Two new feedback points:

  1. When a statement is posted, get_lines_action doesn't work anymore. No new lines are shown.
  2. When an undeclared move is added the statement is not refreshed

@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch from 95558ba to 7396902 Compare March 25, 2020 09:05
@astirpe
Copy link
Member Author

astirpe commented Mar 25, 2020

@CasVissers

  1. When a statement is posted, get_lines_action doesn't work anymore. No new lines are shown.

Fixed.

  1. When an undeclared move is added the statement is not refreshed

In the readme it's explicitely written that the user must click the Update button after adding undeclared invoices. It was already that way in past versions.
I made it that way because I thought that, when a lot of move lines are present in the system, the update could be slow. So recalculating statement lines each time a move is added could cause bad UX.
Maybe in the future I will dedicate more time to improve this point but for the moment I would prefer to keep things as they are.

@CasVissers-360ERP
Copy link

@astirpe

  1. Ok, tested and validated
  2. I can agree with that, although it's confusing. I would prefer a recalculation even though it can take some time.

For me the module is ready to be merged, there might be some future improvements/minor bugfixes but we could handle them in issues. Module seems stable and if there are exceptions they are easy to spot for users.

@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch 3 times, most recently from e5423c5 to 8da9150 Compare April 20, 2020 09:06
@astirpe astirpe mentioned this pull request Apr 22, 2020
Merged
2 tasks
@CasVissers-360ERP
Copy link

CasVissers-360ERP commented May 13, 2020 •
edited
Loading

@astirpe
I think it would be good to add a constraint on account move line or account move. When the move has a l10n_nl_vat_statement_id and the statement is final. The line shouldn't be edited anymore. Because these changes will not become visible anymore. Do you agree?

@astirpe
Copy link
Member Author

astirpe commented May 13, 2020

@CasVissers yes I agree, but I think we should make an explicit list of fields that for sure must be protected from being edited. I don't think it's wise to protect all the move and move line fields indiscriminately. For example, you may want to reconcile some move lines when the statement is final.

@CasVissers-360ERP
Copy link

@astirpe I agree, I think date (invoice date), debit, credit and tax_ids?

@astirpe
Copy link
Member Author

astirpe commented May 14, 2020

@CasVissers done, would you give it a try?

CasVissers-360ERP
CasVissers-360ERP suggested changes May 27, 2020
View reviewed changes
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.

Looks good! One minor feedback point.

@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch from 0f81ba2 to 472a215 Compare May 27, 2020 08:09
@CasVissers-360ERP
Copy link

@yung-wang
Can you validate so we can get this merged?

@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch 2 times, most recently from ff33d04 to a339e7e Compare July 30, 2020 13:49
astirpe and others added 13 commits October 21, 2020 13:57
@astirpe
Add tag_3b_omzet diensten in l10n_nl_tax_statement
42a2cfa
@astirpe
Code review
16ef184
@astirpe
[MIG] Port l10n_nl_tax_statement to 12.0
33866b9
@astirpe
Port latest changes from 11.0
9034a5c
@astirpe
Add coverage
db02a94
@astirpe
[IMP] Add buttons to display detailed move lines
2394cbe
@OCA-git-bot @astirpe
[UPD] README.rst
e745de9
@OCA-git-bot @astirpe
[ADD] icon.png
ba6a12a
@OCA-git-bot @astirpe
[UPD] README.rst
9151df2
@astirpe
[FIX] Include past invoices in report
f5a39ca 
View past invoices moves when country is not NL

[FIX] Button icon

[FIX] Sort past invoices by date

[FIX] Button icon

[FIX] Domain condition

Update roadmap

Auto-update statement lines when adding/removing past invoices

Fix failing test

Coverage

Fix calculation of past invoices in report
@astirpe
Fix _compute_is_invoice_basis()
55c13f1
@astirpe
Add oca_dependencies.txt
2c5f180
@astirpe
Pre-commit
27cfb94
@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch from 50cf600 to fd43fb3 Compare October 21, 2020 11:58
@astirpe astirpe force-pushed the 13_mig_l10n_nl_tax_statement branch from fd43fb3 to 66f2c76 Compare October 21, 2020 12:05
@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

hbrunn commented Jan 22, 2021

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-243-by-hbrunn-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8a4f07a into OCA:13.0 Jan 22, 2021
@OCA-git-bot
Copy link
Contributor

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

@astirpe astirpe deleted the 13_mig_l10n_nl_tax_statement branch January 22, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CasVissers-360ERP CasVissers-360ERP CasVissers-360ERP approved these changes

@tarteo tarteo tarteo approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@astirpe @CasVissers-360ERP @OCA-git-bot @hbrunn @tarteo @oca-transbot @ploegvde @StefanRijnhart