Bug 25444 - Course reserve settings are not saved on edit
Summary: Course reserve settings are not saved on edit
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Course reserves (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 23727
Blocks: 25609
  Show dependency treegraph
 
Reported: 2020-05-10 11:39 UTC by Katrin Fischer
Modified: 2020-06-18 21:17 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05


Attachments
Bug 25444: Set the storage fields when adding new fields to an existing course item, revert fields when removing from existing course item (4.29 KB, patch)
2020-05-26 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25444: Set the storage fields when adding new fields to an existing course item, revert fields when removing from existing course item (5.01 KB, patch)
2020-05-26 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25444: Set the storage fields when adding new fields to an existing course item, revert fields when removing from existing course item (5.01 KB, patch)
2020-05-26 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25444: Add unit tests (8.07 KB, patch)
2020-05-26 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25444: Add unit tests (8.11 KB, patch)
2020-05-26 21:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25444: Backup/restore course items fields correctly (5.41 KB, patch)
2020-05-26 21:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25444: Simplify the code using a loop (4.83 KB, patch)
2020-05-27 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25444: More minor improvements to simplified loop (1.08 KB, patch)
2020-05-27 10:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25444: Add unit tests (8.18 KB, patch)
2020-05-27 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25444: Backup/restore course items fields correctly (5.48 KB, patch)
2020-05-27 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25444: Simplify the code using a loop (4.90 KB, patch)
2020-05-27 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25444: More minor improvements to simplified loop (1.14 KB, patch)
2020-05-27 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2020-05-10 11:39:09 UTC
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?
Comment 1 Jonathan Druart 2020-05-14 10:37:49 UTC
Can you detail what you expect?
Comment 2 Jonathan Druart 2020-05-14 10:39:41 UTC
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)"
Comment 3 Katrin Fischer 2020-05-23 23:56:29 UTC
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.
Comment 4 Jonathan Druart 2020-05-25 08:32:30 UTC
(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.
Comment 5 Katrin Fischer 2020-05-25 10:14:46 UTC
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.
Comment 6 Kyle M Hall 2020-05-26 15:34:52 UTC
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.
Comment 7 Kyle M Hall 2020-05-26 15:35:51 UTC
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!
Comment 8 Kyle M Hall 2020-05-26 16:31:44 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2020-05-26 17:11:07 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2020-05-26 17:11:17 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2020-05-26 21:19:38 UTC
Created attachment 105370 [details] [review]
Bug 25444: Add unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2020-05-26 21:19:42 UTC
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>
Comment 13 Jonathan Druart 2020-05-27 08:26:41 UTC
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
Comment 14 Jonathan Druart 2020-05-27 08:38:10 UTC
(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)
Comment 15 Jonathan Druart 2020-05-27 08:39:30 UTC
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?
Comment 16 Katrin Fischer 2020-05-27 10:14:55 UTC
> 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
Comment 17 Katrin Fischer 2020-05-27 10:18:22 UTC
(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.
Comment 18 Kyle M Hall 2020-05-27 10:23:23 UTC
(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.
Comment 19 Kyle M Hall 2020-05-27 10:29:03 UTC
Created attachment 105379 [details] [review]
Bug 25444: More minor improvements to simplified loop
Comment 20 Kyle M Hall 2020-05-27 10:32:50 UTC
(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.
Comment 21 Jonathan Druart 2020-05-27 10:43:33 UTC
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>
Comment 22 Jonathan Druart 2020-05-27 10:43:37 UTC
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>
Comment 23 Jonathan Druart 2020-05-27 10:43:40 UTC
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>
Comment 24 Jonathan Druart 2020-05-27 10:43:44 UTC
Created attachment 105386 [details] [review]
Bug 25444: More minor improvements to simplified loop

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2020-05-27 10:46:45 UTC
(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.
Comment 26 Jonathan Druart 2020-05-27 10:54:54 UTC
(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.
Comment 27 Jonathan Druart 2020-05-29 09:48:15 UTC
Pushed to master for 20.05
Comment 28 Aleisha Amohia 2020-06-18 21:17:05 UTC
missing dependencies, not backported to 19.11.x