Bug 24772 - Deactivating course reserves before deleting the same course empties/resets course reserve values in the items
Summary: Deactivating course reserves before deleting the same course empties/resets c...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Course reserves (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Bugs List
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 23727
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-01 20:29 UTC by Kelly McElligott
Modified: 2020-05-26 22:11 UTC (History)
5 users (show)

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


Attachments
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled (1.58 KB, patch)
2020-04-29 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled (1.62 KB, patch)
2020-05-01 12:51 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled (1.63 KB, patch)
2020-05-01 14:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled (1.67 KB, patch)
2020-05-01 14:29 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled (1.73 KB, patch)
2020-05-01 14:45 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2020-03-01 20:29:16 UTC
Deactivating a from Course Reserves reverts Item's Shelving Location correctly, then deleting the same course reverts the Course Reserve Items to old shelving location values.

To Recreate: 

1. Add items to a Course - make sure they all have shelving locations prior to being added to the course. 

2. When adding items change their shelving location to something else.

a. See that Shelving Location has changed; permanent location in parentheses
4. Deactivated course
a. See that Shelving Location has changed back to its original location
5. Deleted course
a. See that Shelving Location has changed back to what was picked during adding it to the Course.
Comment 1 Hannah Olsen 2020-03-05 19:09:23 UTC
At my library we use Course Reserves regularly to manage our displays so that we can change the shelving location and have it change back automatically. We found the bug while trying to figure out why things that had been on display weren't moving back to their original shelving locations.
Comment 2 Katrin Fischer 2020-04-05 13:26:09 UTC
I think this might be duplicate to bug 23727
Comment 3 Hannah Olsen 2020-04-06 15:11:38 UTC
(In reply to Katrin Fischer from comment #2)
> I think this might be duplicate to bug 23727

We saw that one, but that bug involves trying to edit items within the course reserve and have those changes reflected in the reserve without removing the item and re-adding it. The behavior here is related to deactivating and deleting courses and what happens to the values that were changed in the reserves module.
Comment 4 Hannah Olsen 2020-04-06 15:14:44 UTC
(In reply to Hannah Olsen from comment #3)
> (In reply to Katrin Fischer from comment #2)
> > I think this might be duplicate to bug 23727
> 
> We saw that one, but that bug involves trying to edit items within the
> course reserve and have those changes reflected in the reserve without
> removing the item and re-adding it. The behavior here is related to
> deactivating and deleting courses and what happens to the values that were
> changed in the reserves module.

Reviewing that bug again and some of the later info, maybe it is related to the same problematic table?
Comment 5 Katrin Fischer 2020-04-29 06:28:24 UTC
Hi Kelly, I have retestet this, but I think even with the recent work on course reserves there is still a bug here. Maybe you can confirm?

To Recreate: 

1. Add items to a Course - make sure they all have shelving locations prior to being added to the course. 

2. When adding items change their shelving location to something else.

a. See that Shelving Location has changed; permanent location in parentheses
4. Deactivated course
a. See that Shelving Location has changed back to its original location
5. Deleted course

Old problem:
a. See that Shelving Location has changed back to what was picked during adding it to the Course.

New problem:
a. has changed for me, instead of showing the 'course reserves' location, the location is emtpy now.

I am updating severity, as this is a data loss bug and the workflow appears pretty standard - first disable, then delete...
Comment 6 Katrin Fischer 2020-04-29 06:34:50 UTC
Updating severity again - I tried with other changed values and it will kill them all:

- change values for reserve item
- disable course reserve
- delete course reserve

collection, location and holding branch were lost and are now longer set.
Comment 7 Kyle M Hall 2020-04-29 14:44:14 UTC
Created attachment 103953 [details] [review]
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled

Deactivating a from Course Reserves reverts Item's Shelving Location
correctly, then deleting the same course reverts the Course Reserve
Items to old shelving location values.

Test Plan:
1) Add items to a Course - make sure they all have shelving locations
prior to being added to the course.
2) When adding items change their shelving location to something else.
   See that Shelving Location has changed; permanent location in
parentheses
3) Deactivated course
   See that Shelving Location has changed back to its original location
4) Deleted course
   See that Shelving Location has changed back to what was picked during
adding it to the Course.
5) Apply this patch, restart all the things!
6) Repeat steps 1-4, note the shelvig location is not lost!
Comment 8 Victor Grousset/tuxayo 2020-04-29 17:12:12 UTC
> 4) Deleted course
>   See that Shelving Location has changed back to what was picked during
> adding it to the Course.

In my case it got changed to nothing. Is it expected?
Should the test plan be amended?

> 6) Repeat steps 1-4, note the shelvig location is not lost!

Indeed it works :D
Comment 9 Hannah Olsen 2020-04-29 18:34:06 UTC
> > 4) Deleted course
> >   See that Shelving Location has changed back to what was picked during
> > adding it to the Course.
> 
> In my case it got changed to nothing. Is it expected?
> Should the test plan be amended?
> 
> > 6) Repeat steps 1-4, note the shelvig location is not lost!
> 
> Indeed it works :D

Does it keep it as the original location (previous to the Course) or does it change it back to what was picked during adding to Course?
Comment 10 Victor Grousset/tuxayo 2020-04-29 19:17:39 UTC
original location
Comment 11 Katrin Fischer 2020-04-30 06:48:46 UTC
If the location was empty before the item was added to the course, it should be empty after the deleting the course too.

If the location was set before, it should have the same value again in the end.

This applies to all item values that were changed while it was in the course reserve, you need to end up with the same item values as before.
Comment 12 Victor Grousset/tuxayo 2020-04-30 20:50:13 UTC
test note: Do not re-enable the course, it won't reproduce the bug.
> 3) Deactivated course
>    See that Shelving Location has changed back to its original location
> 4) Deleted course
>    See that Shelving Location has changed back to what was picked during
> adding it to the Course.
Comment 13 Victor Grousset/tuxayo 2020-04-30 20:51:11 UTC
Katrin:
> If the location was empty before the item was added to the course,
> it should be  empty after the deleting the course too.

Retried: the patch works well in this case too.

Unexpected behavior of comment 8 still happens.
Comment 14 Katrin Fischer 2020-04-30 21:59:15 UTC
If a location that was set before, is not returned (that's comment#8?) then we should set this to Failed QA to alert Kyle.
Comment 15 Victor Grousset/tuxayo 2020-05-01 12:40:01 UTC
Sorry for the confusion. It was an unexpected behavior in the test plan.

That step is in reproducing the bug. A problem still occurs, but not the one described.
Sometimes when that happen that doesn't prevent me from signing off when it's clear that the patch works. And sometimes I wait and ask for confirmation. It depend of how big the difference from was I got and the test plan is.
It's a bit confusing, I haven't settled yet for a more clear strategy.

In this case thanks to the patch, I couldn't reproduce
- the issue from the test plan
- or the one I actually got when testing without patch.

So it seems ok. Should I sign off? Another factor is that I didn't see a hurry on this patch related to the feature freeze. So I could be picky about unexpected stuff ^^
Comment 16 Katrin Fischer 2020-05-01 12:43:07 UTC
I had different finding than Kelly when testing this (comment 0 vs. comment 5), I think if you agree that the results are now as you would expect, you could sign off with a comment.
Comment 17 Victor Grousset/tuxayo 2020-05-01 12:51:08 UTC
Created attachment 104085 [details] [review]
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled

Deactivating a from Course Reserves reverts Item's Shelving Location
correctly, then deleting the same course reverts the Course Reserve
Items to old shelving location values.

Test Plan:
1) Add items to a Course - make sure they all have shelving locations
prior to being added to the course.
2) When adding items change their shelving location to something else.
   See that Shelving Location has changed; permanent location in
parentheses
3) Deactivated course
   See that Shelving Location has changed back to its original location
4) Deleted course
   See that Shelving Location has changed back to what was picked during
adding it to the Course.
5) Apply this patch, restart all the things!
6) Repeat steps 1-4, note the shelvig location is not lost!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 18 Victor Grousset/tuxayo 2020-05-01 12:56:18 UTC
IIUC what I got in comment 8 is the same thing as what Katrin describes in comment 5.

> a. has changed for me, instead of showing the 'course reserves' location, the location is emtpy now.



> I think if you agree that the results are now as you would expect, you could sign off with a comment.

Thanks for the green light! :)
Comment 19 Katrin Fischer 2020-05-01 14:21:13 UTC
Hm, I've amended Kyle's test plan slightly. In my experience it did not revert to the course reserve value but was emptied. It might have got lost in the comments here as the problem presented differently before.

I think we should push the fix, but I'd really love to see tests here - we should make sure this doesn't get a chance to come back unnoticed.
Comment 20 Katrin Fischer 2020-05-01 14:21:43 UTC
Created attachment 104098 [details] [review]
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled

Deactivating a from Course Reserves reverts Item's Shelving Location
correctly, then deleting the same course reverts the Course Reserve
Items to old shelving location values.

Test Plan:
1) Add items to a Course - make sure they all have shelving locations
prior to being added to the course.
2) When adding items change their shelving location to something else.
   See that Shelving Location has changed; permanent location in
parentheses
3) Deactivated course
   See that Shelving Location has changed back to its original location
4) Deleted course
   See that Shelving Loction is now empty/wrong
5) Apply this patch, restart all the things!
6) Repeat steps 1-4, note the shelvig location is not lost!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 ByWater Sandboxes 2020-05-01 14:29:37 UTC
Created attachment 104106 [details] [review]
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled

Deactivating a from Course Reserves reverts Item's Shelving Location
correctly, then deleting the same course reverts the Course Reserve
Items to old shelving location values.

Test Plan:
1) Add items to a Course - make sure they all have shelving locations
prior to being added to the course.
2) When adding items change their shelving location to something else.
   See that Shelving Location has changed; permanent location in
parentheses
3) Deactivated course
   See that Shelving Location has changed back to its original location
4) Deleted course
   See that Shelving Location has changed back to what was picked during
adding it to the Course.
5) Apply this patch, restart all the things!
6) Repeat steps 1-4, note the shelvig location is not lost!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Hannah Olsen <holsen@duncanville.com>
Comment 22 Hannah Olsen 2020-05-01 14:31:03 UTC
Confirmed that behavior is correct, as below:
- when adding items change shelving location to something else
- deactivate course: shelving location goes to original location
- delete deactivated course: shelving location stays the original location
Comment 23 Hannah Olsen 2020-05-01 14:37:45 UTC
new to this and think I may have messed up the status on here -- sorry!
Comment 24 Katrin Fischer 2020-05-01 14:45:46 UTC
Created attachment 104107 [details] [review]
Bug 24772: Don't revert course item fields when deleting a course item unless it is enabled

Deactivating a from Course Reserves reverts Item's Shelving Location
correctly, then deleting the same course reverts the Course Reserve
Items to old shelving location values.

Test Plan:
1) Add items to a Course - make sure they all have shelving locations
prior to being added to the course.
2) When adding items change their shelving location to something else.
   See that Shelving Location has changed; permanent location in
parentheses
3) Deactivated course
   See that Shelving Location has changed back to its original location
4) Deleted course
   See that Shelving Location has changed back to what was picked during
adding it to the Course.
5) Apply this patch, restart all the things!
6) Repeat steps 1-4, note the shelvig location is not lost!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Hannah Olsen <holsen@duncanville.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Martin Renvoize 2020-05-01 14:46:00 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 26 Katrin Fischer 2020-05-01 14:47:48 UTC
Another sign-off is great to have here, thx Hannah!
Comment 27 Victor Grousset/tuxayo 2020-05-01 14:51:58 UTC
Hannah:
> - delete deactivated course: shelving location stays the original location

Odd, doesn't it get emptied?


> new to this and think I may have messed up the status on here -- sorry!

I think you tested at the same time as Katrin. So the last to finish and send the results here overwrites the changes of the other one ^^"
It's hard to avoid aside from looking again on the bug just because signing off. (maybe this is another easy way?)
Comment 28 Hannah Olsen 2020-05-01 15:03:59 UTC
(In reply to Victor Grousset/tuxayo from comment #27)
> Hannah:
> > - delete deactivated course: shelving location stays the original location
> 
> Odd, doesn't it get emptied?

It did not get emptied in my testing. I ran through the testing process twice to be sure since this bug is one that's impacted my library pretty significantly.
Comment 29 Katrin Fischer 2020-05-01 15:11:14 UTC
It's odd... but I left room for both
Comment 30 Katrin Fischer 2020-05-01 15:11:56 UTC
(In reply to Katrin Fischer from comment #29)
> It's odd... but I left room for both... in the commit message and bug description now.

The reassuring thing here is that we all seem to agree it behaves correctly with the patches
Comment 31 Joy Nelson 2020-05-08 22:55:27 UTC
wont apply to 19.11 until 23727 applies.  not backported
Comment 32 Hannah Olsen 2020-05-26 15:10:39 UTC
(In reply to Joy Nelson from comment #31)
> wont apply to 19.11 until 23727 applies.  not backported

The bug exists in our Koha installation right now -- so there will be no fix until we get 20.05?
Comment 33 Katrin Fischer 2020-05-26 22:11:56 UTC
This depends on another bigger change - bug 23727. For fixing the bug, we'd first need the other development and there has been some issues in backporting it (you can check on the other bug). Joy has indicated she needs help rebasing/making things pass to the dev there.