overlaying a record from vandelay does not update editor/edit_date

It also doesn't update the bibliographic record source if that is selected in the vandelay import. I suppose that can get tacked on here rather than made its own bug. The fix for that should be able to happen at the same time, since it all goes in the same place in the database.

I recently fixed the edit date Vandelay overlay issue for our organization (running 2.4.5) by slightly tweaking the database function vandelay.overlay_bib_record. It will add the current time to the bilbio.record_entry.edit_date field whenever an overlay occurs.

It would probably be nice to make this controlled by a library setting, but I don't currently know how to do that :)

I am attaching the full SQL to replace the function, but the relevant changes are :

 ELSE
            UPDATE biblio.record_entry SET edit_date = NOW() WHERE id = eg_id;

I'm wondering if there really needs to be a library setting to have the edit date applied. Is there anyone who would not want the edit date to be updated when the record is overlaid?

I tested Bradley's modified overlay function on 2.6.2 and it works as described. The bib's edit_date was updated. I don't see a need for a library setting to update the edit_date. It would be nice to update the record source also.

I have incorporated Bradley's patch and Jason's suggestion into a branch for testing. No library settings are involved in this branch. One change from Bradley's version: I chose to update the edit_date only if an editor is found, since it doesn't seem good to have an editor's name attached to an edit_date when they were not involved.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/rsteed/lp957466_vl_update_edit_date

I tested Remington's branch and it does the following:

- update the record source

- update the editor and update date only if a valid user is specified in the record's 905 $u

Scrolling back to the beginning of the bug, I think that defaulting the editor to the logged in user in the absence of a 905 $u is important. Without a 905 $u, the edit_date does not get updated but the record is still updated. This leaves an editor attached to a record that has been changed by someone else. Also, records from vendors that are otherwise ready to load would require additional handling to add the 905 $u.

Considering options to address Martha's comment, I also noticed that Vandelay does not honor 905$u values when creating new records. It always sets the creator/editor to the logged in user. I think we need to move (or duplicate) the 905$u logic in the Vandelay Perl code so that we can set the correct value on newly created records as well.

Something like this should resolve the outstanding issues:

1. Vandelay record comes in to the Perl service
2. Extract the 905$u value. If none is present, default to the logged in user.
3. Before passing the record down to the DB for merge/overlay, if no 905$u was originally present, add one to match the value from #2 so the DB has access to the value.
4. When creating new records, use the value from #2 directly as the creator and editor.

Since this sounds like an ongoing discussion and not complete, I'm removing the pullrequest and setting aside milestones till this is made ready for fixing.

This "bug" will bite you if you use my program to import records from Backstage.

It first checks the edit date to see if the record has changed since the export.

It then compares 005 between the incoming and existing MARC recorcds. It the incoming record's 005 is more recent, it replaces the existing record.

Here's the problem scenario that we encountered this week:

We did the export, but due to a problem with configuration, Backstage did not get the email on time. It was almost 1 month later before this was noticed and Backstage was notified. In the mean time, over 300 records were overlayed via Vandelay by staff. When the import was run, those 300 plus records were replaced with "old" data. Now, the staff have to do the overlay of those records again.

Because this bug has had such a big impact on our work flow, I'm going to take on the branch and attempt Bill's suggestions.

To do what Bill suggests in comment #7, I believe it should be done after any fields are stripped from the incoming record. Otherwise, it would be useless to create/add to a 905 if the 905 is configured to be stripped.

Looking at this a bit more, I think we want to just set the 905$u to the current user in Vandelay regardless. After all, this is the latest user to touch the record.

Unless someone objects, I'll add a set_marc_905u method to AppUtils. I suppose it could take the editor and marcdoc, and retrieve the user information from the editor, or it could take the marcdoc and username. In the latter case, the calling code is responsible for retrieving the user information.

I prefer the latter since it reduces the new function's responsibility.

I have signed off on Remington's change, added a change to the Perl code to trigger Remington's changes and pushed to a collab branch: collab/dyrcona/lp957466_vl_update_edit_date

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dyrcona/lp957466_vl_update_edit_date

I tested Jason's and Remington's commits using both Vandelay within the client and marc_stream_importer.pl.

First I tested with records that do not contain a 905 $u. In the client, the records received the logged-in user as the editor and the edit_date and source were updated. Using marc_stream_importer.pl, the editor and source were updated according to the --user and --source arguments. The edit_date was updated as well. This all works great.

When importing records with a 905 $u, the editor in the 905 $u was used and the edit_date and source were updated. This was true when an on-file record was updated.

When importing records with a 905 $u and there is no already-on-file record, the editor was set to the logged in user (vandelay) or the user specified with --user (marc_stream_importer.pl) and the 905 $u was ignored.

I didn't consider marc_stream_importer but it sounds like it uses the same Vandelay.

As for "When importing records with a 905 $u and there is no already-on-file record, the editor was set to the logged in user (vandelay) or the user specified with --user (marc_stream_importer.pl) and the 905 $u was ignored." if you reread my first sentence on comment #11 that is what I was going for. The code actually looks for and strips out any exiting 905$u on the incoming file, then adds its own.

It seems to me that you can't trust any "local" fields on an incoming record to make any sense to Evergreen.

Thanks for the clarification. This works as described and is a huge improvement in record loading.

I have tested this code and consent to signing off on it with my name, Martha Driscoll and my email address, <email address hidden>.

For the record, I'm working on a pgTAP test for the database function change.

Quoting Remington Steed <email address hidden>:

> For the record, I'm working on a pgTAP test for the database function
> change.

For the record, I missed that comment before I committed. We can always add
the pgTAP test after. Sorry, 'bout that.

>
> --
> You received this bug notification because you are a member of Evergreen
> Bug Wranglers, which is subscribed to Evergreen.
> https://bugs.launchpad.net/bugs/957466
>
> Title:
> overlaying a record from vandelay does not update editor/edit_date
>
> Status in Evergreen - Open ILS:
> Confirmed
> Status in Evergreen 2.6 series:
> Confirmed
> Status in Evergreen 2.7 series:
> Confirmed
>
> Bug description:
> EG 2.0.11
> PG 8.4
>
> Evergreen does not seem to update the editor or edit_date of the
> biblio.record_entry when you overlay a record in Vandelay.
>
> Although there is some code related to 905 $u, one would think that
> the default in absense of such a field would be to use the person
> importing the records as the editor.
>
> AFAICT this bug likely persists in master.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/957466/+subscriptions

--
Jason Stephenson
Assistant Director for Technology Services
Merrimack Valley Library Consortium
1600 Osgood ST, Suite 2094
North Andover, MA 01845
Phone: 978-557-5891
Email: <email address hidden>

I'll test the pgTAP test ASAP and push to master. I'll wait on the test to backport the fix to 2.7 and 2.6.

Sorry for jumping the gun. I guess I got a little carried away. :)

I should just call it a day. I'll also fix wrongly attributed comment in 002.schema.config.sql when I merge the pgTAP test.

Sorry again, Remington.

I am concerned about these most recent comments regarding removing the existing 905u for Vandelay imports. This seems contrary to idea of supporting the 905u field at all, which presumably exists to pass control of setting the editor to the cataloger. I would imagine we would want to do as Bill suggests in comment #7 and preserve the 905u (unless explicitly told not to). Am I misunderstanding something?

Well, I think I see where we have different views on this. I come from a mostly public library area where I get "God-knows-what" in MARC records and catalogers who edit records in OCLC before importing them. Adding a 905$u is not on their RADAR.

I s'pose your environment things are different. Where catalogers are used to adding a 905$u. I wonder why you would want that information to be different from the user who imported the record, though?

It also looks like any existing 905$u is being stripped out by the database function anyway. This includes the one being added by the new set_marc_905u function in apputils. I could be wrong, but I don't remember seeing any after I made may changes and tested.

It's only code. It's only in master. It can be changed. I'd rather have consensus and not add another setting though. It would probably be easier to just not do anything if a 905$u already exists.

I've pushed a branch with two commits, 1 fixes the misattribution comment, and the other changes the set_marc_905u function to do what Dan Wells and Bill Erickson suggest.

The new branch is collab/dyrcona/lp957466_alternate_ending:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dyrcona/lp957466_alternate_ending

I pushed a pgTAP test to Jason's collab branch. Before upgrading the db function, 2 tests fail. After upgrading, all succeed.

The question is: Do we want to push the alternate_ending version? If so, someone else should have a look and signoff. Looks like you would only need the top two commits at this point: 7544baccaeb765d2d0f907b74ea98996c4b5e165 & eb01f95140453af32bded9d6b07f2bc72d869f4a.

I tested Jason's latest commit and pushed a small fix to the collab branch.

Looks good to me. Pushed fixes to master, then pushed slightly squashed version back through rel_2_6. Thanks, all!

on today's tester run:

Open-ILS/src/sql/Pg/t/regress/lp957466_update_date_and_source.pg ..............
1..4
ok 1 - Function call succeeded
not ok 2 - Editor was updated
# Failed test 2: "Editor was updated"
# have: 1
# want: 236
not ok 3 - Edit Date was updated
# Failed test 3: "Edit Date was updated"
# have: 2016-05-04
# want: 2016-05-19
not ok 4 - Record source was updated
# Failed test 4: "Record source was updated"
# have: NULL
# want: 2
Failed 3/4 subtests

first instance was here http://testing.evergreen-ils.org/~live/archive/2016-05/2016-05-13_16:00:02/test.21.html

looks like maybe 651020ea6ceb732bc4ac743075bf0080c10ae39f for bug 1447746

I agree that the added feature broke this test. It can be fixed easily by ensuring that the merge_profile is created with the "don't update the bib source info" flag.

Alternatively -- which is to say "additionally" -- another test could be added with the flag in the "update the source info" position, and recast the test cases to expect a difference.

I feel at least 50% responsible for this, so let me know if you want me to do this work.

As the person who merged the code without first running all the tests, I would like to take a stab at this one just to further familiarize myself with tests. However, I may end up taking you up on that offer Dan!

Go wild! It shouldn't take long. I was planning on
* Modifying the original (failing) test to add the appropriate flag argument when creating the template.
* Cloning the test into another test with the flag flipped the other way, and the calls changed from "should be the same" to "should be different", if you know what I mean.

Dan

We're on the same page Dan. Already completed step 1. Working on step 2 now.

Added a fix for the test to bug 1447746