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

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.

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.

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.

QA: Looking here

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.

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>

Note: The title of this report is misleading

(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?

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)

(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 (?)

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.

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?

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.

So let's push it to 18.05 (no backport!) and highlight the change in the release notes?

I think it would work for us.

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.

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.

(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.

(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

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.

(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

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?

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.

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.

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

I have attached a patch to test, it makes the pref multiple to bring flexibility.

We need to resolve this for 18.05

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.

(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?)

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.

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

(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.

Looking here now

+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..

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.

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.

(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..

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?

Marcel,
We talked about that earlier. The pref was the best compromise we found to not break somebody's workflow. See comment 29.

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

The last patch fixes the atomic update (move to .perl and check the existing value of the pref).

I let you adjust the status.

(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 ?

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 );

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

(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!

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 :)

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

(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

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.

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>

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>

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 !

Resuming QA here

(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.

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.

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>

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>

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>

(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 ;)

We will need release notes to explain what we did and why.

Pushed to master for 18.05, thanks to everybody involved!

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 ?

*** Bug 8484 has been marked as a duplicate of this bug. ***

FWIW - the atomicupdate here is not idempotent, possibly too late, but noting in case anyone is hit by this