Skip to content

Conversation

@Godmartinz
Copy link
Member

This is still a work in progress, but take a look as I think the bones of this PR are good.

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

A couple notes from me skimming the code and then trying it out.

It feels weird to mix and match bulk checkout and bulk transfer in one action to me. Under the hood they are very similar so maybe I'm just getting hung up on it but it seems like there should be two menu items: one for bulk checkout and one for transfer. If we keep it under one menu item then we should let the user know which items they have selected are currently checked out and will be transferred. Just for their information.

public function onAssetsTransferredInBulk(AssetsTransferredInBulk $event): void
{
Log::debug('event passed to the listener:');
$event->transferable->logCheckout($event->note, $event->transferredTo, $event->transferable->last_checkout, $event->originalValues, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hit an exception here: Property [last_checkout] does not exist on this collection instance.

* 3. The item should send an email at check-in/check-out
*/

if (Context::get('action') === 'transfer') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be bulk_transfer right?

* 3. The item should send an email at check-in/check-out
*/

if (Context::get('action') === 'transfer') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be bulk_transfer right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants