Copy details repeated in search results when item/volume moved with parts attached

Bug #1411422 reported by Blake GH on 2015-01-15
80
This bug affects 17 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned
2.12
Medium
Unassigned

Bug Description

This looks like a server side issue

This line
[% FOR copy IN args.holdings %]

in

table.tt2

will repeat strangely. Examples:

http://missourievergreen.org/eg/opac/results?detail_record_view=1&query=50+shades+of+grey&qtype=keyword&fi%3Asearch_format=&locg=137&sort=&modifier=metabib&detail_record_view=1

And if you click the top result
http://missourievergreen.org/eg/opac/results?detail_record_view=1;detail_record_view=1;query=50%20shades%20of%20grey;qtype=keyword;locg=137;modifier=metabib;metarecord=310327

You should see a grouped result with a long list of copies. But there are only 5 copies at Little Dixie Regional.

Michele Morgan (mmorgan) wrote :

For these items, is the call_number.owning_lib different from the copy.circ_lib? If so, this looks like a duplicate of this bug:

https://bugs.launchpad.net/evergreen/+bug/1315552

Blake GH (bmagic) wrote :

I think you are right! That is the issue. This bug should be closed/deleted/merged.

Thanks for catching that!

Michele Morgan (mmorgan) wrote :

Marking as duplicate of lp#1315552

Michele Morgan (mmorgan) wrote :

Removing the duplicate designation on this bug as it actually is a separate issue. Both this bug and lp#1315552 result in a display issue where copies are duplicated in the catalog display.

In this bug, however, the underlying issue is that parts are not handled properly when copies or volumes are transferred. Multiple rows for a single copy in asset.copy_part_map can result. A new row can be added, but the existing rows for the same copy are not removed.

Blake, do you want to add your comments from lp#1315552 to this bug?

Blake GH (bmagic) wrote :
Download full text (3.3 KiB)

Here is a query that will identify the bibs in your database:

select bmp.record,acpm.target_copy,acn.record from asset.copy ac, asset.call_number acn, asset.copy_part_map acpm, biblio.record_entry bre, biblio.monograph_part bmp
where
bre.id!=bmp.record and
ac.call_number=acn.id and
bmp.id=acpm.part and
bre.id=acn.record and
acpm.target_copy=ac.id and
not bre.deleted and
not acn.deleted and
not ac.deleted and
acn.record>0
limit 100

These are copies with parts where one of the parts point to a bib that is not equal to the call_number.record for the copy. In these cases, they are repeated in the OPAC.

Running some experiments:

1. Pick a bib X
2. Pick a bib Y
3. Create a part for bib X (vol1)
4. Create a copy on bib X with part assigned vol1 and call number 1234
5. Repeat 3-4 with bib Y
6. Transfer volume from bib X to bib Y
7. The result will not move the part. Copy from bib X still points to the part for bib X
8. Refresh bib Y, right click the newly moved item and "Replace Barcode"
9. Select vol1 from the part dropdown and click "Re-Barcode / Update Items"
10. You will now have two rows in asset.copy_part_map one pointing to the old bib and one pointing to the current bib

This will result in duplicate rows in the OPAC

The issue with moving items is:

var robj = network.simple_request(
            'FM_ACP_FLESHED_BATCH_UPDATE',
            [ ses(), copies, true ],
            null,
            {
                'title' : $("catStrings").getString('staff.cat.util.transfer_copies.override_transfer_failure'),
                'overridable_events' : [
                    1208 /* TITLE_LAST_COPY */,
                    1227 /* COPY_DELETE_WARNING */,
                ]
            }
        );

There needs to be logic to handle the part.

And here is the code for the volume transfer:

var robj = obj.network.simple_request(
                                        'FM_ACN_TRANSFER',
                                        [ ses(), { 'docid' : obj.data.marked_library.docid, 'lib' : obj.data.marked_library.lib, 'volumes' : list } ],
                                        null,
                                        {
                                            'title' : document.getElementById('catStrings').getString('staff.cat.copy_browser.transfer.override.failure'),
                                            'overridable_events' : [
                                                1208 /* TITLE_LAST_COPY */,
                                                1219 /* COPY_REMOTE_CIRC_LIB */,
                                            ],
                                        }
                                    );

and I think that the perl code could handle it instead of the JS:

Cat.pm
batch_volume_transfer

OR*

we just don't care that the part_map remains in the database and we handle the display better in the OPAC.

Watch out for holds on these parts!

select * from action.hold_request where target in(
select acpm.part from asset.copy ac, asset.call_number acn, asset.copy_part_map acpm, biblio.record_entry bre, biblio.monograph_part bmp
where
bre.id!=bmp.record and
ac.call_number=acn.id and
bmp.id=acpm.part and
bre.id=acn.record and
acpm...

Read more...

Blake GH (bmagic) on 2016-02-02
summary: - Copy details repeated in search results scoped to system and grouped
- formats
+ Copy details repeated in search results when item/volume moved with
+ parts attached
Blake GH (bmagic) on 2016-06-10
Changed in evergreen:
assignee: nobody → Blake GH (blake-j)
Blake GH (bmagic) on 2016-06-13
tags: added: pullrequest
Blake GH (bmagic) on 2016-06-24
Changed in evergreen:
assignee: Blake GH (blake-j) → nobody
Kathy Lussier (klussier) on 2016-07-12
Changed in evergreen:
milestone: none → 2.next
status: New → Confirmed
importance: Undecided → Medium
assignee: nobody → Kathy Lussier (klussier)
Kathy Lussier (klussier) wrote :

Thanks Blake!

I've looked at the code, and, overall, it looks good. It fixes the issue described in this report and, by properly handling parts when copies are moved from one bib to another, it also fixes the issue reported in bug 904472.

I just saw a few issues.

1. There appears to be a regression with the code that was merged as part of bug 937789. On a clean master install, when I delete a part with a hold on it, the hold is automatically canceled. However, on a server where this branch is loaded, holds are no longer automatically canceled when a monographic part is deleted from the bib record.

2. Of lesser importance than #1, when you transfer the last copy that belongs to a monographic part, the old part is left behind on the original bib. Could it be deleted as part of the transfer process? Cleaning it up would be particularly helpful in preventing any hopeless holds being left behind.

3. Would it be possible to add a regression test?

Thanks again! We are looking forward to seeing this bug fixed!

Changed in evergreen:
assignee: Kathy Lussier (klussier) → nobody
Kathy Lussier (klussier) wrote :

Disregard issue #1 from my comment above. I was baffled by my findings because I couldn't see why this code would cause a problem with the automatic cancellation of part-level holds. I decided to take a closer look and realized the clean master install I reported using was actually a clean install with 2.9.5. When I tried the same workflow on a clean master install, I found the same problem that I reported in issue #1. It appears to be a pre-existing bug.

I'll test a little further to see if the bug is present in 2.10 and will file a new bug.

Kathy

I tested this on a 2.10.6 dev system, and it seems to fix the problem for us.

Before applying the fix - when I transferred two copies from one volume to a different title/volume the part info wasn't transferred at all and the records show up in the mismatched asset.copy_part_map query.

After applying the fix - the parts do transfer, and nothing new shows up in the mismatched part map query.

I want to try this out in production also, since we need to move a bunch of copies with parts to different records. I'll report back if there are any problems.

Josh

no longer affects: evergreen/2.9
Kathy Lussier (klussier) wrote :

I know a lot of people who would love to see this bug fix get into their systems. Blake, do you have any thoughts on my suggestion in #2. I don't think it's a deal-breaker for getting the code in if we can't do it, but if it's something that can be easily added, I think it will prevent a lot of orphan parts from being left behind on bibs.

Also, would it be possible to add a regression test?

Kathy Lussier (klussier) on 2017-04-06
tags: added: needstest
removed: pullrequest
no longer affects: evergreen/2.10
Changed in evergreen:
milestone: 3.next → 2.12.2
Changed in evergreen:
milestone: 2.12.2 → 2.12.3
Changed in evergreen:
milestone: 2.12.3 → 2.12.4
Changed in evergreen:
milestone: 2.12.4 → 2.12.5
Changed in evergreen:
milestone: 2.12.5 → 3.0-alpha
assignee: nobody → Jason Stephenson (jstephenson)
Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
Blake GH (bmagic) on 2017-09-01
Changed in evergreen:
assignee: nobody → Blake GH (bmagic)
Blake GH (bmagic) wrote :

Regression test created. I hope it's not too late for 3.0. There is always 3.1 :)

Changed in evergreen:
assignee: Blake GH (bmagic) → nobody
tags: added: pullrequest
removed: needstest
Changed in evergreen:
milestone: 3.0-beta → 3.0-beta2
no longer affects: evergreen/2.11
Changed in evergreen:
milestone: 3.0-beta2 → 3.0-rc
Galen Charlton (gmc) on 2017-09-27
Changed in evergreen:
milestone: 3.0-rc → 3.0.1
Changed in evergreen:
milestone: 3.0.1 → 3.0.2
Changed in evergreen:
milestone: 3.0.2 → 3.0.3
assignee: nobody → Evergreen Bug Maintenance (bugmaster)
assignee: Evergreen Bug Maintenance (bugmaster) → nobody
assignee: nobody → Kathy Lussier (klussier)
Changed in evergreen:
milestone: 3.0.3 → 3.0.4
Changed in evergreen:
milestone: 3.0.4 → 3.0.5
Jason Stephenson (jstephenson) wrote :

I think Kathy's item #2 from comment #7 needs to be addressed because of what I reported in bug 1753835.

I'll see if I can take a look at this soonish to fix it.

Changed in evergreen:
assignee: Kathy Lussier (klussier) → nobody
assignee: nobody → Jason Stephenson (jstephenson)
Jason Stephenson (jstephenson) wrote :

So far, in my testing, this does what it says on the tin. I'm going to have a look at implementing Kathy's request #2 from comment #7.

Jason Stephenson (jstephenson) wrote :

I have pushed my signoff with 2 follow up commits to user/dyrcona/lp1411422_Copy_details_repeated_in_search_results_when_item_or_volume_moved_with_parts_attached in the working repo:

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

I decided not to implement the removal of a monograph part when the last copy is transferred because a) it turned out to be way more work then I have time and more work than I feel should go into this bug fix, and b) to do it correctly, i.e. like removing the last volume and bib, would require a new org. unit setting to allow sites to configure that behavior. For those reasons, I decided that looked more like a new feature than a bug fix.

Finally, I did not push this to master or the other repos because I am getting test failures with this branch applied. I do not think this branch is responsible for those failures, but because of the failures, I think another pair of eyes on this are warranted. Below is the output of prove on the perlmods/live_t directory:

est Summary Report
-------------------
Open-ILS/src/perlmods/live_t/09-lp1198465_neg_balances.t (Wstat: 512 Tests: 133 Failed: 2)
  Failed tests: 76, 133
  Non-zero exit status: 2
Open-ILS/src/perlmods/live_t/19-lp1306666-abort-transit-copy-status.t (Wstat: 1792 Tests: 26 Failed: 7)
  Failed tests: 15-17, 21-22, 24, 26
  Non-zero exit status: 7
Open-ILS/src/perlmods/live_t/24-offline-all-assets.t (Wstat: 256 Tests: 1 Failed: 1)
  Failed test: 1
  Non-zero exit status: 1
Files=29, Tests=636, 85 wallclock secs ( 0.20 usr 0.04 sys + 13.73 cusr 1.07 csys = 15.04 CPU)
Result: FAIL

NOTE: The above is after my commit to fix & rename the test added by this branch.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
tags: added: signedoff
Jason Stephenson (jstephenson) wrote :

Oh! I should add that I tested the functionality on 2.12.8 and master. I ran the live tests only on master.

Kathy Lussier (klussier) wrote :

As an additional data point, I wanted to add that I ran live perl tests on a clean master system that was built yesterday. I see the exact same test failures for 19-lp1306666-abort-transit-copy-status.t and for 24-offline-all-assets.t.

For the negative balance tests, I also see test failures for tests 76 and 133, but I have a few more. However, it's I previously have had problems getting those tests to pass. I'm not sure why.

Dan Wells (dbw2) wrote :

I just pushed a data fix for the negative balance test cases. The negative balance tests now all pass for me when running them isolated, but there may still be cases to work out there where the different tests interact.

Jason Stephenson (jstephenson) wrote :

So, with the other test failures being accounted for in bu 1751318 and bug 1755502, I feel confident pushing this branch to master, rel_3_0, and rel_2_12. Thanks, Blake, Kathy, Dan, and Josh!

Changed in evergreen:
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers