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 Accountancy - Blocking on annual closing #30653

Closed
wants to merge 3 commits into from

Conversation

aspangaro
Copy link
Member

Fix related to this forum post : https://www.dolibarr.fr/forum/t/cloture-exercice-et-a-nouveau/47070

If an account is used with an other label, the closure is impossible.

Need feedback

@@ -2707,7 +2715,7 @@ public function closeFiscalPeriod($fiscal_period_id, $new_fiscal_period_id, $sep
$sql .= ' AND aa.pcg_type IN (' . $this->db->sanitize(implode(',', $pcg_type_filter), 1) . ')';
$sql .= " AND DATE(t.doc_date) >= '" . $this->db->idate($fiscal_period->date_start) . "'";
$sql .= " AND DATE(t.doc_date) <= '" . $this->db->idate($fiscal_period->date_end) . "'";
$sql .= ' GROUP BY t.numero_compte, t.label_compte, aa.pcg_type';
$sql .= ' GROUP BY t.numero_compte, aa.pcg_type';
Copy link
Member

@eldy eldy Aug 16, 2024

Choose a reason for hiding this comment

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

Imagine you have in database
1 line: account = 12345, label = 'A label for the transaction", amount 10
1 line: account = 12345, label = 'A different transaction', amount 15

What do we want after creating the "A nouveau" lines ?
Just 1 line with amount 25 and label = 'A nouveau for closure abc' ?

Can you confirm ?

Copy link
Member Author

@aspangaro aspangaro Aug 17, 2024

Choose a reason for hiding this comment

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

I can confirm that this is exactly what we're aiming for. It's the a-nouveaux principle.

The a-nouveaux entry is similar to a trial balance in reality.

In France, this consists of summarizing all class 1 to 5 accounts (CAPIT to FINAN) in a single account.

And the annual result class 7 - 6 (INCOME - EXPENSE) goes to account 120 or 129.

Copy link
Member

Choose a reason for hiding this comment

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

So PR seems good to me.

@eldy eldy added the Discussion Some questions or discussions are opened and wait answers of author or other people to be processed label Aug 16, 2024
@alexandre-janniaux
Copy link
Contributor

alexandre-janniaux commented Oct 7, 2024

Hi, any news on this merge request? I've not checked whether it's fixed in 19.0.3 or 20.x though but I'm a bit stuck for the annual closure. It there any workaround for the mean time?

I can test the closing with patches from this MR since I reproduce 100% the issue.

@alexandre-janniaux
Copy link
Contributor

alexandre-janniaux commented Oct 7, 2024

I did some investigation around the issue, this triggers the BookkeepingRecordAlreadyExists error with the details on auxiliary accounts enabled. After some additional logs, I get :

2024-10-07 13:19:23 NOTICE      --- Access to GET /accountancy/closure/index.php - action=confirm_step_2, massaction=
2024-10-07 13:19:23 WARNING       BookKeeping::create BookkeepingRecordAlreadyExists: $row->nb =1 for doc_type=closure numero_compte=1068 label_operation=Exercice 2024
2024-10-07 13:19:23 INFO        --- End access to /accountancy/closure/index.php

But when I check for any bookkeeping entries in llx_accounting_bookkeeping with either doc_type=closure or label_operation like Exercice%, I don't get any entries, so I suppose it comes from entries being added multiple tests

So I guessed what is happening is that it might create multiple entries with the same label and numero_compte , which is indeed the case:

2024-10-07 13:27:59 WARNING       BookKeeping::create Creating new bookkeeping entry: $row->id=0 for doc_type=closure numero_compte=1013 label_operation=Exercice 2024
2024-10-07 13:27:59 WARNING       BookKeeping::create Creating new bookkeeping entry: $row->id=0 for doc_type=closure numero_compte=1061 label_operation=Exercice 2024
2024-10-07 13:27:59 WARNING       BookKeeping::create Creating new bookkeeping entry: $row->id=0 for doc_type=closure numero_compte=1068 label_operation=Exercice 2024
2024-10-07 13:27:59 WARNING       BookKeeping::create BookkeepingRecordAlreadyExists: $row->id=1 for doc_type=closure numero_compte=1068 label_operation=Exercice 2024

Is grouping those "A-nouveau" or finding a way to create them uniquely in an identifiable manner the root cause of the issue?

@alexandre-janniaux
Copy link
Contributor

Ok I did some investigation without the auxiliary account details yesterday evening and I understand better where the MR is going.

Without the option enabled, I had those errors:

2024-10-07 13:58:03 WARNING       BookKeeping::create Creating new bookkeeping entry: $row->nb=0 entity=1 fk_doc=2 doc_type=closure numero_compte=4421 label_compte=PRELEVEMENT A LA SOURCE label_operation=Exercice 2024
2024-10-07 13:58:03 WARNING       BookKeeping::create Creating new bookkeeping entry: $row->nb=1 entity=1 fk_doc=2 doc_type=closure numero_compte=4421 label_compte=PRELEVEMENT A LASOURCE label_operation=Exercice 2024
2024-10-07 13:58:03 WARNING       BookKeeping::create BookkeepingRecordAlreadyExists: $row->nb=1 entity=1 fk_doc=2 doc_type=closure numero_compte=4421 label_operation=Exercice 2024

A previous bookkeeping operation got an incorrect PRELEVEMENT A LASOURCE label, instead of PRELEVEMENT A LA SOURCE. I guess it was coming from how the account was named at the creation date. Then the account name was fixed but not the operation itself.

I also had another instance of the issue where some operations were labelled as "BANQUE XXX", and others with the same account labelled "BANQUE YYY". The original was that the "BANQUE XXX" was bought by "BANQUE YYY" and changed name, so the bank name was refleted back by modifying the bank record on the bank management page. But then.

In both those case, because the label was different, the grouping created two entries, so changing the grouping to not account for label_compte seems like a good solution here:

-				$sql .= ' GROUP BY t.numero_compte, t.label_compte, aa.pcg_type';
+				$sql .= ' GROUP BY t.numero_compte, aa.pcg_type';

Likewise, doing a fallback to the current value of the account seems like the correct solution if the original code doesn't supply a specific operation name (that might be changed depending on the configuration of the account), instead of using the one from the original bookkeeping posting (that cannot be updated)

+				if (empty($this->label_compte)) {
+					$accountingaccount = new AccountingAccount($this->db);
+					$accountingaccount->fetch('', $this->numero_compte);
+
+					dol_syslog(get_class($this).":: fetch label_compte if empty for number =".$accountingaccount->label, LOG_DEBUG);
+
+					$this->label_compte = $accountingaccount->label;
+				}

I've not checked if the MR would fix the issue on my case, but I'd say it would given the analysis. I will test that during the week. In the mean time, I think I can also write a test mapping the problem I encountered for non-regression testing if it can be integrated to the MR or after the MR.

@josett225
Copy link
Contributor

josett225 commented Nov 28, 2024

Hi @alexandre-janniaux and @aspangaro and @eldy
I did further testing and investigation and I found several issues:

  • 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? What is the rule by default?
  • What to do when the value of opening_balance is 0 as it creates a bookkeeping entry for now. (I will get a return from my accounting expert this morning) We could add this on the SQL resquest or add a if before creating the booking line
    HAVING
    (SUM(t.credit) - SUM(t.debit)) != 0 -- Exclude rows with opening_balance = 0
  • How duplicate bookkeeping have to be handle (see my other PR that I am investigating too FIX - Update Accounting closure with missing too many A-Nouveau #30039)

I already wrote a code to fix all of them but I need advises/guidelines from our experts here on:

  • are you approving generating the label_compte and subledger_label based on the lastest label of the numero_compte and subledger_account number
  • changing the SQL request by the following one to fix the above issues as below:
    SELECT
    t.numero_compte,
    NULLIF(t.subledger_account, '') as subledger_account,
    -- NULLIF(t.subledger_label, '') as subledger_label,
    aa.pcg_type,
    (SUM(t.credit) - SUM(t.debit)) as opening_balance
    FROM
    llx_accounting_bookkeeping as t
    LEFT JOIN
    llx_accounting_account as aa
    ON aa.account_number = t.numero_compte
    WHERE
    t.entity = 1
    AND aa.entity = 1
    AND aa.fk_pcg_version IN (
    SELECT pcg_version
    FROM llx_accounting_system
    WHERE rowid = 50
    )
    AND aa.pcg_type IN ('CAPIT','IMMO','STOCK','THIRDPARTY','FINAN','EXPENSE','INCOME')
    AND DATE(t.doc_date) >= '2023-01-01 00:00:00'
    AND DATE(t.doc_date) <= '2023-12-31 00:00:00'
    GROUP BY
    t.numero_compte,
    aa.pcg_type,
    NULLIF(t.subledger_account, '')
    -- NULLIF(t.subledger_label, '')
    HAVING
    (SUM(t.credit) - SUM(t.debit)) != 0 -- Exclude rows with opening_balance = 0
    ORDER BY
    t.numero_compte ASC

Here are the issues and results in the SQL request moving from 2132 rows to 325:
IssueBalanceZeroValue
IssuewithNullorEmptyValues
FinalResult

@alexandre-janniaux
Copy link
Contributor

Hi, sorry for the answer delay. I'm not sure I'll be able to answer everything but, first, disclamer:

  • I am not an accountant, I mainly only do plain text accounting on my free time, so what follows is the "logical" understand that I have of it, and might not necessarily apply.

are you approving generating the label_compte and subledger_label based on the lastest label of the numero_compte and subledger_account number

I guess what we really want is the current value in the configuration (the only source where we don't have a possible confusion since they are unique).

In short, the bookkeeping entries were generated with labels coming from the AccountingAccount. They cannot be updated, but the new entries cannot have both values anyway so using the alledgedly corrected one is probably better.

Ideally, we would not have different label in the data model but on the technical side, it implies being able to modify previous bookkeeping records.

What to do when the value of opening_balance is 0 as it creates a bookkeeping entry for now. (I will get a return from my accounting expert this morning) We could add this on the SQL resquest or add a if before creating the booking line

No clue on my side, but having the empty bookeeping can serve as proof of processing (to check later that every account have been transfered) and reference (origin point, if we want to compare exercices). Maybe this still has a usage?

@josett225
Copy link
Contributor

josett225 commented Nov 28, 2024

I will submit another PR to accelerate the fix whatever solution will be chosen.

@alexandre-janniaux
Copy link
Contributor

Thanks a lot for the efforts, that's really appreciated. 🙏

@aspangaro aspangaro closed this Dec 15, 2024
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