Dolibarr / dolibarr

Dolibarr ERP CRM is a modern software package to manage your company or foundation's activity (contacts, suppliers, invoices, orders, stocks, agenda, accounting, ...). it's an open source Web application (written in PHP) designed for businesses of any sizes, foundations and freelancers.
https://www.dolibarr.org
GNU General Public License v3.0
5.47k stars 2.8k forks source link

VAT code for all products which is removed from a document (proposal / invoices / command ) when adding a global reduction #29192

Closed donowr closed 4 months ago

donowr commented 7 months ago

Bug

Good morning,

I have a problem with the VAT accounting codes on the lines of a document (invoice/proposal or other).

When I add a global reduction to a document it removes the VAT code from each line.

I tried it on a blank Dolibarr.

I also found how to fix the problem by adding a small piece of code in each card.php of a document.

Here is the line that I added temporarily in all card.php:

$line->tva_tx= $line->tva_tx.' ('.$line->vat_src_code.')'; 

This line was added in the following condition in each in card.php: I dont think it is the best way to patch that.

} elseif ($action == 'addline' && GETPOST('submitforalllines', 'alpha') && GETPOST('remiseforalllines', 'alpha') !== '' && $usercancreate) {
        // Define a discount for all lines
        $remise_percent = (GETPOST('remiseforalllines') ? GETPOST('remiseforalllines') : 0);
        $remise_percent = str_replace('*', '', $remise_percent);
        foreach ($object->lines as $line) {
            $line->tva_tx= $line->tva_tx.' ('.$line->vat_src_code.')'; // temp FIX
            $result = $object->updateline($line->id, $line->subprice, $line->qty, $remise_percent, $line->tva_tx, $line->localtax1_tx, $line->localtax2_tx, $line->desc, 'HT', $line->info_bits, $line->special_code, $line->fk_parent_line, 0, $line->fk_fournprice, $line->pa_ht, $line->label, $line->product_type, $line->date_start, $line->date_end, $line->array_options, $line->fk_unit, $line->multicurrency_subprice);
        }
} elseif ($action == 'addline' && GETPOST('submitforallmargins', 'alpha') && GETPOST('marginforalllines') !== '' && $usercancreate) {

Dolibarr Version

18.0.5

Environment PHP

8.1.27

Environment Database

Mysql 8.3.0

Steps to reproduce the behavior and expected behavior

Apply a discount to all lines of a document:

image_2024_04_02T07_02_50_280Z

Output:

image_2024_04_02T07_03_15_261Z

Attached files

No response

JonBendtsen commented 7 months ago

@donowr why don't you think it is the best way to patch it?

donowr commented 7 months ago

@JonBendtsen

Because it does not solve the problem at its source. Instead of fixing the code that handles the application of global discounts so that it correctly preserves the VAT discount codes, my solution is to manually reinsert the VAT codes after they have been deleted.

While this may temporarily resolve the issue, it does not resolve the underlying cause of the issue.

A more robust approach would be to identify and fix the part of Dolibarr's code that removes VAT codes when applying global discounts.

This would ensure correct and consistent behavior in all situations, without requiring additional fixes in the future. In summary, my solution is a temporary solution that works around the problem, but the real solution would be to modify the Dolibarr code so that it works correctly from the start.

donowr commented 7 months ago

I found where the problem comes from.

The $object->updateline() function uses the content of the $txtva parameter to extract the VAT code:

 // Clean vat code $vat_src_code = ''; 
$reg = array(); 
if (preg_match('/\((.*)\)/', $txtva, $reg)) {
   $vat_src_code = $reg[1]; 
   $txtva = preg_replace('/\s*\(.*\)/', '', $txtva); // Remove code into vatrate. 
} 

However, I looked in $object and it turns out that not all the lines in the documents contain the VAT code when the object is instantiated: php $object->line[x]->tva_tx .

I tried to add the VAT code when the object retrieved the document lines but this is not the right solution because the frontend does not handle it when the VAT code is in $line->tva_tx .

My first solution remains valid but I have a second solution to propose.

We can modify the $object->updateline() function so that it takes the VAT code as a parameter. By default the parameter would be False, this will allow the basic logic to be preserved in the case where the parameter was not provided.

JonBendtsen commented 7 months ago

We can modify the $object->updateline() function so that it takes the VAT code as a parameter. By default the parameter would be False, this will allow the basic logic to be preserved in the case where the parameter was not provided.

@donowr I like that, but I have no authority to say

I wonder if there are more places than proposal/order/invoice (propal/commande/facture) where it needs to be fixed

JonSweet16:dolibarr jonbendtsen$ grep -irl '$object->updateline('
./htdocs/variants/card.php
./htdocs/bom/bom_card.php
./htdocs/fourn/commande/card.php
./htdocs/fourn/facture/card.php
./htdocs/fourn/facture/card-rec.php
./htdocs/commande/card.php
./htdocs/compta/facture/card.php
./htdocs/compta/facture/card-rec.php
./htdocs/expensereport/card.php
./htdocs/comm/propal/card.php
./htdocs/supplier_proposal/card.php

JonSweet16:dolibarr jonbendtsen$ grep -irl 'function updateline('
./htdocs/contrat/class/contrat.class.php
./htdocs/variants/class/ProductAttribute.class.php
./htdocs/bom/class/bom.class.php
./htdocs/fourn/class/fournisseur.commande.class.php
./htdocs/fourn/class/fournisseur.facture.class.php
./htdocs/fourn/class/fournisseur.facture-rec.class.php
./htdocs/commande/class/commande.class.php
./htdocs/compta/bank/class/api_bankaccounts.class.php
./htdocs/compta/facture/class/facture-rec.class.php
./htdocs/compta/facture/class/facture.class.php
./htdocs/expensereport/class/expensereport.class.php
./htdocs/comm/propal/class/propal.class.php
./htdocs/supplier_proposal/class/supplier_proposal.class.php
donowr commented 7 months ago

@JonBendtsen

Who has the authority to tell me what to do? This is my first contribution and I would like to go through to the end, I don't know the process either...

Concerning the places where the modification must be made I cannot answer you with certainty but I think you have listed them all.

I'll ask a colleague this tomorrow

JonBendtsen commented 7 months ago

@JonBendtsen

Who has the authority to tell me what to do? This is my first contribution and I would like to go through to the end, I don't know the process either...

@donowr I think that you can definitely get a final decisive answer from @eldy

donowr commented 7 months ago

@JonBendtsen I have to wait waitfor @eldy ​​to give his answer here?

JonBendtsen commented 7 months ago

@JonBendtsen I have to wait waitfor @eldy ​​to give his answer here?

I think it is either that or submit a pull request

eldy commented 6 months ago

It is better to add the ' ('.$line->vat_src_code.')' In the 5th parameter of update method, concatenated to line->tva_src, instead of modifying the value $line->tva_tx. So line->tva_tx keeps its numeric value.

Can you submit a Pull Request in this way ?

donowr commented 6 months ago

hello, I did a pr but the phan test failed, is this disturbing?

How do I resolve the problem so that my pr passes all the tests?

JonBendtsen commented 6 months ago

hello, I did a pr but the phan test failed, is this disturbing?

How do I resolve the problem so that my pr passes all the tests?

that depends - was it your fault or was it something that you did not change?

JonBendtsen commented 6 months ago

@donowr if you can update your merge request to a newer base version (there should be a button in the bottom of the merge request page if possible) then use that possibility. Even if there is not such a thing, come back regularly and update which triggers a new build and maybe someone else fixed the phan error.

But it might just be your code that has the issue, so check where you edited and what, and correlate with the error message in phan.

donowr commented 6 months ago

@JonBendtsen here is the phan error, unless I'm mistaken I don't think it refers to one of my modifications. Additionally I found a similar error on another pull request : https://github.com/Dolibarr/dolibarr/pull/29481#partial-pull-merging

image

JonBendtsen commented 6 months ago

@JonBendtsen here is the phan error, unless I'm mistaken I don't think it refers to one of my modifications. Additionally I found a similar error on another pull request : #29481 (comment)

image

good, then i'd suggest that you keep updating when possible until all checks are clean, then you leave it.