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: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 2 votes (vote)
Assignee: Katrin Fischer
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-01 16:05 UTC by Patrick Robitaille
Modified: 2019-05-09 13:55 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
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, M. 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, M. 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

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 M. 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 M. 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