Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". To test: - Create a new course - Find an item, make sure it has a location set - Add the item to the course, leave all values unchanged - Edit the course item - Change all the values - Verify the table still says "Unchanged" - Verify the table doesn't show the location - Edit the item again - Verify the changed values don't show
itype, ccode and holdingbranch from the course_items table can only be set on adding a reserve. At first glance I'd say it is the expected behavior.
I don't agree, the GUI suggests otherwiese: We have an "Edit" button. If you can't change those values, we should not be offering them.
It changes the values of the item. I am just saying that it seems it always worked that way :)
Created attachment 95104 [details] PDF highlighting the problematic behaviour
Updating to blocker, as this ends in data loss (original item values can't be restored)
Kyle, any ideas about how we could fix this correctly?
(In reply to Jonathan Druart from comment #6) > Kyle, any ideas about how we could fix this correctly? Something bad has happened to CourseReserves::_UpdateCourseItem. This function should check to see if the course is active. If it is inactive, it should edit the values in the course_items table, if it is active, it should edit the values in the items table. In master, it *always* edits the items table, and there is no way to ever update the course_items table no matter if the course is active or inactive! This is appears to be a deeper more problematic issue than the stated one though. I think the solution for the "Unchanged" issue is to add a 'use/ignore' column for each of the swappable columns (itype_enabled, ccode_enabled, etc), along with a checkbox in the editor to enable them. This would solves the original issue, that "Unchanged" as a value has been unreliable and weird. The module was first written without the expectation that a library would want an item column to be undef while *not* on course reserve, and have a value while *on* course reserve. Undef was used to signify the "Leave unchanged" status. It seems that after solving the more pressing issue above, moving the swap-ability indicator to it's own column would be the once-and-for-all solution to this problem.
I'm wondering if we should have the course items editor *only* update the course items table and *never* the items table. That would also mean disabling the ability to edit course items on active course reserve. This could make the system more understandable, though less flexible.
Created attachment 98943 [details] [review] Bug 23727: Add new columns
Created attachment 98944 [details] [review] Bug 23727: Update Schema
Created attachment 98945 [details] [review] Bug 23727: Add Koha Object(s) related to course reserves
Follow work at: https://github.com/bywatersolutions/bywater-koha-devel/commits/bug_23727
Created attachment 99269 [details] [review] Bug 23727: Add new columns
Created attachment 99270 [details] [review] Bug 23727: Update Schema
Created attachment 99271 [details] [review] Bug 23727: Add Koha Object(s) related to course reserves
Created attachment 99272 [details] [review] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before
Created attachment 99273 [details] [review] Bug 23727: Add new columns
Created attachment 99274 [details] [review] Bug 23727: Update Schema
Created attachment 99275 [details] [review] Bug 23727: Add Koha Object(s) related to course reserves
Created attachment 99276 [details] [review] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before
Created attachment 99277 [details] [review] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before
Just testing this now.. so far so good
Created attachment 99327 [details] [review] Bug 23727: Add new columns Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99328 [details] [review] Bug 23727: Update Schema Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99329 [details] [review] Bug 23727: Add Koha Object(s) related to course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99330 [details] [review] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99331 [details] [review] Bug 23727: Add new columns Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99332 [details] [review] Bug 23727: Update Schema Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99333 [details] [review] Bug 23727: Add Koha Object(s) related to course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99334 [details] [review] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99335 [details] [review] Bug 23727: (QA follow-up) Fix script tags
Created attachment 99336 [details] [review] Bug 23727: (QA follow-up) Fix for boolean flags
Works as expected, Signing off.
(In reply to Kyle M Hall from comment #7) > (In reply to Jonathan Druart from comment #6) > > Kyle, any ideas about how we could fix this correctly? > > Something bad has happened to CourseReserves::_UpdateCourseItem. This > function should check to see if the course is active. If it is inactive, it > should edit the values in the course_items table, if it is active, it should > edit the values in the items table. In master, it *always* edits the items > table, and there is no way to ever update the course_items table no matter > if the course is active or inactive! > > This is appears to be a deeper more problematic issue than the stated one > though. I think the solution for the "Unchanged" issue is to add a > 'use/ignore' column for each of the swappable columns (itype_enabled, > ccode_enabled, etc), along with a checkbox in the editor to enable them. > > This would solves the original issue, that "Unchanged" as a value has been > unreliable and weird. The module was first written without the expectation > that a library would want an item column to be undef while *not* on course > reserve, and have a value while *on* course reserve. Undef was used to > signify the "Leave unchanged" status. It seems that after solving the more > pressing issue above, moving the swap-ability indicator to it's own column > would be the once-and-for-all solution to this problem. Kyle, did you track down which patch (bug number) broke the original feature?
> Kyle, did you track down which patch (bug number) broke the original feature? I did not, considering it's always been broken to some extent ( though not as badly as it is now in master ). I decided to push forward with a more 'correct' fix, rather that revert to past behavior which, while somewhat functional, was still broken as well.
Created attachment 103126 [details] [review] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 103127 [details] [review] Bug 23727: (QA follow-up) Fix script tags
Created attachment 103128 [details] [review] Bug 23727: (QA follow-up) Fix for boolean flags
Looking here
Generally, we are doing refactoring here and resolving a critical bug. We should not. To resolve a critical, I do not expect 876 insertions(+), 172 deletions. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before Rather thin test plan for 800+ lines ? As mentioned on IRC, would have been nice to get a librarian signoff (who knows the topic) for a patch set of this size (instead of the RM now). Glancing through the code: C4 CourseReserves + my $course = Koha::Courses->find( $course_id ); $course->{'instructors'} = $sth->fetchall_arrayref( {} ); => This is a weird way of mixing Koha Objects / DBIx and old school DBI. "the new storage columns store the original item value" The name field_storage is not clear to me right away. If you mean original value, we could think of a better name? No blocker. itype_enabled = 'no' `itype_enabled` tinyint(1) Came across that somewhere. Looks odd. + UPDATE course_items SET + itype = IF( itype_enabled = 'no', NULL, itype ), + ccode = IF( ccode_enabled = 'no', NULL, ccode ), + holdingbranch = IF( holdingbranch_enabled = 'no', NULL, holdingbranch ), + location = IF( location_enabled = 'no', NULL, location ) + WHERE enabled = 'no'; The course_items table has a field enabled (enum yes no). Does this field need to be there? If an item is enabled, it is in the table, right? When I remove it from the course, the record is gone. Interface for add_items. The meaning of the checkbox is not obvious. Conclusion: I should probably set it to Failed QA, which I wont. Since we are on a critical here and it seems to work now, I am passing QA. But note that this area needs more work. Please provide feedback to questions raised.
Created attachment 103131 [details] [review] Bug 23727: Add new columns Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 103132 [details] [review] Bug 23727: Update Schema Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 103133 [details] [review] Bug 23727: Add Koha Object(s) related to course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 103134 [details] [review] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 103135 [details] [review] Bug 23727: (QA follow-up) Fix script tags Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 103136 [details] [review] Bug 23727: (QA follow-up) Fix for boolean flags Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nice work everyone! Pushed to master for 20.05
Kyle-My rebase failed spectacularly. Can you rebase for 19.11.x? Thanks joy
Created attachment 104782 [details] [review] [19.11.x] Bug 23727: Add new columns Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 104783 [details] [review] [19.11.x] Bug 23727: Update Schema Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 104784 [details] [review] [19.11.x] Bug 23727: Add Koha Object(s) related to course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 104785 [details] [review] [19.11.x] Bug 23727: Editing course reserve items is broken Adding an item to course reserves and trying to edit any values in a second step does not work. Values are not saved and the table shows all values as "Unchanged". This patch set adds two new sets of columns to the course_items table. The first set determines if the specified column should be swapped or not. The was previously 'implied' by the column being set to undef which has been the root problem with that way of knowing if a column should swap or not. The second set of new columns are for storing the item field values while the item is on course reserve. Previously, the column values were swapped between the items table and the course_items table, which leaves ambiguity as to what each value is. Now, the original columns *always* store the value when the item is on course reserve, and the new storage columns store the original item value while the item is on reserve, and are NULL when an item is *not* on reserve. Test Plan: 1) Apply this patch 2) Add and edit course items, not the new checkboxes for enabling fields 3) Everything should function as before Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104786 [details] [review] [19.11.x] Bug 23727: (QA follow-up) Fix script tags Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 104787 [details] [review] [19.11.x] Bug 23727: (QA follow-up) Fix for boolean flags Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Thanks for the rebase. Backported to 19.11.x for 19.11.06
Correction. Revert the backport of this due to QA failures in backport. Most are pod coverage except for the first error. see below FAIL C4/CourseReserves.pm FAIL critic # Subroutines::ProhibitExplicitReturnUndef: Got 1 violation(s). OK forbidden patterns OK git manipulation OK pod OK pod coverage OK spelling OK valid FAIL Koha/Course.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 OK spelling OK valid FAIL Koha/Course/Instructor.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 OK spelling OK valid OK Koha/Course/Instructors.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage OK spelling OK valid OK Koha/Course/Item.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage OK spelling OK valid OK Koha/Course/Items.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage OK spelling OK valid FAIL Koha/Course/Reserve.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 OK spelling OK valid
Hey Kyle, can you please take a look at Joy's comment above about qa tests so I can backport to 19.11.x?
(In reply to Aleisha Amohia from comment #57) > Hey Kyle, can you please take a look at Joy's comment above about qa tests > so I can backport to 19.11.x? I believe you can safely ignore those errors. If we want to get pedantic, we should probably fix the ProhibitExplicitReturnUndef warning on a new bug report, as that same code is in master.
(In reply to Kyle M Hall from comment #58) > I believe you can safely ignore those errors. If we want to get pedantic, we > should probably fix the ProhibitExplicitReturnUndef warning on a new bug > report, as that same code is in master. No worries then, I won't backport it. We probably should fix the warning anyway :)
(In reply to Kyle M Hall from comment #58) > (In reply to Aleisha Amohia from comment #57) > > Hey Kyle, can you please take a look at Joy's comment above about qa tests > > so I can backport to 19.11.x? > > I believe you can safely ignore those errors. If we want to get pedantic, we > should probably fix the ProhibitExplicitReturnUndef warning on a new bug > report, as that same code is in master. I have opened bug 25874.
*** Bug 20283 has been marked as a duplicate of this bug. ***