Skip to content

FIX: correctly calculate prices in supplier context + remove warnings from calcul_price_total() #34360

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

Open
wants to merge 1 commit into
base: 18.0
Choose a base branch
from

Conversation

marc-dll
Copy link
Contributor

@marc-dll marc-dll commented Jun 5, 2025

FIX correctly calculate prices in supplier context + remove warnings from calcul_price_total()

In many Dolibarr instances we host, the dolibarr.log file is flooded with warnings related to price calculation :

Price.lib::calcul_price_total Warning: function is called with parameter seller that is missing

To lighten the log file, I searched all instances of calcul_price_total() (itself being called by CommonObject::update_price()), and explicitly passed the $seller argument.

This warning was effective as in most of the supplier part of Dolibarr, it was incorrectly assumed to by $mysoc (which can lead to incorrect local tax amounts if I remember correctly), so I passed the supplier instead.

For expense reports, I passed a modified version of $mysoc as was already done in some places of the ExpenseReport class.

A further step in the future would be to make the $seller argument mandatory in both CommonObject::update_price() and calcul_price_total().

@eldy
Copy link
Member

eldy commented Jun 5, 2025

As this PR mostly fixes hidden warning and not visible bugs, it should be pushed in develop.

@rycks
Copy link
Contributor

rycks commented Jun 6, 2025

As this PR mostly fixes hidden warning and not visible bugs, it should be pushed in develop.

not "visible" for who ? as marc i'm in the same situation : we are flooded with tons of lines in log then we make a "grep -v" when we have to analyse logs but that's a bug in adminsys point of view and really visible ... and log file size could become a problem, for me that's clearly a target of a long term support version

@marc-dll
Copy link
Contributor Author

marc-dll commented Jun 6, 2025

@eldy Well, those warnings are so much well "hidden" that they are everywhere in the logs of most of our customers! I'm really surprised nobody catched them before, they come from the core for the most part, as our customers don't use much external modules. The PRs I suggested this week all come from a simple task I had never done before in 8 years of working with Dolibarr : monitor log file size, read heavy ones and see what you can find there.

Ans so here we are, I mean, I use the word "flooded" properly : in the state of the code before this PR and all others of the same kind (which you merged, by the way), we are speaking about megabytes of output in dolibarr.log in LOG_WARNING level in just several days of time. About this particular PR, any CRUD operation on customer or supplier document lines (or expense report lines) trigger it, and they happen everyday even for small Dolibarr instances.

As such, I'm convinced it unnecessary heavy logs prevent us developpers from catching others bugs, that are less log-intrusive but potentially more impactful, of instances of the same bug but coming from external modules.

This PR doesn't add anything new and/or structural, it doesn't need going through the upgrade process, it doesn't modify existing documents, it doesn't discriminate instances based on the hosting conditions, modules used, etc. It's only simple fixes, pushing towards better code quality, cleaner instance management, easier bug discovery, so it fully has its place in a stable version. Especially version 18.0, which is engaged in a long-term support process.

Above all, it contains a fix about an issue in tax calculation, which is in itself a very serious topic. I would not want for our community to be liable for any financial damage done to a fiscal administration or a Dolibarr user due to this oversight.

@eldy eldy added the PR no feature/refacto into freezed/released branch PR can't be accepted because it's a feature change or refactoring pushed into a stable frozen branch label Jun 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR no feature/refacto into freezed/released branch PR can't be accepted because it's a feature change or refactoring pushed into a stable frozen branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants