Setting patch complexity to medium because of small DB changes.

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

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.

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

> 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 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. :)

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.

fyi, There is nothing in the current patch that would prevent "4=109 and 4=1".

(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 :)

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. :)

Other QA pov needed.

(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 ;)

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.

(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! :)

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

Probably won't have time to test this one actually...

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.

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

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.

(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!

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.

fixed conflicts, patch should apply correctly.

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.

Fixed conflicts

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.

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

I have attached a follow-up patch, but stumble on the missing unit tests for the change.

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.

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.

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

(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?

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?

(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).

I rebase patches, it's apply correctly

Leaving final decisions about unit tests to QA team.

QA: Looking here now

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.

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.

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

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.

Splitting  correctly into different patches is more helpful for the author than testers :)

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

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.

(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

I was just thinking about https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18202 and how it really relates to this...

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.

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.

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"

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

Created attachment 77722 [details] [review]
Bug 11297 - Add atomicupdate patch and modify kohastructure.sql

Created attachment 77723 [details] [review]
Bug 11297 - Modify Z3950server schema file

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>

Created attachment 78295 [details] [review]
Bug 11297: Add atomicupdate patch and modify kohastructure.sql

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 78296 [details] [review]
Bug 11297: Modify Z3950server schema file

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 78297 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field

Created attachment 80372 [details] [review]
Bug 11297: (follow-up) Add html filter to new input field

i had missed one missing filter in my follow-up. As I signed off... can someone else QA?

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>

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>

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>

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>

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.

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

Created attachment 81328 [details] [review]
Bug 11297: (QA follow-up) Make the database update script idempotent

Removed the SQL script and based the new perl atomic update on skeleton.perl instead.

Hope this came fast enough for 18.11 :)

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>

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>

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>

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>

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>

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 ;)

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.

We're in Slush.. so a quick followup could still just make it.

We're in mud, we've no way to prioritize this in the next few weeks.

I'm sorry, we've already passed feature slush :(

Hm, didn't realize Martin's comments were from today, not sure if someone else would be able to help out here?

I'll try and take a look at tests tomorrow, sorry guys.

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

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>

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>

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>

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>

Rebase for new progress

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>

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>

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>

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>

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>

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>

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>

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>

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

Still missing tests... :(

Nice work everyone!

Pushed to master for 20.05

Reminds me how I really should work on https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18202 one of these days ﷐[U+1F605]﷑

enhancement not backported to 19.11.x