[Koha-bugs] [Bug 21159] Update item shelving location (952$c) on checkout

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21159

Tomás Cohen Arazi <tomascohen at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tomascohen at gmail.com

--- Comment #87 from Tomás Cohen Arazi <tomascohen at gmail.com> ---
I'm pushing this one.

I appreciate the effort to refactor things in a good way and thanks for that.

I have some remarks, though. Those I feel deserve to be treated on their own
bugs but I'll post here for now.

1. I think the method name should be changed to something like
`location_update_trigger`. I hope it is clear the why the name change proposal.

2. I'm not sure this method should internally call ->store. Calling it triggers
a series of things like logging, and maybe others. It feels like the caller
should _do all the changes to the item they need and then finally save_.

3. I don't feel like the return value is cool or either documented enough. I
recognize this is inherited from the original code in the C4 namespace. Moving
to Koha:: should be part of cleaning and improving, so more thought is needed
here. At a very first sight, I'd say this shouldn't be calling ->store
implicitly, and should be returning $self for method chainability. If we need
to return some messages for the caller to be aware of, we already have a
standard way to do it, using the Koha::Object::Message class, which is
encapsulated inside Koha::Object which Koha::Item inherits from (e.g. see
Koha::Object->add_message).

-- 
You are receiving this mail because:
You are watching all bug changes.