Bug 19974 - Marking an item as 'lost' will not actually modify the current item (cataloguing/additem.pl)
Summary: Marking an item as 'lost' will not actually modify the current item (catalogu...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Charles Farmer
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 8484 (view as bug list)
Depends on:
Blocks: 20528 19287
  Show dependency treegraph
 
Reported: 2018-01-15 21:05 UTC by Charles Farmer
Modified: 2018-10-31 13:40 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
The behaviour for marking a checked out item as 'Lost' is different, depending on the path you use: sometimes the item is checked in, sometimes not. The system preference 'MarkLostItemsAsReturned' now allows libraries to choose if the item is checked for each of the 4 ways an item can be marked as 'Lost': from the edit item form, from the 'Items' tab of the catalog module, from the batch item modification and with the longoverdue cronjob.


Attachments
Bug19974 - Marking an item as 'lost' in additem.pl will not modify the current item (2.61 KB, patch)
2018-01-15 22:05 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 19974: Marking an item as 'lost' in additem.pl will not modify the current item (2.70 KB, patch)
2018-03-23 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19974: (QA follow-up) Move lost logic to else branch (2.14 KB, patch)
2018-03-23 08:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19974: Make MarkLostItemsAsReturned multiple (8.62 KB, patch)
2018-04-02 17:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19974: Make MarkLostItemsAsReturned multiple (8.62 KB, patch)
2018-04-12 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19974: Make MarkLostItemsAsReturned multiple (9.36 KB, patch)
2018-04-13 13:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19974: Make MarkLostItemsAsReturned multiple (9.36 KB, patch)
2018-04-16 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19974: Make MarkLostItemsAsReturned multiple (10.15 KB, patch)
2018-04-19 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19974: Marking an item as 'lost' in additem.pl will not modify the current item (2.66 KB, patch)
2018-04-19 14:28 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 19974: (QA follow-up) Move lost logic to else branch (2.11 KB, patch)
2018-04-19 14:28 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 19974: Make MarkLostItemsAsReturned multiple (10.25 KB, patch)
2018-04-19 14:28 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 19974: Marking an item as 'lost' in additem.pl will not modify the current item (2.70 KB, patch)
2018-04-20 06:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19974: (QA follow-up) Move lost logic to else branch (2.14 KB, patch)
2018-04-20 06:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19974: Make MarkLostItemsAsReturned multiple (10.34 KB, patch)
2018-04-20 06:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19974: (QA follow-up) Do not use bar separator in atomic update (1.28 KB, patch)
2018-04-20 06:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Farmer 2018-01-15 21:05:53 UTC
When modifying an item through the intranet at the page /cgi-bin/koha/cataloguing/additem.pl?op=edititem, instead of changing the active item's information, the markaslost parameter will be applied to the first Item in the database that has no itemnumber.

I'll submit a patch soon with a clear test plan. This should fortunately not take too long to test.
Comment 1 Charles Farmer 2018-01-15 22:05:47 UTC
Created attachment 70525 [details] [review]
Bug19974 - Marking an item as 'lost' in additem.pl will not modify the current item

    TEST PLAN:

    1) Log in with your superlibrarian account
    2) Borrow any book
    3) Visit your Checkouts page, and click 'Show Checkouts'
    4) Click on the item's barcode to visit the item's page
    5) On the item's page, click the 'Edit' button, and choose 'Edit items'
    6) In the items table, click the 'Actions->Edit' button of the item you borrowed
    7) Mark that item as lost (it should be the first row of the form) and click the button 'Save changes'
    8) Visit your Checkouts page. The item should still be there, despite BZ12363 claiming it should've been automagically returned
        8.1) Your koha-log should also output a warning message: 'DBIx::Class::Storage::DBI::select_single(): Query returned more than one row...'
        8.2) If you visit the item's page, the modification had no effect. It should not be marked as lost.
    9) APPLY PATCH
    10) Start back from step 2), but this time, after marking the item as lost, the item's page should
        reflect the change, and the item you borrowed should've been automatically returned to the library
Comment 2 Charles Farmer 2018-01-15 22:23:45 UTC
To my knowledge, emptying $itemnumber is only done so we don't fall back onto the same item after saving changes. On one hand, we load back into the $op=additem interface if changes were accepted, otherwise, we load the same item again.

What's lost on me is the reason why we'd want to proceed with the deletion logic at all if the newitem has a barcode already used. Is there code anywhere edging on that voodoo logic of deleting an item from an unsaved biblionotice?

What would make sense, imo, is pushing that deletion logic upward inside the 'else' block and doing that at the same time as ModItemFromMarc. But again, without a strong regression test suite of that particular part, I went with the easy way out and changed as little as I could with my patch, behavior-wise.
Comment 3 Charles Farmer 2018-01-16 02:27:33 UTC
I made a mistake describing what happens at point 8.2 of the test plan: the item _will_ surely be marked as lost, make no mistake about that. If you filled information inside additem.pl's form, it will definitely have an impact on the item you were editing (i.e. you _should_ see the item marked as lost in your account and on the item's page).

What's missing from BZ12363 is the _LostItem_ logic that should happen right after an item is newly marked as lost.

My apologies for the misunderstanding.
Comment 4 jmbroust 2018-03-13 11:14:33 UTC
Signed off on sandbox 8. I had an error message from the sandbox when I signed off : 
"It seems you don't have applied a patch, so you cannot sign it off. If you applied patches from the right report, check the commit message of the last patch. It should start with "Bug XXXXX", if not, please inform the author of the patch."

I tried to install again and still have a positive message result for the install : Sandbox setup by jean-manuel.broust@univ-lyon2.fr with database -1 and bug 19974 on Tue Mar 13 12:06:26 2018

=== translation installation ===
No error

So I signed it off directly in bugzilla.
Comment 5 Marcel de Rooy 2018-03-23 08:40:18 UTC
QA: Looking here
Comment 6 Marcel de Rooy 2018-03-23 08:56:56 UTC
Created attachment 73162 [details] [review]
Bug 19974: Marking an item as 'lost' in additem.pl will not modify the current item

TEST PLAN:

    1) Log in with your superlibrarian account
    2) Borrow any book
    3) Visit your Checkouts page, and click 'Show Checkouts'
    4) Click on the item's barcode to visit the item's page
    5) On the item's page, click the 'Edit' button, and choose 'Edit items'
    6) In the items table, click the 'Actions->Edit' button of the item you borrowed
    7) Mark that item as lost (it should be the first row of the form) and click the button 'Save changes'
    8) Visit your Checkouts page. The item should still be there, despite BZ12363 claiming it should've been automagically returned
        8.1) Your koha-log should also output a warning message: 'DBIx::Class::Storage::DBI::select_single(): Query returned more than one row...'
        8.2) If you visit the item's page, the modification had no effect. It should not be marked as lost.
    9) APPLY PATCH
    10) Start back from step 2), but this time, after marking the item as lost, the item's page should
        reflect the change, and the item you borrowed should've been automatically returned to the library

Signed-off-by: Jean-Manuel Broust <jean-manuel.broust@univ-lyon2.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Using $item->{itemnumber} instead of new variable.
Comment 7 Marcel de Rooy 2018-03-23 08:57:00 UTC
Created attachment 73163 [details] [review]
Bug 19974: (QA follow-up) Move lost logic to else branch

If an error is raised for the barcode, we should not try to perform the
lost logic succeeding it.
Futhermore there is no need to go to GetMarcFromKohaField etc. if we just
use the output of ModItemFromMarc.

Note: It seems unnecessary to clear $itemnumber, but I can understand
the anxiety about passing it to the template with op=additem. So just
leaving it here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2018-03-23 08:58:47 UTC
Note: The title of this report is misleading
Comment 9 Jonathan Druart 2018-03-26 18:04:21 UTC
(In reply to Marcel de Rooy from comment #8)
> Note: The title of this report is misleading

Please adjust before push.

It seems it is a quite big change in workflow, should we highlight it in release notes if backported?
Comment 10 Katrin Fischer 2018-03-27 06:15:56 UTC
If I understand it correctly MarkLostItemsAsReturned should determine if the item is checked in on return or not, independent now of where you set the item lost status, correct? (bug 12363)

What's the difference this patch introduces? (sorry, ran out of time to check)
Comment 11 Jonathan Druart 2018-03-27 17:17:02 UTC
(In reply to Katrin Fischer from comment #10)
> If I understand it correctly MarkLostItemsAsReturned should determine if the
> item is checked in on return or not, independent now of where you set the
> item lost status, correct? (bug 12363)
> 
> What's the difference this patch introduces? (sorry, ran out of time to
> check)

When an item is marked as lost when cataloguing (additem.pl), LostItem will be called and the item checked in if MarkLostItemsAsReturned is set.
This is the expected behavior (I think): what the code wanted to do (but failed) and it is what is done in other areas.

My concern is that we are going to fix a long standing bug and may break librarian's workflows (?)
Comment 12 Katrin Fischer 2018-03-30 14:02:10 UTC
It's a possibility that this might upset some libraries as there is a lot of documentation stating the difference between item form and item tab... which doesn't mean it was a good idea to have that difference to start with.

It might be good to clean up behaviour.

The only other thing I can think of is having a multi-select pref, that allows you to pick behaviour for item edit, item tab and cron.

Adding Nick and Kyle to get some additional perspective. I know Bywater has published things about lost behaviour on their blog in the past.
Comment 13 Nick Clemens 2018-03-30 14:19:58 UTC
I am torn here, I like that we fix this code, and it does make sense, however, we have firmly established the current behvaiour as a valid workflow and hsould not break it.

The longoverdue cronjob allows for the options of marking items as lost and not returning them - if we fix this here users no longer have an option for marking an itemlost without returning it.

I would almost lean towards removing the LostItem call rather than fixing it, otherwise I think we need some sort of preference or confirmation or option to charge/eturn the item if they mark it lost via additem.pl

Kyle?
Comment 14 Katrin Fischer 2018-03-30 14:28:51 UTC
Just another note, we already got a system preference now that says:

MarkLostItemsAsReturned 	
Mark/don'mark items as returned when flagged as lost

So this is highly misleading - whatever we do, we should make this more explicit.
Comment 15 Jonathan Druart 2018-03-30 15:32:15 UTC
So let's push it to 18.05 (no backport!) and highlight the change in the release notes?
Comment 16 Katrin Fischer 2018-03-30 15:43:16 UTC
I think it would work for us.
Comment 17 Nick Clemens 2018-03-30 15:50:58 UTC
I don't like it.

Currently the users have two options (if the pref is set to Mark returned) one page returns and charges one page doesn't

After this they have only one option, and no way to mark returned sometimes and not others, I think some libraries have different statuses that they want to mark in different ways.

I would like another opinion here.
Comment 18 Katrin Fischer 2018-03-30 15:55:04 UTC
I can see what you mean - we only have a few customers and I know they are probably not using the difference. Harder with so many libraries.

What about changing the pref to be multi-select and choose?

I'd also like a option or checkbox one could tick on doing the action, but that's probably hard to implement on additem.
Comment 19 Jonathan Druart 2018-03-30 15:57:57 UTC
(In reply to Nick Clemens from comment #17)
> I don't like it.
> 
> Currently the users have two options (if the pref is set to Mark returned)
> one page returns and charges one page doesn't

Where are these behavior documented? When you read the code it is obvious that it is a long standing bug, i.e. the original author wanted the item to be checked in.
Comment 20 Nick Clemens 2018-03-30 16:04:47 UTC
(In reply to Jonathan Druart from comment #19)

https://bywatersolutions.com/2015/02/23/lost-koha/

I agree that was the original intention, but at a certain point I think bug workflow became standardized i.e. we didn't know it was a bug, promoted as a feature and we will break more workflows than we would fix
Comment 21 Gaetan Boisson 2018-03-30 16:09:29 UTC
I think the behavior should really be the one selected in the system preference. Having two different behaviors in the two different tabs on the interface looks like a horrible workaround to me ;)

I don't think we work with libraries who use (and expect this). In fact i'm pretty convinced they look at the system preference setting as what should happen.

In fact I would happily get rid of the 'items' tab which i find redundant and confusing. But that's not the topic here.

Anyway, i am for following the system preference. So i guess Katrin's suggestion of having one extra choice for the system preference is fine with me. Since i really dislike this items tab though, what i would suggest is something similar to what we have for notforloan, where negative values have a different meaning that positive ones.

Maybe we could have positive values of lost triggering the return, and negative ones not triggering it, so librarians would be able to choose, and we could have consistent behavior in the different tabs.
Comment 22 Nick Clemens 2018-03-30 16:17:17 UTC
(In reply to Gaetan Boisson from comment #21)
> I don't think we work with libraries who use (and expect this). 
We definitely do, and have a lot of them

> Since i really dislike this items tab though, what i would suggest is
> something similar to what we have for notforloan, where negative values have
> a different meaning that positive ones.
> 
> Maybe we could have positive values of lost triggering the return, and
> negative ones not triggering it, so librarians would be able to choose, and
> we could have consistent behavior in the different tabs.

Looking at the code it seems this page already does that - it will call LostItem for positive values and not for negative.

If we document that and make a corresponding change to moredetails.pl/updateitem.pl I think that works
Comment 23 Katrin Fischer 2018-03-30 16:40:43 UTC
I think the positive/negative is even less documented and dangerous. I like the idea, but how to explain that to libraries? And how to explain change in behaviour for lost values they already use?
Comment 24 Jessie Zairo 2018-03-30 17:01:07 UTC
I think we can explain this similarly to how the positive and negative values work for the not for loan status. We have a lot of users that will be impacted by this and I agree, we will need to document this new behavior so they understand the change.
Comment 25 Katrin Fischer 2018-03-31 09:54:59 UTC
If we moved the logic to the authorised values, we should remove the system preference and we'd also need to change the longoverdues cronjob to take the different values into account.
At the moment the sample values only include positive values, should they be changed?

Maybe another idea: instead of relying on positive and negative values, change the preference to take the lost values (numbers) that should be checked in on marking lost. We could set those values accordingly to the current syspref setting.
Comment 26 Jonathan Druart 2018-04-02 17:12:11 UTC
Created attachment 73531 [details] [review]
Bug 19974: Make MarkLostItemsAsReturned multiple

Given the confusion regarding this behaviour it sounds better to make it
configurable.
This pref will take 4 different values, 1 per place an item can be
marked as lost.

Test plan:
Mark items as lost and confirm the item is returned or not, depending on
the value of the system preference.

- from the longoverdue cronjob (--mark-returned takes precedence if set)
- from the batch item modification tool
- when cataloguing an item
- from the items tab of the catalog module
Comment 27 Jonathan Druart 2018-04-02 17:14:10 UTC
I have attached a patch to test, it makes the pref multiple to bring flexibility.

We need to resolve this for 18.05
Comment 28 Gaetan Boisson 2018-04-05 14:46:51 UTC
I must say i don't really like the idea of making the syspref more flexible here.

I don't think we should keep an option to make the interface not consistent and have it behave one way in a tab and another way in another tab. This is in my opinion much worse than the poorly documented feature that negative values having a different effect is.

I know i'm just blabbering since i won't be submitting a patch, but i'm all for removing the system preference, having different behaviors for negative and positive values, documenting this properly and making the cron consistent with this.

I've added a bug (blocked by this one) to suggest removing the tab, but this issue needs to be fixed first anyway of course.
Comment 29 Jonathan Druart 2018-04-05 14:58:18 UTC
(In reply to Gaetan Boisson from comment #28)
> I must say i don't really like the idea of making the syspref more flexible
> here.
> 
> I don't think we should keep an option to make the interface not consistent
> and have it behave one way in a tab and another way in another tab. This is
> in my opinion much worse than the poorly documented feature that negative
> values having a different effect is.
> 
> I know i'm just blabbering since i won't be submitting a patch, but i'm all
> for removing the system preference, having different behaviors for negative
> and positive values, documenting this properly and making the cron
> consistent with this.
> 
> I've added a bug (blocked by this one) to suggest removing the tab, but this
> issue needs to be fixed first anyway of course.

I think the flexible pref is the best way to fix the issue without breaking anybody's workflow.
We want the behaviour consistent for those who wants and let time to adapt the workflow for others. We should not scope screw here and fix the inconsistency in 18.05.
We could add a warning in the pref description (native English speaker around?)
Comment 30 Katrin Fischer 2018-04-12 10:22:16 UTC
Looking here:

1) LostItem has no POD whatsoever, could we fix that?
2) Is this line correct? 
+LostItem($itemnumber, 'updateitem') if $itemlost;
updateitem is not listed in the preference.
Comment 31 Jonathan Druart 2018-04-12 14:17:21 UTC
Created attachment 74106 [details] [review]
Bug 19974: Make MarkLostItemsAsReturned multiple

Given the confusion regarding this behaviour it sounds better to make it
configurable.
This pref will take 4 different values, 1 per place an item can be
marked as lost.

Test plan:
Mark items as lost and confirm the item is returned or not, depending on
the value of the system preference.

- from the longoverdue cronjob (--mark-returned takes precedence if set)
- from the batch item modification tool
- when cataloguing an item
- from the items tab of the catalog module
Comment 32 Jonathan Druart 2018-04-12 14:18:29 UTC
(In reply to Katrin Fischer from comment #30)
> Looking here:
> 
> 1) LostItem has no POD whatsoever, could we fix that?

On a separate bug report please, we are outside the scope of the bug fix since a long time already...

> 2) Is this line correct? 
> +LostItem($itemnumber, 'updateitem') if $itemlost;
> updateitem is not listed in the preference.

Indeed, it is "moredetail", fixed.
Comment 33 Marcel de Rooy 2018-04-13 08:25:21 UTC
Looking here now
Comment 34 Marcel de Rooy 2018-04-13 08:41:17 UTC
+update systempreferences set type="multiple", options="batchmod|moredetail|cronjob|additem", value="batchmod|moredetail|cronjob|additem" where variable="MarkLostItemsAsReturned";

The atomic update is not correct. If I had set the pref to 0, the value should be empty string?
Moving status to FQA now, but still working on it. Please wait..
Comment 35 Marcel de Rooy 2018-04-13 09:04:42 UTC
Somehow I have the feeling that this pref is a bit overkill now. Who wants to mark an item as lost with catalogue but not via cataloguing, etc.? I do not think that we should offer such options.
And as to the discussion, we are really fixing a bug here. If libraries built a workflow on a bug, they should adjust. Is that really so hard in this case? Should they evaluate the other pref values again?
What we are doing in LostItem, is already flexible and controlled by prefs WhenLostForgiveFine and WhenLostChargeReplacementFee (and of course MarkLostItemsAsReturned).
I would recommend: print an additional warning at upgrade time, and put some extra text about this change in the release notes.
Comment 36 Katrin Fischer 2018-04-13 09:16:47 UTC
The problem is that this behaviour was 'advertised' prominently for quite a long time and there is no way to handle different cases then. 

The pref makes it so or so - but you don't get to choose when an item is returned or not. I think that is the need that the 'bug' might have filled. I think ultimately making it a choice (additional checkbox?) to say 'if checked out, return it' would be the ultimate best solution. Still: that doesn't fit our current item editor well (or I lack imagination)

As we are not using this a lot, I think I am ok with whatever decision we come to. But we need to decide now.
Comment 37 Marcel de Rooy 2018-04-13 09:28:44 UTC
(In reply to Katrin Fischer from comment #36)
> The problem is that this behaviour was 'advertised' prominently for quite a
> long time and there is no way to handle different cases then. 
> 
> The pref makes it so or so - but you don't get to choose when an item is
> returned or not. I think that is the need that the 'bug' might have filled.
> I think ultimately making it a choice (additional checkbox?) to say 'if
> checked out, return it' would be the ultimate best solution. Still: that
> doesn't fit our current item editor well (or I lack imagination)
> 
> As we are not using this a lot, I think I am ok with whatever decision we
> come to. But we need to decide now.

So we need a Maybe value in between with some manual interaction? But what should batchmod do then?
The simplest approach could be: if you want to return conditionally, put the pref to No and checkin if you need to.
We probably need more information about why they sometimes want to return and sometimes not. Without that info we are just guessing..
Comment 38 Katrin Fischer 2018-04-13 10:15:13 UTC
Hm, batchmod could also have a checkbox? Default 'don't check in'. I am not sure if checking it in with lost does a different thing than marking lost and checking in. Maybe some consequence on refund etc?
Comment 39 Jonathan Druart 2018-04-13 13:26:17 UTC
Marcel,
We talked about that earlier. The pref was the best compromise we found to not break somebody's workflow. See comment 29.
Comment 40 Jonathan Druart 2018-04-13 13:44:48 UTC
Created attachment 74167 [details] [review]
Bug 19974: Make MarkLostItemsAsReturned multiple

Given the confusion regarding this behaviour it sounds better to make it
configurable.
This pref will take 4 different values, 1 per place an item can be
marked as lost.

Test plan:
Mark items as lost and confirm the item is returned or not, depending on
the value of the system preference.

- from the longoverdue cronjob (--mark-returned takes precedence if set)
- from the batch item modification tool
- when cataloguing an item
- from the items tab of the catalog module
Comment 41 Jonathan Druart 2018-04-13 13:45:37 UTC
The last patch fixes the atomic update (move to .perl and check the existing value of the pref).

I let you adjust the status.
Comment 42 Marcel de Rooy 2018-04-13 14:45:56 UTC
(In reply to Jonathan Druart from comment #41)
> The last patch fixes the atomic update (move to .perl and check the existing
> value of the pref).
> 
> I let you adjust the status.

Back to NSO for now.
Any QAers around for additional feedback ?
Comment 43 Nicolas Legrand 2018-04-16 13:28:41 UTC
It seems to me the atomic dev fails because of the quote operator with "|" delimiters while having "|" in the delimited strings. Works better with other delimiters:

diff --git a/installer/data/mysql/atomicupdate/bug_19974.perl b/installer/data/mysql/atomicupdate/bug_19974.perl
index 15f39e9..deea5de 100644
--- a/installer/data/mysql/atomicupdate/bug_19974.perl
+++ b/installer/data/mysql/atomicupdate/bug_19974.perl
@@ -4,21 +4,21 @@ if( CheckVersion( $DBversion ) ) {
         SELECT value FROM systempreferences WHERE variable="MarkLostItemsAsReturned"
     |);
     if ( $original_value and $original_value eq '1' ) {
-        $dbh->do(q|
+        $dbh->do(q/
             UPDATE systempreferences
             SET type="multiple",
                 options="batchmod|moredetail|cronjob|additem",
                 value="batchmod|moredetail|cronjob|additem"
             WHERE variable="MarkLostItemsAsReturned"
-        |);
+        /);
     } else {
-        $dbh->do(q|
+        $dbh->do(q/
             UPDATE systempreferences
             SET type="multiple",
                 options="batchmod|moredetail|cronjob|additem",
                 value=""
             WHERE variable="MarkLostItemsAsReturned"
-        |);
+        /);
     }
 
     SetVersion( $DBversion );
Comment 44 Jonathan Druart 2018-04-16 13:56:46 UTC
Created attachment 74238 [details] [review]
Bug 19974: Make MarkLostItemsAsReturned multiple

Given the confusion regarding this behaviour it sounds better to make it
configurable.
This pref will take 4 different values, 1 per place an item can be
marked as lost.

Test plan:
Mark items as lost and confirm the item is returned or not, depending on
the value of the system preference.

- from the longoverdue cronjob (--mark-returned takes precedence if set)
- from the batch item modification tool
- when cataloguing an item
- from the items tab of the catalog module
Comment 45 Jonathan Druart 2018-04-16 13:57:38 UTC
(In reply to Nicolas Legrand from comment #43)
> It seems to me the atomic dev fails because of the quote operator with "|"
> delimiters while having "|" in the delimited strings. Works better with
> other delimiters:

Indeed, thanks for catching that. I tried the queries directly in the MariaDB CLI.
Fixed!
Comment 46 Séverine Queune 2018-04-19 09:09:56 UTC
About the use of 'MarkLostItemsAsReturned', I first understood 'checked=return' and 'unchecked=don't return', but it's the opposite.
Given that, all behaviours are correct except for longoverdue.pl : with 'MarkLostItemsAsReturned' checked or not, item are not returned. But I'm not used to this script, so maybe I messed somewhere when I used it (I just used ./misc/cronjobs/longoverdue.pl --confirm).
Probably someone else should try.
I update the ticket on 'Failed' to get some attention :)
Comment 47 Jonathan Druart 2018-04-19 11:53:17 UTC
Created attachment 74542 [details] [review]
Bug 19974: Make MarkLostItemsAsReturned multiple

Given the confusion regarding this behaviour it sounds better to make it
configurable.
This pref will take 4 different values, 1 per place an item can be
marked as lost.

Test plan:
Mark items as lost and confirm the item is returned or not, depending on
the value of the system preference.

- from the longoverdue cronjob (--mark-returned takes precedence if set)
- from the batch item modification tool
- when cataloguing an item
- from the items tab of the catalog module
Comment 48 Jonathan Druart 2018-04-19 11:53:34 UTC
(In reply to Séverine Queune from comment #46)
> About the use of 'MarkLostItemsAsReturned', I first understood
> 'checked=return' and 'unchecked=don't return', but it's the opposite.

That was not the expected behavior, there was a bug. It should be fixed now.

> Given that, all behaviours are correct except for longoverdue.pl : with
> 'MarkLostItemsAsReturned' checked or not, item are not returned. But I'm not
> used to this script, so maybe I messed somewhere when I used it (I just used
> ./misc/cronjobs/longoverdue.pl --confirm).
> Probably someone else should try.
> I update the ticket on 'Failed' to get some attention :)

IIRC you need --charge too
Comment 49 Séverine Queune 2018-04-19 14:28:22 UTC
Created attachment 74564 [details] [review]
Bug 19974: Marking an item as 'lost' in additem.pl will not modify the current item

TEST PLAN:

    1) Log in with your superlibrarian account
    2) Borrow any book
    3) Visit your Checkouts page, and click 'Show Checkouts'
    4) Click on the item's barcode to visit the item's page
    5) On the item's page, click the 'Edit' button, and choose 'Edit items'
    6) In the items table, click the 'Actions->Edit' button of the item you borrowed
    7) Mark that item as lost (it should be the first row of the form) and click the button 'Save changes'
    8) Visit your Checkouts page. The item should still be there, despite BZ12363 claiming it should've been automagically returned
        8.1) Your koha-log should also output a warning message: 'DBIx::Class::Storage::DBI::select_single(): Query returned more than one row...'
        8.2) If you visit the item's page, the modification had no effect. It should not be marked as lost.
    9) APPLY PATCH
    10) Start back from step 2), but this time, after marking the item as lost, the item's page should
        reflect the change, and the item you borrowed should've been automatically returned to the library

Signed-off-by: Jean-Manuel Broust <jean-manuel.broust@univ-lyon2.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Using $item->{itemnumber} instead of new variable.
Comment 50 Séverine Queune 2018-04-19 14:28:26 UTC
Created attachment 74565 [details] [review]
Bug 19974: (QA follow-up) Move lost logic to else branch

If an error is raised for the barcode, we should not try to perform the
lost logic succeeding it.
Futhermore there is no need to go to GetMarcFromKohaField etc. if we just
use the output of ModItemFromMarc.

Note: It seems unnecessary to clear $itemnumber, but I can understand
the anxiety about passing it to the template with op=additem. So just
leaving it here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Séverine Queune 2018-04-19 14:28:30 UTC
Created attachment 74566 [details] [review]
Bug 19974: Make MarkLostItemsAsReturned multiple

Given the confusion regarding this behaviour it sounds better to make it
configurable.
This pref will take 4 different values, 1 per place an item can be
marked as lost.

Test plan:
Mark items as lost and confirm the item is returned or not, depending on
the value of the system preference.

- from the longoverdue cronjob (--mark-returned takes precedence if set)
- from the batch item modification tool
- when cataloguing an item
- from the items tab of the catalog module

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 52 Séverine Queune 2018-04-19 14:31:35 UTC
Now the use of checked/unchecked box for 'MarkLostItemsAsReturned' is more logical.
It works for all cases, even cronjob after adding '--confirm --marked-return'.
Thanks Jonathan !
Comment 53 Marcel de Rooy 2018-04-20 05:37:25 UTC
Resuming QA here
Comment 54 Marcel de Rooy 2018-04-20 05:48:11 UTC
(In reply to Jonathan Druart from comment #39)
> Marcel,
> We talked about that earlier. The pref was the best compromise we found to
> not break somebody's workflow. See comment 29.

I will QA what is here now, but I still do not see why we should avoid (per se) breaking a workflow based on a bug.
Comment 55 Marcel de Rooy 2018-04-20 06:28:47 UTC
Created attachment 74591 [details] [review]
Bug 19974: Marking an item as 'lost' in additem.pl will not modify the current item

TEST PLAN:

    1) Log in with your superlibrarian account
    2) Borrow any book
    3) Visit your Checkouts page, and click 'Show Checkouts'
    4) Click on the item's barcode to visit the item's page
    5) On the item's page, click the 'Edit' button, and choose 'Edit items'
    6) In the items table, click the 'Actions->Edit' button of the item you borrowed
    7) Mark that item as lost (it should be the first row of the form) and click the button 'Save changes'
    8) Visit your Checkouts page. The item should still be there, despite BZ12363 claiming it should've been automagically returned
        8.1) Your koha-log should also output a warning message: 'DBIx::Class::Storage::DBI::select_single(): Query returned more than one row...'
        8.2) If you visit the item's page, the modification had no effect. It should not be marked as lost.
    9) APPLY PATCH
    10) Start back from step 2), but this time, after marking the item as lost, the item's page should
        reflect the change, and the item you borrowed should've been automatically returned to the library

Signed-off-by: Jean-Manuel Broust <jean-manuel.broust@univ-lyon2.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Using $item->{itemnumber} instead of new variable.
Comment 56 Marcel de Rooy 2018-04-20 06:28:52 UTC
Created attachment 74594 [details] [review]
Bug 19974: (QA follow-up) Move lost logic to else branch

If an error is raised for the barcode, we should not try to perform the
lost logic succeeding it.
Futhermore there is no need to go to GetMarcFromKohaField etc. if we just
use the output of ModItemFromMarc.

Note: It seems unnecessary to clear $itemnumber, but I can understand
the anxiety about passing it to the template with op=additem. So just
leaving it here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Marcel de Rooy 2018-04-20 06:28:57 UTC
Created attachment 74595 [details] [review]
Bug 19974: Make MarkLostItemsAsReturned multiple

Given the confusion regarding this behaviour it sounds better to make it
configurable.
This pref will take 4 different values, 1 per place an item can be
marked as lost.

Test plan:
Mark items as lost and confirm the item is returned or not, depending on
the value of the system preference.

- from the longoverdue cronjob (--mark-returned takes precedence if set)
- from the batch item modification tool
- when cataloguing an item
- from the items tab of the catalog module

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Marcel de Rooy 2018-04-20 06:29:02 UTC
Created attachment 74596 [details] [review]
Bug 19974: (QA follow-up) Do not use bar separator in atomic update

The multiple select saves the selected values with a comma.
Furthermore, preferences.pl also splits on a comma. In other words, the
upgrade will not work with the bar character. All options will be
deselected.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Jonathan Druart 2018-04-20 13:31:42 UTC
(In reply to Marcel de Rooy from comment #54)
> (In reply to Jonathan Druart from comment #39)
> > Marcel,
> > We talked about that earlier. The pref was the best compromise we found to
> > not break somebody's workflow. See comment 29.
> 
> I will QA what is here now, but I still do not see why we should avoid (per
> se) breaking a workflow based on a bug.

Long standing bugs become features ;)
Comment 60 Jonathan Druart 2018-04-20 15:27:30 UTC
We will need release notes to explain what we did and why.
Comment 61 Jonathan Druart 2018-04-20 15:49:34 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 62 Séverine Queune 2018-04-21 07:47:43 UTC
This is one of my first release-notes : I won't be offended if you think something/anything/everything must de rewrite !

To me, it seems the feature of the patch is quite different for comment#1.
Maybe the name of the ticket should be modified ?
Comment 63 Jonathan Druart 2018-10-31 13:40:48 UTC
*** Bug 8484 has been marked as a duplicate of this bug. ***