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 Bug Estimated Value stockatdate.php #23782

Closed
wants to merge 3 commits into from

Conversation

josett225
Copy link
Contributor

FIX # Bug Estimated Stock at date value

In stockatdate page, the estimatedvalue is wrong. In fact the calculated value in the SQL request is the current stock value and not the stock at date value.
P.S : I changed in code estimatedvalue to currentvalue and kept it for future use (other PR) as it is in interesting to get a comparison between stockatdate value to currentstock value.

@eldy
Copy link
Member

eldy commented Feb 5, 2023

It seems bug was fixed inside v17 but in a different manner.
Can you check to backport the way it was fixed in v17 (using same name as possible for the lines that are modified) to avoid conflict ?

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

josett225 commented Feb 6, 2023

Hi @eldy
Sorry that's right I found the fix in v17 (version I did not tested initially as still in beta if I am not wrong).
I can of course apply the same change in my fix but I have 2 concerns here :

  • One is the fact estimatedvalue in the SQL request is not an estimated value. It is the current stock value.
  • Second : I reused the $estimatedvalue variable later in the code to calculate the right estimated stock value once and to avoid to do 3 times the same calculation as seen in V17. The code is slow so removing a bit a process could help.

Don't you think It could be better to clean v14 and above to avoid the 2 concerns above?

I will do anyway what you will recommend.

@eldy
Copy link
Member

eldy commented Feb 10, 2023

To avoid conflict and regression the maximum we can, and keep code analysis of past changes easier as possible, changes on old version must be lower as possible to fix the bug for the end user.
If you have improvement (renaming variable, or reuse), it is better to push this into develop branch.

@eldy eldy changed the base branch from 14.0 to develop February 19, 2023 02:01
@@ -245,12 +245,12 @@

$sql = 'SELECT p.rowid, p.ref, p.label, p.description, p.price, p.pmp,';
$sql .= ' p.price_ttc, p.price_base_type, p.fk_product_type, p.desiredstock, p.seuil_stock_alerte,';
$sql .= ' p.tms as datem, p.duration, p.tobuy, p.stock, ';
$sql .= ' p.tms as datem, p.duration, p.tobuy, p.stock, p.pmp, ';
Copy link
Member

Choose a reason for hiding this comment

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

Field p.pmp seems already added 2 lines before...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @eldy
I will review the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @eldy
Sorry for having been so late.

In version 14.0, I just reviewed the latest code and there is no p.pmp in line 246 (or 245 to be exact).
I will anyway make the same change as seen in v17 or even v19 if you want.

I will also submit a new PR on develop to change the estimatedvalue to currentvalue in the SQL request to clear the code.
Let me know your decision.

This reverts commit 0b1c623, reversing
changes made to c45581a.
josett225 added a commit to josett225/dolibarr that referenced this pull request Nov 7, 2023
In stockatdate page, the estimatedvalue is wrong. Backport of V17 solution as explained in Dolibarr#23782
josett225 added a commit to josett225/dolibarr that referenced this pull request Nov 7, 2023
As commented in Dolibarr#23782 this update is to clean and clarify the code for future evolution of stocktodate.
In fact the calculated value in the SQL request is the current stock value and not the estimated stock value at date value.
Therefore I changed in SQL request the estimatedvalue to currentvalue and reused as a variable for a better understanding and for future use (other PR) as it will be interesting to get a comparison between stockatdate value to currentstock value. Also some others changes are required to improve the page speed.
@josett225
Copy link
Contributor Author

I will close this PR as it has been replaced by #26479 for v14 and #26481 for V19

@josett225 josett225 closed this Nov 7, 2023
eldy pushed a commit that referenced this pull request Nov 7, 2023
As commented in #23782 this update is to clean and clarify the code for future evolution of stocktodate.
In fact the calculated value in the SQL request is the current stock value and not the estimated stock value at date value.
Therefore I changed in SQL request the estimatedvalue to currentvalue and reused as a variable for a better understanding and for future use (other PR) as it will be interesting to get a comparison between stockatdate value to currentstock value. Also some others changes are required to improve the page speed.
eldy pushed a commit that referenced this pull request Nov 7, 2023
In stockatdate page, the estimatedvalue is wrong. Backport of V17 solution as explained in #23782
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.

2 participants