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 - Update Accounting closure with missing too many A-Nouveau #30039

Closed
wants to merge 4 commits into from

Conversation

josett225
Copy link
Contributor

FIX The aim of this fix is to make sure we generate all the A-Nouveaux with an Update of bookkeeping.class.php code

If we limit the duplicates analysis to limited variables (actual situation) then in some cases with large bookkeeping lines we failed in generating all the A-nouveaux.
With the following fix in one customer case, we went from 150 lines to more than 2000. This fix used 3 more variables :
montant - compte tiers - label compte tiers or in SQL wording
montant - subledger_account - subledger_label

New Comment : Note that we must include then 'doc_type - fk_doc - numero_compte - label - montant - compte tiers - label compte tiers' to be sure to have unicity of line (because we may have several lines identical)

FIX The aim of this fix is to make sure we generate all the A-Nouveaux with an Update bookkeeping.class.php code

If we limit the duplicates analysis to limited variables (actual situation) then in some cases we failed in generating all the A-nouveaux.
With the following fix in one customer case, we went from 150 lines to more than 2000.
This fix used 3 more variables : 
montant - compte tiers - label compte tiers 
Note that we must include then 'doc_type - fk_doc - numero_compte - label - montant - compte tiers - label compte tiers' to be sure to have unicity of line (because we may have several lines identical)
@aspangaro
Copy link
Member

Hi @josett225 ,

With "amount", we have all the lines from the previous year. This may be an option, but it wasn't the original intention. The aim was to group the amounts of customer or supplier invoices under a single amount, not detailed so as to be able to letter.

@@ -341,6 +342,9 @@ public function create(User $user, $notrigger = false)
}
$sql .= " AND numero_compte = '".$this->db->escape($this->numero_compte)."'";
$sql .= " AND label_operation = '".$this->db->escape($this->label_operation)."'";
$sql .= " AND montant = '".((float) $this->montant)."'";
Copy link
Member

@eldy eldy Jun 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you introduce this lines, it means you want to remove the protection.
Without the protection, it will be possible to have duplicate line with same
doc_type, doc id, entity, account number and account label
This protection is not very good because we should have a protection on
doc_type, doc id, entity, account number. But it is better than nothing.
I don't see the use cases when we need to have duplicates on this 4 fields. Can you provide an example of values for this 4 parameters and the use case that explain why we need to be able to insert such duplicate ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eldy You were right

@eldy eldy added the Discussion Some questions or discussions are opened and wait answers of author or other people to be processed label Jun 15, 2024
@josett225
Copy link
Contributor Author

Hi @eldy and @aspangaro
Thanks for your answers.
I will dig into your comments and run again all my tests to give you a better view and to check results with and without modifications.

FYI this has been refused by 2 accounting experts for now.

@josett225
Copy link
Contributor Author

josett225 commented Jun 19, 2024

Hi @eldy and @aspangaro
After discussing longely with our experts acoounting, they finally brung to my attention that the A-nouveau could come either with global amounts per account or with details with an activation option in their accounting software. My 2 experts preferred to get the A-nouveau with details as it makes easier to follow and letter.

I suggest we could add either an option in Setup Other or in the accounting admin to activate the A-nouveau detail feature.

Thoughts?

@eldy
Copy link
Member

eldy commented Jul 1, 2024

Hi @eldy and @aspangaro After discussing longely with our experts acoounting, they finally brung to my attention that the A-nouveau could come either with global amounts per account or with details with an activation option in their accounting software. My 2 experts preferred to get the A-nouveau with details as it makes easier to follow and letter.

I suggest we could add either an option in Setup Other or in the accounting admin to activate the A-nouveau detail feature.

Thoughts?

@josett225 Yes, please add your new behaviour as an option (first step as a hidden option with no setup page). so it is easier to analyse the change the option has into code for impact analysis.

Also, can you answer this:
If you introduce the a line an amount, it means you want to remove the protection to avoid to get duplicate lines.
So without the protection, it will be possible to have duplicate line with same:
doc_type, doc id, entity, account number and account label
This protection is not very good because the real protection should be on
doc_type, doc id, entity, account number.
But it is better than nothing.

I don't see the use cases when we need to have duplicates on this 4 fields. Can you provide an example of values (at least 2 lines) for this 4 parameters and the use case that explain why we need to be able to insert such duplicate ?

@josett225
Copy link
Contributor Author

Hi @eldy
Yes I will review all the code this week.
Regards
Jose

@alexandre-janniaux
Copy link
Contributor

Hi, by any chance, are you still working on this ? @josett225

@josett225
Copy link
Contributor Author

josett225 commented Nov 24, 2024

Hi @alexandre-janniaux
I did some changes on my customers side and I did not bring the code back here as I jumped to Asset module.
Let me come back on it and review the latest code v20 and v21.
Which version are you running?

@alexandre-janniaux
Copy link
Contributor

Hi, that's incredible, great! I can finish adding the bookeeping test infrastructure so that I can help you integrate this.

I'm running 19.0.3 currently, will upgrade to 19.0.4 after I get some time for more testing, but I can adapt.

Copy link
Contributor Author

@josett225 josett225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eldy, @alexandre-janniaux and @aspangaro
After deep investigation, I found out that I was wrong in including all the values to find duplicates.
The issue I was facing on getting duplicates was coming from the database not be clean enough due to changes happening during the company journey in account labels or sub_account labels and when trying do a closure to generate a-nouveaux. Another PR is coming in a few minutes to clean the closure code.
However it is important to add the subledger_account to find duplicates when required.

josett225 added a commit to josett225/dolibarr that referenced this pull request Dec 1, 2024
FIX I did further testing and investigation and I fixed the following issues that stop doing a full closure without duplicate lines generated by an unclean database :

    - different label_compte with same account number
    - removing label_compte is raising an issue and the code in the line around 2770 $bookkeeping->label_compte = $obj->label_compte;
    - different subledger_label with same subledger_account
    - empty versus null values for subledger_label and subledger_account
    - opening_balance is 0 as it creates a bookkeeping entry for now.  

FIX - Update Accounting closure with missing too many A-Nouveau Dolibarr#30039)
josett225 added a commit to josett225/dolibarr that referenced this pull request Dec 8, 2024
This will help to better and understand and follow this fix
@josett225 josett225 closed this Dec 8, 2024
@josett225
Copy link
Contributor Author

As agreed with @eldy, I close this PR. Code has been moved to another PR

@josett225 josett225 deleted the patch-3 branch December 8, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Some questions or discussions are opened and wait answers of author or other people to be processed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants