[Bug 14504] New: Add command-line script to batch delete items based on lost/withdrawn statuses
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Bug ID: 14504 Summary: Add command-line script to batch delete items based on lost/withdrawn statuses Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Command-line Utilities Assignee: gmcharlt@gmail.com Reporter: barton@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: robin@catalyst.net.nz Intention of development: To create a script to delete items based on a series of criteria, primarily itemlost, timestamp, itemlost_on, and withdrawn delete_items.pl --criteria EXPRESSION [--commit] Where EXPRESSION is a valid SQL conditional, operating on the items table, e.g. items.itemlost => 1 or items.timestamp < 2014-06-07 --criteria may be called multiple times. --commit is necessary to actually delete the items. Here are a few examples of how this might be called: delete_items.pl --verbose --criteria "items.itemlost >= 1" --criteria "items.itemlost <= 4" --criteria "items.itemlost_on < '$(date --date="13 month ago" --rfc-3339=date)'" --commit delete_items.pl --verbose --criteria "items.itemlost >= 1" --criteria "items.itemlost <= 4" --criteria "items.timestamp < '$(date --date="13 month ago" --rfc-3339=date)'" --commit delete_items.pl --verbose --criteria "items.withdrawn != 0" --criteria "items.timestamp < '$(date --date="13 month ago" --rfc-3339=date)'" --commit delete_items.pl --verbose --criteria "items.withdrawn != 0" --criteria "items.itemlost_on < '$(date --date="13 month ago" --rfc-3339=date)'" --commit -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |barton@bywatersolutions.com Status|NEW |ASSIGNED Change sponsored?|--- |Sponsored -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #1 from Barton Chittenden <barton@bywatersolutions.com> --- NAME delete_items.pl - A batch item deletion tool, which generates a query against the items database and deletes the items matching the criteria specified in the command line arguments. SYNOPSIS delete_items.pl [--help|--manual|--version] delete_items.pl [--verbose] [--dry-run] --criteria "*SQL CONDITIONAL EXPRESSION*" ... [--commit] OPTIONS --help Show the brief help information. --manual Read the manual, with examples. --version Show the version number and exit. --verbose Send the "WHERE" clause generated by the collected "--criteria" arguments, as well as items affected to Standard Out. --criteria The "--criteria" option may called multiple times. The following argument must be a syntactically valid SQL statement which is part of the "WHERE" clause querying the items table. These are joined by "AND". --commit No items will be deleted unless the "--commit" flag is present. --dry-run Disables "--commit" flag and enables "--verbose" flag. EXAMPLES The following is an example of this script: delete_items.pl --criteria "items.withdrawn ! 0" --criteria "items.withdrawn_on < $(date --date="13 month ago" --rfc-3339=date)" --commit delete_items.pl --criteria "itemlost >= '1'" --criteria "itemlost <='4'" --criteria "itemlost_on < '2014-04-28'" --commit DESCRIPTION This is a lightweight batch deletion tool for items, suitable for running in a cron job. AUTHOR Barton Chittenden <barton@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #2 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 40830 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=40830&action=edit add delete_items.pl: a command line batch deletion tool -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #3 from Barton Chittenden <barton@bywatersolutions.com> --- Test plan: TESTING THE MAIN FUNCTIONALITY OF THE SCRIPT: ============================================= Create items with various values of itemlost and, withdrawn. Back-date the respective timestamps (itemlost_on, timestamp). Check out one or two of these items (this will exercise the safeguards against deleting checked out items). run delete_items.pl --criteria "items.itemlost = <your lost value here>" --criteria "itemlost_on < <one day after items was lost>" --dry-run do the same using items.withdrawn and items.timestamp. The results of the queries will be written to STDOUT. If you are working in an instance that has lost or withdrawn items that you want to keep, you may want to adjust the itemlost_on or timestamp values of your test items accordingly. Once satisfied with the output using the '--dry-run' option, use '--commit' instead, then verifiy that the items have been deleted. run delete_items.pl on checked-out items. If you are running '--verbose' or '--dry-run', you should receive a message to stdout showing that the item was not deleted. OTHER THINGS TO TEST: ===================== Run the script with the following options. Check the spelling and accuracy on --help and --manual, and make sure that the output from --version contains a version number. --help Show the brief help information. --manual Read the manual, with examples. --version Show the version number and exit. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Tom Misilo <misilot@fit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |misilot@fit.edu -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Nicole Engard (ByWater) <nicole@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nicole@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Zeno Tajoli <z.tajoli@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |z.tajoli@cineca.it 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=14504 Zeno Tajoli <z.tajoli@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|z.tajoli@cineca.it | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Heather Braum <hbraum@nekls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |hbraum@nekls.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #40830|0 |1 is obsolete| | --- Comment #4 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 42978 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42978&action=edit [SIGNED-OFF] Bug 14504: Add delete_items.pl: a command line batch deletion tool http://bugs.koha-community.org/show_bug.cgi?id=14504 Signed-off-by: Heather Braum <hbraum@nekls.org> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 42978 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42978 [SIGNED-OFF] Bug 14504: Add delete_items.pl: a command line batch deletion tool Review of attachment 42978: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14504&attachment=42978) ----------------------------------------------------------------- FAIL misc/cronjobs/delete_items.pl FAIL pod *** WARNING: Verbatim paragraph in NAME section in file misc/cronjobs/delete_items.pl ::: misc/cronjobs/delete_items.pl @@ +1,4 @@
+#! /usr/bin/perl + +use warnings; +use strict;
warnings and strict are useless when Modern::Perl is already used. @@ +8,5 @@
+use C4::Circulation; +use Modern::Perl; +use Pod::Usage; + +my $VERSION='1.0';
How this is useful? We usually don't use it in scripts. @@ +30,5 @@
+ , help => '' + , manual => '' + , version => '' + } +};
I am not really in favor of these 2 variables. IMO it is preferable to stick to the structure of the already existing scripts in misc/* @@ +39,5 @@
+ , 'V|version' => sub { $OPTIONS->{flags}->{version} = 1 } + , 'h|help' => sub { $OPTIONS->{flags}->{help} = 1 } + , 'm|manual' => sub { $OPTIONS->{flags}->{manual} = 1 } + , 'c|commit' => sub { $OPTIONS->{flags}->{commit} = 1 } # aka DO-EET! + , 'dry-run' => sub { $OPTIONS->{flags}->{commit} = 0;
I don't think dry-run is useful, it's in dry-run mode if commit is not given. @@ +51,5 @@
+ exit; +} + +pod2usage( -verbose => 2 ) if $OPTIONS->{flags}->{manual}; +pod2usage(1) if ( $OPTIONS->{flags}->{help} || scalar @criteria == 0 );
The script should not return 1 if help is specified. It would be good to display on error for the @criteria==0 case (see msg option of pod2usage). @@ +65,5 @@
+my $where_clause = ' where ' . join ( " and ", @criteria ); + +verbose "Where statement: $where_clause"; + +$GLOBAL->{sth}->{target_tiems} = $dbh->prepare( $query->{target_items} . $where_clause );
typo tiems vs items, I suppose. @@ +71,5 @@
+ +DELITEM: while ( my $item = $GLOBAL->{sth}->{target_tiems}->fetchrow_hashref() ) { + my $issue = GetOpenIssue( $item->{itemnumber} ); + if( defined $issue ) { + verbose "Cannot delete '$item->{itemnumber}' -- item is checked out."
Shouldn't we also search for holds? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> 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=14504 --- Comment #6 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 43445 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43445&action=edit Bug 14504 [QA Follow-up] * Fix POD warning. * Remove redundant 'use stric' and 'use warnings' * Remove $VERSION and --version option. * Remove --dry-run option * Split test for --help and check for @criteria into two separate pod2usage calls, enabling -msg on the latter. * Fix 'target_tiems' typo. * Test for holds on items to be deleted. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Barton, it does not work: the items are never deleted. You should also launch the qa script every time you submit a patch, it will catch warnings/errors for you (here 2 trailing white-spaces). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> 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=14504 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #43445|0 |1 is obsolete| | --- Comment #8 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 43533 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43533&action=edit Bug 14504 [QA Follow-up] * Fix POD warning. * Remove redundant 'use stric' and 'use warnings' * Remove $VERSION and --version option. * Remove --dry-run option * Split test for --help and check for @criteria into two separate pod2usage calls, enabling -msg on the latter. * Fix 'target_tiems' typo. * Test for holds on items to be deleted. * Fix whitespace * Fix test for holds. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Barton, Actually I think you should reuse the C4::Items::DelItemCheck subroutine, which does exactly what you are doing. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #10 from Barton Chittenden <barton@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #9)
Barton, Actually I think you should reuse the C4::Items::DelItemCheck subroutine, which does exactly what you are doing.
Jonathan, C4::Items::DelItemCheck doesn't have any kind of 'commit' check... I'm torn, because I would *like* to use it to avoid duplication of code (plus there are a number of cases that it covers that I don't), but I don't want to sacrifice the ability to accurately report what's going to happen if I run the script in verbose mode without '--commit'. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #11 from Barton Chittenden <barton@bywatersolutions.com> --- (In reply to Barton Chittenden from comment #10)
(In reply to Jonathan Druart from comment #9)
Barton, Actually I think you should reuse the C4::Items::DelItemCheck subroutine, which does exactly what you are doing.
Jonathan,
C4::Items::DelItemCheck doesn't have any kind of 'commit' check... I'm torn, because I would *like* to use it to avoid duplication of code (plus there are a number of cases that it covers that I don't), but I don't want to sacrifice the ability to accurately report what's going to happen if I run the script in verbose mode without '--commit'.
Jonathan, Jesse Weaver (pianohacker) and I discussed this: 18:15 <@pianohacker> barton: I'd suggest to Joubu to either add a 'check_only' kind of parameter to DelItemCheck or move the check logic into a completely different function like ItemSafeToDelete (with a slight personal preference for the latter) 18:18 < barton> cleanest would probably be both -- write ItemSafeToDelete, then have DelItemCheck be a wrapper around DelItem which uses ItemSafeToDelete as its test. 18:18 <@pianohacker> yeah, definitely So I'd like to open a separate bug to * add C4::Items::ItemSafeToDelete() * add a 'do-not-commit' option to DelItemCheck (omitting the flag would make it work as usual) * Use C4::Items::ItemSafeToDelete to do the internal checking for DelItemCheck. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I think an even better solution would be to do the job in a transaction and rollback if the commit flag is not there. So it would be something like: start transaction foreach items: is_deleted = DelItemCheck display error unless is_deleted if commit flag: commit else: rollback What do you think about that? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #13 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 43788 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43788&action=edit bug 14504: split logic from DelItemCheck() into ItemSafeToDelete() -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #14 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 43789 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43789&action=edit bug 14504: use C4::Items::DelItemCheck in delete_items.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #15 from Barton Chittenden <barton@bywatersolutions.com> --- Jonathan, The call to C4::Context->IsSuperLibrarian() in C4::Items::ItemSafeToDelete (formerly in C4::Items::DelItemCheck) is throwing the following warning: C4::Context->userenv not defined! at /var/lib/koha/nekls2/kohaclone/C4/Items.pm line 2336. I've gone ahead and attached the patches, because everything else works, but thoughts about how to fix the "not_same_branch" code would be appreciated. --Barton -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Barton, have you seen my suggestion on comment 12? I think it will be really easy to implement what you want using it (no change in pm and no additional tests). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle.m.hall@gmail.com --- Comment #17 from Kyle M Hall <kyle.m.hall@gmail.com> --- (In reply to Jonathan Druart from comment #16)
Barton, have you seen my suggestion on comment 12? I think it will be really easy to implement what you want using it (no change in pm and no additional tests).
While your suggestions would definitely work, I think what Barton's changes are a good and worthwhile improvement. The logic to for deciding if an item is deletable should definitely be separate from the actual deleting of items, and now we've got more through unit tests as well! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Kyle M Hall from comment #17)
(In reply to Jonathan Druart from comment #16)
Barton, have you seen my suggestion on comment 12? I think it will be really easy to implement what you want using it (no change in pm and no additional tests).
While your suggestions would definitely work, I think what Barton's changes are a good and worthwhile improvement. The logic to for deciding if an item is deletable should definitely be separate from the actual deleting of items, and now we've got more through unit tests as well!
Yes maybe you are right but we are adding 1 new subroutine and lot of code when the 2 start_transaction/rollback lines would make the job. Anyway, if you really want this way, I have still some concerns: 1/ Tests should create their own data (branchcode/categorycode), it is easier to use TestBuilder for that. 2/ I don't think a subroutine should take a do_not_commit parameter, this should not do it in the subroutine but from the scripts. Keeping the signed off status to get feedback from other QAers. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #19 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 44013 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=44013&action=edit Bug 14504 QA Fixes -- use TestBuilder, remove do_not_commit Use t::lib::TestBuilder in t/db_dependent/Items_DelItemCheck.t Remove the option 'do_not_commit' from C4::Items::DelItemCheck. Whitespace cleanup. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 44013 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=44013 Bug 14504 QA Fixes -- use TestBuilder, remove do_not_commit Review of attachment 44013: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14504&attachment=44013) ----------------------------------------------------------------- ::: C4/Items.pm @@ +2378,2 @@
+ DelItemCheck( $dbh, $biblionumber, $itemnumber );
$dbh shouldn't pass as a parameter. @@ +2381,4 @@
=cut
sub DelItemCheck { + my ( $dbh, $biblionumber, $itemnumber ) = @_;
$dbh shouldn't pass as a parameter. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 140 "DelItemCheck should delete item if 'do_not_commit' not set" => 'do_not_commit' occurrence. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> 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=14504 --- Comment #22 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 44149 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=44149&action=edit bug 14504: QA Followup -- fixing DelItemCheck arguments Remove $dbh as argument to C4::Items::DelItemCheck and C4::Items::ItemSafeToDelete, also change all calls to these functions throughout the codebase. Also remove remaining reference to 'do_not_commit' in t/db_dependent/Items_DelItemCheck.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #23 from Tom Misilo <misilot@fit.edu> --- Is there any chance of this getting into the next version? Just curious as this would be very beneficial with automating our weeding procedures base on being missing for a set time period. Thanks! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #24 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Tom, this patch has missed the feature slush, which means it was not 'passed QA' by a certain date. It won't be included in 3.22. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I have asked Martin to give his opinion. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I'm in agreement with Jonathan that adding subs needlessly is a bad thing and therefore I do tend towards the transaction idea. More and more I'm of the opinion that data integrity lies firmly within the remit of the database. We really aught to get the constraints correct there first and not rely on lots of additional code logic all over the place doing it instead :| It's a hard thing to say, but I would prefer to have some of the foundations cleaned up rather than just adding yet more code to koha to compensate for your poor foundations. Sorry if this sounds really negative, but it's something I've felt for a while and I totally understand the want to get new and exciting features in quicker and quicker but I feel this is really leading to obfuscation and code bloat that becoming unmanageable.. where do we drawer the line? I won't complain if this is pushed.. but I don't think it's the most forward thinking solution I'm afraid. Also, as we've missed feature slush now it may be worth mentioning a few other queries: 1) Why use a dbh over using the dbic objects? (By using C4::Context to get your handle, you are already going through the pain of instantiating all the dbic class objects I believe anyway, so I don't see that using the handle directly here is of any benefit - Jonathan let me know if this is wrong?) 2) Line 928 in the final patch.. appears to have a double $$ (C4/ImportBatch.pm) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 --- Comment #27 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- s/your poor foundations/our poor foundations/ s/that becoming/that are becoming/ -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #28 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Martin Renvoize from comment #26) I think I must disagree on this one. I think the logic to determine if an item is safe to delete should definitely be divorced from the actual deletion. I think these changes along with the provided unit tests are a good improvement! I guess I don't see how using transactions in this situation is anything but a band-aid so to speak. If you think I've missed your point, please let me know! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #44149|0 |1 is obsolete| | --- Comment #29 from Barton Chittenden <barton@bywatersolutions.com> --- Created attachment 45789 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45789&action=edit bug 14504: QA Followup -- fixing DelItemCheck arguments Remove $dbh as argument to C4::Items::DelItemCheck and C4::Items::ItemSafeToDelete, also change all calls to these functions throughout the codebase. Also remove remaining reference to 'do_not_commit' in t/db_dependent/Items_DelItemCheck.t Fixed doubled "$$" in C4/ImportBatch.pm -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14504 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- These patches don't apply anymore. There are too much changes and too many patches, I have suggested a very simple solution months ago, now we have patches modifying 10 different files... -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org