Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches
Summary: Add support for custom PQF attributes for Z39.50 server searches
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Z39.50 / SRU / OpenSearch Servers (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bouzid
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-25 16:23 UTC by Frédérick Capovilla
Modified: 2017-05-01 23:30 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Add support for custom Z39.50 attributes for Z39.50 servers. (10.83 KB, patch)
2013-11-25 16:23 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
Add support for custom Z39.50 attributes to authority searches. (5.74 KB, patch)
2013-11-25 16:24 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.01 KB, patch)
2015-07-07 19:52 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
[Signed-off] Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.24 KB, patch)
2015-07-09 14:51 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.22 KB, patch)
2016-03-03 19:29 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.10 KB, patch)
2016-04-19 14:39 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.10 KB, patch)
2016-04-19 14:40 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.18 KB, patch)
2016-04-19 14:59 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 11297: QA follow-up - Some fixes (2.81 KB, patch)
2016-04-28 20:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: QA follow-up - Some fixes (2.81 KB, patch)
2017-01-23 17:28 UTC, Bouzid
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.26 KB, patch)
2017-05-01 23:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11297: QA follow-up - Some fixes (2.88 KB, patch)
2017-05-01 23:30 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 Frédérick Capovilla 2013-11-25 16:23:24 UTC Comment hidden (obsolete)
Comment 1 Frédérick Capovilla 2013-11-25 16:24:53 UTC Comment hidden (obsolete)
Comment 2 Frédérick Capovilla 2013-11-25 16:30:02 UTC
Setting patch complexity to medium because of small DB changes.
Comment 3 David Cook 2014-02-07 02:57:09 UTC
Frédérick, do you have any use cases besides the @attr 4 you mentioned?

In this case, I think it might be a better idea to add a "structure" drop-down menu to the Z39.50 search pages. That way, people could specify "phrase" (@attr 4=1) or word (@attr 4=2). If we default to "phrase", that would resolve your problem, I believe.

I haven't tried the patch, but your patch seems a bit unwieldy to me conceptually. Perhaps you could explain its utility a bit more?

--

Personally, I would really like to see a table for mapping Bib-1/PQF attributes to specific Z39.50 servers, since different servers (especially Innovative Interfaces, iirc) often implement different Bib-1 attributes for particular indexes, despite the standard profile. 

However, I think that's tangential to the issue you're describing here, yes?



Source: http://www.loc.gov/z3950/agency/defns/bib1.html#structure
Comment 4 Frédérick Capovilla 2014-02-17 21:22:44 UTC
Unfortunately, I cannot provide a use-case you could reproduce because we encountered our search problems on a private server requiring a subscription with a username and a password.

In our case, the BANQ server would not return results if we did not specify a value for "@attr 4".
With this patch, we can set default PQF attributes for a all searches on this Z39.50 server without affecting search results on other Z39.50 servers. This is useful if we want to do a single Z39.50 search on multiple servers at the same time, each with their own supported or required attributes.

For example, specifying "@attr 4=1" on server 1 will not cause an error on server 2 if this one doesn't support "@attr 4" attributes.

--

Finally, what you are describing affects "Use" (@attr1) attributes. The main use of this patch is to set the other attributes which won't change depending of the used search terms.

However, the same methods could be used to implement a "mapping table". This patch changes the PQF query generation technique to use an Array instead of a String, so each part of the query can easily be overwritten to fit each server's requirements.
Comment 5 David Cook 2014-02-18 00:42:17 UTC
(In reply to Frédérick Capovilla from comment #4)
> Unfortunately, I cannot provide a use-case you could reproduce because we
> encountered our search problems on a private server requiring a subscription
> with a username and a password.
> 

Sorry for the confusion, Frédérick. I didn't mean a test case. Rather, do you have any other examples (than your current case) where a person might want to provide arbitrary attributes at the server level?

> In our case, the BANQ server would not return results if we did not specify
> a value for "@attr 4".
> With this patch, we can set default PQF attributes for a all searches on
> this Z39.50 server without affecting search results on other Z39.50 servers.
> This is useful if we want to do a single Z39.50 search on multiple servers
> at the same time, each with their own supported or required attributes.
> 
> For example, specifying "@attr 4=1" on server 1 will not cause an error on
> server 2 if this one doesn't support "@attr 4" attributes.

That's fair enough. I've noticed that a lot of Z39.50 servers have very different levels of implementation of Bib-1 attributes. It would be unfortunate to find the solution for one server causing an error on another.

> 
> --
> 
> Finally, what you are describing affects "Use" (@attr1) attributes. The main
> use of this patch is to set the other attributes which won't change
> depending of the used search terms.
> 
> However, the same methods could be used to implement a "mapping table". This
> patch changes the PQF query generation technique to use an Array instead of
> a String, so each part of the query can easily be overwritten to fit each
> server's requirements.

Yes, I know that I was describing "Use" attributes. I'm fairly familiar with Bib-1 attributes. I was referring to configuring the "Use" attributes at the server level (since different servers sometimes use different "Use" attributes for the same access point), as you were mentioning setting "Structure" attributes at the server level. In theory, you would map attributes to access points. In this case, maybe one would map @attr 1=4 @attr 4=1 to "Title" so that the default Z39.50 "Title" search would be a "Title Phrase" search. You could also do something like @attr 1=1016 @attr 4=6 to do a "Keyword" search. Since the mappings would be at the server level, you wouldn't have to worry about servers that couldn't support "Structure" attributes.

I disagree that the "@attr 4" "Structure" attributes won't change depending on the search terms. As I mention in the above paragraph, you might want to use @attr 4=1 (phrase) for a phrase search, but you might want to use @attr 4=4 (4 digit year) for a year search. Or maybe @attr 4=6 (word list) where you you're performing a more "Google-like" search. That said, Bib-1 attributes are quite esoteric. Most staff users probably wouldn't know that they could even provide them. Of course, it's probably more likely to have Koha admin than a staff members setting up Z39.50 servers, so it might not matter.

After looking at the patch, it looks like it would work for your use case, but it seems like a bit of a hack to me. I also wonder how many people will use it incorrectly.

Maybe it does make sense to make all queries for a server use one structure attribute like 6 (word list), but I think it makes more sense to use a drop-down menu than a free text field. That said, the free text field is more flexible (albeit more error prone). 

I don't mean to sound harsh or overly critical. I'm just trying to add another perspective.
Comment 6 Frédérick Capovilla 2014-02-18 16:04:07 UTC
> Yes, I know that I was describing "Use" attributes. I'm fairly familiar with
> Bib-1 attributes. I was referring to configuring the "Use" attributes at the
> server level (since different servers sometimes use different "Use"
> attributes for the same access point), as you were mentioning setting
> "Structure" attributes at the server level. In theory, you would map
> attributes to access points. In this case, maybe one would map @attr 1=4
> @attr 4=1 to "Title" so that the default Z39.50 "Title" search would be a
> "Title Phrase" search. You could also do something like @attr 1=1016 @attr
> 4=6 to do a "Keyword" search. Since the mappings would be at the server
> level, you wouldn't have to worry about servers that couldn't support
> "Structure" attributes.

I understood what you were saying in your first reply, and it would be a great to add this feature in Koha.

I was just explaining that the attached patch should not be used to set "Use" attributes in its current state, because it would force a single "Use" value for all searches.

> I disagree that the "@attr 4" "Structure" attributes won't change depending
> on the search terms. As I mention in the above paragraph, you might want to
> use @attr 4=1 (phrase) for a phrase search, but you might want to use @attr
> 4=4 (4 digit year) for a year search. Or maybe @attr 4=6 (word list) where
> you you're performing a more "Google-like" search. That said, Bib-1
> attributes are quite esoteric. Most staff users probably wouldn't know that
> they could even provide them. Of course, it's probably more likely to have
> Koha admin than a staff members setting up Z39.50 servers, so it might not
> matter.

Normally, Z39.50 servers should have a default "Structure" value that is automatically used if the "Structure" attribute is not provided in the search query. In our case, the BANQ server does not provide a default value and requires us to always provide this attribute. The new preference added by this patch is there to force a default value for this server so it actually returns results. It could be used on other servers with special attribute requirements.

> After looking at the patch, it looks like it would work for your use case,
> but it seems like a bit of a hack to me. I also wonder how many people will
> use it incorrectly.
> 
> Maybe it does make sense to make all queries for a server use one structure
> attribute like 6 (word list), but I think it makes more sense to use a
> drop-down menu than a free text field. That said, the free text field is
> more flexible (albeit more error prone). 

Since the preference is kind of a "power user" feature with rare use cases, I didn't think it was worth the effort to create a special field structure for this one feature. And, like you said, the free text field allows more flexibility.

> I don't mean to sound harsh or overly critical. I'm just trying to add
> another perspective.

I'm happy to receive constructive criticism like yours.

In the end, I agree that you idea of using some sort of "mapping table" would be the ideal solution for many Z39.50 search problems including ours. In our case, with that kind of feature, we could simply add "@attr 4=1" to all search terms in the mapping table for the BANQ server and it would fix our problem.

For now, this patch fixes a problem we are experiencing with one of the Z39.50 servers we use.
It also improves the PQF query generation code of Z39.50 searches so query attributes can be overwritten on a per-server basis. This patch could be improved to permit more advanced per-server query manipulations.
Comment 7 M. Tompsett 2015-05-17 14:42:40 UTC
Comment on attachment 23144 [details] [review]
Add support for custom Z39.50 attributes for Z39.50 servers.

Review of attachment 23144 [details] [review]:
-----------------------------------------------------------------

Also, missing kohastructure.sql modifications.

::: installer/data/mysql/updatedatabase.pl
@@ +7331,5 @@
>  }
>  
> +$DBversion = "3.13.00.XXX";
> +if ( CheckVersion($DBversion) ) {
> +    $dbh->do("ALTER TABLE z3950servers ADD COLUMN attributes VARCHAR(255);");

please include an explicit AFTER, so that things run out of order will generate some layout. :)
Comment 8 Maxime Beaulieu 2015-07-07 19:52:37 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2015-07-09 14:51:23 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-08-05 11:20:05 UTC
I have the same feeling as David, it sounds a bit hacky.
I understand your problem, but you have to provide something useful for others, not a specific stuffs for you.
I would like to see some docs on the help page to explain what it is possible to do with this new fields.
As David said, I don't think it's a good idea to add this field at the server level, we could have the need to use 4=109 and 4=1 in the same query.
A dropdown list in front of each field sounds more useful and flexible.

I don't mark it as failed, I would like to get another QAer's opinion.
Comment 11 Blou 2015-08-17 13:24:30 UTC
fyi, There is nothing in the current patch that would prevent "4=109 and 4=1".
Comment 12 Jonathan Druart 2015-08-18 14:50:57 UTC
(In reply to Blou from comment #11)
> fyi, There is nothing in the current patch that would prevent "4=109 and
> 4=1".

There is nothing in the current patch to make it possible :)
Comment 13 Blou 2015-08-18 15:47:04 UTC
Actually, I said that because the old patch (which was waaay impossible to apply anymore) would have replaced one 4= by another.  In this new patch, Maxime prepends the 4=1 to whatever is there by default (4=109, let say), so I figure both are sent.

small characters: But I might be wrong. :)
Comment 14 Jonathan Druart 2015-09-23 12:06:41 UTC
Other QA pov needed.
Comment 15 David Cook 2015-09-25 03:23:58 UTC
(In reply to Blou from comment #13)
> Actually, I said that because the old patch (which was waaay impossible to
> apply anymore) would have replaced one 4= by another.  In this new patch,
> Maxime prepends the 4=1 to whatever is there by default (4=109, let say), so
> I figure both are sent.
> 
> small characters: But I might be wrong. :)

I just took a look at the patch, and that sounds...interesting. I'd have to do more research, but http://www.loc.gov/z3950/agency/defns/bib1.html recommends not repeating attribute types in an attribute list. I imagine server behaviour might vary here as well... I imagine some servers might use the first value (so 4=1 even though 4=109 comes later), or might return an error, or do some other behaviour. 

I think what Jonathan was saying is that there's no way to provide 4=1 and 4=109 in a single query. For instance:

@and @attr 1=4 @attr 4=1 "The cat in the hat" @attr 1=12 @attr 4=109 "123456789"

The patch would only allow the following:

@attr 4=1 @and @attr 1=4 "The cat in the hat" @attr 1=12 "123456789"

Which effectively is the same as 

@and @attr 1=4 @attr 4=1 "The cat in the hat" @attr 1=12 @attr 4=1 "123456789"

--

This could cause a problem if "123456789" isn't stored in a phrase index but is instead stored in a numeric index. 

That being said, Zebra's default structure attribute is phrase (4=1)(http://www.indexdata.com/zebra/doc/querymodel-rpn.html). 

But the default structure attribute can range from server to server... so I don't think it would be wise to add default structure attributes for all servers...

--

After some musing, I think that I'm OK with this patch. Overall, I don't like it. I rather given control of the structure to the user doing the Z39.50 search.

However, since the user is only inputting the terms and we currently control everything else about the PQF query formation, perhaps it doesn't hurt to allow us to provide values that apply to the whole query.

That being said, I think this attribute field needs a validator. Only valid PQF should be allowed in this field, and of that PQF, USE attributes should never ever be allowed. 

I'm skeptical about other attribute types being allowed as well, but I suppose they may be necessary in the event that a Z39.50 target doesn't have default values... 

--

I also think tests should be done to see what happens when you provide a structure attribute at the query level and what happens when you provide a different one at the term level... in case we want to change the structure attribute for specific fields in the future...

Anyway, that's my 2 cents ;)
Comment 16 David Cook 2015-09-25 03:56:43 UTC
tl;dr

In summary, if this patch is to pass QA, I think the attributes field needs to be validated to only contain PQF attributes, to contain no USE attributes, and to include no duplicated attribute types.

As per http://www.loc.gov/z3950/agency/defns/bib1.html, an attribute type should only appear once in an attribute list. (There is a mention that you can repeat attribute types if you provide semantic guidance but I don't see a way of doing that within the constraints of the PQF grammar: http://www.indexdata.com/yaz/doc/tools.html#PQF.)

This means that Blou's mention of 4=1 4=109 is not in accordance with the spec.

However, it seems to me that you can provide 4=1 at the query level and 4=109 at the term level in a query, since they belong to separate attribute lists. If you look at http://www.indexdata.com/zebra/doc/querymodel-rpn.html, it states that:

"Any of the orthogonal attribute types may be omitted, these are inherited from higher query tree nodes, or if not inherited, are set to the default Zebra configuration values."

To me, that sounds like you could provide 4=1 as a query level default structure attribute, and then provide a more specific structure attribute at the term level (e.g. 4=109) which will be used instead of 4=1 as it's more specific. If other terms don't have a more specific attribute, they'll inherit from the query level, or from the servers default, if there is one.

Anyway, going back to whether or not this patch is suitable in Koha...


I re-read the Bib-1 Set docs from the Library of Congress (http://www.loc.gov/z3950/agency/defns/bib1.html), and stumbled upon an important passage:

"If an attribute type does not occur in an attribute list, then (in the absence of any prior understanding, either outside of the standard or via the Explain facility) the origin should not expect any particular default target behavior."

This led me to another passage at http://www.loc.gov/z3950/agency/bib1.html:

"Within an attribute list, each attribute type is optional.  However, if
a particular attribute type is not supplied, this document does not
address target behavior -- a given target might supply a default
attribute, dynamically select an appropriate attribute based on the
other attributes supplied, or fail the search because it requires that
the attribute type be supplied."

Based on these passages, I think it is imperative that we provide some capability for providing default attributes to a query. Providing them at the query level rather than at the term level is perhaps the most sensible. 

Ultimately... I've changed my mind and I'm now in favour of this feature... but I think is needs to be validate the attributes that the user is providing.
Comment 17 Jonathan Druart 2015-09-28 11:42:34 UTC
(In reply to David Cook from comment #16)
> Ultimately... I've changed my mind and I'm now in favour of this feature...
> but I think is needs to be validate the attributes that the user is
> providing.

So could we get your signoff, 2 is better than 1! :)
Comment 18 David Cook 2015-09-29 04:36:14 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to David Cook from comment #16)
> > Ultimately... I've changed my mind and I'm now in favour of this feature...
> > but I think is needs to be validate the attributes that the user is
> > providing.
> 
> So could we get your signoff, 2 is better than 1! :)

Well, I'd prefer a validation follow-up be added, but I'll look at testing this next week in any case.
Comment 19 David Cook 2015-10-06 03:01:14 UTC
Probably won't have time to test this one actually...
Comment 20 Katrin Fischer 2015-10-22 21:22:35 UTC
Some first results:
- Please submit the Schema changes as a separate patch next time.
- The new attributes field should not show up on the "New SRU server" page - I believe it does not apply there. Please fix.
- While validation would be nice, we don't provide it for the other fields here and I am not sure how it could work. So I would not insist on it, but any help we can give the user is of course always welcome. That's why I second Jonathan's comment 10 - we need some end user documentation on how to use this. Please provide an update to the help file.
- Last but not least: I'd really like to see a second use case where this feature is helpful - so not only the one server causing problems, but maybe another good/clever idea on how to use this feature.
Comment 21 David Cook 2015-10-22 22:27:27 UTC
(In reply to Katrin Fischer from comment #20)
> - Last but not least: I'd really like to see a second use case where this
> feature is helpful - so not only the one server causing problems, but maybe
> another good/clever idea on how to use this feature.

It's not just about the one server. It theoretically could be any server.

http://www.loc.gov/z3950/agency/bib1.html:

"Within an attribute list, each attribute type is optional.  However, if
a particular attribute type is not supplied, this document does not
address target behavior -- a given target might supply a default
attribute, dynamically select an appropriate attribute based on the
other attributes supplied, or fail the search because it requires that
the attribute type be supplied."

At the moment, we don't have a way for users to specify Bib-1 attributes, so some servers will fail for all Z39.50 searches. 

Without this patch (or a similar one), I think the way we do Z39.50 searches is short-sighted. 

While Zebra will provide default attributes, it's clear that not all Z39.50 targets do. 

I don't think there are any other good/clever ideas for other ways to use this feature. It basically lets you provide attributes that are applied to the rest of the query. I suppose in other use cases... it lets you specify new defaults.

For instance, Zebra will default to a Structure attribute of "phrase". Maybe you would prefer to default to a Structure attribute of "word list" instead, so you can specify that using this patch, and there you go. 

Or maybe you want to use a "relevance relation" for all your queries to a target, so you specify @attr 2=102.

Or maybe you don't want to truncate anything in your queries so you supply @attr 5=100.
Comment 22 Katrin Fischer 2015-10-22 22:33:26 UTC
Hi David,
thx for the quick reply and the examples - I'll say that reasolved that point in my list. Your examples would be great for the help page.
Comment 23 David Cook 2015-10-25 23:26:25 UTC
(In reply to Katrin Fischer from comment #22)
> Hi David,
> thx for the quick reply and the examples - I'll say that reasolved that
> point in my list. Your examples would be great for the help page.

I keep meaning to update the manual and help pages in regards to Zebra and Z39.50... I'll keep this on my to do list!
Comment 24 Blou 2015-12-01 14:28:58 UTC
If I summarize,
- Need for the help file to be modified
- Require dropdown at the server level.

We do not understand that last one : "As David said, I don't think it's a good idea to add this field at the server level, we could have the need to use 4=109 and 4=1 in the same query. A dropdown list in front of each field sounds more useful and flexible."

The 4=109 4=1 are currently possible with this patch.  As for the dropdown, where would that be?  I can't picture it, I'm sorry.
But wherever, that would be a more complex patch for a specialized need that "if you need it, you'll know what to do here".  IMO.

I'm sorry I can't offer any other use case.  For us it's major since EVERYBODY requires this since this is our national library's z3950 that needs it.
Comment 25 Rémi Mayrand-Provencher 2016-03-03 19:29:24 UTC Comment hidden (obsolete)
Comment 26 Rémi Mayrand-Provencher 2016-03-03 19:30:24 UTC
fixed conflicts, patch should apply correctly.
Comment 27 Marc Véron 2016-04-17 12:45:45 UTC
Sorry, patch does not apply:
Applying: Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/z3950servers.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/z3950servers.tt
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Breeding.pm
Failed to merge in the changes.
Comment 28 Rémi Mayrand-Provencher 2016-04-19 14:39:31 UTC Comment hidden (obsolete)
Comment 29 Rémi Mayrand-Provencher 2016-04-19 14:40:19 UTC Comment hidden (obsolete)
Comment 30 Rémi Mayrand-Provencher 2016-04-19 14:40:56 UTC
Fixed conflicts
Comment 31 Marc Véron 2016-04-19 14:59:52 UTC
Created attachment 50420 [details] [review]
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches.

Adds the "Attributes" field to z3950 servers.

The feature here is not quite de same.

In the old patches, the attributes were applied to individual query parts if the part already contains "@attr" and the additionnal attribute is not already in the query part.

Here, the content of the new field is prepended to all PQF queries sent to the server.

This new way of doing is simpler and works for the sponsor.

Test plan:
 I) Apply the patch
II) Run updatedatabase.pl

1) Add a new z3950 server with the following parameters:
Hostname : catalogue.banq.qc.ca
Port     : 210
Database : IRIS
Syntax   : Marc21

2) Perform a z3950 search on that server.
    Keyword (Any) : egypt
2.1) Nothing Found.

3) Add attributes on the server administration page
    @attr 4=1

4) Perform the same z3950 search
4.1) A lot of results

	modified:   C4/Breeding.pm
	modified:   Koha/Schema/Result/Z3950server.pm
	modified:   admin/z3950servers.pl
	new file:   installer/data/mysql/atomicupdate/Bug11297_z3950servers_attributes.sql
	modified:   installer/data/mysql/kohastructure.sql
	modified:   koha-tmpl/intranet-tmpl/prog/en/modules/admin/z3950servers.tt

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

New sign-off, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 32 David Cook 2016-04-22 06:50:18 UTC
I know I did a 180 on my support for this patch over time, but just adding a verbal/textual endorsement of this latest patch. It doesn't need it, as Marc has already signed it off, but a quick review of the code makes it look good.

You never know when you need to add a different PQF attribute to the entire query.
Comment 33 Katrin Fischer 2016-04-28 20:45:18 UTC
Created attachment 50954 [details] [review]
Bug 11297: QA follow-up - Some fixes

- Makes Attributes only show up when adding a new Z39.50 server,
  not for SRU
- Fixes typo in db documentation
- Fixes sorting of table columns
Comment 34 Katrin Fischer 2016-04-28 20:46:21 UTC
I have attached a follow-up patch, but stumble on the missing unit tests for the change.
Comment 35 Jesse Weaver 2016-04-28 20:48:47 UTC
This should have unit tests; one approach would be to move the attribute-prepending logic into a small function (along the lines of _build_query and _translate_query) and add some tests to t/db_dependent/Breeding.t.
Comment 36 Blou 2016-04-29 12:55:28 UTC
I understand the need for unit test, practically and philosophically.  But this is splitting hair.  It's just a string concat.  Creating a local function for a simple string concat simply to test it it just... ugly.  Practically and philosophically.  Testing the wellness of the whole function, I can dig.  Testing what equals to a perl keyword (I exagerate), I think that's just pain.
Comment 37 Marcel de Rooy 2016-04-30 13:23:16 UTC
(In reply to Blou from comment #36)
> I understand the need for unit test, practically and philosophically.  But
> this is splitting hair.  It's just a string concat.  Creating a local
> function for a simple string concat simply to test it it just... ugly. 
> Practically and philosophically.  Testing the wellness of the whole
> function, I can dig.  Testing what equals to a perl keyword (I exagerate), I
> think that's just pain.

Splitting hair. Nice to learn some new expressions here (no native speaker, sorry).
I would agree with you that is not necessary to test string concatenation. We should be pragmatic in this regard.
I guess the point here is not if you can concatenate strings but if you add this stuff to the query, will we still have results? Can you predict what will happen in a test case?
This will be hard because we probably do not like to query external sources in a unit test.
I once added some stuff in t/db_dependent/Breeding.t in order to start some testing of this module.
Comment 38 David Cook 2016-05-02 00:31:40 UTC
(In reply to Marcel de Rooy from comment #37)
> I would agree with you that is not necessary to test string concatenation.

I disagree. I think it is important to test string concatenation, as the query is just a string, and it's possible that concatenation could mess up the query.

> We should be pragmatic in this regard.
> I guess the point here is not if you can concatenate strings but if you add
> this stuff to the query, will we still have results? Can you predict what
> will happen in a test case?
> This will be hard because we probably do not like to query external sources
> in a unit test.
> I once added some stuff in t/db_dependent/Breeding.t in order to start some
> testing of this module.

Spot on! 

Alternatives to querying an external source would be parsing the query. We might be able to use the ZOOM modules for that.

Example:
$q = new ZOOM::Query::PQF('@attr 1=4 dinosaur');

"Creates a new query object, compiling the query passed as its argument according to the rules of the particular query-type being instantiated. If compilation fails, an exception is thrown. Otherwise, the query may be passed to the Connection method search()."

http://search.cpan.org/~mirk/Net-Z3950-ZOOM/lib/ZOOM.pod#ZOOM::Query

BOOM! Surely that should work?
Comment 39 Katrin Fischer 2016-08-14 10:47:18 UTC
Patches still apply - can we rescue this? 

The only left obstacle appears to be a missing unit test. Looking at comment#38 from David - could this be a possible way?
Comment 40 David Cook 2016-08-31 03:12:16 UTC
(In reply to Katrin Fischer from comment #39)
> Patches still apply - can we rescue this? 
> 
> The only left obstacle appears to be a missing unit test. Looking at
> comment#38 from David - could this be a possible way?

I reckon that method I suggested would be a good test. It would be nice to rescue this one. While I don't think it'll be used by many, it would be good to get it in for the sake of CCSR and anyone else afflicted by this issue (or anyone wanting to customize their queries I suppose).
Comment 41 Bouzid 2017-01-23 17:28:04 UTC
Created attachment 59448 [details] [review]
Bug 11297: QA follow-up - Some fixes

- Makes Attributes only show up when adding a new Z39.50 server,
  not for SRU
- Fixes typo in db documentation
- Fixes sorting of table columns
Comment 42 Bouzid 2017-01-23 17:29:03 UTC
I rebase patches, it's apply correctly
Comment 43 Katrin Fischer 2017-05-01 23:29:47 UTC
Created attachment 62938 [details] [review]
[SIGNED OFF] Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches.

Adds the "Attributes" field to z3950 servers.

The feature here is not quite de same.

In the old patches, the attributes were applied to individual query parts if the part already contains "@attr" and the additionnal attribute is not already in the query part.

Here, the content of the new field is prepended to all PQF queries sent to the server.

This new way of doing is simpler and works for the sponsor.

Test plan:
 I) Apply the patch
II) Run updatedatabase.pl

1) Add a new z3950 server with the following parameters:
Hostname : catalogue.banq.qc.ca
Port     : 210
Database : IRIS
Syntax   : Marc21

2) Perform a z3950 search on that server.
    Keyword (Any) : egypt
2.1) Nothing Found.

3) Add attributes on the server administration page
    @attr 4=1

4) Perform the same z3950 search
4.1) A lot of results

	modified:   C4/Breeding.pm
	modified:   Koha/Schema/Result/Z3950server.pm
	modified:   admin/z3950servers.pl
	new file:   installer/data/mysql/atomicupdate/Bug11297_z3950servers_attributes.sql
	modified:   installer/data/mysql/kohastructure.sql
	modified:   koha-tmpl/intranet-tmpl/prog/en/modules/admin/z3950servers.tt

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

New sign-off, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 44 Katrin Fischer 2017-05-01 23:30:09 UTC
Created attachment 62939 [details] [review]
[SIGNED OFF] Bug 11297: QA follow-up - Some fixes

- Makes Attributes only show up when adding a new Z39.50 server,
  not for SRU
- Fixes typo in db documentation
- Fixes sorting of table columns

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 45 Katrin Fischer 2017-05-01 23:30:44 UTC
Leaving final decisions about unit tests to QA team.