Description
Kelly McElligott
2020-03-01 20:29:16 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. (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. (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? 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... 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. 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! > 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
> > 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?
original location 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. 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.
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. 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. 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 ^^ 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. 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> 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! :) 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. 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> 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> 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 new to this and think I may have messed up the status on here -- sorry! 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> Nice work everyone! Pushed to master for 20.05 Another sign-off is great to have here, thx Hannah! 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?) (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. It's odd... but I left room for both (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 wont apply to 19.11 until 23727 applies. not backported (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? 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. |