Support for inventory date

I've pushed up a branch here: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/khuckins/lp1777675-inventory-date-support

We've added the two new fields, inventory_date and inventory_workstation, and added information necessary for the API. The checkin API handles updating the new fields when the checkbox added to the checkin interface is activated.

Thanks Kyle!

I don't see anything in your branch that adds the two new fields to the database, either through an upgrade script or the baseline schema.

An example showing how this was done when another field was added to the database can be found at:

http://git.evergreen-ils.org/?p=Evergreen.git;a=commit;h=1cedb423902810e9df5355ab18c7ca7e18d5391d

The changes to 090.schema.action.sql and the ugprade script in XXXX-create-transit-cancel-time-column.sql were the pieces that added the new field to the database.

Kathy, Kyle-

Would you be open to recording inventory actions in a distinct table, rather than adding new fields to the asset.copy table which only contain the most recent inventory data?

-jeff

Whoops, I added them through pgadmin, the sql file itself slipped my mind. Thanks for the example Kathy! I've pushed another commit on top of this branch for the sql additions.

Jeff,

I'm open to different approaches, but, to be honest, this is the kind of feedback I would love to get when MassLNC shares its requirements for upcoming development projects to the general list or even when the LP bug is first submitted. I'm still trying to figure out a good way to do so in a way that encourages this type of discussion before we move on to the next step of contracting with devvelopers.

Aside from that, to give some background on this project, there have been many inventory projects discussed over the years, but none have been implemented because of the large cost of the project to do a full inventory module. We also did not have the resources to pursue a larger project. However, our thought was that these small additions to the database, along with being able to update the fields and view them from existing client interfaces, could go a long way toward adding some support for inventory, rather than pushing off this project again to some future time when we can do something larger.

For our purposes, the most recent inventory data is what our staffs are most interested in storing when they are performing inventory in the system. Is there a use case for storing dates that go further back? What are the advantages you see for adding them to a different table? Would you want to see those older inventory dates/workstations from the client?

Hi Kathy and Jeff,

I would like to second storing the data in a separate table. The functionality could remain the same (even to the point of actively purging previous scan data, perhaps controlled by YAOUS), and copy+scan_time would be a strict subset of a future, full inventory function. It would also reduce churn on the copy table, which is already subject to churn and bloat, and make a possible future implementation simpler -- no need to forcibly copy data to the asset.copy table, or change code and schema during an upgrade.

As an implementation detail, putting the new table in either the action schema, or a new inventory schema, would make sense (but would not be strictly necessary -- we can move a table).

My $0.02...

I also dislike increasing churn on the copy table and like the idea of
breaking the inventory data into a separate table, for the same reasons
that Mike mentions. If doing this immediate inventory support is in part
to get us a few step forwards and create something to build on top of then
it makes sense to go ahead and think about something extensible. I have a
slight preference for a new inventory schema. Ideal inventory
functionality for things like recording inventory events, batches, etc...
would benefit from more tables and grouping them in a schema makes sense to
me.

On Thu, Jul 12, 2018 at 4:15 PM, Mike Rylander <email address hidden> wrote:

> Hi Kathy and Jeff,
>
> I would like to second storing the data in a separate table. The
> functionality could remain the same (even to the point of actively
> purging previous scan data, perhaps controlled by YAOUS), and
> copy+scan_time would be a strict subset of a future, full inventory
> function. It would also reduce churn on the copy table, which is
> already subject to churn and bloat, and make a possible future
> implementation simpler -- no need to forcibly copy data to the
> asset.copy table, or change code and schema during an upgrade.
>
> As an implementation detail, putting the new table in either the action
> schema, or a new inventory schema, would make sense (but would not be
> strictly necessary -- we can move a table).
>
> My $0.02...
>
> --
> You received this bug notification because you are subscribed to
> Evergreen.
> Matching subscriptions: evergreenbugs
> https://bugs.launchpad.net/bugs/1777675
>
> Title:
> Support for inventory date
>
> Status in Evergreen:
> New
>
> Bug description:
> Evergreen should have support for inventory date and workstation
> fields in the asset.copy table. Dedicated inventory fields would
> facilitate the process of performing inventory on a library's
> collection and can also be leveraged for potential future development
> of a full inventory module.
>
> We propose adding the current date and workstation to these fields in
> the following places:
> - From the check in screen using a check in modifier.
> - From the item status screen
>
> Full requirements for this enhancement are available at
> http://masslnc.org/node/3379.
>
> The MassLNC Evergreen Development Initiative is working with Catalyte
> on this project.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1777675/+subscriptions
>

Thank you for the feedback Mike and Rogan. While it's good to hear there is consensus around this issue, I still would like responses to my previous question regarding immediate use cases for storing previous inventory data and if it needs to display in the client. This will help us in understanding how much the current implementation needs to be changed. If it's primarily to support possible future use, I'm assuming we don't need to worry about displaying previous inventory data in the client?

Regarding previous inventory data, I would imagine that it would be useful to be able to report on the inventory status of an item at any point in its past. That's certainly been a consideration in the previous rounds of discussion and planning. But that's secondary to my concerns about 1) making a more robust feature set harder to provide and 2) bloating the copy table with rarely used (if important) information. For example, if we provide an index over the workstation or scan time columns for reporting or search purposes, that will prevent HOT update (a postgres technology that reduces bloat and increases speed) of rows on the copy table when updating unindexed columns. The impact is small per row, but that table is one of the longest and most heavily updated tables we have, so the cumulative impact is high.

On an effort-related note, after having looked at the code, moving the two fields to a new table (along with a copy pointer and primary key), and even providing, say, an asset.last_copy_inventory view over the raw data to hide the history, should really be the work of maybe an hour. The logic doesn't need to change, just the specific syntax. I'm willing to make those changes if it comes to that.

I can see a use case for retaining and being able to see in the staff interface previous inventory dates. It is my understanding that K-12 libraries in Georgia are required to do yearly inventories. Having the inventory history of an item missing from the current inventory, and in an available status, would be useful for them.

However,if an item has an inventory date set for a previous inventory and is in status available and is NOT inventoried in a current inventory, the date from that previous inventory should be retained since it would not be scanned in the process. Staff could run reports to find those items with the inventory date set prior to the current inventory so that they could be further investigated.

While having a history of past inventories would be nice, I don't think that it is necessary. Staff can assume that if the item has the previous inventory date set that it was on the shelf for any prior ones. Running a report to catch items with the previous inventory date would be part of the cleanup for the inventory.

I do, however, understand Mike and Rogan's concerns about bloating the copy tables. I don't see where we would need to retain all inventory dates in any separate table. Just the last date inventoried can be retained.

Thanks, Elaine!

WRT inventory history, one problem with looking at the historical scan date alone, including just the "last" inventory time, is that without some sort of session and physical locality concepts to provide grouping, it can quickly become very difficult to identify (and even to analyze, short of some time-series visualization where a human looks at the peaks on a graph) when scans are nominally "concurrent" for an inventory process.

It's certainly useful to see that a copy has not been inventory-scanned within the last year and its status is Available, but things become murky when you want to see the state of the collection in real-time. If the scanning process, for a particularly large or particularly understaffed library, or even just particularly unlucky library, takes a while to complete -- perhaps done over the course of a few months -- then during that time you can't really use "last scan time" alone reliably to indicate presence or lack thereof.

The fact that those situations can't be reasonably covered by the simpler proposed feature does not mean I'm against the simpler feature existing, it's that I don't want the simpler feature to make the fuller feature significantly harder to engineer down the road by having to take the existence of fields (for reporting, etc) into account in a future implementation. If we make a reasonable guess about how just the data we need now would fit into a more full implementation, we can make our future selves happier.

(I can reiterate my database-level concerns more if that's unclear, but they require some preexisting understanding of how PG works internally. This bug probably isn't the forum for a PG-internals workshop -- I've provided tangents enough thus far. ;) )

[As a side note, with historical scans you can, at least to an approximation, investigate the collection by each run's bounding dates to see changes over time, such as identifying sub-collections with more "stock lossage" as a percentage of the part or the whole. That may not be important for annual reporting, but it seems it could be important for placement and security.

Of course, with the appropriate run and collection grouping mechanisms of the full proposal (granted, the cost is non-trivial) those sorts of reports and analyses are both natural and well-defined.]

Basically, have a single inventory date stored but in such a way that it
does not preclude later development to retain all inventory dates?

J. Elaine Hardy
PINES & Collaborative Projects Manager
Georgia Public Library Service/PINES
1800 Century Place, Ste. 150
Atlanta, GA 30045

404.235.7128 Office
404.548.4241 Cell
404.235.7201 FAX

On Fri, Jul 13, 2018 at 4:07 PM, Mike Rylander <email address hidden> wrote:

> Thanks, Elaine!
>
> WRT inventory history, one problem with looking at the historical scan
> date alone, including just the "last" inventory time, is that without
> some sort of session and physical locality concepts to provide grouping,
> it can quickly become very difficult to identify (and even to analyze,
> short of some time-series visualization where a human looks at the peaks
> on a graph) when scans are nominally "concurrent" for an inventory
> process.
>
> It's certainly useful to see that a copy has not been inventory-scanned
> within the last year and its status is Available, but things become
> murky when you want to see the state of the collection in real-time. If
> the scanning process, for a particularly large or particularly
> understaffed library, or even just particularly unlucky library, takes a
> while to complete -- perhaps done over the course of a few months --
> then during that time you can't really use "last scan time" alone
> reliably to indicate presence or lack thereof.
>
> The fact that those situations can't be reasonably covered by the
> simpler proposed feature does not mean I'm against the simpler feature
> existing, it's that I don't want the simpler feature to make the fuller
> feature significantly harder to engineer down the road by having to take
> the existence of fields (for reporting, etc) into account in a future
> implementation. If we make a reasonable guess about how just the data
> we need now would fit into a more full implementation, we can make our
> future selves happier.
>
> (I can reiterate my database-level concerns more if that's unclear, but
> they require some preexisting understanding of how PG works internally.
> This bug probably isn't the forum for a PG-internals workshop -- I've
> provided tangents enough thus far. ;) )
>
> [As a side note, with historical scans you can, at least to an
> approximation, investigate the collection by each run's bounding dates
> to see changes over time, such as identifying sub-collections with more
> "stock lossage" as a percentage of the part or the whole. That may not
> be important for annual reporting, but it seems it could be important
> for placement and security.
>
> Of course, with the appropriate run and collection grouping mechanisms
> of the full proposal (granted, the cost is non-trivial) those sorts of
> reports and analyses are both natural and well-defined.]
>
> --
> You received this bug notification because you are subscribed to
> Evergreen.
> Matching subscriptions: <email address hidden>
> https://bugs.launchpad.net/bugs/1777675
>
> Title:
> Support for inventory date
>
> Status in Evergreen:
> New
>
> Bug description:
> Evergreen should have support for inventory date and workstation
> fields in the asset...

Thanks for the feedback everyone!

Since this implementation is so small, I would prefer to see the new inventory fields added to a new table within the action schema rather than a new schema, particularly since Mike noted above that moving the table to a new schema should be easy enough if/when we build out inventory functionality.

Would there be any objection if, for the current implementation, we just store the most recent inventory date/workstation? If somebody then wants to store historical data, we could add the YAOUS at that time.

Let me know what you think.

I've learned to love incremental improvements, and think this proposal sounds fine. My only suggestion would be to give the table a suitably specific name to leave room for the fuller and more general future table(s). Think something like Mike's "asset.last_copy_inventory" rather than "asset.inventory", etc. Eventually going from single to multi-entry (which I support) will be a different shape for reporting reasons, so we won't really be able to extend the proposed table and keep reporting compatibility. We would instead at that point be talking about turning the existing table into a view over the expanded table, so perhaps we can save ourselves from awkward naming mismatches down the road and give it the view-like name today.

My two cents.

Dan

Kathy,

I am fine with storing the most recent inventory date/workstation

I've pushed a commit to the branch that refactors the previous additions, creating the new last_copy_inventory table under the asset schema. This is still WIP, and I'm running into some issues with updating the new inventory table through the do_checkin function. I think I'm missing something event-related , so despite the addition to the checkin_flesh_events subroutine's payload, entries won't get created or updated. Due to the way checkin functionality is implemented, going through the checkin API still seems to be the best way to handle updating the inventory. I'm having a bit of trouble wrapping my head around how to actually update this table in the Perl - any guidance on that would be much appreciated.

I figured out the issue I was running into, and I've pushed up an improvement to this branch(squashing down the WIP commit). All the interfaces referenced in the MassLNC requirements page have been edited to work with the new table addition.

Kyle,

There is a problem with the changes made to the 040.schema.asset.sql file that is preventing the asset schema from getting created. Nothing immediately jumps out at me as the source of the problem.

Kathy

I see the problem. The sql to create the new table references the asset.copy table, which hasn't been created yet. I'll post a new branch with a commit that fixes it.

Kathy

An issue with Reports, where the last copy inventory information doesn't display in the tree for the asset copy data, was pointed out to me. I'm wondering if it would be worth it add a nullable last_copy_inventory field to the copy class, mapping to the last copy inventory table entry associated with the copy. This does still somewhat bloat the copy table, but being a mapped field, it may be less of an issue. I wanted to probe for community input before going ahead with this change.

Kathy, please note that we can't use a real foreign key against asset.copy.id, it needs to follow the same pattern as other tables that link to a copy and use the inheritance-aware custom trigger.

Kyle, please don't do that. All that is needed is a might_have linked virtual field in the IDL. There's no need for an actual column.

Thanks Mike, looks like I was overthinking it. I've updated the branch to have the might_have linked virtual field, as well as point to a custom trigger rather than directly pointing to asset.copy.id.

This looks great Kyle! I just see one issue. On the check in screen, if you are showing the inventory date field, and use the Update Inventory check-in modifier, when you check in items, the current date/time shows as expected. However, if you wait a minute and move the column in the check-in grid, it the time of the displayed inventory date updates. The grid appears to be showing the current timestamp for these items rather than what is stored in the database.

If you don't use the check-in modifier and are just checking in an item that already has an inventory date set, it is displaying the value from the database. It only appears to be an issue with items that are changing their inventory date as part of the check-in.

Everything else looks great!

Thanks for the feedback! I'm puzzled by this issue - my understanding is, it's happening because we're bringing the data for the item into the grid before the actual response updates everything in the DB, so we can't retrieve the timestamp after it's processed by postgres. I've made attempts to pass in an actual timestamp by parsing DateTime->now, and also by trying to change the data in the JS. I'm thinking a solution may be to add an _inventory_date to the alci information for this specific UI, although it feels like it would be very hacky.

Glad to see this moving along.

Not sure if it is directly related to the above, but there is a prevalent design issue with the current branch in that we are doing separate fetches where it seems like we should just be fleshing the fields in normally. For example, see 'cat/services/holdings.js'. Now that we have the IDL link in place, I see no obvious reason to not just flesh in this field with the rest of the linked data. This would also entail not renaming the fields, but just using the "natural" names as defined in the IDL.

Another example would be the bucket interface. Rather than manually fetch the inventory information, we might instead just follow the example of the other fields, letting the grid do the work.

I may be missing reasons why this field needs special treatment, but these are just things which stuck out at me while reading through the code for review purposes. I think these areas deserve a second look before this goes in.

I've pushed up a commit that I intend on flattening down once we have this completely working that should deal with the design issue mentioned above. The checkin table date issue still persists, as the checkin interface gathers its data via a payload returned from the API just prior to updating the DB.

Quick update, I've amended that commit with a fix to the Inventory Date column issue in the checkin interface. Any further code review would be much appreciated.

Thank you, Dan, for the feedback on the branch and to Kyle for the latest modifications! The behavior is working as expected in my most recent round of testing. I have resolved a minor conflict, rebased the branch, added my signoff, and added a new commit that adds the new eg.circ.checkin.do_inventory_update setting to config.workstation_setting_type.

My signoff branch is available at
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/lp1777675-inventory-date-support-signoff

I would like additional code review from another developer as well as a signoff on the top commit before this is merged to master.

I also added a commit for the release notes entry. I filed it under Cataloging, but I'll leave it up to the Release Notes Manager to put it somewhere else if it doesn't seem to be a good place for it.

I just heard a member from my team that they find the name of the reporting source "Last Copy Inventory" to be a little confusing, as if we're doing an inventory of the last copy available. I know there was a request to apply the more specific name to the table, but would there be any objection to using the more general "Inventory" as the label in the reporter, which would be more understandable to end users?

I'm willing to make the change to that or another label, but I don't want to touch the branch while Dan is assigned to the bug.

Thanks!

That confused me for a moment, too - I would prefer the more general "Inventory" label.

+1 for Inventory

Okay, I have tested and re-reviewed this code. A few things:

1. The areas of redundant/separate fetching look completely eliminated, so thank you, Kyle, for cleaning that up!

2. Remington was also involved in testing, and I have pushed and signed-off on a commit from him which fixes a couple use cases.

3. I added two commits myself. The first simply restores a few "missing" columns from the checkin interface (i.e. makes the checkin row the expected 12 units wide). The second involves our shared discomfort with the IDL naming. However, rather than just "Inventory", I'm proposing a new label of "Latest Inventory". My reasoning is, if we still expect a future "All Inventories" entry, then "All Inventories" and "Latest Inventory" seem to go better together than "All Inventories" and "Inventory". Then again, it is just a label we can change without too much trouble, so I support either way. I feel like we *may* want to also change the underlying table name to "latest_inventory" to save on future developer comprehension, but have not done so yet. Still, it's probably now or never, and I would definitely sign-off on that :)

4. Finally, Jeff Godin contacted me while I was reviewing this as he has separate concerns, so I am pushing my branch for its sign-offs/fixes, but also assigning this bug to him for any changes he is still considering.

Latest branch is here:
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1777675-inventory-date-support-signoff-2

I'm good with "Latest Inventory" as well.

"Latest Inventory" is fine with me, too.

+1 to Latest Inventory from me too. I'm happy to update the underlying tables when Jeff is done reviewing.

Thanks for the review and additions Dan and Remington!

Kathy-

In regard to your inquiry elsewhere:

I'm not going to be able to accomplish what I had wanted with this. Back over to you.

Thanks!

-jeff

I have signed off on the latest commits and added another commit that changes the name of the database table to be consistent with the new label changes that came from Dan.

New working branch at http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/lp1777675-inventory-date-support-signoff-3

I think this is a nice usability enhancement which can meet many common inventory use cases. Pushed to master. Thank you to Kyle, Kathy, and the rest for your work on this!