Bug 10382 - collection and location not returning to null when removed from course reserves
Summary: collection and location not returning to null when removed from course reserves
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Alex Arnaud
QA Contact: Marcel de Rooy
URL:
Keywords: release-notes-needed
: 15876 (view as bug list)
Depends on: 8215
Blocks: 22142 22652
  Show dependency treegraph
 
Reported: 2013-05-30 14:34 UTC by Nicole C. Engard
Modified: 2019-06-13 14:19 UTC (History)
22 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
These patches ensure that unset values for items added to course reserves are unset when the course is disabled.
Version(s) released in:


Attachments
Bug 10382 - collection not returning to null when removed from course reserves (1.42 KB, patch)
2013-06-05 14:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10382 - collection not returning to null when removed from course reserves (7.46 KB, patch)
2013-07-12 17:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10382 - collection not returning to null when removed from course reserves (7.58 KB, patch)
2017-03-09 13:43 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382 - Add unit tests for course items (6.21 KB, patch)
2017-03-09 13:43 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382 - Course reserves: handle empty values (6.56 KB, patch)
2017-03-10 09:44 UTC, Alex Arnaud
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10382 - collection not returning to null when removed from course reserves (7.65 KB, patch)
2017-04-04 12:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10382 - Add unit tests for course items (6.27 KB, patch)
2017-04-04 12:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10382 - Course reserves: handle empty values (6.62 KB, patch)
2017-04-04 12:17 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 10382 - Course reserves: handle empty values (8.69 KB, patch)
2017-05-18 13:52 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382 - Course reserves: handle empty values (9.55 KB, patch)
2018-02-21 13:11 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382 - collection not returning to null when removed from course reserves (7.78 KB, patch)
2018-09-17 13:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382 - Add unit tests for course items (6.26 KB, patch)
2018-09-17 13:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382 - Course reserves: handle empty values (9.56 KB, patch)
2018-09-17 13:39 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382 - collection not returning to null when removed from course reserves (7.85 KB, patch)
2018-09-19 14:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 10382 - Add unit tests for course items (6.32 KB, patch)
2018-09-19 14:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 10382 - Course reserves: handle empty values (9.62 KB, patch)
2018-09-19 14:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 10382: collection not returning to null when removed from course reserves (7.78 KB, patch)
2018-09-21 10:14 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382: Add unit tests for course items (6.26 KB, patch)
2018-09-21 10:14 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382: Course reserves: handle empty values (9.56 KB, patch)
2018-09-21 10:14 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 10382: collection not returning to null when removed from course reserves (7.79 KB, patch)
2018-10-01 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10382: Add unit tests for course items (6.33 KB, patch)
2018-10-01 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10382: Course reserves: handle empty values (9.53 KB, patch)
2018-10-01 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10382: collection not returning to null when removed from course reserves (7.88 KB, patch)
2018-10-05 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10382: Add unit tests for course items (6.41 KB, patch)
2018-10-05 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10382: Course reserves: handle empty values (9.71 KB, patch)
2018-10-05 09:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2013-05-30 14:34:18 UTC
If an item that is on reserves did not have a collection code and one was added a the time of adding the reserve then when you remove the item from reserves the collection is not returned to NULL instead it stays as what it was set in reserves.
Comment 1 Kyle M Hall 2013-06-05 14:03:40 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2013-06-07 12:43:36 UTC
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?
Comment 3 Kyle M Hall 2013-06-07 13:57:35 UTC
(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.
Comment 4 Jared Camins-Esakov 2013-06-07 15:03:03 UTC
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.
Comment 5 Kyle M Hall 2013-06-13 10:54:01 UTC
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.
Comment 6 Owen Leonard 2013-07-12 13:39:38 UTC
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.
Comment 7 Kyle M Hall 2013-07-12 17:51:27 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2013-07-12 18:22:23 UTC
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.
Comment 9 Katrin Fischer 2014-05-19 06:07:30 UTC
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.
Comment 10 Katrin Fischer 2014-11-03 22:44:48 UTC
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)
Comment 11 Marc Véron 2015-12-24 13:23:17 UTC
Still a critical bug?
Comment 12 Kyle M Hall 2015-12-24 15:00:33 UTC
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?
Comment 13 Katrin Fischer 2016-02-22 20:35:15 UTC
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
Comment 14 Kyle M Hall 2016-03-07 19:52:19 UTC
(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.
Comment 15 Alex Arnaud 2017-03-09 13:43:03 UTC Comment hidden (obsolete)
Comment 16 Alex Arnaud 2017-03-09 13:43:11 UTC Comment hidden (obsolete)
Comment 17 Alex Arnaud 2017-03-09 13:54:06 UTC
(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.
Comment 18 Alex Arnaud 2017-03-10 09:44:33 UTC Comment hidden (obsolete)
Comment 19 M. Tompsett 2017-03-10 19:01:14 UTC
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?
Comment 20 Alex Arnaud 2017-03-13 10:13:53 UTC
> 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.
Comment 21 Josef Moravec 2017-04-04 12:17:13 UTC
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>
Comment 22 Josef Moravec 2017-04-04 12:17:23 UTC Comment hidden (obsolete)
Comment 23 Josef Moravec 2017-04-04 12:17:32 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2017-04-12 16:50:05 UTC
This <UNCHANGED> stuff smells like a very bad codding pattern.
Could not we use empty string and undef/NULL to handle empty values?
Comment 25 Alex Arnaud 2017-04-18 13:05:27 UTC
(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.
Comment 26 Katrin Fischer 2017-04-18 13:12:42 UTC
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.
Comment 27 Alex Sassmannshausen 2017-05-10 13:38:03 UTC
(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
Comment 28 Jonathan Druart 2017-05-11 15:04:43 UTC
Please add a reference to the DB changes in the commit message.

You also have to provide the same changes for kohastructure!
Comment 29 Alex Arnaud 2017-05-18 13:52:04 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2017-05-18 18:11:43 UTC
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.
Comment 31 Marcel de Rooy 2017-05-19 06:38:32 UTC
(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. ?
Comment 32 Marcel de Rooy 2017-05-24 14:55:26 UTC
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?
Comment 33 Kyle M Hall 2018-02-01 19:19:54 UTC
*** Bug 15876 has been marked as a duplicate of this bug. ***
Comment 34 Alex Arnaud 2018-02-21 13:11:58 UTC Comment hidden (obsolete)
Comment 35 Josef Moravec 2018-03-05 11:55:19 UTC
I can't even add an item without collection code after last patch
Comment 36 Katrin Fischer 2018-08-12 21:45:28 UTC Comment hidden (obsolete)
Comment 37 Alex Arnaud 2018-09-17 13:38:43 UTC
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>
Comment 38 Alex Arnaud 2018-09-17 13:38:55 UTC
Created attachment 79031 [details] [review]
Bug 10382 - Add unit tests for course items

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 39 Alex Arnaud 2018-09-17 13:39:08 UTC
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
Comment 40 ByWater Sandboxes 2018-09-19 14:34:20 UTC
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>
Comment 41 ByWater Sandboxes 2018-09-19 14:34:25 UTC
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>
Comment 42 ByWater Sandboxes 2018-09-19 14:34:30 UTC
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>
Comment 43 M. Tompsett 2018-09-19 15:03:46 UTC
ByWater Sandboxes seem to not be marking things signed off.
Comment 44 Theodoros Theodoropoulos 2018-09-20 06:10:01 UTC
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).
Comment 45 Marcel de Rooy 2018-09-21 08:56:22 UTC
        * 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
Comment 46 Marcel de Rooy 2018-09-21 09:00:34 UTC
create_dependent_objets();

French spelling :) Better use objects
Comment 47 Marcel de Rooy 2018-09-21 09:05:05 UTC
QA: Looking here
Comment 48 Marcel de Rooy 2018-09-21 09:10:56 UTC
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?
Comment 49 Alex Arnaud 2018-09-21 10:14:31 UTC
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>
Comment 50 Alex Arnaud 2018-09-21 10:14:44 UTC
Created attachment 79208 [details] [review]
Bug 10382: Add unit tests for course items

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 51 Alex Arnaud 2018-09-21 10:14:56 UTC
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
Comment 52 Kyle M Hall 2018-09-21 18:53:51 UTC
(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!
Comment 53 Marcel de Rooy 2018-09-24 07:46:52 UTC
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.
Comment 54 Alex Arnaud 2018-09-24 08:57:40 UTC
(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.
Comment 55 Marcel de Rooy 2018-09-24 10:12:17 UTC
(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.
Comment 56 Alex Arnaud 2018-09-24 14:36:10 UTC
(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.
Comment 57 Marcel de Rooy 2018-09-25 13:00:09 UTC
(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).
Comment 58 Marcel de Rooy 2018-09-28 07:17:25 UTC
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 :)
Comment 59 Caroline Cyr La Rose 2018-09-28 17:26:06 UTC
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.
Comment 60 Katrin Fischer 2018-09-30 11:58:45 UTC
Hi Caroline,
I can't reproduce the error - can you try again with another course reserve?
Comment 61 Caroline Cyr La Rose 2018-10-01 13:28:36 UTC
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.
Comment 62 Caroline Cyr La Rose 2018-10-01 13:53:09 UTC
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
Comment 63 Kyle M Hall 2018-10-01 14:39:06 UTC
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.
Comment 64 Kyle M Hall 2018-10-01 14:40:33 UTC
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>
Comment 65 Kyle M Hall 2018-10-01 14:40:49 UTC
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>
Comment 66 Kyle M Hall 2018-10-01 14:40:55 UTC
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>
Comment 67 Marcel de Rooy 2018-10-05 09:57:26 UTC
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>
Comment 68 Marcel de Rooy 2018-10-05 09:57:33 UTC
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>
Comment 69 Marcel de Rooy 2018-10-05 09:57:40 UTC
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>
Comment 70 Marcel de Rooy 2018-10-05 10:00:27 UTC
@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.
Comment 71 Christopher Brannon 2018-10-05 14:17:06 UTC
(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.
Comment 72 Nick Clemens 2018-10-05 17:37:56 UTC
(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?
Comment 73 Katrin Fischer 2018-10-05 17:55:41 UTC
I don't think we should have empty entries in collections in locations. They will  appear doubled up in lots of places.
Comment 74 Christopher Brannon 2018-10-05 20:08:35 UTC
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.
Comment 75 Marcel de Rooy 2018-10-09 11:03:16 UTC
(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..
Comment 76 Marcel de Rooy 2018-10-09 11:04:36 UTC
(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.
Comment 77 Christopher Brannon 2018-10-09 14:37:31 UTC
(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.
Comment 78 Marcel de Rooy 2018-10-10 11:49:52 UTC
RM: Your decision now. I said enough..
Comment 79 Nick Clemens 2018-10-15 16:56:14 UTC
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
Comment 80 Martin Renvoize 2018-10-16 13:04:39 UTC
Pushed to 18.05.x for 18.05.05
Comment 81 Katrin Fischer 2018-10-20 10:52:34 UTC
Does this fix location and collection and should the bug title be adjusted accordingly?
Comment 82 Fridolin SOMERS 2018-10-24 08:59:01 UTC
Pushed to 17.11.x for 17.11.11