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

[14.0][FIX] l10n_nl_xaf_auditfile_export: exclude section and note move lines #375

Merged
merged 2 commits into from
May 8, 2023
Merged

[14.0][FIX] l10n_nl_xaf_auditfile_export: exclude section and note move lines #375

merged 2 commits into from
May 8, 2023

Conversation

astirpe
Copy link
Member

@astirpe astirpe commented Jan 19, 2023

Fixes #348

CasVissers-360ERP
CasVissers-360ERP approved these changes Jan 24, 2023
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.

LGTM functional review.

NL66278
NL66278 requested changes Jan 24, 2023
View reviewed changes
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.

Please adapt the test class

@astirpe astirpe marked this pull request as draft January 25, 2023 09:53
@astirpe astirpe force-pushed the 14_fix_l10n_nl_xaf_auditfile_export branch from d47a527 to ca624bd Compare January 25, 2023 10:08
@NL66278 NL66278 changed the title [FIX] l10n_nl_xaf_auditfile_export: exclude section and note move lines [14.0][FIX] l10n_nl_xaf_auditfile_export: exclude section and note move lines Feb 22, 2023
@NL66278
Copy link
Contributor

NL66278 commented Feb 22, 2023

@astirpe The account.move.line's have apparently not been flushed to the database when get_move_line_count is called. I replaced the SQL select with an ORM search_count in the 15.0 migration. That solves the problem. As retrieving a single count will have a neglegible effect on performance (as date is indexed), I think this is the way to solve this. Tried first with a flush(0 on the model before the SQL selecct, but that had no effect. My 15.0 commit is here:
7c12194

@astirpe astirpe marked this pull request as ready for review February 24, 2023 08:54
@astirpe
Copy link
Member Author

astirpe commented Feb 24, 2023

@NL66278 it works now, thank you!

@NL66278
Copy link
Contributor

NL66278 commented May 3, 2023

@astirpe Please rebase on current 14.0

@astirpe astirpe force-pushed the 14_fix_l10n_nl_xaf_auditfile_export branch from 802fd0d to 9e3ef58 Compare May 8, 2023 07:37
@NL66278
Copy link
Contributor

NL66278 commented May 8, 2023

@astirpe It is missing 7c12194 now.

@astirpe
Copy link
Member Author

astirpe commented May 8, 2023

@NL66278 done, thanks!

NL66278
NL66278 approved these changes May 8, 2023
View reviewed changes
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). 🤖

@NL66278
Copy link
Contributor

NL66278 commented May 8, 2023

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-375-by-NL66278-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit cdd74af into OCA:14.0 May 8, 2023
6 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 436d748. 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
Reviewers

@NL66278 NL66278 NL66278 approved these changes

@CasVissers-360ERP CasVissers-360ERP CasVissers-360ERP 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.

[14.0] XAF Audit file improvements
4 participants
@astirpe @NL66278 @OCA-git-bot @CasVissers-360ERP