While the setting and resetting of item values now works much better than before (see 23727) the values displayed in the course reserves item table are not correct. It's supposed to show you if something was changed, what it was changed from and what to. But what you see there doesn't match reality or what will happen when the course is set inactive/deleted. To test: - Create a course reserve - Add course reserve items - either change item values directly when adding or - use the edit functionality provided - Take note of the display - does it match changes made? - Disable course and check your items - does it match what was shown?
Can you detail what you expect?
It shows: "Children's Area (General Stacks)" "Children's Area" is the value of the course, and "General Stacks" the original value of the item (and so the value that has been set back). Are we expecting the reverse? "General Stacks (Children's Area)"
Retesting with some more details: 1) Item used, original values: Item type: Books Holding library: Fairview Home library: Fairview Location: General Stacks Collection: Reference Barcode: 39999000000399 (from sample data) 2) Added to an active course reserve, using "Add reserves": Item type: Maps (changed) Holding library: Fairview Home library: Riverside (changed) Location: General Stacks Collection: Fiction (changed) 3) Display on course reserves table in staff: Maps (Books) Fiction (Reference) Unchanged (General Stacks) Riverside (Riverside) = WRONG! Unchanged (Fairview) 4) Using "Edit" option from course reserves: Additionally changing: Location: On Display (was General stacks) Holding library: Midway (was Fairview) 5) Display on course reserves table in staff: Maps (Books) Fiction (Reference) On Display () = WRONG! Expected: On display (General stacks) Riverside (Riverside) = Still wrong... Midway () = WRONG! Expected: Midway (Fairview) 6) Remove item from course reserves using "remove" button: Item type: Books Holding library: EMPTY = WRONG! Home library: Fairview Location: EMPTY = WRONG! Collection: Reference Barcode: 39999000000399 We still seem to somehow lose data here :( Updating severity.
(In reply to Katrin Fischer from comment #3) > Retesting with some more details: > > 1) Item used, original values: > > Item type: Books > Holding library: Fairview > Home library: Fairview > Location: General Stacks > Collection: Reference > Barcode: 39999000000399 (from sample data) > > 2) Added to an active course reserve, using "Add reserves": > > Item type: Maps (changed) > Holding library: Fairview > Home library: Riverside (changed) > Location: General Stacks > Collection: Fiction (changed) > > 3) Display on course reserves table in staff: > > Maps (Books) > Fiction (Reference) > Unchanged (General Stacks) > Riverside (Riverside) = WRONG! > Unchanged (Fairview) Will be fixed by the follow-up I have just attached to bug 22630: Bug 22630: Display correct "in storage" value > 4) Using "Edit" option from course reserves: > > Additionally changing: > Location: On Display (was General stacks) > Holding library: Midway (was Fairview) > > 5) Display on course reserves table in staff: > > Maps (Books) > Fiction (Reference) > On Display () = WRONG! Expected: On display (General stacks) > Riverside (Riverside) = Still wrong... > Midway () = WRONG! Expected: Midway (Fairview) At this point we have, in DB: holdingbranch: MPL holdingbranch_enabled: 1 holdingbranch_storage: NULL location: DISPLAY location_enabled: 1 location_storage: NULL My understanding is that the *_storage values are not retrieved when a course is edited. Reading the code it seems that it happens in _SwapAllFields, only called by EnableOrDisableCourseItem.
Sorry, I am not sure I understand your second point. At the moment we experience data loss - neither the new nor the old branch comes back... For the mechanics - I am not sure how this is supposed to work db wise.
Created attachment 105361 [details] [review] Bug 25444: Set the storage fields when adding new fields to an existing course item, revert fields when removing from existing course item If a new field is enabled on an existing course reserve, the storage field is not given a value, so when the item goes off reserve, the item field will always be updated to NULL.
Katrin, can you test this patch? I think it fixes the data loss issue you are having. I think it's causing failing unit tests so I just want to see if this works for you before I dig in to those!
Created attachment 105362 [details] [review] Bug 25444: Set the storage fields when adding new fields to an existing course item, revert fields when removing from existing course item If a new field is enabled on an existing course reserve, the storage field is not given a value, so when the item goes off reserve, the item field will always be updated to NULL.
Created attachment 105363 [details] [review] Bug 25444: Set the storage fields when adding new fields to an existing course item, revert fields when removing from existing course item If a new field is enabled on an existing course reserve, the storage field is not given a value, so when the item goes off reserve, the item field will always be updated to NULL.
Created attachment 105364 [details] [review] Bug 25444: Add unit tests
Created attachment 105370 [details] [review] Bug 25444: Add unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 105371 [details] [review] Bug 25444: Backup/restore course items fields correctly This patch makes the code set the *_storage fields when adding new fields to an existing course item. And reverts those fields correctly when removing the item from the course. If a new field is enabled on an existing course reserve, the storage field is not given a value, so when the item goes off reserve, the item field will always be updated to NULL. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/CourseReserves/CourseItems.t => FAIL: Tests fail, data is not reverted correctly 3. Apply this patch and repeat 2 => SUCCESS: Tests pass! Data is correctly reverted 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 105378 [details] [review] Bug 25444: Simplify the code using a loop In order to prevent typos or further regressions it is better (I think) to have this code into a loop
(In reply to Katrin Fischer from comment #3) > Retesting with some more details: > > 1) Item used, original values: > > Item type: Books > Holding library: Fairview > Home library: Fairview > Location: General Stacks > Collection: Reference > Barcode: 39999000000399 (from sample data) > > 2) Added to an active course reserve, using "Add reserves": > > Item type: Maps (changed) > Holding library: Fairview > Home library: Riverside (changed) > Location: General Stacks > Collection: Fiction (changed) > > 3) Display on course reserves table in staff: => https://snipboard.io/e59AiY.jpg Maps (Books) Fiction (Reference) Unchanged => WRONG Should be Unchanged (General Stacks) Riverside (Fairview) Unchanged (Fairview)
The template is displaying permanent_location, is that correct? In C4::CourseReserves we always play with items.location. I have, in DB: homebranch: RPL holdingbranch: FPL location: GEN permanent_location: NULL I guess that's why I don't see the "location". What is the expected behaviour here?
> Maps (Books) > Fiction (Reference) > Unchanged => WRONG Should be Unchanged (General Stacks) > Riverside (Fairview) > Unchanged (Fairview) I thought you said we would be dealing with this here: >Will be fixed by the follow-up I have just attached to bug 22630: > Bug 22630: Display correct "in storage" value
(In reply to Katrin Fischer from comment #16) > > Maps (Books) > > Fiction (Reference) > > Unchanged => WRONG Should be Unchanged (General Stacks) > > Riverside (Fairview) > > Unchanged (Fairview) > > I thought you said we would be dealing with this here: > > >Will be fixed by the follow-up I have just attached to bug 22630: > > Bug 22630: Display correct "in storage" value (In reply to Jonathan Druart from comment #15) > The template is displaying permanent_location, is that correct? > In C4::CourseReserves we always play with items.location. > I have, in DB: > homebranch: RPL > holdingbranch: FPL > location: GEN > permanent_location: NULL > > I guess that's why I don't see the "location". > What is the expected behaviour here? It's a good question. I am not sure about the mechanics - if permanent_location should be automatically pre-populated or will onyl be if you use the CART feature. I checked or oldest database and permanent_location is almost always empty. I think we should better use location here or have a fallback to it if permanent_location is empty.
(In reply to Jonathan Druart from comment #15) > The template is displaying permanent_location, is that correct? > In C4::CourseReserves we always play with items.location. > I have, in DB: > homebranch: RPL > holdingbranch: FPL > location: GEN > permanent_location: NULL > > I guess that's why I don't see the "location". > What is the expected behaviour here? That was a change made on bug 21446, which doesn't really justify the change. In my original code, everything operates on the 'location' column. The code has never touch permanent location. I would suggest reverting the behavior of 21446.
Created attachment 105379 [details] [review] Bug 25444: More minor improvements to simplified loop
(In reply to Kyle M Hall from comment #18) > (In reply to Jonathan Druart from comment #15) > > The template is displaying permanent_location, is that correct? > > In C4::CourseReserves we always play with items.location. > > I have, in DB: > > homebranch: RPL > > holdingbranch: FPL > > location: GEN > > permanent_location: NULL > > > > I guess that's why I don't see the "location". > > What is the expected behaviour here? > > That was a change made on bug 21446, which doesn't really justify the > change. In my original code, everything operates on the 'location' column. > The code has never touch permanent location. I would suggest reverting the > behavior of 21446. I think we should also move this particular visual issue to a new bug. The description of this bug is "Course reserve settings are not saved on edit", and these patches fix that issue. This discussion relates to a visual bug and seems out of scope for this bug report.
Created attachment 105383 [details] [review] Bug 25444: Add unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 105384 [details] [review] Bug 25444: Backup/restore course items fields correctly This patch makes the code set the *_storage fields when adding new fields to an existing course item. And reverts those fields correctly when removing the item from the course. If a new field is enabled on an existing course reserve, the storage field is not given a value, so when the item goes off reserve, the item field will always be updated to NULL. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/CourseReserves/CourseItems.t => FAIL: Tests fail, data is not reverted correctly 3. Apply this patch and repeat 2 => SUCCESS: Tests pass! Data is correctly reverted 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 105385 [details] [review] Bug 25444: Simplify the code using a loop In order to prevent typos or further regressions it is better (I think) to have this code into a loop Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 105386 [details] [review] Bug 25444: More minor improvements to simplified loop Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Kyle M Hall from comment #20) > (In reply to Kyle M Hall from comment #18) > > (In reply to Jonathan Druart from comment #15) > > > The template is displaying permanent_location, is that correct? > > > In C4::CourseReserves we always play with items.location. > > > I have, in DB: > > > homebranch: RPL > > > holdingbranch: FPL > > > location: GEN > > > permanent_location: NULL > > > > > > I guess that's why I don't see the "location". > > > What is the expected behaviour here? > > > > That was a change made on bug 21446, which doesn't really justify the > > change. In my original code, everything operates on the 'location' column. > > The code has never touch permanent location. I would suggest reverting the > > behavior of 21446. > > I think we should also move this particular visual issue to a new bug. The > description of this bug is "Course reserve settings are not saved on edit", > and these patches fix that issue. This discussion relates to a visual bug > and seems out of scope for this bug report. Agreed, I opened bug 25609.
(In reply to Katrin Fischer from comment #16) > > Maps (Books) > > Fiction (Reference) > > Unchanged => WRONG Should be Unchanged (General Stacks) > > Riverside (Fairview) > > Unchanged (Fairview) > > I thought you said we would be dealing with this here: > > >Will be fixed by the follow-up I have just attached to bug 22630: > > Bug 22630: Display correct "in storage" value It was a problem with the homebranch only.
Pushed to master for 20.05
missing dependencies, not backported to 19.11.x