Description
Marcel de Rooy
2014-05-15 13:23:51 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. Created attachment 28325 [details] [review] 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. 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? Created attachment 28326 [details] [review] 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. Created attachment 28718 [details] [review] [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> 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". Created attachment 29670 [details] [review] 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> (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. 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> 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> 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> Patches pushed to master. Thanks Marcel! |