search

Vauxoo / addons-vauxoo

All our modules related to developments that solves generic issues on Odoo, or that solve internal problems on Odoo Core, if something is here, maybe it is solving an issue in your company, try it and report what you see.
http://www.vauxoo.com
193 stars 288 forks source link

[issue#9479] [FIX] partner_credit_limit: avoid error `OverflowError: date value out of range` #1405

Closed edy1192 closed 4 years ago

edy1192 commented 4 years ago

Since the grace_payment_days field is a float, in some cases the user can set really high values (> 999999) which causes the traceback OverflowError: date value out of range.

Since that field is used for date operations in the debit_credit_maturity method of the res.partner model.

issue

When the user see the contact, it gets the following traceback

image

edy1192 commented 4 years ago

Hi @luisg123v @moylop260 Could you check this, please?

Regards.

edy1192 commented 4 years ago

Hi @JulioSerna Could you check this please?

I made this PR to avoid the traceback that the user of DHM reported, (Vx issue 9479). I am going to create test instances for DHM and TyP which are the customers that I know have this module (in v11)

cc @luisg123v @moylop260

moylop260 commented 4 years ago

Big Value means disabled?

Why the user is using a big value?

For me a big value is disabling the feature really. What about allow -1 in order to disable it?

sql constraint instead

If you can use an sql_constraint instead of an odoo constraint better using sql_constraint. It is faster and it allows to validate the database layer too. You can check similar examples here:

So you can create a check to validate -1 to 999999 values.

What do you think?

task?

Could you add in the title the number of task/ticket of this change?

moylop260 commented 4 years ago

Check if the widget from fields definition allow use a particular quantity of digits in this case 6 In order to validate it from UI layer too. So you will have validate it from: GUI, DB, PY

luisg123v commented 4 years ago

Hi @moylop260

An SQL constraint wouldn't work here, because the involved field is company-dependent, so it's not stored, at least not in this table.

moylop260 commented 4 years ago

Hi @luisg123v You have a good point, thanks for clarify it.

@edy1192

Could you change simply the validation, please? if not 0 < record.grace_payment_days < 999999: raise...

moylop260 commented 4 years ago

Question: Why grace_payment_days is a float instead of integer if it is talking number of days?

luisg123v commented 4 years ago

Question: Why grace_payment_days is a float instead of integer if it is talking number of days?

I was wondering the same when I noticed it :confused:

Dear customer, you have a grace period of 0.5 days

moylop260 commented 4 years ago

@hugho-ad Should we change to Integer instead of Float?

moylop260 commented 4 years ago

@JulioSerna your comments are welcome too!

edy1192 commented 4 years ago

Why grace_payment_days is a float instead of integer if it is talking number of days?

Hi @moylop260 Same question :( Maybe it's because the customer can have a day and a half or half a day? I don't know really.

Could you change simply the validation, please? if not 0 < record.grace_payment_days < 999999: raise...

Done

Could you add in the title the number of task/ticket of this change?

Done. It is worth mentioning that the ticket is closed because it was resolved by setting the field value to an acceptable value and the cause of the error was told to the client.

cc @luisg123v

moylop260 commented 4 years ago

Ok, I got it

Consider for another PR use a -1 value (I'm not sure yet. I need the opinion of @hugho-ad @JulioSerna @luisg123v)

moylop260 commented 4 years ago

...and the float to integer and force number of digits Low priority

luisg123v commented 4 years ago

@moylop260

Consider for another PR use a -1 value If you mean allowing -1 as a valid value, it would be pointless, because with 0 the customer already has no gracing days, i.e. neets to pay immediately.

hugho-ad commented 4 years ago

I'm more with attr size of the field

we can define the size of the field.Float as following https://github.com/odoo/odoo/blob/12.0/addons/l10n_it_edi/models/account_invoice.py#L42 and avoid user set more than 6 digits. (I don't know if decimals included, we must check)

Changing the Float by an integer it's a good option too, but I would prefer to keep it as Float because it gives us the chance to handle a portion of a day, including hours left, maybe if we take the decimals as the hours.

Regards!

luisg123v commented 4 years ago

AFAIK, @edy1192 tried using:

edy1192 commented 4 years ago

I'm more with attr size of the field

we can define the size of the field.Float as following https://github.com/odoo/odoo/blob/12.0/addons/l10n_it_edi/models/account_invoice.py#L42 and avoid user set more than 6 digits. (I don't know if decimals included, we must check)

Changing the Float by an integer it's a good option too, but I would prefer to keep it as Float because it gives us the chance to handle a portion of a day, including hours left, maybe if we take the decimals as the hours.

Regards!

Hi @hugho-ad

As @luisg123v comments, I did tests using size and digits but saw no change.

Here a video with my tests (data demo) https://youtu.be/-zxF-64eQ9I

Regards.