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

zero could be a value #32616

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

rycks
Copy link
Contributor

@rycks rycks commented Jan 10, 2025

fix #32615 (i hope) :-)

there is a race condition with zero because empty(0) is true ... then we did not display the value

@rycks
Copy link
Contributor Author

rycks commented Jan 10, 2025

please wait i forget to do the "update/edit" part of the problem

image

but on clic on 'update' the value will become ''

image

@rycks
Copy link
Contributor Author

rycks commented Jan 10, 2025

the last commit do the job for edit step ... but there is something funny now when i try to update the row with "code zero" :

image

@hregis
Copy link
Contributor

hregis commented Jan 10, 2025

@rycks maybe the unique key for the field

@rycks
Copy link
Contributor Author

rycks commented Jan 10, 2025

@hregis No ... that is really a test :

image

now i have to find the reason why zero is prohibited and why some dictionnaries use that number :-)

@rycks
Copy link
Contributor Author

rycks commented Jan 10, 2025

@hregis ok but tinyint could store (int)0 as a value ... no ?

i dig into commit history, here is the origin of the test: 20 years before :-)

thanks to comments we could understand that race condition : zero and '' could not be desactivated entries ... that's not really what nowdays code do (without comment i can't imagine the real target of that code in current version) ...

image

@lvessiller-opendsi lvessiller-opendsi self-requested a review January 16, 2025 11:12
Copy link
Contributor

@lvessiller-opendsi lvessiller-opendsi left a comment

Choose a reason for hiding this comment

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

I'm ok with this fix.
@rycks
Have you something else to add for the error message when you update the dictionary line with "0" as code ?

@eldy
Copy link
Member

eldy commented Jan 20, 2025

The protection is here because we want to avoid to have 0 as a code (it generates a lot of trouble).
Here we have a case where legacy code was using 0 as code, so we must fix something to allow this.
However, the fix suggested is a generic fix that allows the 0 as code everywhere, so it breaks the protection everywhere, when it should remove the protection only for the exception case.

@eldy eldy added the PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do) label Jan 20, 2025
@rycks
Copy link
Contributor Author

rycks commented Jan 20, 2025

@eldy ok, but if the original code was set to zero into sql import from install can user change that value to something else ? that question is to make a good fix.

First idea: if code was zero -> read-only cell & value then it could become a "generic" solution

@eldy
Copy link
Member

eldy commented Jan 20, 2025

@eldy ok, but if the original code was set to zero into sql import from install can user change that value to something else ? that question is to make a good fix.

First idea: if code was zero -> read-only cell & value then it could become a "generic" solution

We shoul dnot be able to change for the dictionnary "nature of product".

This property is hard coded at different place in application with some test if (nature == 0) ... or if (nature == 1) then ... so for this dictionnary the value 0 and 1 should not be editable (In the past this was not a dictionnary, dictionary was introduced to allow addition of a third value).
In v20 and v21, it seems this has been fixed by blocking edition of this two lines that are used as hard coded value and only addition of other value is possible.

image

and i pushed a fix to have the 0 visible as "0"

image

So i think best fix is to try to retreive same changes done into v20 if we can (if the "always active" is hard coded, in this case no problem to backport, if it uses a new entry in database, in this case we must make a different fix).

@rycks
Copy link
Contributor Author

rycks commented Feb 10, 2025

@eldy solution backported from 20.0

image

@eldy eldy merged commit 09df626 into Dolibarr:18.0 Feb 11, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants