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: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Z39.50 / SRU / OpenSearch Servers (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Charles Farmer
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 24708
  Show dependency treegraph
 
Reported: 2013-11-25 16:23 UTC by Frédérick Capovilla
Modified: 2020-11-30 21:45 UTC (History)
20 users (show)

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


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 Fergani
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
Bug 11297: QA follow-up - Avoid carrying server attributes (1.34 KB, patch)
2017-12-28 20:31 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.25 KB, patch)
2017-12-28 20:55 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297: QA follow-up - Some fixes (5.37 KB, patch)
2017-12-28 20:56 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297: QA follow-up - Avoid carrying server attributes (1.34 KB, patch)
2017-12-28 20:56 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.25 KB, patch)
2017-12-28 20:59 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297: QA follow-up - Some fixes (5.37 KB, patch)
2017-12-28 20:59 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297: QA follow-up - Avoid carrying server attributes (1.34 KB, patch)
2017-12-28 20:59 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.24 KB, patch)
2018-02-20 06:13 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297: QA follow-up - Some fixes (5.39 KB, patch)
2018-02-20 06:13 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297: QA follow-up - Avoid carrying server attributes (1.34 KB, patch)
2018-02-20 06:13 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (8.26 KB, patch)
2018-02-21 20:15 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add atomicupdate patch and modify kohastructure.sql (1.75 KB, patch)
2018-02-21 20:15 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Modify Z3950server schema file (1.46 KB, patch)
2018-02-21 20:15 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (6.09 KB, patch)
2018-08-10 21:02 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add atomicupdate patch and modify kohastructure.sql (1.75 KB, patch)
2018-08-10 21:02 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Modify Z3950server schema file (1.46 KB, patch)
2018-08-10 21:03 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches. (6.09 KB, patch)
2018-08-10 21:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Add atomicupdate patch and modify kohastructure.sql (1.75 KB, patch)
2018-08-10 21:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297 - Modify Z3950server schema file (1.46 KB, patch)
2018-08-10 21:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297: Add support for custom PQF attributes for Z39.50 server searches. (6.17 KB, patch)
2018-08-29 20:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: Add atomicupdate patch and modify kohastructure.sql (1.80 KB, patch)
2018-08-29 20:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: Modify Z3950server schema file (1.51 KB, patch)
2018-08-29 20:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: (follow-up) Add html filter to new input field (1.32 KB, patch)
2018-08-29 20:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: (follow-up) Add html filter to new input field (1.74 KB, patch)
2018-10-10 21:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: Add support for custom PQF attributes for Z39.50 server searches. (6.23 KB, patch)
2018-10-12 14:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: Add atomicupdate patch and modify kohastructure.sql (1.86 KB, patch)
2018-10-12 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: Modify Z3950server schema file (1.58 KB, patch)
2018-10-12 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: (follow-up) Add html filter to new input field (1.81 KB, patch)
2018-10-12 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: (QA follow-up) Make the database update script idempotent (1.71 KB, patch)
2018-10-26 18:32 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 11297: (QA follow-up) Make the database update script idempotent (1.77 KB, patch)
2018-10-27 07:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: Add support for custom PQF attributes for Z39.50 server searches. (6.23 KB, patch)
2018-10-27 07:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: Add atomicupdate patch and modify kohastructure.sql (2.19 KB, patch)
2018-10-27 07:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: Modify Z3950server schema file (1.58 KB, patch)
2018-10-27 07:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: (follow-up) Add html filter to new input field (1.81 KB, patch)
2018-10-27 07:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11297: Add support for custom PQF attributes for Z39.50 server searches. (6.32 KB, patch)
2019-09-12 13:33 UTC, Bin Wen
Details | Diff | Splinter Review
Bug 11297: Add atomicupdate patch and modify kohastructure.sql (2.19 KB, patch)
2019-09-12 13:33 UTC, Bin Wen
Details | Diff | Splinter Review
Bug 11297: Modify Z3950server schema file (1.53 KB, patch)
2019-09-12 13:33 UTC, Bin Wen
Details | Diff | Splinter Review
Bug 11297: (follow-up) Add html filter to new input field (1.81 KB, patch)
2019-09-12 13:33 UTC, Bin Wen
Details | Diff | Splinter Review
Bug 11297: Add support for custom PQF attributes for Z39.50 server searches. (6.38 KB, patch)
2020-02-07 22:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 11297: Add atomicupdate patch and modify kohastructure.sql (2.25 KB, patch)
2020-02-07 22:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 11297: Modify Z3950server schema file (1.58 KB, patch)
2020-02-07 22:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 11297: (follow-up) Add html filter to new input field (1.85 KB, patch)
2020-02-07 22:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 11297: Add support for custom PQF attributes for Z39.50 server searches. (6.44 KB, patch)
2020-02-19 00:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: Add atomicupdate patch and modify kohastructure.sql (2.31 KB, patch)
2020-02-19 00:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: Modify Z3950server schema file (1.63 KB, patch)
2020-02-19 00:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: (follow-up) Add html filter to new input field (1.91 KB, patch)
2020-02-19 00:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11297: (QA follow-up) Make table headings match other pages - use Actions instead of Options (1.47 KB, patch)
2020-02-19 00:16 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 Mark 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 Comment hidden (obsolete)
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 Fergani 2017-01-23 17:28:04 UTC Comment hidden (obsolete)
Comment 42 Bouzid Fergani 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 Comment hidden (obsolete)
Comment 44 Katrin Fischer 2017-05-01 23:30:09 UTC Comment hidden (obsolete)
Comment 45 Katrin Fischer 2017-05-01 23:30:44 UTC
Leaving final decisions about unit tests to QA team.
Comment 46 Marcel de Rooy 2017-08-25 09:21:51 UTC
QA: Looking here now
Comment 47 Marcel de Rooy 2017-08-25 09:31:12 UTC
Please look at the changed code: 

 my ( $zquery, $squery ) = _build_query( $pars );
 [...]
 foreach my $server ( @servers ) {
        if(my $attributes = $server->{attributes}){
            $zquery = "$attributes $zquery";
        }
        $oConnection[$s] = _create_connection( $server );
        $oResult[$s] =
            $server->{servertype} eq 'zed'?
                $oConnection[$s]->search_pqf( $zquery ):
                $oConnection[$s]->search(new ZOOM::Query::CQL(
                    _translate_query( $server, $squery )));
        $s++;
    }

After we build zquery, you start prefixing it each time with more server attributes. So the last server will get them all. Wrong !
Probably this was tested with one server at a time. But it might fail for a search on multiple servers.
Comment 48 Charles Farmer 2017-12-28 20:31:01 UTC Comment hidden (obsolete)
Comment 49 Charles Farmer 2017-12-28 20:55:53 UTC Comment hidden (obsolete)
Comment 50 Charles Farmer 2017-12-28 20:56:01 UTC Comment hidden (obsolete)
Comment 51 Charles Farmer 2017-12-28 20:56:06 UTC Comment hidden (obsolete)
Comment 52 Charles Farmer 2017-12-28 20:59:25 UTC Comment hidden (obsolete)
Comment 53 Charles Farmer 2017-12-28 20:59:35 UTC Comment hidden (obsolete)
Comment 54 Charles Farmer 2017-12-28 20:59:43 UTC Comment hidden (obsolete)
Comment 55 Charles Farmer 2017-12-28 21:03:37 UTC Comment hidden (obsolete)
Comment 56 Charles Farmer 2018-02-20 06:13:21 UTC Comment hidden (obsolete)
Comment 57 Charles Farmer 2018-02-20 06:13:26 UTC Comment hidden (obsolete)
Comment 58 Charles Farmer 2018-02-20 06:13:30 UTC Comment hidden (obsolete)
Comment 59 Charles Farmer 2018-02-20 06:24:17 UTC
Freshly rebased.

To facilitate future git apply, Z3950Server.pm was regenerated. Field `attributes` now sits under `sru_fields` and `add_xslt` with longtext, instead of the previous mediumtext that conflicted with the master's version of the Schema.
Comment 60 Jonathan Druart 2018-02-20 15:08:02 UTC
(In reply to Charles Farmer from comment #59)
> Freshly rebased.
> 
> To facilitate future git apply, Z3950Server.pm was regenerated. Field
> `attributes` now sits under `sru_fields` and `add_xslt` with longtext,
> instead of the previous mediumtext that conflicted with the master's version
> of the Schema.

That is why it is suggested to add 1 patch for atomic update/kohastructure and 1 patch for the DBIC schema changes.
Comment 61 Charles Farmer 2018-02-20 15:28:31 UTC
If it'll make everyone's life easier, I'll split this development appropriately in the coming days.

Marking 'failed QA', so nobody touches it in the meantime.
Comment 62 Jonathan Druart 2018-02-20 15:35:05 UTC
Splitting  correctly into different patches is more helpful for the author than testers :)
Comment 63 Charles Farmer 2018-02-21 20:15:33 UTC Comment hidden (obsolete)
Comment 64 Charles Farmer 2018-02-21 20:15:41 UTC Comment hidden (obsolete)
Comment 65 Charles Farmer 2018-02-21 20:15:50 UTC Comment hidden (obsolete)
Comment 66 Charles Farmer 2018-02-21 20:20:24 UTC
Split in three patches for easier maintainability.

Since this patch is an amalgam of code and QA fixes that not everybody tested, I went ahead and removed the signoffs from the original patch.

Back to 'Needs signoff'. Should apply cleanly on master, and pass qa -c 3
Comment 67 Brendan Gallagher 2018-03-16 10:31:45 UTC
Tried it out - whenever I click on the Modify Button - the pop_up window is complete blank.  I don't see any error messages so I can't be more helpful.
Comment 68 Marcel de Rooy 2018-03-16 10:57:20 UTC
(In reply to Brendan Gallagher from comment #67)
> Tried it out - whenever I click on the Modify Button - the pop_up window is
> complete blank.  I don't see any error messages so I can't be more helpful.

Caused by 19608 and fixed by last followup on 19436
Comment 69 David Cook 2018-04-16 05:42:34 UTC
I was just thinking about https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18202 and how it really relates to this...
Comment 70 Katrin Fischer 2018-08-09 20:50:23 UTC
Comment on attachment 72061 [details] [review]
Bug 11297 - Add support for custom PQF attributes for Z39.50 server searches.

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/z3950servers.tt
@@ +28,5 @@
> +            $("#syntax").val('[% server.syntax %]');
> +            $("#encoding").val('[% server.encoding %]');
> +            $("#recordtype").val('[% server.recordtype %]');
> +        [% END %]
> +        // Disable recordtype (and default to bib) for non-Z3950 servers until auth is supported

Noticed this comment here earlier: bug 19436 has now been pushed, so it might need to be adapted now.
Comment 71 Charles Farmer 2018-08-10 20:35:44 UTC
I'll fail it for now, just to explicitly show that this shouldn't be tested as-is.

The patch already had to be rebased anyway because of bug 19436. The modification you're pointing at shouldn't take too long.
Comment 72 Charles Farmer 2018-08-10 21:02:39 UTC Comment hidden (obsolete)
Comment 73 Charles Farmer 2018-08-10 21:02:48 UTC Comment hidden (obsolete)
Comment 74 Charles Farmer 2018-08-10 21:03:34 UTC Comment hidden (obsolete)
Comment 75 Charles Farmer 2018-08-10 21:13:54 UTC
All we changed inside the JS was a datatable sort, but the whole JS got accidentally bundled inside one the previous patches when the JS code was moved at the bottom of the page.

I went ahead and reread every last patches we submitted to make sure to carry only what was necessary. Back to "Needs Signoff"
Comment 76 Charles Farmer 2018-08-10 21:29:37 UTC
Created attachment 77721 [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
Comment 77 Charles Farmer 2018-08-10 21:29:43 UTC
Created attachment 77722 [details] [review]
Bug 11297 - Add atomicupdate patch and modify kohastructure.sql
Comment 78 Charles Farmer 2018-08-10 21:29:50 UTC
Created attachment 77723 [details] [review]
Bug 11297 - Modify Z3950server schema file
Comment 79 Katrin Fischer 2018-08-29 20:28:22 UTC
Created attachment 78294 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 80 Katrin Fischer 2018-08-29 20:28:30 UTC
Created attachment 78295 [details] [review]
Bug 11297: Add atomicupdate patch and modify kohastructure.sql

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 81 Katrin Fischer 2018-08-29 20:28:37 UTC
Created attachment 78296 [details] [review]
Bug 11297: Modify Z3950server schema file

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 82 Katrin Fischer 2018-08-29 20:28:45 UTC
Created attachment 78297 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field
Comment 84 Katrin Fischer 2018-10-10 21:52:52 UTC
Created attachment 80372 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field
Comment 85 Katrin Fischer 2018-10-10 21:53:15 UTC
i had missed one missing filter in my follow-up. As I signed off... can someone else QA?
Comment 86 Martin Renvoize 2018-10-12 14:04:56 UTC
Created attachment 80496 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 87 Martin Renvoize 2018-10-12 14:05:04 UTC
Created attachment 80497 [details] [review]
Bug 11297: Add atomicupdate patch and modify kohastructure.sql

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 88 Martin Renvoize 2018-10-12 14:05:10 UTC
Created attachment 80498 [details] [review]
Bug 11297: Modify Z3950server schema file

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 89 Martin Renvoize 2018-10-12 14:05:16 UTC
Created attachment 80499 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 90 Martin Renvoize 2018-10-12 14:06:24 UTC
I must admit, this isn't an area of the codebase I have deep dived into for a long time.. but, the code does look sound, doesn't appear to cause any regressions I could see and passes the qa scripts.

Passing QA.
Comment 91 Nick Clemens 2018-10-24 13:30:04 UTC
Please make the database update idempotent and use skeleton.perl as a base so you can check if the column exists first - example:
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80478&action=diff
Comment 92 Charles Farmer 2018-10-26 18:32:59 UTC
Created attachment 81328 [details] [review]
Bug 11297: (QA follow-up) Make the database update script idempotent
Comment 93 Charles Farmer 2018-10-26 18:35:13 UTC
Removed the SQL script and based the new perl atomic update on skeleton.perl instead.

Hope this came fast enough for 18.11 :)
Comment 94 Martin Renvoize 2018-10-27 07:02:52 UTC
Created attachment 81342 [details] [review]
Bug 11297: (QA follow-up) Make the database update script idempotent

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 95 Martin Renvoize 2018-10-27 07:08:46 UTC
Created attachment 81343 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 96 Martin Renvoize 2018-10-27 07:08:52 UTC
Created attachment 81344 [details] [review]
Bug 11297: Add atomicupdate patch and modify kohastructure.sql

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 97 Martin Renvoize 2018-10-27 07:08:58 UTC
Created attachment 81345 [details] [review]
Bug 11297: Modify Z3950server schema file

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 98 Martin Renvoize 2018-10-27 07:09:05 UTC
Created attachment 81346 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 99 Martin Renvoize 2018-10-27 07:11:01 UTC
I took the liberty of squashing the QA Followup patch into the original atomicupdate patch (to silence the 'git manipulation warning' from the qa scripts).

Back to passed qa, I'm sure this will make 18.11 ;)
Comment 100 Martin Renvoize 2018-11-06 16:30:05 UTC
I jumped the gun here.. C4::Breeding needs updated tests to cover the new functionality.

Sorry guys.. it had been a long week when I qa'd this first time around.
Comment 101 Martin Renvoize 2018-11-06 16:30:39 UTC
We're in Slush.. so a quick followup could still just make it.
Comment 102 Blou 2018-11-06 16:34:30 UTC
We're in mud, we've no way to prioritize this in the next few weeks.
Comment 103 Katrin Fischer 2018-11-06 16:53:14 UTC
I'm sorry, we've already passed feature slush :(
Comment 104 Katrin Fischer 2018-11-06 16:54:45 UTC
Hm, didn't realize Martin's comments were from today, not sure if someone else would be able to help out here?
Comment 105 Martin Renvoize 2018-11-06 17:01:31 UTC
I'll try and take a look at tests tomorrow, sorry guys.
Comment 106 José-Mario Monteiro-Santos 2019-03-01 19:07:01 UTC
(In reply to Martin Renvoize from comment #105)
> I'll try and take a look at tests tomorrow, sorry guys.

Any specific plan in mind ? It's a pretty daunting task to correctly test the entirety of the Z3950Search sub. I can take a stab at it, but I'd love some pointers to help me get started.
Comment 107 Bin Wen 2019-09-12 13:33:10 UTC
Created attachment 92719 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 108 Bin Wen 2019-09-12 13:33:27 UTC
Created attachment 92720 [details] [review]
Bug 11297: Add atomicupdate patch and modify kohastructure.sql

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 109 Bin Wen 2019-09-12 13:33:46 UTC
Created attachment 92721 [details] [review]
Bug 11297: Modify Z3950server schema file

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 110 Bin Wen 2019-09-12 13:33:55 UTC
Created attachment 92722 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 111 Bin Wen 2019-09-12 13:44:23 UTC
Rebase for new progress
Comment 112 David Nind 2020-02-07 22:18:36 UTC
Created attachment 98598 [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:
Server name: BAnQ
Hostname   : catalogue.banq.qc.ca
Port       : 210
Database   : IRIS
Syntax     : MARC21/USMARC

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 113 David Nind 2020-02-07 22:18:42 UTC
Created attachment 98599 [details] [review]
Bug 11297: Add atomicupdate patch and modify kohastructure.sql

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 114 David Nind 2020-02-07 22:18:47 UTC
Created attachment 98600 [details] [review]
Bug 11297: Modify Z3950server schema file

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 115 David Nind 2020-02-07 22:18:53 UTC
Created attachment 98601 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 116 Katrin Fischer 2020-02-19 00:13:13 UTC
Created attachment 99227 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 117 Katrin Fischer 2020-02-19 00:13:28 UTC
Created attachment 99228 [details] [review]
Bug 11297: Add atomicupdate patch and modify kohastructure.sql

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 118 Katrin Fischer 2020-02-19 00:13:42 UTC
Created attachment 99229 [details] [review]
Bug 11297: Modify Z3950server schema file

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 119 Katrin Fischer 2020-02-19 00:13:59 UTC
Created attachment 99230 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 120 Katrin Fischer 2020-02-19 00:16:14 UTC
Created attachment 99231 [details] [review]
Bug 11297: (QA follow-up) Make table headings match other pages - use Actions instead of Options

The patch set adds a table hading for the Actions column on the
summary page. If you compare with other pages, this should
be labelled 'Actions' instead of 'Options'.
Comment 121 Katrin Fischer 2020-02-19 00:24:43 UTC
Still missing tests... :(
Comment 122 Martin Renvoize 2020-02-19 16:12:54 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 123 David Cook 2020-02-20 04:05:36 UTC
Reminds me how I really should work on https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18202 one of these days 😅
Comment 124 Joy Nelson 2020-03-13 00:43:48 UTC
enhancement not backported to 19.11.x