Description
Nicole C. Engard
2013-05-30 14:34:18 UTC
Created attachment 18674 [details] [review] Bug 10382 - collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible There is a conflict between the bug description and the test plan that leaves me rather confused. The bug report talks about removing an item from course reserves, but the test plan talks about disabling the course while leaving the item in the course reserves. Are item fields supposed to get set back to what they were before being put on reserve if they are removed from course reserves, or is that functionality that has not been implemented? (In reply to comment #2) > There is a conflict between the bug description and the test plan that > leaves me rather confused. The bug report talks about removing an item from > course reserves, but the test plan talks about disabling the course while > leaving the item in the course reserves. Are item fields supposed to get set > back to what they were before being put on reserve if they are removed from > course reserves, or is that functionality that has not been implemented? Yes, disabling all courses an item is attached to and removing an item from all courses it is attached to are functionally equivalent ( as far as the `items` table is concerned ). I wrote the test plan to make it quicker to test. In both cases fields should revert to the original values. Nobody including mysql thought of a scenario where a field would go from not having a value to having a value and back. It was always switching values, or not modifying the field at all. Okay, in that case the fix doesn't work for me. I changed ccode to non-fiction when putting the item into course reserves, and when I removed it from course reserves, ccode remained non-fiction. I'm afraid I cannot reproduce your issue with this patch. I've tested it using both location and ccode, and checked the revert by 1) Disabling the course the item is part of 2) Deleting the item reserve from the course 3) Deleting the course itself ( and thus all reserves for the course ). Kyle (In reply to comment #4) > Okay, in that case the fix doesn't work for me. I changed ccode to > non-fiction when putting the item into course reserves, and when I removed > it from course reserves, ccode remained non-fiction. This doesn't work for me either. Here's what I did: 1) Pick an item which isn't checked out, isn't lost, and set its ccode to NULL. 2) Add the item to an existing course, changing the ccode to one of my existing ones. 3) Looking at the entry on the course detail page, the ccode column shows "Unchanged ([my ccode])." 4) Click "remove" to take the item off the course. 5) View the detail page for that item: ccode is not null. It's still the ccode I set it to when I added it to the course. Created attachment 19611 [details] [review] Bug 10382 - collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible The latest patch corrects this problem: > 3) Looking at the entry on the course detail page, the ccode > column shows "Unchanged ([my ccode])." ...but not this one: > 5) View the detail page for that item: ccode is not null. > It's still the ccode I set it to when I added it to > the course. I ran into this during my workshop last week as well - upping severity a bit as it seems to me that this means basic functionality of course reserves is not working correctly. Kyle, could you take another look at this? I think it's almost a blocker, as your items will not be reset to the correct state if the fields has been empty (which is kind of a data loss) Still a critical bug? If someone would be willing to write some unit tests to demonstrate the issues I'd be more than happy to continue work on this bug. I think the issue is triggered by removing an item from course reserve, and *not* triggered by simply disabling the course reserves. Can someone confirm this? Hi Kyle, I think this could be related to the new bug 15876 for locations - sounds like the same or at least a very similar issue (In reply to Katrin Fischer from comment #13) > Hi Kyle, I think this could be related to the new bug 15876 for locations - > sounds like the same or at least a very similar issue Yes, I expect the issue will be the same for all non-required fields that can be changed when an item is on reserve. Created attachment 60947 [details] [review] Bug 10382 - collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Created attachment 60948 [details] [review] Bug 10382 - Add unit tests for course items (In reply to Kyle M Hall from comment #12) > If someone would be willing to write some unit tests to demonstrate the > issues I'd be more than happy to continue work on this bug. I think the > issue is triggered by removing an item from course reserve, and *not* > triggered by simply disabling the course reserves. Can someone confirm this? Hello Kyle, I have rebased your patch and provided some unit tests that reveal the problem. Unfortunately, it seems that your patch does not fix it. Created attachment 60979 [details] [review] Bug 10382 - Course reserves: handle empty values Comment on attachment 60979 [details] [review] Bug 10382 - Course reserves: handle empty values Review of attachment 60979 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/bug_10382-changes-on-courseitems-table.sql @@ +1,4 @@ > +ALTER TABLE course_items DROP FOREIGN KEY course_items_ibfk_2; > +ALTER TABLE course_items MODIFY itype VARCHAR(20); > +ALTER TABLE course_items MODIFY ccode VARCHAR(20); > +ALTER TABLE course_items MODIFY holdingbranch VARCHAR(20); \ No newline at end of file Why drop the foreign key completely? Shouldn't the key be recreated? > Why drop the foreign key completely? Shouldn't the key be recreated?
With this patch, the value '<UNCHANGED>' is inserted in the column course_items.holdingbranch if no change is requested. This is why i remove the foreign key.
Created attachment 61814 [details] [review] [SIGNED-OFF] Bug 10382 - collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 61815 [details] [review] [SIGNED-OFF] Bug 10382 - Add unit tests for course items Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 61816 [details] [review] [SIGNED-OFF] Bug 10382 - Course reserves: handle empty values Signed-off-by: Josef Moravec <josef.moravec@gmail.com> This <UNCHANGED> stuff smells like a very bad codding pattern. Could not we use empty string and undef/NULL to handle empty values? (In reply to Jonathan Druart from comment #24) > This <UNCHANGED> stuff smells like a very bad codding pattern. > Could not we use empty string and undef/NULL to handle empty values? Hello Jonathan, I don't really see how to do that. If you create a course item this way: my $ci_id2 = ModCourseItem( itemnumber => $itemnumber, itype => '', ccode => 'BOOK', holdingbranch => '<UNCHANGED>', location => '<UNCHANGED>', ); or this way my $ci_id2 = ModCourseItem( itemnumber => $itemnumber, ccode => 'BOOK', holdingbranch => '<UNCHANGED>', location => '<UNCHANGED>', ); This results to the same when getting the course with GetCourseItem: itype is undef. Empty or null value are retrieved the same and we are unable to know if the value has changed or not. Hm, so is the problem that we save 'unchanged' instead of the original value, no matter if it was changed or not? And 'null' would be confusing, as it could mean 'unchanged' as well as 'was empty before'? So could it work like that? - change from a to b (changed, store a) - change from none to a (changed, store none) < current problem - change from a to none (changed, store a) - change from none to none (unchanged, store unchanged) - change from a to a (unchanged, store unchanged) I think the advantage of <unchanged> is currently that if you change the collection/location while the item is in the course reserve, with 'unchanged' it won't reset it by accident when you remove the item from the course reserve - the new value will stay. (In reply to Alex Arnaud from comment #25) > (In reply to Jonathan Druart from comment #24) > > This <UNCHANGED> stuff smells like a very bad codding pattern. > > Could not we use empty string and undef/NULL to handle empty values? > > Hello Jonathan, > > I don't really see how to do that. > > If you create a course item this way: > > my $ci_id2 = ModCourseItem( > itemnumber => $itemnumber, > itype => '', > ccode => 'BOOK', > holdingbranch => '<UNCHANGED>', > location => '<UNCHANGED>', > ); > > or this way > > my $ci_id2 = ModCourseItem( > itemnumber => $itemnumber, > ccode => 'BOOK', > holdingbranch => '<UNCHANGED>', > location => '<UNCHANGED>', > ); > > This results to the same when getting the course with GetCourseItem: itype > is undef. > Empty or null value are retrieved the same and we are unable to know if the > value has changed or not. Yeah, it looks to me like: - either you need to add a boolean column for each field that can be changed when added to course-modifier to indicate the value was changed - you add a column with a parseable string that indicates which fields were changed - you use a magic string like '<UNCHANGED>' to indicate absence of change - you add a linking table, which has an entry for each itemnumber and each changed column The latter means we would remove the ccode, holdingbranch, location, itype fields from the course_items table, and would solve the code smell but might be more work to implement. Other than that, I recon the '<UNCHANGED>' is the most workable solution. my 2¢, Alex Please add a reference to the DB changes in the commit message. You also have to provide the same changes for kohastructure! Created attachment 63531 [details] [review] Bug 10382 - Course reserves: handle empty values DB changes: - remove course_items_ibfk_2, - set course_items.itype to VARCHAR(20), - set course_items.ccode to VARCHAR(20), - set course_items.holdingbranch to VARCHAR(20), https://bugs.koha-community.org/show_bug.cgi?id=10383 I do not like the way this bug is fixed (especially the drop of the FK). I will not oppose to a push as I do not have anything else to suggest. I let the QA step to someone else. (In reply to Jonathan Druart from comment #30) > I do not like the way this bug is fixed (especially the drop of the FK). Agree. Why not keep the FK and do not use 'UNCHANGED' etc. ? Still have the impression that this is an ugly hack for a design problem in course reserves. And that we should not push it hastily just before release in its current form. Can't you add a status field or store an original state somewhere else instead of removing a PK and allowing UNCHANGED? *** Bug 15876 has been marked as a duplicate of this bug. *** Created attachment 72022 [details] [review] Bug 10382 - Course reserves: handle empty values Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible I can't even add an item without collection code after last patch This is such a long standing bug... anyone willing to have another go? Created attachment 79030 [details] [review] Bug 10382 - collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 79031 [details] [review] Bug 10382 - Add unit tests for course items Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 79032 [details] [review] Bug 10382 - Course reserves: handle empty values Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Created attachment 79114 [details] [review] Bug 10382 - collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Spencer <ssmith3@mckinneytexas.org> Created attachment 79115 [details] [review] Bug 10382 - Add unit tests for course items Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Spencer <ssmith3@mckinneytexas.org> Created attachment 79116 [details] [review] Bug 10382 - Course reserves: handle empty values Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Spencer <ssmith3@mckinneytexas.org> ByWater Sandboxes seem to not be marking things signed off. I have tested the patch with biblibre Sandboxes (MARC21 flavor) and works fine. Apart from handling collection code properly (as described in test plan), it also handles properly changes in shelving location, item type and holding library. That is, when you add an item to a course reserve and change any of: - Item type - Collection code - Shelving location - Holding library -> The changes are kept while the item is in the course reserve. -> The changes are reverted to old values when the item is removed from the course reserve. I'm not Signing it off (many have done this already), I just had to mention that the proposed patch also fixes an issue in shelving location (mentioned in BZ15876). * Commit title does not start with 'Bug XXXXX: ' - 22c50da * Commit title does not start with 'Bug XXXXX: ' - a9c2f43 * Commit title does not start with 'Bug XXXXX: ' - 71a3b0a create_dependent_objets(); French spelling :) Better use objects QA: Looking here Please correct above minor things. @Kyle: Please let me know if you agree with the changes in the third patch. Saving me the time to study what happened there and what happens now. It sure is a simplification, but hopefully not an oversimplification? Created attachment 79207 [details] [review] Bug 10382: collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 79208 [details] [review] Bug 10382: Add unit tests for course items Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 79209 [details] [review] Bug 10382: Course reserves: handle empty values Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible (In reply to Marcel de Rooy from comment #48) > Please correct above minor things. > > @Kyle: Please let me know if you agree with the changes in the third patch. > Saving me the time to study what happened there and what happens now. It > sure is a simplification, but hopefully not an oversimplification? I think this looks excellent! Alex, Thx for your prompt corrections. If I add this as line 113 in your test: $item->ccode(undef)->store; the test still passes, which means that an original NULL is converted to empty string after removing the course. So theoretically speaking, this is not right. Practically maybe not. Please clarify. (In reply to Marcel de Rooy from comment #53) > Alex, > Thx for your prompt corrections. > If I add this as line 113 in your test: > $item->ccode(undef)->store; > the test still passes, which means that an original NULL is converted to > empty string after removing the course. So theoretically speaking, this is > not right. Practically maybe not. > Please clarify. Hello Marcel, If you put this at the line #113, this is just after the ccode test. So why would it change anything ? Also, $item->ccode(undef)->store; and $item->ccode('')->store; is different. Undef is not converted to null: $item->ccode(undef)->store; MariaDB [koha]> select ccode from items where itemnumber = xxxx; +-------+ | ccode | +-------+ | NULL | +-------+ 1 row in set (0.00 sec) $item->ccode('')->store; MariaDB [koha]> select ccode from items where itemnumber = xxxx; +-------+ | ccode | +-------+ | | +-------+ 1 row in set (0.00 sec) But i think it is transparent for us when using dbic methods. Returned values are the same. (In reply to Alex Arnaud from comment #54) > If you put this at the line #113, this is just after the ccode test. So why > would it change anything ? It should be after that test. But its purpose is to check what happens later when you revert the course reserve and put back the original value.. > Also, $item->ccode(undef)->store; and $item->ccode('')->store; is different. > Undef is not converted to null: Yes it is. Your example below is the proof ! NULL vs empty string > $item->ccode(undef)->store; > MariaDB [koha]> select ccode from items where itemnumber = xxxx; > +-------+ > | ccode | > +-------+ > | NULL | > +-------+ > 1 row in set (0.00 sec) > > $item->ccode('')->store; > MariaDB [koha]> select ccode from items where itemnumber = xxxx; > +-------+ > | ccode | > +-------+ > | | > +-------+ > 1 row in set (0.00 sec) > > But i think it is transparent for us when using dbic methods. Returned > values are the same. They are not the same. (In reply to Marcel de Rooy from comment #55) > (In reply to Alex Arnaud from comment #54) > > If you put this at the line #113, this is just after the ccode test. So why > > would it change anything ? > It should be after that test. But its purpose is to check what happens later > when you revert the course reserve and put back the original value.. > > > Also, $item->ccode(undef)->store; and $item->ccode('')->store; is different. > > Undef is not converted to null: > Yes it is. Your example below is the proof ! NULL vs empty string > > > $item->ccode(undef)->store; > > MariaDB [koha]> select ccode from items where itemnumber = xxxx; > > +-------+ > > | ccode | > > +-------+ > > | NULL | > > +-------+ > > 1 row in set (0.00 sec) > > > > $item->ccode('')->store; > > MariaDB [koha]> select ccode from items where itemnumber = xxxx; > > +-------+ > > | ccode | > > +-------+ > > | | > > +-------+ > > 1 row in set (0.00 sec) > > > > But i think it is transparent for us when using dbic methods. Returned > > values are the same. > They are not the same. Right. And this it's managed afaik. Change '' by undef at line #109: You'll have do to do the same thing at #line #112 to make the test pass and vice versa. But i don't understand why puting your code at line #113 should be a problem if the test still pass. (In reply to Alex Arnaud from comment #56) > But i don't understand why puting your code at line #113 should be a problem > if the test still pass. This is probably the clue. You should ;) My extra line should make the test fail since it does not reset back to original value (empty string instead of null). I am requesting another signoff here. The third patch has no signoff line yet. And additionally please tell me explicitly what you tested! I have been looking more carefully at CourseReserves.pm and I am not really impressed by what I saw unfortunately. I have the impression that this patch set might improve things a bit, but I am not sure if we break something else now. Saw tricky things like: push( @values, $params{$_} || undef ); Together with constructions like: foreach (@FIELDS) { if ( defined($params{$_}) ) { push( @fields_to_update, $_ ); push( @values_to_update, $params{$_} ); } } ModCourseItem adds an course item without affecting items or updates it and does affect items. EnableOrDisable etc. etc. The way we handle the data between items and course items is unclear. Including handling empty string and null values. I am not unwilling to pass QA after another signoff but the traffic lights will not be green :) I'm getting a software error when trying to disable the course. I'm not sure if it's related... Undefined subroutine &C4::Items::ModZebra called at /inlibro/git/koha-master-dev-inlibro/C4/Items.pm line 606. Hi Caroline, I can't reproduce the error - can you try again with another course reserve? Created another course, added items (changed collection code), tried disabling the course Software error: Undefined subroutine &C4::Items::ModZebra called at /inlibro/git/koha-master-dev-inlibro/C4/Items.pm line 606. Maybe I'll try with a sandbox. I get the same error message in a sandbox (tried with BibLibre sandbox 5 and PTFS Europe sandbox 0) Software error: Undefined subroutine &C4::Items::ModZebra called at /home/koha/src/C4/Items.pm line 606. Here are the exact steps I went through in the sandboxes 1- Go to Administration 2- Search for UseCourseReserves 3- Change UseCourseReserves to "Use" 4- Click on "Save all Circulation preferences" 5- Go to Home > Course reserves 6- Click on "New course" 7- Click on link to add department 8- Click on "New authorized value for DEPARTMENT" 9- Fill in Authorizd value and Description 10- Click on "Save" 11- Go back to More > Course reserves 12- Click on "New Course" 13- Fill in the mandatory info (dept, course number, course name) 14- Leave course "enabled" 15- Click on "Save" 16- Click on "Batch add reserves" 17- Open Item search in another tab 18- Search for all items 19- Copy/Paste barcodes in the course batch add box 20- Change collection code 21- Click "Submit" 22- Click "View course" 23- Open one of the records in another tab 24- Click on "Edit course" 25- Uncheck "Enabled" 26- Click "Save" <- Software Error I've been completely unable to reproduce those errors on koha-testing-docker. It seems that either changes in master have obviated the issue or it is a peculiarity of the test servers used. Created attachment 79722 [details] [review] Bug 10382: collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 79723 [details] [review] Bug 10382: Add unit tests for course items Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 79724 [details] [review] Bug 10382: Course reserves: handle empty values Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 80007 [details] [review] Bug 10382: collection not returning to null when removed from course reserves Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 80008 [details] [review] Bug 10382: Add unit tests for course items Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 80009 [details] [review] Bug 10382: Course reserves: handle empty values Test Plan: 1) Create an item, do not set a collection code 2) Add the item to a course, and choose to set a collection code 3) Ensure the course is enabled, and the collection code is now visible 4) Disable the course, ensure the collection code is no longer visible Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> @RM: Passed QA but a bit 'reluctant or restrained'? This patch set improves things but imo this area needs more attention. See comment58 and the earlier comments/discussion. (In reply to Marcel de Rooy from comment #58) > I am requesting another signoff here. > The third patch has no signoff line yet. > And additionally please tell me explicitly what you tested! > > I have been looking more carefully at CourseReserves.pm and I am not really > impressed by what I saw unfortunately. I have the impression that this patch > set might improve things a bit, but I am not sure if we break something else > now. > > Saw tricky things like: > push( @values, $params{$_} || undef ); > Together with constructions like: > foreach (@FIELDS) { > if ( defined($params{$_}) ) { > push( @fields_to_update, $_ ); > push( @values_to_update, $params{$_} ); > } > } > ModCourseItem adds an course item without affecting items or updates it and > does affect items. EnableOrDisable etc. etc. > The way we handle the data between items and course items is unclear. > Including handling empty string and null values. > > I am not unwilling to pass QA after another signoff but the traffic lights > will not be green :) Can you explain with more detail. I'm not sure everyone understands the concern. If we have a little bit more understanding of what this piece of code is doing, more of us can weigh in. (In reply to Marcel de Rooy from comment #70) > @RM: > > Passed QA but a bit 'reluctant or restrained'? > This patch set improves things but imo this area needs more attention. > See comment58 and the earlier comments/discussion. Scenario: 1 - Define a new authorised value for CCODE with authorised value "" and description "Blank space" 2 - Create two (or more) items on a bib record, neither with ccode defined 3 - Add one of those items to an enabled course 4 - Disable the course 5 - View the record 6 - One item has no collection, the other has 'Blank space' Do we think this is okay? I don't think we should have empty entries in collections in locations. They will appear doubled up in lots of places. It should be returned to the exact state it was originally. It should not be modified in any way by this module when inactive. If it was null before, is should be returned to null. If it was a blank entry, it should be returned to a blank entry. (In reply to Nick Clemens from comment #72) > (In reply to Marcel de Rooy from comment #70) > > @RM: > > > > Passed QA but a bit 'reluctant or restrained'? > > This patch set improves things but imo this area needs more attention. > > See comment58 and the earlier comments/discussion. > > Scenario: > 1 - Define a new authorised value for CCODE with authorised value "" and > description "Blank space" > 2 - Create two (or more) items on a bib record, neither with ccode defined > 3 - Add one of those items to an enabled course > 4 - Disable the course > 5 - View the record > 6 - One item has no collection, the other has 'Blank space' > > Do we think this is okay? Edge case; could live with that.. (In reply to Christopher Brannon from comment #74) > It should be returned to the exact state it was originally. It should not > be modified in any way by this module when inactive. If it was null before, > is should be returned to null. If it was a blank entry, it should be > returned to a blank entry. Well, thats a point in my qa comment. So theoretically you are right, but pragmatically we could move into the right direction with these changes. (In reply to Marcel de Rooy from comment #76) > (In reply to Christopher Brannon from comment #74) > > It should be returned to the exact state it was originally. It should not > > be modified in any way by this module when inactive. If it was null before, > > is should be returned to null. If it was a blank entry, it should be > > returned to a blank entry. > > Well, thats a point in my qa comment. > So theoretically you are right, but pragmatically we could move into the > right direction with these changes. Yes, you COULD do that here, but it is not the function of this module to correct mistakes. Otherwise, we are opening the door for people to correct mistakes everywhere else in Koha, and it would be more complicated to track down where changes are being made. Those types of issue (blank or null entries) should be addressed at time of edit. If it is an issue for course reserves, then we have the option to have course reserves refuse to touch a record that is incomplete, or give the option to correct the record manually before adding to course reserves. In my opinion, I don't think course reserves should be silently 'fixing' things. RM: Your decision now. I said enough.. This is not perfect, but it reduces the scope of the problem here greatly. We probably should decide if there is a valid reason to have "" vs. undef for some of the values in items. Awesome work all on getting this moved along. Pushed to master for 18.11 Pushed to 18.05.x for 18.05.05 Does this fix location and collection and should the bug title be adjusted accordingly? Pushed to 17.11.x for 17.11.11 |