[Bug 12265] New: Improve Z39.50 servers administration
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 Bug ID: 12265 Summary: Improve Z39.50 servers administration Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Z39.50 / SRU / OpenSearch Servers Assignee: gmcharlt@gmail.com Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org CC: m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 --- Comment #1 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 28281 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=28281&action=edit Edit Z3950 servers More comments follow This patch is for 3.14 actually; will resolve merge conflicts for master. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@enger.priv.no -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #28281|Edit Z3950 servers |[3.14.X] Edit Z3950 servers description| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Improve Z39.50 servers |Improve Z39.50 servers |administration |administration (incl DBIx) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 --- Comment #2 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 28325 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=28325&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 --- Comment #3 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #28325|0 |1 is obsolete| | --- Comment #4 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 28326 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=28326&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 --- Comment #5 from Marc Véron <veron@veron.ch> --- Created attachment 28718 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=28718&action=edit [Signed-Off] 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 Véron <veron@veron.ch> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #28326|0 |1 is obsolete| | CC| |veron@veron.ch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |kyle@bywatersolutions.com --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- 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". -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 --- Comment #7 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 29670 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29670&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 --- Comment #8 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #28281|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #28718|0 |1 is obsolete| | Attachment #29670|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 29671 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29671&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 29672 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29672&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 29673 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29673&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #28281|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12265 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks Marcel! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org