|
Description
Benjamin Daeuber
2022-01-27 18:27:55 UTC
this same bad UX experience we have from our customers libraries too. Confirming this ticket. 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 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. 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.
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. Created attachment 135581 [details] [review] Bug 29963: remove set_to_today function from dateaccessioned.pl Removed the function that wasn't used anywhere outside of dateaccessioned.pl, as it was causing inconveniences when user was trying to edit items in batch, it would constantly set Date Acquired field to today's date which created the need for user to constantly manually clear the field, so that it doesn't overwrite the original Acquired date. When you add item without specifying any date for Date Acquired it automatically sets the today's date for it so this change doesn't seem to break any logic there. And when you try to edit item with already set Date Acquired before it will show the previously defined date, not disrupting anything here as well. To reproduce: 1. Edit items in batch for any biblio, see that "d - Date acquired" field is automatically set to current day's date. 2. Apply the patch 3. "d - Date acquired" should be empty after that. 4. Additionally, go to the additem.pl to edit single item and check that it correctly shows Date acquired in that field. Created attachment 135582 [details] [review] Bug 29963: remove set_to_today function from dateaccessioned.pl Removed the function that wasn't used anywhere outside of dateaccessioned.pl, as it was causing inconveniences when user was trying to edit items in batch, it would constantly set Date Acquired field to today's date which created the need for user to constantly manually clear the field, so that it doesn't overwrite the original Acquired date. When you add item without specifying any date for Date Acquired it automatically sets the today's date for it so this change doesn't seem to break any logic there. And when you try to edit item with already set Date Acquired before it will show the previously defined date, not disrupting anything here as well. To reproduce: 1. Edit items in batch for any biblio, see that "d - Date acquired" field is automatically set to current day's date. 2. Apply the patch 3. "d - Date acquired" should be empty after that. 4. Additionally, go to the additem.pl to edit single item and check that it correctly shows Date acquired in that field. You are then reverting commit bd197c64567fba6899f12e4897d786adf52384d8 Bug 29815: Pre-populate 'Date acquired' field when adding/editing items Created attachment 135674 [details] [review] Bug 29963: don't use set_to_today function at dateaccessioned.pl Don't use set_to_today function that was causing inconveniences when user was trying to edit items in batch, it would constantly set Date Acquired field to today's date which created the need for user to constantly manually clear the field, so that it doesn't overwrite the original Acquired date. When you add item without specifying any date for Date Acquired it automatically sets the today's date for it so this change doesn't seem to break any logic there. And when you try to edit item with already set Date Acquired before it will show the previously defined date, not disrupting anything here as well. To reproduce: 1. Edit items in batch for any biblio, see that "d - Date acquired" field is automatically set to current day's date. 2. Apply the patch 3. "d - Date acquired" should be empty after that. 4. Additionally, go to the additem.pl to edit single item and check that it correctly shows Date acquired in that field. (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. Created attachment 135683 [details] [review] Bug 29963: do not prefill empty date fields with today's date 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. 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. 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". 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. 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. 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> 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. 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. 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. 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? (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? 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 It's past midnight here in NZ now, but will check back in on comments on this bug report in the morning! >
> 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.
(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. 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. 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. 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. (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? Sounds good to me Alex. (In reply to Katrin Fischer from comment #30) > Sounds good to me Alex. Thanks Katrin! 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 :) (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 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 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> (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! Pushed to master for 22.11. Nice work everyone, thanks! Backported to 22.05.x for 22.05.03 depends on 29815, won't backport |