-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Manage EOL date by checkbox of eol_explicit #14248
base: develop
Are you sure you want to change the base?
Manage EOL date by checkbox of eol_explicit #14248
Conversation
not necessary, EOL is managed by AssetsConroller
Improvement to manage 'EOL explicit' for create or update asset
Improvement to manage 'EOL explicit' for bulk update of assets
Improvement to manage 'EOL explicit' for create or update asset via API
Manage 'EOL explicit' checkbox
Manage 'EOL date' checkbox
Added description for 'EOL date help'
PR Summary
|
I'll work on getting this reviewed this week. Thanks @Robert-Azelis! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to actually keep the behavior the same on the API, just allowing the eol_explicit
option to be set manually. Let me know if that doesn't make sense to you.
/** | ||
* rule of set up EOL date and explicit status during create asset | ||
*/ | ||
if ($request->has('asset_eol_date')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this, I think I'd like to still do a diffInMonths()
to make sure it is explicit, unless they are also sending eol_explicit
in the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For create new asset via API (store()
method), I think it is ok, not required to use function diffInMonths()
addtionally for check. In this case we leave decision for user. Using attribut asset_eol_date
user want to set eol date manully, out of the model eol rule. This same if is used eol_explicit
attribut with value 1
. In both cases it's intentionally user's action.
Otherwaise applyed is asset model eol rule if purchase date filled and model eol rate greaten then zero.
/** | ||
* rule of set up EOL date and explicit status during update asset | ||
*/ | ||
if ($request->filled('asset_eol_date')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above comment on the store()
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For update asset via API (update()
method), similar as it is for store()
method.
Only one difference is to check, what is current status of eol_excplicit
set under asset to make update asset_eol_date
or not, if purchase date or model is changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use this modification by some time (last few months) no issue so far 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Robert-Azelis , while this method is working for you, we have lots of people that may be using this feature differently, so need to make sure that any change is going to work for as many people as possible.
If someone isn't aware of the changes, they might not be aware that they need to send that explicit value now - making their data invalid or misleading.
If we can do the diffInMonths()
check than they can continue to just send us asset_eol_date
and still have a reliable value in eol_explicit
and we can then also allow them to set that manually if they'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @spencerrlongg , thank you for your comment, but still don't catch your poin.
Argument eol_explicit
is optional used only if user would like to force manually set explicity marker, independents to other arguments (much more usefull for switch on/off eol_explicit
during update asset - it's not in this part of code.
The main triger is usage of asset_eol_date
argument, if has been provided during create or update asset it means user would like to set date manually, not based on automatically calculation. It can be also used in scenario when purchase date is not set, but user would like to keep EOL.
Can you describe scenario or propose exact changes in code (how you would like to use diffInMonths()
function)?
I'm open for improvements, but need to good understand how it is expected for use :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @spencerrlongg, no feedback from you so far, if proposed changes in API/AssetsController.php are so difficult to accept, we can skip it at this moment, allow to implment other changes and will return to this part under new PR, what do you think about this?
I don't want to keep on hold this PR.
Added 'eol_explicit' to show for request
no feedback so far, @snipe can I ask for review? |
Description
[ It's re-created PR #13846 as I made adaption code to new version 6.3.0 , sorry for confusion ]
This improvement allow setup EOL date manually by user on damand by set eol_explicit checkbox.
BEFORE:
AFTER:
If checkbox is not checked then EOL date is hiden and calculation of date is on the base Model EOL rate
If checkbox is checked then EOL date is showed and user can input EOL Date manually.
Status of checkbox is saved as eol_explicit value
In this way is much easier to manage EOL date and explicit marker status instead of build complex rules :)
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: