Multiple URI's will show when using opac.located_uri.act_as_copy flag

Bug #1582720 reported by Steve Callender
60
This bug affects 12 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Medium
Unassigned

Bug Description

When enabling the global flag opac.located_uri.act_as_copy (When enabled, Located URIs will provide visiblity behavior identical to copies.)

It will list out the URI's per branch in the TPAC.

For example, having a single 856 tag with indicator 1 = "4" and indicator 2 = "0", a single u subfield (URL), a single y subfield (Label) and multiple 9 subfields (for each branch) will cause the TPAC to display the same exact link and label for however many 9 tags there are.

The args are setup for the URI's in Open-ILS/src/templates/opac/parts/misc_util.tt2 and the screen is Open-ILS/src/templates/opac/parts/record/summary.tt2

I believe it would be a cleaner display to display the URI links only once if they are identical then displaying the same exact url and label multiple times in a row.

Steve

Revision history for this message
Kathy Lussier (klussier) wrote :

I don't know what the original intended behavior was, but I agree that if there is just one label associated with the Located URI, then the URL should only display once per record.

Changed in evergreen:
status: New → Confirmed
importance: Undecided → Medium
Revision history for this message
Rogan Hamby (rogan-hamby) wrote :

I was looking at this one today during the Hack-A-Way some. We can certainly go over the name value pairs and prevent duplicates from going in when they're processed in misc_util but I'm increasingly of the thought we shouldn't. There's a question about to the extent that code should compensate for bad data and if the label, note and href are all duplicates I think that's bad data and something that catalogers should be alerted to to clean out, not cleaned up by the system.

Revision history for this message
Kathy Lussier (klussier) wrote :

I don't know that we're talking about the same thing here. As I understood Steve's original report, he is talking about one 856 field with one URL, one label, and one note. The 856 just happens to have multiple subfield 9s, which is something that the documentation says we should be able to do.
For example:

856 4 0 ‡uhttps://masslnc.org ‡yLinks to web site ‡9BR1 ‡9BR2 ‡9BR3

Rogan, it sounds like you're talking about multiple 856 fields with duplicate URLS, labels, etc.

Elaine Hardy (ehardy)
tags: added: cataloging displayfields tpac
tags: added: opac
removed: tpac
Andrea Neiman (aneiman)
tags: added: needsdiscussion
Revision history for this message
Derek C. Zoladz (derekz) wrote :

Is this fundamentally a duplicate bug 1353643?

https://bugs.launchpad.net/bugs/1353643

Revision history for this message
Jeff Davis (jdavis-sitka) wrote :

I don't think they're duplicates. This bug is about redundant display of identical URIs; bug 1353643 is about improved scoping for not-necessarily-identical URIs.

Revision history for this message
Jason Boyer (jboyer) wrote (last edit ):

There are 3 commits here; one for the BPAC, TPAC, and staff client: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1582720_number_nine / working/user/jboyer/lp1582720_number_nine

The staff client patch works and passes lint, but I wouldn't be surprised to learn there's a better way to do it.

Testing, assuming Concerto:
1. Edit record 76, add a $9 for SYS2 to the existing 856 with the SYS1 $9
2. Enable the opac.located_uri.act_as_copy global flag
3. Visit record 76, see 3 identical links now (un-scoped 856, 856$9SYS1, and 856$9SYS2)
4. Apply the branch above
5. Visit record 76, behold but 2 links

Two though? This patch only addresses duplicate URI links caused by $9 entries. If you have identical links in multiple 856's (even in this case, one scoped and one not) that's a data problem, fix that in the marc editor. For testing it may help to change the link text or label to make it more clear which of the links have $9's attached and which do not.

tags: added: pullrequest
removed: needsdiscussion
Revision history for this message
Shula Link (slink-g) wrote :

In https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=c33f2afdab96386741042ed1737771418077718c we noticed something during collaborative code review today:

1) In the added de-duplication function, the variable names elb, indb, and aryb and els, inds, and arys aren't clearly descriptive of the data being passed to the function.
2) The aryb/arys variables are passed but never used.

Revision history for this message
Shula Link (slink-g) wrote :

For the record: The patches work as described, we just had questions about the variables in the function.

Revision history for this message
Jason Boyer (jboyer) wrote :

They are fairly bad param names and indeed 2 (rather, 4) of them are unused; the number of params comes from here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach . The forEach callback function is called with 3 params: the current element, index of the element in the array, and the array itself (so element = array[index]) and since it's called with all 3, I named all three. They're not helpful for this, but I don't really like accepting only 1 param when the callback will be given 3.
To distinguish the generic params given the suffixes try to indicate what goes where, so elb is the forEach element from bibSummary.uris, and els is the forEach element of the summary.eResourceUrls. The names are a bit short because they're only used in 3-4 lines all together (were it only called once I'd probably reach for elm, idx, and ary) but they can be changed if there's a desire from the reviewers and / or newdevs groups to make them more clear.

Revision history for this message
Shula Link (slink-g) wrote :

Maybe this is a case where a comment in the code explaining the function would be useful as in-line documentation for future developers? Like, this is what it does, this is what the passed variables are, this is what the expected behavior is.

I know I'm personally terrible at remembering to put comments in my code, but when I do future me always appreciates it.

Revision history for this message
Jason Boyer (jboyer) wrote :

Thanks for the suggestion Shula, I've dropped a comment in the branch that should help.

Revision history for this message
Jane Sandberg (sandbergja) wrote :

Thanks for the comment, and for the patches, Jason! And thanks to Shula and the code review group for reviewing it. I've pushed it to rel_3_12 and main.

Changed in evergreen:
status: Confirmed → Fix Committed
milestone: none → 3.12.2
tags: added: signedoff
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.