Bug 12265 - Improve Z39.50 servers administration (incl DBIx)
Summary: Improve Z39.50 servers administration (incl DBIx)
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: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-15 13:23 UTC by Marcel de Rooy
Modified: 2015-06-04 23:33 UTC (History)
5 users (show)

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


Attachments
[3.14.X] Edit Z3950 servers (33.99 KB, patch)
2014-05-15 13:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12265: Improve Z39.50 servers administration (33.47 KB, patch)
2014-05-19 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12265: Improve Z39.50 servers administration (33.48 KB, patch)
2014-05-19 10:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[Signed-Off] Bug 12265: Improve Z39.50 servers administration (33.63 KB, patch)
2014-06-08 08:09 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12265: [QA Follow-up] Replace DBIx search by find (4.10 KB, patch)
2014-07-14 10:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 12265: Improve Z39.50 servers administration (33.60 KB, patch)
2014-07-14 11:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12265: [QA Follow-up] Replace DBIx search by find (4.12 KB, patch)
2014-07-14 11:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12265: [QA Follow-up] - Improve usage of find (2.18 KB, patch)
2014-07-14 11:48 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2014-05-15 13:23:51 UTC

    
Comment 1 Marcel de Rooy 2014-05-15 13:25:22 UTC
Created attachment 28281 [details] [review]
[3.14.X] Edit Z3950 servers

More comments follow
This patch is for 3.14 actually; will resolve merge conflicts for master.
Comment 2 Marcel de Rooy 2014-05-19 09:34:11 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2014-05-19 09:41:15 UTC
Inviting anyone to comment on the following:

The DBIx statements are in the script code itself now. Another approach would have been to move such statements to a module.
In this case my idea was that the call to schema, 3 search, 1 first, 1 delete, 1 update and 1 create statement (7 DBIx calls) do not really justify adding an additional layer between the script and DBIx.

What do you think?
Comment 4 Marcel de Rooy 2014-05-19 10:01:33 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2014-06-08 08:09:30 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2014-07-11 17:16:54 UTC
A couple place in the code you use search instead of fine. For example, I think

my $rs=$schema->resultset('Z3950server')->search( { id => $id } );
my $name= $rs->first?$rs->first->name:'';
y $cnt=$rs->delete;
$template->param( msg_deleted => 1, msg_add => $name ) if $cnt==1;

Could be rewritten as

my $server = $schema->resultset('Z3950server')->find($id);
if ( $server ) {
    $server->delete();
    $template->param( msg_deleted => 1, msg_add => $server->name() );
}

Even after calling delete, the object is still perfectly usable, no need to store the name in a separate variable.

Marking as failed qa. This is still only suggestion, so feel free to add a followup, or just set status back to "signed off".
Comment 7 Marcel de Rooy 2014-07-14 10:11:55 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2014-07-14 10:14:59 UTC
(In reply to Kyle M Hall from comment #6)
> A couple place in the code you use search instead of fine. For example, I
> think
> 
> my $rs=$schema->resultset('Z3950server')->search( { id => $id } );
> my $name= $rs->first?$rs->first->name:'';
> y $cnt=$rs->delete;
> $template->param( msg_deleted => 1, msg_add => $name ) if $cnt==1;
> 
> Could be rewritten as
> 
> my $server = $schema->resultset('Z3950server')->find($id);
> if ( $server ) {
>     $server->delete();
>     $template->param( msg_deleted => 1, msg_add => $server->name() );
> }
> 

Thanks for QAing. Replaced two occurrences. The third one should be a search. Added a few extra lines for additional testing.
I have changed the status back to Signed-off. If you feel that the follow-up needs additional signoff, please set it to Needs Signoff.
Comment 9 Kyle M Hall 2014-07-14 11:46:28 UTC
Created attachment 29671 [details] [review]
[PASSED QA] Bug 12265: Improve Z39.50 servers administration

This patch makes a few tiny improvements on the form, does some
housekeeping/tidying up, and replaces SQL code by DBIC statements.
In detail:
- Adds an id parameter (more precise than searchfield).
- With the move from searchfield to id, you can rename a server now.
- A Copy button is added to clone a server.
- Confirming a delete is moved to javascript. No additional form anymore.
- A message about an insert, update or delete is shown in a div dialog
  alert above the table instead of a separate form.
- Remove offset parameter, Next/Prev button and associated logic.
- All SQL statements are replaced by DBIC.
- Function StringSearch is renamed to DBICified ServerSearch.
- Remove tabs from script and template. Adjust some indentation.

Test plan:
- Test adding, editing and deleting Z3950 servers.
- Test searching for a server with the top search box (not table).
- Add a server with quotes in the name. Search for it. Edit or delete.

Followed tet plan. Patch behaves as expected.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2014-07-14 11:46:40 UTC
Created attachment 29672 [details] [review]
[PASSED QA] Bug 12265: [QA Follow-up] Replace DBIx search by find

This patch replaces DBIx search by find at two places in z3950servers.pl.
Furthermore, it adds testing on the results of the find method, passing
a not-found-message to the template whenever needed.
The function ServerSearch is adjusted to use HashRefInflator instead of
manually mapping the columns of result objects.
An additional defined-test in _form_data_hashref prevents shifting of
names and values in the result hash.

Test plan:
List all servers. Edit, copy and delete a server.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2014-07-14 11:48:05 UTC
Created attachment 29673 [details] [review]
[PASSED QA] Bug 12265: [QA Follow-up] - Improve usage of find

When finding a row by its primary key, it is not necessary to
pass a hashref, only the primary key value itself is required.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Tomás Cohen Arazi 2014-07-14 12:19:49 UTC
Patches pushed to master.

Thanks Marcel!