-
-
Couldn't load subscription status.
- Fork 1.5k
Main add history on consumables and cartridge #21618
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
base: main
Are you sure you want to change the base?
Main add history on consumables and cartridge #21618
Conversation
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.
Please add tests
src/Cartridge.php
Outdated
| $changesCartrige = [ | ||
| 0, | ||
| '', | ||
| __('Cartridge') . ' (' . $input['id'] . ') : ' . strtolower(__('Back to stock')), |
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.
It's probably easier to read and translate to use one sentence with replacements rather than a string concatenation:
| __('Cartridge') . ' (' . $input['id'] . ') : ' . strtolower(__('Back to stock')), | |
| sprintf(__('Cartridge (%1$s): back to stock', $input['id']), |
(same applies for other cases in this PR)
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.
Thanks for your return, I realized the change
src/Consumable.php
Outdated
| switch ($itemtype) { | ||
| case 'User' : | ||
| $model = new User(); | ||
| $model->getFromDB($items_id); | ||
| $name = strtoupper($model->fields['name']); | ||
| break; | ||
| case 'Group' : | ||
| default: | ||
| $model = new Group(); | ||
| $model->getFromDB($items_id); | ||
| break; | ||
| } |
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.
`switch` seems useless:
| switch ($itemtype) { | |
| case 'User' : | |
| $model = new User(); | |
| $model->getFromDB($items_id); | |
| $name = strtoupper($model->fields['name']); | |
| break; | |
| case 'Group' : | |
| default: | |
| $model = new Group(); | |
| $model->getFromDB($items_id); | |
| break; | |
| } | |
| $item = getItemForItemtype($itemtype); | |
| $item->getFromDB($items_id); |
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.
Thanks for your return, I realized the change
src/Consumable.php
Outdated
| [ | ||
| 0, | ||
| '', | ||
| $this->getPreAdditionalInfosForName() . ' (' . $ID . ') ' . strtolower(__('Given to')) . ' : '. $model->fields['name'] . ' (' . $ID . ')', |
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.
Isn't the itemtype information missing here?
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.
Adding the itemtype allows for better understanding, I hadded the information
|
@trasher Hi, thanks for your returns, which types of tests you want here ? Thanks |
Just make sure logs table contains expected information after each impacted step. |
Checklist before requesting a review
Please delete options that are not relevant.
Description
Hello,
my client want to have more logs about consumables and cartridges.
The code in this PR is to add logs when :