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

Fixed #12531 Expected Checkin Date on Asset Checkout throws an error #12537

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

inietov
Copy link
Collaborator

@inietov inietov commented Feb 19, 2023

Description

When an asset get checkout, the user can set a date for the day they expected the asset to got checkin. We made a couple days ago that the expexted_checkin value came in the form Y-m-d but whenever this value is set, the validation always fails. This was caused because for some reason, Laravel put the asset object together with that date as format Y-m-d H:i:s
image
So I tested various forms to make this works as expected. And my results where this:

  • Change the validation rule from: 'expected_checkin' => 'date_format:Y-m-d|nullable', to 'expected_checkin' => 'date_format:Y-m-d H:i:s|nullable',. This way the validation pass and the date still is saved as Y-m-d in the database, but I don't like that we are expressing in the code something we really don't like.
  • Remove the expected_checkin field from the protected $dates array. This way the validation pass but I don't like that we are not putting a date field with the others, and also we have dates in this array that fulfill the needed format.
  • Put a setter in the Asset model class, as the purchase_date have in SnipeModel model class. I read the code and see this field have the same format as we need. It's defined in the Snipe model class because the purchase_date field is common to a lot of types of assets. So I put it in the Asset model class because the expected_checkin field only is used in asset types. We can see that using the setter, the object is formed with this attribute in the correct format.
    image

As always let me know if there is a more elegant/simpler solution. This was the only way I could figure out.

Fixes #12531

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Test Configuration:

  • PHP version: 8.2
  • MySQL version: 8.0.31
  • Webserver version: PHP dev server
  • OS version: Debian 11

@what-the-diff
Copy link

what-the-diff bot commented Feb 19, 2023

  • Added a setExpectedCheckinAttribute function to the Asset model.
  • This is used for setting expected checkins as Y-m-d format, and if it's empty then null will be returned instead of an error message being thrown by Laravel.

@snipe
Copy link
Owner

snipe commented Feb 19, 2023

I'm just not clear on why we would need this workaround, when we don't need it anywhere else. :-/

@inietov
Copy link
Collaborator Author

inietov commented Feb 19, 2023

I'm not sure either... but the only other date field I can found with the same format was purchase_date and it does use a setter:
image

@svpernova09
Copy link
Contributor

Latest comment on the issue this fixes is that it's already fixed on master. Do we still need this change or can it be closed?

@snipe snipe merged commit 5a1e5f7 into snipe:develop Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants