Bug 29963 - Date accessioned plugin should not automatically fill today's date on cataloguing screens
Summary: Date accessioned plugin should not automatically fill today's date on catalog...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Peter Vashchuk
QA Contact: Testopia
URL:
Keywords:
Depends on: 29815
Blocks: 30931 33931
  Show dependency treegraph
 
Reported: 2022-01-27 18:27 UTC by Benjamin Daeuber
Modified: 2023-06-07 08:02 UTC (History)
8 users (show)

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


Attachments
Bug 29963: remove set_to_today function from dateaccessioned.pl (2.36 KB, patch)
2022-06-02 11:48 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29963: remove set_to_today function from dateaccessioned.pl (1.78 KB, patch)
2022-06-02 12:08 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29963: don't use set_to_today function at dateaccessioned.pl (1.74 KB, patch)
2022-06-03 10:02 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29963: do not prefill empty date fields with today's date (2.12 KB, patch)
2022-06-03 16:15 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29963: do not prefill empty date fields with today's date (2.24 KB, patch)
2022-06-03 16:16 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 29963: Do not prefill empty date fields with today's date (2.31 KB, patch)
2022-06-07 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29963: Restore original intention of bug 29815 (1.06 KB, patch)
2022-06-07 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29963: Restore original intention of bug 29815 (1.22 KB, patch)
2022-06-07 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29963: Display a hint that the 952$d will be filled with current date if blank (3.23 KB, patch)
2022-06-07 12:05 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 29963: Do not prefill empty date fields with today's date (2.36 KB, patch)
2022-06-16 11:38 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Daeuber 2022-01-27 18:27:55 UTC
When using batch modification or other item edit screens, the date picker automatically fills todays date when the "Date acquired" field is selected and blank. This does not appear to be the case for other instances of the date picker (when creating a new patron, for example). I'm not sure if there's a design reason for this, but it's especially annoying here since the label element extends the clickable area quite a bit.
Comment 1 Andrii Nugged 2022-06-01 13:17:18 UTC Comment hidden (obsolete)
Comment 2 Andrii Nugged 2022-06-01 13:44:31 UTC
this same bad UX experience we have from our customers libraries too.
Confirming this ticket but more from POV of Batch Modificaitons of items:

In our case this was mostly inconvenient for customers in batch modification, it prefills date to today making librarians doing big mistake "pressing saving by they used to have empty form" so they broke hundreds of records.

i.e. in /cgi-bin/koha/tools/batchMod.pl
Comment 3 Andrii Nugged 2022-06-01 13:49:37 UTC
it's still questionable is it needed or not auto-filled on the "new item addition" screen. Maybe or maybe not, I'll check with our librarians too.

But seems for sure it makes noise and mistakes in batch modification screens.
Comment 4 Andrii Nugged 2022-06-01 14:04:22 UTC
but it seems impossible to easily separate because dateaccessioned.pl has this on-load code:

    \$(document).ready(function(){
        ...
        /* Set current date on page load */
        set_to_today($function_name.id);
    });


which is executed for any kind of form (both new item or edit or batch item mod).

Hm. We need to decide.
Comment 5 Andrii Nugged 2022-06-01 14:10:53 UTC
Tested, and found more inconvenience, yes: 

when you EDIT an old item that has for example empty 952w (on which this plugin assigned for our customers) and you want this field to STAY empty after editing - you always need to clean up that field manually, it really makes very BAD UX! 

Should be removed, definitely.
Comment 6 Peter Vashchuk 2022-06-02 11:48:50 UTC Comment hidden (obsolete)
Comment 7 Peter Vashchuk 2022-06-02 12:08:43 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2022-06-03 04:38:29 UTC
You are then reverting
  commit bd197c64567fba6899f12e4897d786adf52384d8
  Bug 29815: Pre-populate 'Date acquired' field when adding/editing items
Comment 9 Peter Vashchuk 2022-06-03 10:02:46 UTC Comment hidden (obsolete)
Comment 10 Benjamin Daeuber 2022-06-03 13:09:05 UTC
(In reply to Jonathan Druart from comment #8)
> You are then reverting
>   commit bd197c64567fba6899f12e4897d786adf52384d8
>   Bug 29815: Pre-populate 'Date acquired' field when adding/editing items

I can't imagine why you would want date acquired field prepopulated when you are editing items. Adding items makes sense to me, but not editing. We would need to clear the field every time we made a small change or batch modified shelf locations or really any other daily activity.
Comment 11 Peter Vashchuk 2022-06-03 16:15:45 UTC Comment hidden (obsolete)
Comment 12 Peter Vashchuk 2022-06-03 16:16:51 UTC
Created attachment 135684 [details] [review]
Bug 29963: do not prefill empty date fields with today's date

Revert "Bug 29815: Pre-populate 'Date acquired' field when adding/editing items"
commit bd197c64567fba6899f12e4897d786adf52384d8

Items date fields in MARC framework with dateaccessioned.pl assigned as
plugin automatically prefilled by it with today's date if empty on page
load.

This becomes big problem because it silently changes empty date fields
in single item edit form with today's date. This drastically degrades
UX because user won't noitce that fields were filled.

Also, even when user aware about this problem, user is forced to clean
the field manually each time they edit items with empty fields.

Also in item batch edit tool the probability heavily increases to make
mistake and leave those fields prefilled on mass.

To reproduce problem with single item edit:
1. Edit single item that doesn't have a date set in any of the date
fields where dateaccessioned.pl in MARC framework assigned as plugin,
see that the field got prefilled with today's date.
2. Save the item. See that it overwrote the previously empty date.
3. Apply the patch.
4. Edit single item again, and ensure that the empty date fields don't
get overwriten like it happened previously.

To reproduce problem with bath edit:
1. Edit items in batch for any biblio, see that the date fields where
dateaccessioned.pl in MARC framework assigned as plugin is automatically
 set to current day's date.
2. Apply the patch.
3. Check the fields again, they should be empty after that.
Comment 13 Andrii Nugged 2022-06-03 17:37:38 UTC
Benjamin Daeuber: I also support your problem - we have the same issues and very concerned customers :). So this patch (reverting) is needed.

I suspect Jonathan just said that the ticket patch should be just a reversal of the previous commit, not a standalone commit.

to Jonathan: please hint Petro on how to make a "reversal".
Comment 14 Jonathan Druart 2022-06-03 18:20:15 UTC
I was actually saying I am concerned about reverting a patch that has been considered an enhancement in the last release. We need feedback from people involved in bug 29815 to know how we can fit everybody's needs.
Comment 15 Andrii Nugged 2022-06-04 15:19:01 UTC
Obviously, we should respect other's work and efforts,
I fully agree.

------

I have all my customers-librarians reported and confirmed myself too,
that UX not improved, but degraded.

The goal of "bug 29815" was to autofill "today" just for some particular field - probably only for "new item" and only for "Date Acquired".

But unintentionally it brought other things - probably it wasn't considered that it autofill also:

- it will autofill all available item marc-fields with dates, to where plugin attached? Some never should be "today", but various.

- it will autofill empty value  in editing, when the empty value should be kept. More: it doesn't asks/warns and makes silent data modification when operator edits one item other line, and never sees that something "changed" (!).

- it works also for batch edit items (there "today" date not needed and even makes inconvenience) - and for one library for us made thousands of items mistakenly filled date, because operator get used not to "pre-clean" batch fields.


This patch in bug 29815 makes those undesirable consequences.

and "pressing three dots on the right" in field where plugin attached always fills "today" date if operator wants,

so I am to stay on request to remove this.
Comment 16 Martin Renvoize 2022-06-07 07:29:05 UTC
Created attachment 135744 [details] [review]
Bug 29963: Do not prefill empty date fields with today's date

Revert "Bug 29815: Pre-populate 'Date acquired' field when adding/editing items"
commit bd197c64567fba6899f12e4897d786adf52384d8

Items date fields in MARC framework with dateaccessioned.pl assigned as
plugin automatically prefilled by it with today's date if empty on page
load.

This becomes big problem because it silently changes empty date fields
in single item edit form with today's date. This drastically degrades
UX because user won't noitce that fields were filled.

Also, even when user aware about this problem, user is forced to clean
the field manually each time they edit items with empty fields.

Also in item batch edit tool the probability heavily increases to make
mistake and leave those fields prefilled on mass.

To reproduce problem with single item edit:
1. Edit single item that doesn't have a date set in any of the date
fields where dateaccessioned.pl in MARC framework assigned as plugin,
see that the field got prefilled with today's date.
2. Save the item. See that it overwrote the previously empty date.
3. Apply the patch.
4. Edit single item again, and ensure that the empty date fields don't
get overwriten like it happened previously.

To reproduce problem with bath edit:
1. Edit items in batch for any biblio, see that the date fields where
dateaccessioned.pl in MARC framework assigned as plugin is automatically
 set to current day's date.
2. Apply the patch.
3. Check the fields again, they should be empty after that.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-06-07 07:29:09 UTC
Created attachment 135745 [details] [review]
Bug 29963: Restore original intention of bug 29815

This patch restores the original intended behaviour of bug 29815; To
pre-populate the dateaccessioned field on item creations.
Comment 18 Martin Renvoize 2022-06-07 07:33:20 UTC
I'm a little torn here.. I've grabbed the original patch from bug 29815 that accomplished the goal in a different manner, but failed QA by Katrin as it doesn't affect all data entry locations.

I have renamed the bug, as this is directly related to the dateaccessioned plugin, not all date fields.. it's only date field where end users have explicitly set the field to use dateaccessioned.. should there be a simple datepicker plugin to go along with dateaccessioned as an alternative to these patches.. it highlights a flaw that people are re-using that plugin for non-accession fields in my opinion.
Comment 19 Martin Renvoize 2022-06-07 08:28:24 UTC
Created attachment 135746 [details] [review]
Bug 29963: Restore original intention of bug 29815

This patch restores the original intended behaviour of bug 29815; To
pre-populate the dateaccessioned field on item creations only.
Comment 20 Alex Buckley 2022-06-07 08:43:18 UTC
Martin: Thank you for attaching the original patch from bug 29815 to this bug report.

I agree with you Martin that the problem is compounded if libraries have linked multiple MARC subfields, not just the 952$d (date acquired) subfield, to the dateaccessioned.pl plugin.

---

Andrew: I am sorry that my patch on 29815 has caused these unintended problems for people!
 
You’re right the intention of 29815 was to populate the 952$d (Date acquired) field to the current date when adding a new item or receiving a new serial issue. 

---

Jonathan: You said it would be useful to have feedback from people involved in 29815 so everyone's needs were met. So here is the background context and needs that 29815 was intended to address:

We added the 29815 patch because it was thought that Koha did not previously make it clear to cataloguers that if they save the ‘Add item’ page (or receive serial issue page) without setting anything in the 952$d subfield, Koha would still populate the 952$d with today's date.

Pre-populating the 952$d on page load was intended to show that a value would be set for that subfield.

---

Going forward if there are concerns with the original patch changing the behavior of one item form making it inconsistent with others in cataloguing and acq. Then I think, instead of a behavior change, it should be sufficient to add a hint saying that if you save the 952$d with nothing in it Koha will still auto-populate the 952$d with today's date. Perhaps this is the most straight forward solution?
Comment 21 Alex Buckley 2022-06-07 12:04:33 UTC
(In reply to Alex Buckley from comment #20)

Following on from comment #20 I see on the additem page there is a hint span we could use to display to users that if they leave the subfield linked to items.dateaccessioned (which is usually the 952$d (date acquired) subfield) blank then upon saving Koha will fill it with today's date. 

See https://github.com/Koha-Community/Koha/blob/7704f51d5131d3f1104244a705294db614eae76e/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc#L201

I'm about to attach a patch that adds this hint.

I also added a span hint with the same wording to serials-edit.tt so that the same hint displays when receiving serial issues. 

The text in both spans may need to be amended if you think it's bad, so please feel free to do so, but this, in combination with the patch reversion might be the way forward?
Comment 22 Alex Buckley 2022-06-07 12:05:08 UTC
Created attachment 135755 [details] [review]
Bug 29963: Display a hint that the 952$d will be filled with current date if blank

This hint will show when:
- Adding an item
- Editing an item
- Receiving a serial issue

Test plan:
1. Apply patch

2. Add an item to a biblio record, notice there is a hint message for the
952$d (date acquired) subfield

3. Edit an item, notice there is a hint message displaying for the 952$d
subfield

4. Receive a serial (The subscription must be setup to create an item
upon receival to test this).

5. Notice there is a hint message displaying for the 952$d (Date acquired) subfield in both the main issue you are receiving and the supplemental issue form as well

Sponsored-by: Catalyst IT
Comment 23 Alex Buckley 2022-06-07 12:09:34 UTC
It's past midnight here in NZ now, but will check back in on comments on this bug report in the morning!
Comment 24 Benjamin Daeuber 2022-06-07 14:11:37 UTC
> 
> Going forward if there are concerns with the original patch changing the
> behavior of one item form making it inconsistent with others in cataloguing
> and acq. Then I think, instead of a behavior change, it should be sufficient
> to add a hint saying that if you save the 952$d with nothing in it Koha will
> still auto-populate the 952$d with today's date. Perhaps this is the most
> straight forward solution?

I'm not sure I agree with this. I don't really see an issue with being context aware. Hints get missed with potentially far reaching consequences in the case of a batch edit. Logical, context aware behavior seems more user friendly. It makes sense to populate that field in the case of serials and new items, but not in the case of item edits. I don't really find that to be an inconsistent behavior.
Comment 25 Alex Buckley 2022-06-07 21:12:33 UTC
(In reply to Benjamin Daeuber from comment #24)
> > 
> > Going forward if there are concerns with the original patch changing the
> > behavior of one item form making it inconsistent with others in cataloguing
> > and acq. Then I think, instead of a behavior change, it should be sufficient
> > to add a hint saying that if you save the 952$d with nothing in it Koha will
> > still auto-populate the 952$d with today's date. Perhaps this is the most
> > straight forward solution?
> 
> I'm not sure I agree with this. I don't really see an issue with being
> context aware. Hints get missed with potentially far reaching consequences
> in the case of a batch edit. Logical, context aware behavior seems more user
> friendly. It makes sense to populate that field in the case of serials and
> new items, but not in the case of item edits. I don't really find that to be
> an inconsistent behavior.

Thanks for sharing your thoughts on this Benjamin. I think it could be helpful to get Katrin's thoughts on the best approach because as Martin noted in comment #18 she failed QA the original 29815 patch as it doesn't affect all data entries.
Comment 26 Katrin Fischer 2022-06-07 22:10:34 UTC
Hi,

the inconsistency that we discussed was that the same field should show the same behaviour in all places where we can add items. These are:

* Adding items in serials
* Adding items in cataloguing
* Adding items in acquisitions

I believe that serials and acq use similar code, but the code for cataloguing is different.

The issue as I understand it was the 'hidden' behaviour. People assumed that leaving the field empty would mean 'no dateaccessioned' while Koha would _always_ use today's date if left empty. Or they would feel a need to fill out the field, when it was not necessary, adding extra steps.

I think reverting and adding a hint would actually be a good start.

Being context aware only pre-filling when the item is added/new would also be a good compromise. I just wonder if it might upset people that do a lot of cataloguing of items with past accession dates where it might feel like an issue having to change the pre-filled value.

Also another thought pro hint: if we pre-fill, setting date-accessioned to mandatory will have no effect/the warning will never trigger. This might be an issue for libraries that want to make sure the date is verified manually.
Comment 27 Benjamin Daeuber 2022-06-08 20:41:27 UTC
A few things:

1. Applying this patch on master and deleting the accessioned date now fills todays date, even if I have don't have that field set to mandatory. Can anyone confirm that?

2. Assuming this is the case, this makes batch modification even more fraught because batch modifying an item with a date and another item without a date will cause one to end up with todays date. There's really no winning for the end user there: either change all items to the same date, or change any item without a date to todays date.

3. I think the hint wording is confusing on batchmod.pl since it implies that leaving that field blank will set all batch modified items to todays date.

I still think leaving batchmod.pl out of this entirely is best.
Comment 28 Katrin Fischer 2022-06-09 07:05:11 UTC
I think there is 2 sides to this bug or even 2 separate bugs:

1) Handling in the item editor with the field being filled in
2) Adding the dateeaccessioned on editing even when emptied out. 

2) is actually this bug and was not caused by the pre-fill change, but bug 23463: 
Bug 29958 - Missing dateaccessioned is set to today when storing an item

I believe we might want to treat them separately to not have things even more confusing.
Comment 29 Alex Buckley 2022-06-09 07:37:02 UTC
(In reply to Katrin Fischer from comment #28)
> I think there is 2 sides to this bug or even 2 separate bugs:
> 
> 1) Handling in the item editor with the field being filled in
> 2) Adding the dateeaccessioned on editing even when emptied out. 
> 
> 2) is actually this bug and was not caused by the pre-fill change, but bug
> 23463: 
> Bug 29958 - Missing dateaccessioned is set to today when storing an item
> 
> I believe we might want to treat them separately to not have things even
> more confusing.

Thank you Katrin, I agree. 

Also, to fix issue #1 that you describe I wonder if it would be best to do the following:

1. Upstream the patch reverting 29815
2. Then create a separate bug report to discuss whether the community prefers a context-specific pre-filling of the dateaccessioned or a hint.

By proceeding with upstreaming the reversion at least we would fix the urgent bug without getting bogged down in the discussion. 

What do you think?
Comment 30 Katrin Fischer 2022-06-09 08:31:36 UTC
Sounds good to me Alex.
Comment 31 Alex Buckley 2022-06-09 09:10:19 UTC
(In reply to Katrin Fischer from comment #30)
> Sounds good to me Alex.

Thanks Katrin!
Comment 32 Alex Buckley 2022-06-09 09:28:55 UTC
I have shifted Martin's and my patches to bug 30931 to keep this bug report, hopefully, focused on reverting bug 29815 as quickly as possible.

Discussion around how the community would like te item editor to handle the dateaccessioned field is very welcome on bug 30931 :)
Comment 33 Alex Buckley 2022-06-16 10:06:55 UTC
(In reply to Alex Buckley from comment #32)
> I have shifted Martin's and my patches to bug 30931 to keep this bug report,
> hopefully, focused on reverting bug 29815 as quickly as possible.
> 
> Discussion around how the community would like the item editor to handle the
> dateaccessioned field is very welcome on bug 30931 :)

Hi Katrin, Martin and others who may know, 

As I wrote the original patch 29815 am I allowed to sign off the patch on this ticket - which is reverting 29815?

I had assumed not, but I just wanted to check :)

Alex
Comment 34 Martin Renvoize 2022-06-16 11:37:17 UTC
Sorry for the slow reply.. yes, it's a distinct, if related, patch. I'm happy to have your SO on it Alex.

In fact.. I think with the discussions here we can go ahead and call this one PQA.. lots of eyes and the debatable bits are now on other bugs, thanks Alex.

I'll add Alex's SO line above mine and call it PQA
Comment 35 Martin Renvoize 2022-06-16 11:38:44 UTC
Created attachment 136153 [details] [review]
Bug 29963: Do not prefill empty date fields with today's date

Revert "Bug 29815: Pre-populate 'Date acquired' field when adding/editing items"
commit bd197c64567fba6899f12e4897d786adf52384d8

Items date fields in MARC framework with dateaccessioned.pl assigned as
plugin automatically prefilled by it with today's date if empty on page
load.

This becomes big problem because it silently changes empty date fields
in single item edit form with today's date. This drastically degrades
UX because user won't noitce that fields were filled.

Also, even when user aware about this problem, user is forced to clean
the field manually each time they edit items with empty fields.

Also in item batch edit tool the probability heavily increases to make
mistake and leave those fields prefilled on mass.

To reproduce problem with single item edit:
1. Edit single item that doesn't have a date set in any of the date
fields where dateaccessioned.pl in MARC framework assigned as plugin,
see that the field got prefilled with today's date.
2. Save the item. See that it overwrote the previously empty date.
3. Apply the patch.
4. Edit single item again, and ensure that the empty date fields don't
get overwriten like it happened previously.

To reproduce problem with bath edit:
1. Edit items in batch for any biblio, see that the date fields where
dateaccessioned.pl in MARC framework assigned as plugin is automatically
 set to current day's date.
2. Apply the patch.
3. Check the fields again, they should be empty after that.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Alex Buckley 2022-06-16 11:51:57 UTC
(In reply to Martin Renvoize from comment #34)
> Sorry for the slow reply.. yes, it's a distinct, if related, patch. I'm
> happy to have your SO on it Alex.
> 
> In fact.. I think with the discussions here we can go ahead and call this
> one PQA.. lots of eyes and the debatable bits are now on other bugs, thanks
> Alex.
> 
> I'll add Alex's SO line above mine and call it PQA

Thanks very much Martin!
Comment 37 Tomás Cohen Arazi 2022-06-17 13:36:31 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 38 Lucas Gass 2022-07-13 20:32:39 UTC
Backported to 22.05.x for 22.05.03
Comment 39 Arthur Suzuki 2022-07-21 13:44:42 UTC
depends on 29815, won't backport