Bug 17519 - Item editor handles clearing dateaccessioned and replacementpricedate inconsistently
Summary: Item editor handles clearing dateaccessioned and replacementpricedate inconsi...
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-31 09:46 UTC by Marcel de Rooy
Modified: 2017-03-18 08:42 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17519: Item editor handles clearing dateaccessioned and replacementpricedate inconsistently (1.71 KB, patch)
2016-10-31 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17519: Item editor handles clearing dateaccessioned and replacementpricedate inconsistently (1.85 KB, patch)
2017-03-12 10:32 UTC, Marc Véron
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2016-10-31 09:46:41 UTC

    
Comment 1 Marcel de Rooy 2016-10-31 09:52:52 UTC
Created attachment 56984 [details] [review]
Bug 17519: Item editor handles clearing dateaccessioned and replacementpricedate inconsistently

This patch adds dateaccessioned to the hash built by Items routine
_build_default_values_for_mod_marc, used in ModItemFromMarc when modifying
an item.

Normally, dateaccessioned will already have a value, and this change has no
effect. The only exception is: when you modify an item and for some reason
want to clear the dateaccessioned field, the item editor did not respond
to that change. This change makes it possible to do so. This is consistent
with the other date in the item editor (replacementpricedate) where you can
already clear the date.

Test plan:
[1] Do not yet apply this patch.
[2] Try to clear date acquired and replacementpricedate in the Item editor.
    Save and check results.
    You should still find the original date acquired in the saved record.
[3] Apply the patch (restart Plack?), restore replacementpricedate, and
    repeat step 2. Both dates should be cleared.
Comment 2 Marc Véron 2017-03-12 10:32:36 UTC
Created attachment 61013 [details] [review]
Bug 17519: Item editor handles clearing dateaccessioned and replacementpricedate inconsistently

This patch adds dateaccessioned to the hash built by Items routine
_build_default_values_for_mod_marc, used in ModItemFromMarc when modifying
an item.

Normally, dateaccessioned will already have a value, and this change has no
effect. The only exception is: when you modify an item and for some reason
want to clear the dateaccessioned field, the item editor did not respond
to that change. This change makes it possible to do so. This is consistent
with the other date in the item editor (replacementpricedate) where you can
already clear the date.

Test plan:
[1] Do not yet apply this patch.
[2] Try to clear date acquired and replacementpricedate in the Item editor.
    Save and check results.
    You should still find the original date acquired in the saved record.
[3] Apply the patch (restart Plack?), restore replacementpricedate, and
    repeat step 2. Both dates should be cleared.

Followed test plan, date acquired can be deleted as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 3 Marcel de Rooy 2017-03-13 07:40:31 UTC
(In reply to Marc Véron from comment #2)
> Signed-off-by: Marc Véron <veron@veron.ch>

Thanks!
Comment 4 Jonathan Druart 2017-03-13 14:38:08 UTC
It seems that replacement price is ok before this patch, and the patch does not affect it. Should we rename the bug report and the commit msg?
Comment 5 Marcel de Rooy 2017-03-14 12:48:24 UTC
(In reply to Jonathan Druart from comment #4)
> It seems that replacement price is ok before this patch, and the patch does
> not affect it. Should we rename the bug report and the commit msg?

No problem. What would you suggest?
The inconsistency is in the comparison between how both fields behave. So maybe: Handle clearing dateaccessioned just like replacementpricedate?
Or just Fix clearing dateaccessioned?
Comment 6 Jonathan Druart 2017-03-14 13:26:14 UTC
Marcel, sorry but I do not understand your answer.
I am just saying that replacementpricedate is working correctly without your patch, so we should remove it from the bug title and commit message.
Do you agree?
Comment 7 Marcel de Rooy 2017-03-14 13:55:36 UTC
(In reply to Jonathan Druart from comment #6)
> Marcel, sorry but I do not understand your answer.
> I am just saying that replacementpricedate is working correctly without your
> patch, so we should remove it from the bug title and commit message.
> Do you agree?

Yes, no problem.
Comment 8 Katrin Fischer 2017-03-14 15:29:46 UTC
Will this effect the automatic setting of dateaccessioned on leaving it empty when saving? We rely on this behaviour at the moment.
Comment 9 Marcel de Rooy 2017-03-14 15:37:14 UTC
(In reply to Katrin Fischer from comment #8)
> Will this effect the automatic setting of dateaccessioned on leaving it
> empty when saving? We rely on this behaviour at the moment.

Please explain what you mean. Where do you rely on exactly?
Comment 10 Katrin Fischer 2017-03-14 15:48:07 UTC
We have trained libraries that they don't have to fill out the field because Koha does it in the background automatically. I was wondering, because this patch talks about clearing the field. At the moment it's simply not possible to have an item without a dateaccessioned which is often used as creation data in various reports.
Comment 11 Marcel de Rooy 2017-03-14 16:01:28 UTC
(In reply to Katrin Fischer from comment #10)
> We have trained libraries that they don't have to fill out the field because
> Koha does it in the background automatically. I was wondering, because this
> patch talks about clearing the field. At the moment it's simply not possible
> to have an item without a dateaccessioned which is often used as creation
> data in various reports.

No, this does not affect adding an item with an empty date. The sub _set_defaults_for_add will default dateaccessioned to today.
The change is in ModItemFromMarc. If the user cleared the date when modifying an item, this change was not picked up. Now it is possible just as it already was for replacementpricedate.

Note that we are still not 100% consistent imo. Only a little bit more ;)
If we want a default in dateaccessioned, we should better take care of that in the interface rather than doing it invisibly in a set_defaults sub. Why should a user understand that we fill dateaccessioned but we do not fill replacementpricedate? But that is a topic for another report..
Comment 12 Katrin Fischer 2017-03-14 16:08:04 UTC
Ok, so when adding an item the behaviour will be the same. 
But it will be possible to empty out the date on changing an item, correct?

I am not sure how replacementpricedate was intended to work. I have heard somewhere that it was used to decrease the replacement price over time as the item aged, but not entirely sure. I am not sure we have any functionality tied to it at the moment in Koha itself.
Comment 13 Marcel de Rooy 2017-03-15 12:02:44 UTC
(In reply to Katrin Fischer from comment #12)
> Ok, so when adding an item the behaviour will be the same. 
> But it will be possible to empty out the date on changing an item, correct?

Correct.
Comment 14 Marcel de Rooy 2017-03-17 12:18:29 UTC
The discussion seems to be finished ;)
Comment 15 Katrin Fischer 2017-03-17 13:42:54 UTC
Actually no - I just didn't have the time to write another answer.

I think this patch makes the behaviour inconsistent. If you can empty out on editing, but not on adding the item -  that will look like a bug. 

The behaviour right now is consistent as the field is forced to be mandatory and as I said, we rely on this in some contexts - for example in context of inventory books.

Maybe we should discuss use cases first?

Things that come to my mind:
- new books list
- inventory book (electronical, sometimes printed out)
- sorting by accession date in the OPAC
- weeding reports/workflows
Comment 16 Marcel de Rooy 2017-03-17 13:47:07 UTC
(In reply to Katrin Fischer from comment #15)
> Actually no - I just didn't have the time to write another answer.
> 
> I think this patch makes the behaviour inconsistent. If you can empty out on
> editing, but not on adding the item -  that will look like a bug. 
> 
> The behaviour right now is consistent as the field is forced to be mandatory
> and as I said, we rely on this in some contexts - for example in context of
> inventory books.
> 
> Maybe we should discuss use cases first?
> 
> Things that come to my mind:
> - new books list
> - inventory book (electronical, sometimes printed out)
> - sorting by accession date in the OPAC
> - weeding reports/workflows

OK. The current behavior is not really consistent (if you compare with the other date). But in view of its low importance, I would rather close this report now if it disturbs other users too much.. No problem!
Comment 17 Katrin Fischer 2017-03-18 08:42:07 UTC
Sorry Marcel, didn't want to upset you - it just felt not 'quite right' yet to me. Maybe having a separate 'created_on' column on items would be a good idea long term.