If you open an existing record in the cataloguing editor, default framework values are applied. This may not always be the desired behavior. For instance, when you created the record and the defaults were applied, you did remove one value. When reopening the record, you dont like it to reappear? Any thoughts?
I agree. I think it should only default on creation of a new record. If I decide to remove the content of a field or the field itself, that should be respected.
Changing enhancement to bug status normal. What Koha does for years, is actually just wrong. The positive side is that many people will add the record, and not edit it any longer ;) But my cataloger tells me: I reopen the record, and remove the wrong defaults. Which is obviously kind of job creation.
Created attachment 125541 [details] [review] Bug 29146: Apply framework defaultvalue only to new records This applies only to the 'regular' cataloguing editor. Another report could be opened for the advanced one, if needed. Test plan: [1] Add a default value to some field X in framework Y. [2] Open an existing biblio record in that fw. Clear field X. Save. [3] Reopen the record. The default should not be in field X. [4] Add a new record in that fw. You should see default value in X. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Item edition is affected as well.
And authorities?
Created attachment 126981 [details] [review] Bug 29146: Same for item editor Test plan: Put some defaultvalue in an item subfield. Test adding new item; is defaultvalue supplied already? Test adding existing item. Clear the subfield. Save. Open again. Is the subfield still empty now? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #4) > Item edition is affected as well. Fixed now. Please test. The change is in the new module ;)
(In reply to Jonathan Druart from comment #5) > And authorities? Will open another report for it. Bug 29334
(In reply to Marcel de Rooy from comment #7) > (In reply to Jonathan Druart from comment #4) > > Item edition is affected as well. > > Fixed now. Please test. The change is in the new module ;) As it's a module, you need to provide a test :)
(In reply to Jonathan Druart from comment #9) > (In reply to Marcel de Rooy from comment #7) > > (In reply to Jonathan Druart from comment #4) > > > Item edition is affected as well. > > > > Fixed now. Please test. The change is in the new module ;) > > As it's a module, you need to provide a test :) Ah yes, very unfortunate :) You still have a chance to increase your FQA score!
Created attachment 126985 [details] [review] Bug 29146: Add test for editing existing item Test plan: Run t/db_dependent/Koha/UI/Form/Builder/Item.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #9) > (In reply to Marcel de Rooy from comment #7) > > (In reply to Jonathan Druart from comment #4) > > > Item edition is affected as well. > > > > Fixed now. Please test. The change is in the new module ;) > > As it's a module, you need to provide a test :) Fixed
+ item => { itemnumber => 999 }, You should build_sample_item and use its itemnumber instead.
Created attachment 126988 [details] [review] Bug 29146: Add test for editing existing item Test plan: Run t/db_dependent/Koha/UI/Form/Builder/Item.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #13) > + item => { itemnumber => 999 }, > > You should build_sample_item and use its itemnumber instead. Doing it with unblessed. But you know that the only essential thing here is existence of itemnumber key in a hash ;)
Created attachment 127041 [details] [review] Bug 29146: Apply framework defaultvalue only to new records This applies only to the 'regular' cataloguing editor. Another report could be opened for the advanced one, if needed. Test plan: [1] Add a default value to some field X in framework Y. [2] Open an existing biblio record in that fw. Clear field X. Save. [3] Reopen the record. The default should not be in field X. [4] Add a new record in that fw. You should see default value in X. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 127042 [details] [review] Bug 29146: Same for item editor Test plan: Put some defaultvalue in an item subfield. Test adding new item; is defaultvalue supplied already? Test adding existing item. Clear the subfield. Save. Open again. Is the subfield still empty now? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 127043 [details] [review] Bug 29146: Add test for editing existing item Test plan: Run t/db_dependent/Koha/UI/Form/Builder/Item.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Testing notes (using koha-testing-docker): - Set default values for items in the framework using 952 subfields
Created attachment 127578 [details] [review] Bug 29146: Apply framework defaultvalue only to new records This applies only to the 'regular' cataloguing editor. Another report could be opened for the advanced one, if needed. Test plan: [1] Add a default value to some field X in framework Y. [2] Open an existing biblio record in that fw. Clear field X. Save. [3] Reopen the record. The default should not be in field X. [4] Add a new record in that fw. You should see default value in X. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 127579 [details] [review] Bug 29146: Same for item editor Test plan: Put some defaultvalue in an item subfield. Test adding new item; is defaultvalue supplied already? Test adding existing item. Clear the subfield. Save. Open again. Is the subfield still empty now? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 127580 [details] [review] Bug 29146: Add test for editing existing item Test plan: Run t/db_dependent/Koha/UI/Form/Builder/Item.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I really should have got to this earlier.. it was the question in the title that put me off.. but it's clear from the top comments that the question was answered. I've updated the title to reflect the consensus. Patches are clear and simple enough to understand.. testing provides positive results and no regressions are found.. QA scripts are happy... Passing QA.
(In reply to Martin Renvoize from comment #23) > I really should have got to this earlier.. it was the question in the title > that put me off.. but it's clear from the top comments that the question was > answered. > > I've updated the title to reflect the consensus. > > Patches are clear and simple enough to understand.. testing provides > positive results and no regressions are found.. > > QA scripts are happy... Passing QA. Thanks. Will try to use less questions in bug titles :)
By all means start with a question.. but amend it once a conclusion has been reached perhaps?
Pushed to master for 21.11, thanks to everybody involved!
Does not apply to 21.05.x. Please rebase and let me know if this is needed!
See follow-up report bug 29592 and discussion about impact on importing records from Z39.50 into basic editor.
This has created a problem for one of our Koha libraries. When they duplicate a record (i.e 'Edit as new') they want empty fields to be filled with framework defaults. Although, due to this fix, they no longer get them. Duplicating records is like creating new records, so I think the check for a biblionumber needs to be alongside a check for whether this is a duplicate function. I've opened Bug 30250 for this follow-up.
Also behavior is now different between biblio and authorities forms. See method create_input().