Bug 20689 - Make it possible to add capital letter subfields to item search fields
Summary: Make it possible to add capital letter subfields to item search fields
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 8 votes (vote)
Assignee: Katrin Fischer
QA Contact: Martin Renvoize
URL:
Keywords:
: 13561 15384 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-05-01 16:05 UTC by Patrick Robitaille
Modified: 2021-08-23 23:58 UTC (History)
7 users (show)

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


Attachments
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config (3.20 KB, patch)
2018-08-09 23:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config (3.26 KB, patch)
2018-09-27 22:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config (3.41 KB, patch)
2018-10-01 19:45 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (4.10 KB, patch)
2018-10-12 16:31 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (4.16 KB, patch)
2018-10-12 16:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config (3.47 KB, patch)
2018-10-17 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (4.22 KB, patch)
2018-10-17 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (6.51 KB, patch)
2018-10-25 12:32 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config (3.53 KB, patch)
2018-10-25 13:19 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (6.59 KB, patch)
2018-10-25 13:19 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config (3.52 KB, patch)
2019-01-11 14:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (6.57 KB, patch)
2019-01-11 14:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (6.64 KB, patch)
2019-01-11 20:20 UTC, José-Mario Monteiro-Santos
Details | Diff | Splinter Review
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config (3.58 KB, patch)
2019-04-14 21:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (6.71 KB, patch)
2019-04-14 22:00 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 20689 Follow - tiny typo fix (1.29 KB, patch)
2019-04-14 22:00 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 20689: (follow-up) Correct scope of template variables (2.86 KB, patch)
2019-04-18 12:21 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config (3.64 KB, patch)
2021-08-23 23:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields (6.76 KB, patch)
2021-08-23 23:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20689: (follow-up) Tiny typo fix (1.35 KB, patch)
2021-08-23 23:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20689: (follow-up) Correct scope of template variables (2.93 KB, patch)
2021-08-23 23:40 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 Patrick Robitaille 2018-05-01 16:05:13 UTC
Hi, 

when you add a new subfield in a marc bibliographic framework and that subfield is not in the marc format normally, you can not add it as a item search fields. It would be useful to be able to use these new subfields for item search for exemple.

Thanks, Patrick.
Comment 1 Katrin Fischer 2018-05-01 16:23:22 UTC
Do you mean capital letter subfields?
Comment 2 Patrick Robitaille 2018-05-07 12:21:05 UTC
Hi Katrin, 

It is exactly what I mean. 

Thanks, Patrick.
Comment 3 Katrin Fischer 2018-08-09 23:12:28 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2018-09-27 22:13:37 UTC Comment hidden (obsolete)
Comment 5 Séverine Queune 2018-10-01 19:45:36 UTC Comment hidden (obsolete)
Comment 6 Séverine Queune 2018-10-01 19:47:03 UTC
Great enhancement Katrin !
Thanks a lot !
Comment 7 Katrin Fischer 2018-10-01 20:48:44 UTC
(In reply to Séverine Queune from comment #6)
> Great enhancement Katrin !
> Thanks a lot !

Thx for testing!
Comment 8 Jonathan Druart 2018-10-02 12:02:05 UTC
I may be a bad person but, ofc, I tried to find a bug. So I filled field="~" and subfield="€"
The search did not like it :)

I think we should keep the list of field/subfield exhaustive, and maybe add select2 to these 2 selects? Or at least add validation code to catch possible problems.

Note: You will need the patch I have just submitted on bug 13618 comment 261 to test it correctly.
Comment 9 Katrin Fischer 2018-10-02 14:05:08 UTC
We don't do this in the MARC frameworks and I think some odd characters would be allowed in the standard... I've thought about it and tried to implement an HTML filter, but didn't succeed. But we are not that strict in other places and I think a list with 999+ elements is not the solution :) As I said, we even fields with letters in our database.
Comment 10 Jonathan Druart 2018-10-02 14:54:10 UTC
So maybe just a JS check for \d | \w ?
Comment 11 Martin Renvoize 2018-10-12 15:06:06 UTC
Checking for digits and word characters as per Jonathans suggestion sounds sane to me... is there really a case out there where a field may contain another character outside that range?
Comment 12 Owen Leonard 2018-10-12 16:31:41 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2018-10-12 16:43:20 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2018-10-12 16:59:43 UTC
Not sure if we should facilitate MARC deviation ?
Comment 15 Katrin Fischer 2018-10-12 17:02:51 UTC
We could also start supporting other ASCII characters like ;/% for subfield codes, that would give us more subfields for items to use - but it won't work right now and the upper case letter use is a reality not only for us but other libraries too (see Severine's comment).
Comment 16 Martin Renvoize 2018-10-17 14:05:13 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2018-10-17 14:05:18 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2018-10-17 14:06:24 UTC
(In reply to Marcel de Rooy from comment #14)
> Not sure if we should facilitate MARC deviation ?

I don't think this is too terrible a deviation, especially as it appears to already be widely in use.
Comment 19 Martin Renvoize 2018-10-17 14:06:53 UTC
It all works as expected for me and I can't find any regressions.

Passing QA
Comment 20 Jonathan Druart 2018-10-17 15:08:06 UTC
Sorry but... the validation does not work when a new field is added, only when it is edited.
Comment 21 Owen Leonard 2018-10-25 12:32:52 UTC Comment hidden (obsolete)
Comment 22 Mark Tompsett 2018-10-25 13:19:48 UTC
Created attachment 81150 [details] [review]
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config

The pull down for marc subfields on the item search fields config page is
long and hard to use. It also doesn't include the commonly used
upper case letters for custom item subfields.

Also hard to use with its 999 entries is the marc field list. It doesn't
allow for tags like 01e, which exist in our MARC21 default framework.

To test:
- Go to Administration > Item search fields
- Add different mappings with and without subfields
- Verify the entered values are stored correctly
- Edit mappings
- Verify editing works and changes save correctly

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 23 Mark Tompsett 2018-10-25 13:19:51 UTC
Created attachment 81151 [details] [review]
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields

This patch adds some custom validation to the MARC tag and subfield
fields so that they are limited to alphanumeric characters.

Both templates (the main view and the edit view) have been modified so
that item_search_fields.js can be included in both.

To test, apply the patch and test the form by entering a variety of
different character combinations. The "MARC field" and "MARC subfield"
inputs should only accept alphanumeric entries.

Test both "new" and "edit" operations.
Test other operations like delete and cancel.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 24 Martin Renvoize 2018-10-31 09:29:19 UTC
Maybe I'm being stupid.. but this doesn't seem to be working for me.

First off the validation doesn't seem to be activating on edit or new for me ( managed to enter 1~$ for example)

Second, when I go to edit, I'm faced with an empty form rather than one that contains the existing data for me to edit.

What am I doing wrong ?
Comment 25 Owen Leonard 2019-01-11 14:38:26 UTC Comment hidden (obsolete)
Comment 26 Owen Leonard 2019-01-11 14:38:29 UTC Comment hidden (obsolete)
Comment 27 José-Mario Monteiro-Santos 2019-01-11 20:20:42 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2019-02-27 18:12:02 UTC
Owen, I see

 pattern="[a-zA-Z0-9]+

and

 jQuery.validator.addMethod("marcfield", function(value, element) {
     return this.optional(element) || /^[0-9a-zA-Z]+$/.test(value);
 }, _("Please enter letters or numbers") );


Should not we have only 1 check?
Comment 29 Owen Leonard 2019-02-27 18:29:05 UTC
(In reply to Jonathan Druart from comment #28)

> Should not we have only 1 check?

The pattern attribute in the markup is a non-JS fallback to the jQuery validation. It's not strictly necessary since we require JS, but it feels like good practice to me.
Comment 30 Jonathan Druart 2019-02-27 18:43:53 UTC
My concern it that they must be identical, but nothing indicate it (reading the code).

In that case why the JS check when the patterns is enough?
Comment 31 Katrin Fischer 2019-02-27 18:54:02 UTC
It gives us a nicer error message in the correct GUI language I guess?
Comment 32 Owen Leonard 2019-02-27 19:13:58 UTC
(In reply to Jonathan Druart from comment #30)
> My concern it that they must be identical, but nothing indicate it (reading
> the code).

Similar to how the maxlength attribute on input fields should match the size of database columns and those get out of sync. I can submit a followup which adds comments if you'd like.
Comment 33 Chris Cormack 2019-04-14 21:59:37 UTC
Created attachment 87955 [details] [review]
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config

The pull down for marc subfields on the item search fields config page is
long and hard to use. It also doesn't include the commonly used
upper case letters for custom item subfields.

Also hard to use with its 999 entries is the marc field list. It doesn't
allow for tags like 01e, which exist in our MARC21 default framework.

To test:
- Go to Administration > Item search fields
- Add different mappings with and without subfields
- Verify the entered values are stored correctly
- Edit mappings
- Verify editing works and changes save correctly

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 34 Chris Cormack 2019-04-14 22:00:27 UTC
Created attachment 87956 [details] [review]
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields

This patch adds some custom validation to the MARC tag and subfield
fields so that they are limited to alphanumeric characters.

Both templates (the main view and the edit view) have been modified so
that item_search_fields.js can be included in both.

To test, apply the patch and test the form by entering a variety of
different character combinations. The "MARC field" and "MARC subfield"
inputs should only accept alphanumeric entries.

Test both "new" and "edit" operations.
Test other operations like delete and cancel.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Jose-Mario <jose-mario.monteiro-santos@inLibro.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 35 Chris Cormack 2019-04-14 22:00:53 UTC
Created attachment 87957 [details] [review]
Bug 20689 Follow - tiny typo fix
Comment 36 Nick Clemens 2019-04-18 11:07:00 UTC
I had two issues here:
1 - When editing the Item search fields the values set previously don't populate to the form i.e. create a new fsearch fields for 520$A - edit it and you must set the field/subfield again
2 - While other fields worked, setting 520$A and searching for a unique value in that field failed, not sure why
Comment 37 Owen Leonard 2019-04-18 12:21:49 UTC
Created attachment 88294 [details] [review]
Bug 20689: (follow-up) Correct scope of template variables

The removal of the <select>s should have been accompanied by a
correction to the scope of tagfield and tagsubfield. It is also
unnecessary to conditionally include the "value" attribute.

To test, apply the patch and test the process for editing an item search
field. All the correct values should appear in form fields during edit,
and all values should save correctly.
Comment 38 Owen Leonard 2019-04-18 14:46:49 UTC
My patch doesn't address this issue:

> 2 - While other fields worked, setting 520$A and searching for a unique
> value in that field failed, not sure why

I was unable to reproduce the problem. I modified a framework to include 520$A, added a value to a record, did an item search for that value, and the correct result was returned.
Comment 39 Nick Clemens 2019-04-19 10:58:34 UTC
The problem seems to be in ExtractValue:

SELECT 
    ExtractValue(metadata, '//record/datafield[@tag="520"]/subfield[@code="A"]')
FROM biblio_metadata 
WHERE 
    ExtractValue(metadata, '//record/datafield[@tag="520"]/subfield[@code="A"]') !="";

This returns both 'A' and 'a' subfields

We need to figure a method to make the query case sensitive
Comment 40 Fridolin Somers 2019-04-24 10:05:29 UTC
(In reply to Nick Clemens from comment #39)
> We need to figure a method to make the query case sensitive

Looks like we can set COLLATE like :
select city_name from cities where city_name='Paris' COLLATE utf8mb4_bin;
Comment 41 Owen Leonard 2019-05-08 15:56:42 UTC
(In reply to Nick Clemens from comment #39)
> The problem seems to be in ExtractValue:
...
> This returns both 'A' and 'a' subfields

Should this be considered another bug, which this bug should be made dependent on?
Comment 42 Nick Clemens 2019-05-09 13:55:03 UTC
(In reply to Owen Leonard from comment #41)
> (In reply to Nick Clemens from comment #39)
> > The problem seems to be in ExtractValue:
> ...
> > This returns both 'A' and 'a' subfields
> 
> Should this be considered another bug, which this bug should be made
> dependent on?

I think it can be done here or on another bug - it should be sorted before this is pushed though
Comment 43 Katrin Fischer 2020-01-06 14:08:29 UTC
*** Bug 13561 has been marked as a duplicate of this bug. ***
Comment 44 Katrin Fischer 2020-01-08 22:23:26 UTC
*** Bug 15384 has been marked as a duplicate of this bug. ***
Comment 45 Katrin Fischer 2021-08-23 23:17:57 UTC
(In reply to Fridolin Somers from comment #40)
> (In reply to Nick Clemens from comment #39)
> > We need to figure a method to make the query case sensitive
> 
> Looks like we can set COLLATE like :
> select city_name from cities where city_name='Paris' COLLATE utf8mb4_bin;

I believe that is not quite the solution yet, as it would affect the strings we compare to instead of only the subfield codes, as we intend.
Comment 46 Katrin Fischer 2021-08-23 23:40:22 UTC
Created attachment 124056 [details] [review]
Bug 20689: Replace marc (sub)field pull downs with input fields on item search fields config

The pull down for marc subfields on the item search fields config page is
long and hard to use. It also doesn't include the commonly used
upper case letters for custom item subfields.

Also hard to use with its 999 entries is the marc field list. It doesn't
allow for tags like 01e, which exist in our MARC21 default framework.

To test:
- Go to Administration > Item search fields
- Add different mappings with and without subfields
- Verify the entered values are stored correctly
- Edit mappings
- Verify editing works and changes save correctly

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 47 Katrin Fischer 2021-08-23 23:40:27 UTC
Created attachment 124057 [details] [review]
Bug 20689: (follow-up) Make it possible to add capital letter subfields to item search fields

This patch adds some custom validation to the MARC tag and subfield
fields so that they are limited to alphanumeric characters.

Both templates (the main view and the edit view) have been modified so
that item_search_fields.js can be included in both.

To test, apply the patch and test the form by entering a variety of
different character combinations. The "MARC field" and "MARC subfield"
inputs should only accept alphanumeric entries.

Test both "new" and "edit" operations.
Test other operations like delete and cancel.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Jose-Mario <jose-mario.monteiro-santos@inLibro.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 48 Katrin Fischer 2021-08-23 23:40:33 UTC
Created attachment 124058 [details] [review]
Bug 20689: (follow-up) Tiny typo fix

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 49 Katrin Fischer 2021-08-23 23:40:37 UTC
Created attachment 124059 [details] [review]
Bug 20689: (follow-up) Correct scope of template variables

The removal of the <select>s should have been accompanied by a
correction to the scope of tagfield and tagsubfield. It is also
unnecessary to conditionally include the "value" attribute.

To test, apply the patch and test the process for editing an item search
field. All the correct values should appear in form fields during edit,
and all values should save correctly.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 50 Katrin Fischer 2021-08-23 23:52:10 UTC
I've fixed a little conflict, signed the patches off, filed bug 28892 for the search issue and... would like to throw them back into the queue.

I feel like we should still push these patches, even with the search issue:

* To me this patch set provides a great usability improvement to the item search fields form as it gets rid of the mighty long pull downs that were hard to handle.

* The search issue already exists right now: if you are using capital subfields you will find their contents already mixed with the configurable lower case subfields. It doesn't work correctly now - that's why I made bug 28892 a "see also" instead of a "blocks" or "depends on".

* At the moment you could get around the form limitation by configuring the lower case letter for your upper case subfields. But as the MARC field and subfield are part of the displayed search option description, I think it will be nicer to allow the correct thing to be entered in the first place here.

* The search issue is probably only going to 'hit', when upper case subfields are used in bibliographic records. The more common use case for this kind of subfield are item fields, as all the lower case letters and numbers are already used up in MARC21. This also means, that there is a really high chance that only the capital letter subfield will be queried via more_subfields_xml, as the lower case one is very likely to be mapped to a database field. So in this case, the results will be accurate.

* The new form allows entering capital letter subfields, but it doesn't advertise it. If people think it's better we could change the bug title to highlight this as a usability improvement ... or we could add a hint explaining the limitation to the configuration page.
Comment 51 Katrin Fischer 2021-08-23 23:58:20 UTC
Note: only the first patch was from me - as that had been signed off, I felt I could for the follow-ups.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20689
bugs.koha-community.org
crawl=CC-MAIN-2021-43
121895
application/http; msgtype=response
sha1:VFFKHS3STFWUCUV3RQABL27U63D2NV7A
<urn:uuid:84d8e034-81f7-4bc3-b7f9-fc23ddebb007>
2021-10-23T18:22:57Z
72.14.190.217
text/html
sha1:ZDRYMYEJUSTD4HSCD73BQKABHQ4BHSYJ
<urn:uuid:f89c73a7-8a59-499a-9a34-ae30c329dade>
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20689
response
<urn:uuid:ce85ba47-e178-4bc2-8ed9-12e5ccd57857>
isPartOf: CC-MAIN-2021-43
publisher: Common Crawl
description: Wide crawl of the web for October 2021
operator: Common Crawl Admin (info@commoncrawl.org)
hostname: ip-10-67-67-149
software: Apache Nutch 1.18 (modified, https://github.com/commoncrawl/nutch/)
robots: checked via crawler-commons 1.2-SNAPSHOT (https://github.com/crawler-commons/crawler-commons)
format: WARC File Format 1.1
conformsTo: https://iipc.github.io/warc-specifications/specifications/warc-format/warc-1.1/