Bug 3462 - Link see also in authorities
Summary: Link see also in authorities
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.10
Hardware: PC All
: P5 - low enhancement (vote)
Assignee: Jared Camins-Esakov
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 8212 8581
  Show dependency treegraph
 
Reported: 2009-07-31 06:15 UTC by Nicole C. Engard
Modified: 2014-05-26 21:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
authority search from cataloging record (80.44 KB, image/png)
2009-07-31 18:15 UTC, Chris Cormack
Details
authority search (19.00 KB, image/png)
2009-07-31 18:16 UTC, Chris Cormack
Details
Bug 3462: Links in authorities should be hyperlinks (8.56 KB, patch)
2012-06-22 03:04 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
screenshot (142.87 KB, image/png)
2012-07-08 12:09 UTC, Katrin Fischer
Details
Bug 3462: Links in authorities should be hyperlinks (9.03 KB, patch)
2012-07-08 13:01 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3462: Links in authorities should be hyperlinks (10.00 KB, patch)
2012-07-08 13:24 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3462: Links in authorities should be hyperlinks (11.11 KB, patch)
2012-07-08 13:29 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3462: Links in authorities should be hyperlinks (15.86 KB, patch)
2012-07-08 14:01 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3462: quiet warn (1.06 KB, patch)
2012-07-08 14:06 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3462: Links in authorities should be hyperlinks (16.37 KB, patch)
2012-07-08 14:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 3462: Links in authorities should be hyperlinks (16.48 KB, patch)
2012-07-26 15:08 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3462: Links in authorities should be hyperlinks (17.00 KB, patch)
2012-08-01 13:35 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3462: Links in authorities should be hyperlinks (17.09 KB, patch)
2012-08-06 17:54 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3462: Links in authorities should be hyperlinks (16.04 KB, patch)
2012-09-11 18:59 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:10:57 UTC


---- Reported by nengard@gmail.com 2009-07-31 18:15:57 ----

Created an attachment
authority search from cataloging record

It would be nice is the see also statements in Authorities were linked to the authority they were referencing.  See attached images.



---- Additional Comments From nengard@gmail.com 2009-07-31 18:16:19 ----

Created an attachment
authority search





--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:10 UTC  ---

This bug was previously known as _bug_ 3462 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3462
Imported an attachment (id=1315)
Imported an attachment (id=1316)

Actual time not defined. Setting to 0.0
The original submitter of attachment 1315 [details] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
The original submitter of attachment 1316 [details] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Jared Camins-Esakov 2012-06-22 03:04:33 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2012-07-08 12:09:36 UTC
Created attachment 10691 [details]
screenshot

Hi Jared, 

I tested this and I think there is a small mistake in the linking. In my catalog it didn't only link the "see also", but also the "used for/see from". 
The links are not correct too.
The last "see also" link is used for all links below until the next "see also".

I was also wondering what happens when the linked authority does not exist. I tried on of the links and I had a empty line on top of the list with a "^" instead of a heading.
Comment 3 Jared Camins-Esakov 2012-07-08 13:01:51 UTC Comment hidden (obsolete)
Comment 4 Jared Camins-Esakov 2012-07-08 13:17:19 UTC
The problem, it turns out, is that when you call a TT block enough times, variables don't get cleared when they go out of scope. The fix was simply to explicitly unset the URL variable whenever there was no URL.
Comment 5 Jared Camins-Esakov 2012-07-08 13:24:39 UTC Comment hidden (obsolete)
Comment 6 Jared Camins-Esakov 2012-07-08 13:29:20 UTC Comment hidden (obsolete)
Comment 7 Jared Camins-Esakov 2012-07-08 14:01:56 UTC Comment hidden (obsolete)
Comment 8 Jared Camins-Esakov 2012-07-08 14:06:53 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2012-07-08 14:17:27 UTC Comment hidden (obsolete)
Comment 10 Paul Poulain 2012-07-26 15:01:35 UTC
The patch does not apply anymore:
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/prog/en/modules/opac-authoritiessearchresultlist.tt

I suspect it's an easy conflict to solve, but won't do it myself to avoid any mistake.

Other than this, I don't see any reason not to pass QA. I'll just test with UNIMARC to see if this patch does not break anything.
Comment 11 Jared Camins-Esakov 2012-07-26 15:08:57 UTC Comment hidden (obsolete)
Comment 12 Jared Camins-Esakov 2012-08-01 13:35:36 UTC Comment hidden (obsolete)
Comment 13 Jared Camins-Esakov 2012-08-06 17:54:16 UTC Comment hidden (obsolete)
Comment 14 Mason James 2012-08-10 08:16:34 UTC
(In reply to comment #13)
> Created attachment 11390 [details] [review]
> Bug 3462: Links in authorities should be hyperlinks

> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
> Rebased 26 July 2012
> 
> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
> Rebased on master 1 August 2012
> 
> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
> Rebased on master 6 August 2012
> 
> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>


patch looks good, passing QA
well done everyone :)


mason@xen1:~/git/head$ koha-qa.pl
        * 02ea726 Bug 3462: Links in authorities should be hyperlinks
                C4/AuthoritiesMarc.pm
                koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search-results.inc
                koha-tmpl/intranet-tmpl/prog/en/modules/authorities/searchresultlist.tt
                koha-tmpl/opac-tmpl/prog/en/includes/authorities-search-results.inc
                koha-tmpl/opac-tmpl/prog/en/modules/opac-authoritiessearchresultlist.tt
                opac/opac-authorities-home.pl

- perlcritic-progressive tests... OK
- perl -c syntax tests... OK
- xt/tt_valid.t tests... OK
- xt/author/valid-template.t tests... OK
Comment 15 Paul Poulain 2012-08-31 16:20:32 UTC
Testing with UNIMARC before pushing, I can't see any difference before and after the patch.

In AuthoritiesMarc.pm, there is some code related to that:
    if (C4::Context->preference('marcflavour') eq 'UNIMARC') {
# construct UNIMARC summary, that is quite different from MARC21 one
# accepted form
        foreach my $field ($record->field('2..')) {
            push @authorized
        }
        foreach my $field ($record->field('3..')) {
            push @notes
        }
        foreach my $field ($record->field('4..')) {
 push @seefrom
        }
        foreach my $field ($record->field('5..')) {
 push @seealso
        }

I have 400 and 500 in my record (just freshly added, and can't see anything appearing :\
Comment 16 Paul Poulain 2012-09-07 13:31:00 UTC
Jared, ping on my comment 15 = I need some explanations. Should I see differences in UNIMARC ? (I think yes, but can't see them)
Comment 17 Jared Camins-Esakov 2012-09-07 14:26:04 UTC
Paul,

(In reply to comment #16)
> Jared, ping on my comment 15 = I need some explanations. Should I see
> differences in UNIMARC ? (I think yes, but can't see them)

I just spent a while trying to figure out why this wasn't working in UNIMARC before I realized that in a properly-configured UNIMARC system, it shouldn't. UNIMARC uses a user-specified summary format in auth_types.summary. MARC21 does not have that. This patch keeps the existing behavior, since presumably it matches what you would want. A follow-up patch could remove the auth_types.summary bit, if you wanted.
Comment 18 Jared Camins-Esakov 2012-09-11 18:59:44 UTC
Created attachment 12149 [details] [review]
Bug 3462: Links in authorities should be hyperlinks

Make see also links in both the OPAC and authority module search results
into hyperlinks and not just textual strings.

To test:
1. Do a search for an authority that will bring up a heading with a
   see also reference in the staff client and the OPAC.
2. Confirm that the see also references listed in the search results
   are now hyperlinks, which work.

Also quiets an unnecessary warning about an uninitialized value.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Rebased 26 July 2012

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Rebased on master 1 August 2012

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Rebased on master 6 August 2012

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Rebased on master 11 September 2012

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Patch works nicely and is a great improvement.

Tests:
- Checked links for existing and non existing "see alsos" give correct
  search results
- Verified links show up for all "see alsos" in a result list
- Verified links are properly linked with the correct names
- Checked logs don't show errors
Comment 19 Paul Poulain 2012-09-13 14:55:58 UTC
(In reply to comment #17)
> A follow-up patch
> could remove the auth_types.summary bit, if you wanted.

I think it would be useful to have a consistent behaviour for UNIMARC and MARC21, so yes, a follow-up patch would be good.
does the "if you wanted" imply you will write it, or is it a "do it yourself if you want it" ?
(in the 2nd option, it will wait until i'm no more RM I think ;-) Plus I don't know the MARC21 behaviour at all for now :\ )

Other than this: patch pushed (I haven't tested it, because in UNIMARC there's no difference. trusting kf for testing well)
Comment 20 Jared Camins-Esakov 2012-09-13 14:59:33 UTC
(In reply to comment #19)
> (In reply to comment #17)
> > A follow-up patch
> > could remove the auth_types.summary bit, if you wanted.
> 
> I think it would be useful to have a consistent behaviour for UNIMARC and
> MARC21, so yes, a follow-up patch would be good.
> does the "if you wanted" imply you will write it, or is it a "do it yourself
> if you want it" ?
> (in the 2nd option, it will wait until i'm no more RM I think ;-) Plus I
> don't know the MARC21 behaviour at all for now :\ )

I would write it, but only if you wanted me to. :)
Comment 21 Paul Poulain 2012-09-13 15:15:39 UTC
(In reply to comment #20)
> I would write it, but only if you wanted me to. :)

/me throw a coin ... jared face appear. OK, you win, if you know & agree to write it, you'll be thanked for centuries