Bug 23727 - Editing course reserve items is broken
Summary: Editing course reserve items is broken
Status: RESOLVED FIXED
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: Marcel de Rooy
URL:
Keywords:
: 20283 (view as bug list)
Depends on:
Blocks: 22630 24772 25444 25874
  Show dependency treegraph
 
Reported: 2019-10-03 09:33 UTC by Katrin Fischer
Modified: 2020-11-30 21:51 UTC (History)
9 users (show)

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


Attachments
PDF highlighting the problematic behaviour (352.92 KB, application/pdf)
2019-11-06 12:31 UTC, Katrin Fischer
Details
Bug 23727: Add new columns (3.11 KB, patch)
2020-02-14 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Update Schema (2.55 KB, patch)
2020-02-14 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Add Koha Object(s) related to course reserves (10.84 KB, patch)
2020-02-14 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Add new columns (6.47 KB, patch)
2020-02-19 20:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Update Schema (3.20 KB, patch)
2020-02-19 20:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Add Koha Object(s) related to course reserves (10.84 KB, patch)
2020-02-19 20:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Editing course reserve items is broken (44.12 KB, patch)
2020-02-19 20:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Add new columns (6.47 KB, patch)
2020-02-19 20:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Update Schema (3.20 KB, patch)
2020-02-19 20:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Add Koha Object(s) related to course reserves (10.84 KB, patch)
2020-02-19 20:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Editing course reserve items is broken (44.12 KB, patch)
2020-02-19 20:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Editing course reserve items is broken (44.04 KB, patch)
2020-02-19 20:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23727: Add new columns (6.51 KB, patch)
2020-02-20 17:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: Update Schema (3.25 KB, patch)
2020-02-20 17:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: Add Koha Object(s) related to course reserves (10.88 KB, patch)
2020-02-20 17:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: Editing course reserve items is broken (44.08 KB, patch)
2020-02-20 17:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: Add new columns (6.51 KB, patch)
2020-02-20 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: Update Schema (3.25 KB, patch)
2020-02-20 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: Add Koha Object(s) related to course reserves (10.88 KB, patch)
2020-02-20 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: Editing course reserve items is broken (44.08 KB, patch)
2020-02-20 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: (QA follow-up) Fix script tags (1.56 KB, patch)
2020-02-20 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: (QA follow-up) Fix for boolean flags (1.03 KB, patch)
2020-02-20 17:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23727: Editing course reserve items is broken (44.25 KB, patch)
2020-04-17 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23727: (QA follow-up) Fix script tags (1.56 KB, patch)
2020-04-17 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23727: (QA follow-up) Fix for boolean flags (1.03 KB, patch)
2020-04-17 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23727: Add new columns (6.62 KB, patch)
2020-04-17 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23727: Update Schema (3.35 KB, patch)
2020-04-17 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23727: Add Koha Object(s) related to course reserves (11.04 KB, patch)
2020-04-17 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23727: Editing course reserve items is broken (44.40 KB, patch)
2020-04-17 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23727: (QA follow-up) Fix script tags (1.67 KB, patch)
2020-04-17 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23727: (QA follow-up) Fix for boolean flags (1.12 KB, patch)
2020-04-17 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[19.11.x] Bug 23727: Add new columns (6.59 KB, patch)
2020-05-12 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
[19.11.x] Bug 23727: Update Schema (3.26 KB, patch)
2020-05-12 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
[19.11.x] Bug 23727: Add Koha Object(s) related to course reserves (10.97 KB, patch)
2020-05-12 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
[19.11.x] Bug 23727: Editing course reserve items is broken (44.76 KB, patch)
2020-05-12 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
[19.11.x] Bug 23727: (QA follow-up) Fix script tags (1.64 KB, patch)
2020-05-12 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
[19.11.x] Bug 23727: (QA follow-up) Fix for boolean flags (1.07 KB, patch)
2020-05-12 13:53 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2019-10-03 09:33:49 UTC
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
Comment 1 Jonathan Druart 2019-10-16 09:46:35 UTC
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.
Comment 2 Katrin Fischer 2019-10-16 10:31:04 UTC
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.
Comment 3 Jonathan Druart 2019-10-16 10:50:17 UTC
It changes the values of the item.

I am just saying that it seems it always worked that way :)
Comment 4 Katrin Fischer 2019-11-06 12:31:45 UTC
Created attachment 95104 [details]
PDF highlighting the problematic behaviour
Comment 5 Katrin Fischer 2019-11-06 12:59:23 UTC
Updating to blocker, as this ends in data loss (original item values can't be restored)
Comment 6 Jonathan Druart 2020-02-14 10:11:02 UTC
Kyle, any ideas about how we could fix this correctly?
Comment 7 Kyle M Hall 2020-02-14 12:22:43 UTC
(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.
Comment 8 Kyle M Hall 2020-02-14 12:25:14 UTC
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.
Comment 9 Kyle M Hall 2020-02-14 13:57:28 UTC
Created attachment 98943 [details] [review]
Bug 23727: Add new columns
Comment 10 Kyle M Hall 2020-02-14 13:57:38 UTC
Created attachment 98944 [details] [review]
Bug 23727: Update Schema
Comment 11 Kyle M Hall 2020-02-14 13:57:41 UTC
Created attachment 98945 [details] [review]
Bug 23727: Add Koha Object(s) related to course reserves
Comment 12 Kyle M Hall 2020-02-14 20:23:05 UTC
Follow work at:

https://github.com/bywatersolutions/bywater-koha-devel/commits/bug_23727
Comment 13 Kyle M Hall 2020-02-19 20:00:12 UTC
Created attachment 99269 [details] [review]
Bug 23727: Add new columns
Comment 14 Kyle M Hall 2020-02-19 20:00:20 UTC
Created attachment 99270 [details] [review]
Bug 23727: Update Schema
Comment 15 Kyle M Hall 2020-02-19 20:00:23 UTC
Created attachment 99271 [details] [review]
Bug 23727: Add Koha Object(s) related to course reserves
Comment 16 Kyle M Hall 2020-02-19 20:00:26 UTC
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
Comment 17 Kyle M Hall 2020-02-19 20:17:37 UTC
Created attachment 99273 [details] [review]
Bug 23727: Add new columns
Comment 18 Kyle M Hall 2020-02-19 20:17:45 UTC
Created attachment 99274 [details] [review]
Bug 23727: Update Schema
Comment 19 Kyle M Hall 2020-02-19 20:17:49 UTC
Created attachment 99275 [details] [review]
Bug 23727: Add Koha Object(s) related to course reserves
Comment 20 Kyle M Hall 2020-02-19 20:17:52 UTC
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
Comment 21 Kyle M Hall 2020-02-19 20:31:04 UTC
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
Comment 22 Martin Renvoize 2020-02-20 16:49:06 UTC
Just testing this now.. so far so good
Comment 23 Martin Renvoize 2020-02-20 17:06:44 UTC
Created attachment 99327 [details] [review]
Bug 23727: Add new columns

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2020-02-20 17:06:47 UTC
Created attachment 99328 [details] [review]
Bug 23727: Update Schema

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2020-02-20 17:06:50 UTC
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>
Comment 26 Martin Renvoize 2020-02-20 17:06:54 UTC
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>
Comment 27 Martin Renvoize 2020-02-20 17:17:46 UTC
Created attachment 99331 [details] [review]
Bug 23727: Add new columns

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2020-02-20 17:17:49 UTC
Created attachment 99332 [details] [review]
Bug 23727: Update Schema

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2020-02-20 17:17:52 UTC
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>
Comment 30 Martin Renvoize 2020-02-20 17:17:56 UTC
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>
Comment 31 Martin Renvoize 2020-02-20 17:17:59 UTC
Created attachment 99335 [details] [review]
Bug 23727: (QA follow-up) Fix script tags
Comment 32 Martin Renvoize 2020-02-20 17:18:02 UTC
Created attachment 99336 [details] [review]
Bug 23727: (QA follow-up) Fix for boolean flags
Comment 33 Martin Renvoize 2020-02-20 17:18:27 UTC
Works as expected, Signing off.
Comment 34 Jonathan Druart 2020-03-02 13:45:09 UTC
(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?
Comment 35 Kyle M Hall 2020-03-02 14:28:28 UTC
> 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.
Comment 36 Jonathan Druart 2020-04-17 08:26:03 UTC
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>
Comment 37 Jonathan Druart 2020-04-17 08:26:07 UTC
Created attachment 103127 [details] [review]
Bug 23727: (QA follow-up) Fix script tags
Comment 38 Jonathan Druart 2020-04-17 08:26:11 UTC
Created attachment 103128 [details] [review]
Bug 23727: (QA follow-up) Fix for boolean flags
Comment 39 Marcel de Rooy 2020-04-17 08:45:03 UTC
Looking here
Comment 40 Marcel de Rooy 2020-04-17 10:05:36 UTC
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.
Comment 41 Marcel de Rooy 2020-04-17 10:06:32 UTC
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>
Comment 42 Marcel de Rooy 2020-04-17 10:06:37 UTC
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>
Comment 43 Marcel de Rooy 2020-04-17 10:06:41 UTC
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>
Comment 44 Marcel de Rooy 2020-04-17 10:06:46 UTC
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>
Comment 45 Marcel de Rooy 2020-04-17 10:06:51 UTC
Created attachment 103135 [details] [review]
Bug 23727: (QA follow-up) Fix script tags

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2020-04-17 10:06:56 UTC
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>
Comment 47 Martin Renvoize 2020-04-17 12:51:35 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 48 Joy Nelson 2020-05-08 19:12:00 UTC
Kyle-My rebase failed spectacularly.  Can you rebase for 19.11.x?
Thanks
joy
Comment 49 Kyle M Hall 2020-05-12 13:52:41 UTC
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>
Comment 50 Kyle M Hall 2020-05-12 13:52:50 UTC
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>
Comment 51 Kyle M Hall 2020-05-12 13:52:53 UTC
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>
Comment 52 Kyle M Hall 2020-05-12 13:52:56 UTC
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>
Comment 53 Kyle M Hall 2020-05-12 13:52:59 UTC
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>
Comment 54 Kyle M Hall 2020-05-12 13:53:02 UTC
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>
Comment 55 Joy Nelson 2020-05-19 23:30:55 UTC
Thanks for the rebase.  Backported to 19.11.x for 19.11.06
Comment 56 Joy Nelson 2020-05-19 23:49:09 UTC
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
Comment 57 Aleisha Amohia 2020-06-12 02:20:32 UTC
Hey Kyle, can you please take a look at Joy's comment above about qa tests so I can backport to 19.11.x?
Comment 58 Kyle M Hall 2020-06-18 13:50:23 UTC
(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.
Comment 59 Aleisha Amohia 2020-06-18 20:44:52 UTC
(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 :)
Comment 60 Jonathan Druart 2020-06-25 10:23:06 UTC
(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.
Comment 61 Myka Kennedy Stephens 2020-08-06 23:46:20 UTC
*** Bug 20283 has been marked as a duplicate of this bug. ***