Bug 14306 - Show URL from MARC21 field 555$u in basket and detail
Summary: Show URL from MARC21 field 555$u in basket and detail
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-01 10:57 UTC by Marcel de Rooy
Modified: 2017-12-07 22:15 UTC (History)
4 users (show)

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


Attachments
Bug 14306: Show URL from MARC21 field 555$u under Title Notes/Descriptions (5.58 KB, patch)
2015-06-01 11:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[Signed-off] Bug 14306: Show URL from MARC21 field 555$u under Title Notes/Descriptions (5.74 KB, patch)
2015-06-01 13:12 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14306: Show URL from MARC21 field 555$u under Title Notes/Descriptions (5.84 KB, patch)
2015-07-30 17:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14306: Show URL from MARC21 field 555$u under Title Notes/Descriptions (5.93 KB, patch)
2015-08-26 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14306: Follow-up for URLs in 555$u (4.35 KB, patch)
2015-08-26 15:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14306: Show URL from MARC21 field 555$u under Title Notes/Descriptions (5.93 KB, patch)
2015-08-27 08:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14306: Follow-up for URLs in 555$u (4.35 KB, patch)
2015-08-27 08:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14306: Changes for basket and detail templates (5.75 KB, patch)
2015-08-27 08:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14306: Remove call to GetMarcNotes from sendshelf (2.64 KB, patch)
2015-08-27 08:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14306: Remove call to GetMarcNotes from sendshelf (2.74 KB, patch)
2016-01-04 09:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14306: Show URL from MARC21 field 555$u under Title Notes/Descriptions (6.08 KB, patch)
2016-01-05 13:04 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14306: Follow-up for URLs in 555$u (4.45 KB, patch)
2016-01-05 13:04 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14306: Changes for basket and detail templates (5.84 KB, patch)
2016-01-05 13:07 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14306: Remove call to GetMarcNotes from sendshelf (2.83 KB, patch)
2016-01-05 13:08 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 14306: Show URL from MARC21 field 555$u under Title Notes/Descriptions (6.13 KB, patch)
2016-03-04 13:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14306: Follow-up for URLs in 555$u (4.51 KB, patch)
2016-03-04 13:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14306: Changes for basket and detail templates (5.90 KB, patch)
2016-03-04 13:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14306: Remove call to GetMarcNotes from sendshelf (2.89 KB, patch)
2016-03-04 13:23 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2015-06-01 10:57:38 UTC
If you enter a URL in 555$u, you want to click it in Title Notes/Descriptions.
This requires a small adjustment in GetMarcNotes.
Comment 1 Marcel de Rooy 2015-06-01 11:05:14 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-06-01 13:12:04 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2015-06-01 13:24:23 UTC
(In reply to Marc Véron from comment #2)
> Signed-off-by: Marc Véron <veron@veron.ch>
Thanks, Marc.
Comment 4 Katrin Fischer 2015-06-01 19:27:52 UTC
I feel like it's a bit backwards to add HTML output to a core function. Maybe this would be better handled on template side? There has also been a request to add a span with the field number somewhere, that would allow to hide some notes fields for format them in an easy way.
Comment 5 Marcel de Rooy 2015-06-01 20:19:55 UTC
(In reply to Katrin Fischer from comment #4)
> I feel like it's a bit backwards to add HTML output to a core function.
> Maybe this would be better handled on template side? There has also been a
> request to add a span with the field number somewhere, that would allow to
> hide some notes fields for format them in an easy way.

Thanks for your feedback.
In the current form it is a small fix.
Now we pass just an array of notes to the template. We probably do not want to parse the marc record in the template. So that would mean: passing the URLs separately, maybe put a marker somewhere (or changing behavior still more), etc.
I did not have/take the time for now to put it in a much larger perspective..
Comment 6 Katrin Fischer 2015-06-06 15:38:41 UTC
I think passing a 'url' marker and also the field name to the template here would be better. There is an option to mark fields as URLs in the templates, it would make sense to use this, but I would not oppose hardcoding 555 in that case.
Comment 7 Marcel de Rooy 2015-07-30 17:30:54 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-08-05 06:47:02 UTC
What is this second patch, there is no signoff on it?

I don't think generating html in the module is a good idea. But it's certainly the quickest fix and will avoid to copy/paste code in the different templates.
Comment 9 Marcel de Rooy 2015-08-05 10:57:33 UTC
(In reply to Jonathan Druart from comment #8)
> What is this second patch, there is no signoff on it?
Hm. I rebased the patch but forgot to take the one with the signoff.. I will resubmit.

> I don't think generating html in the module is a good idea. But it's
> certainly the quickest fix and will avoid to copy/paste code in the
> different templates.
I will still have a look for a simple follow-up.
Comment 10 Marcel de Rooy 2015-08-26 13:45:29 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2015-08-26 15:40:49 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2015-08-26 15:45:54 UTC
Still a patch coming for one/more templates..
Comment 13 Marcel de Rooy 2015-08-27 08:39:13 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2015-08-27 08:39:18 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2015-08-27 08:39:22 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2015-08-27 08:39:25 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-12-28 11:28:10 UTC
CONFLICT (content): Merge conflict in virtualshelves/sendshelf.pl
Comment 18 Marcel de Rooy 2016-01-04 09:41:49 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2016-01-04 09:43:40 UTC
Rebased. Who adds a signoff?
Comment 20 Marc Véron 2016-01-04 11:28:20 UTC
Hi Marcel, a question about the test plan:
Do you expect the values (500$a, 555$a and a URL in 555$u) to be inserted into the mails? Or should we test only that the mails still work?
Comment 21 Marcel de Rooy 2016-01-05 12:03:02 UTC
(In reply to Marc Véron from comment #20)
> Hi Marcel, a question about the test plan:
> Do you expect the values (500$a, 555$a and a URL in 555$u) to be inserted
> into the mails? Or should we test only that the mails still work?

Hi Marc,
You may check the mails for unexpected side-effects or so. The main changes in this patch are template changes. Please check the tabs mentioned.
The 555 field is not passed in the mail IIRC (the code there did not use GetMarcNotes at all).
Comment 22 Marc Véron 2016-01-05 13:04:03 UTC Comment hidden (obsolete)
Comment 23 Marc Véron 2016-01-05 13:04:55 UTC Comment hidden (obsolete)
Comment 24 Marc Véron 2016-01-05 13:07:15 UTC Comment hidden (obsolete)
Comment 25 Marc Véron 2016-01-05 13:08:47 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2016-01-06 12:15:45 UTC
(In reply to Marc Véron from comment #25)
> Signed-off-by: Marc Véron <veron@veron.ch>

Thanks, Marc
Comment 27 Katrin Fischer 2016-03-04 13:22:51 UTC
Created attachment 48681 [details] [review]
[PASSED QA] Bug 14306: Show URL from MARC21 field 555$u under Title Notes/Descriptions

This patch includes:
[1] Add some logic to GetMarcNotes to embed the contents of MARC21 field
    555$u in a html anchor tag.
[2] Add a unit test for GetMarcNotes in Biblio.t
[3] Remove calls to GetMarcNotes from sendbasket.pl (opac and staff).
    A closer look revealed that the data was not used; the notes in the
    mail of sendbasket are taken from GetBiblioData.

Test plan:
[1] Edit a record. Add one or two URLS in 555$u. Add something in 500$a too.
[2] Check if you can click the URLs in opac and staff detail tab Notes or
    Descriptions.
[3] Run the unit test t/db../Biblio.t
[4] Add something in the cart. Click More Details and send the cart.
    Verify that you have something in Notes (from 500$a).

Signed-off-by: Marc Veron <veron@veron.ch>
Followed test plan. Works as expected. QA tools OK.

Tested with all patches together, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2016-03-04 13:22:55 UTC
Created attachment 48682 [details] [review]
[PASSED QA] Bug 14306: Follow-up for URLs in 555$u

This patch removes the code for inserting the <a> anchor tags around
URLs in GetMarcNotes (as added originally).
The URLs are placed in separate array elements; the template should take
care of further handling.
The unit test has been adjusted accordingly.

Test plan:
Run the unit test.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2016-03-04 13:23:01 UTC
Created attachment 48683 [details] [review]
[PASSED QA] Bug 14306: Changes for basket and detail templates

A simple regex is added to the basket and detail templates to select the
URLs passed separately from MARC21 555$u by GetMarcNotes. Note that the
regex tests if a note starts with http:// or https:// and does not contain
any whitespace in order to be considered as a url.
These URLs are put in an anchor tag.

This touches four places:
[1] opac detail, tab title notes
[2] catalogue detail, tab Descriptions
[3] opac basket, more details, notes
[4] staff basket, more details, notes

Test plan:
[1] Edit a record. Add a 500$a, 555$a and a URL in 555$u.
    Put "http://this is not a url" in the 500$a (whitespace!).
[2] Check opac-detail, tab Title Notes. Check the URL.
[3] Do the same for catalogue/detail.
[4] Add record to cart in OPAC. Open basket. Check More details.
[5] Repeat previous step in staff.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2016-03-04 13:23:06 UTC
Created attachment 48684 [details] [review]
[PASSED QA] Bug 14306: Remove call to GetMarcNotes from sendshelf

Just as mentioned in the first patch for sendbasket, I discovered that
the call of GetMarcNotes is not really used in sendshelf. The array is
passed to the template, but the template does not use it. (It uses the
information from GetBiblioData.)

Test plan:
[1] Send a list that includes a record with some notes (opac and staff).

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Marcel de Rooy 2016-03-04 14:33:02 UTC
(In reply to Katrin Fischer from comment #30)
> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Great. Thx
Comment 32 Brendan Gallagher 2016-03-07 18:00:33 UTC
Pushed to Master - Should be in the May 2016 - release.  Thanks!