Bug 29146

Summary: Default values from the framework should only be applied at biblio/item creation
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: CatalogingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: aleisha, david, dcook, fridolin.somers, jonathan.druart, kyle, lauren_denny, m.de.rooy, martin.renvoize, pierre.genty
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29334
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes the standard cataloguing editor and the item editor so that when updating existing records and items empty fields are not updated with the default framework values.
Version(s) released in:
21.11.00
Bug Depends on:    
Bug Blocks: 30250, 33670, 29592    
Attachments: Bug 29146: Apply framework defaultvalue only to new records
Bug 29146: Same for item editor
Bug 29146: Add test for editing existing item
Bug 29146: Add test for editing existing item
Bug 29146: Apply framework defaultvalue only to new records
Bug 29146: Same for item editor
Bug 29146: Add test for editing existing item
Bug 29146: Apply framework defaultvalue only to new records
Bug 29146: Same for item editor
Bug 29146: Add test for editing existing item

Description Marcel de Rooy 2021-09-30 12:54:46 UTC
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?
Comment 1 Katrin Fischer 2021-09-30 12:57:54 UTC
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.
Comment 2 Marcel de Rooy 2021-09-30 13:53:52 UTC
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.
Comment 3 Marcel de Rooy 2021-09-30 13:54:25 UTC
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>
Comment 4 Jonathan Druart 2021-10-25 13:24:42 UTC
Item edition is affected as well.
Comment 5 Jonathan Druart 2021-10-25 13:26:33 UTC
And authorities?
Comment 6 Marcel de Rooy 2021-10-27 12:09:06 UTC
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>
Comment 7 Marcel de Rooy 2021-10-27 12:10:02 UTC
(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 ;)
Comment 8 Marcel de Rooy 2021-10-27 12:13:18 UTC
(In reply to Jonathan Druart from comment #5)
> And authorities?

Will open another report for it. Bug 29334
Comment 9 Jonathan Druart 2021-10-27 12:18:03 UTC
(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 :)
Comment 10 Marcel de Rooy 2021-10-27 12:21:50 UTC
(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!
Comment 11 Marcel de Rooy 2021-10-27 12:32:45 UTC
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>
Comment 12 Marcel de Rooy 2021-10-27 12:33:02 UTC
(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
Comment 13 Jonathan Druart 2021-10-27 12:46:43 UTC
+        item => { itemnumber => 999 },

You should build_sample_item and use its itemnumber instead.
Comment 14 Marcel de Rooy 2021-10-27 12:58:47 UTC
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>
Comment 15 Marcel de Rooy 2021-10-27 13:00:29 UTC
(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 ;)
Comment 16 David Nind 2021-10-28 11:01:16 UTC
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>
Comment 17 David Nind 2021-10-28 11:01:20 UTC
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>
Comment 18 David Nind 2021-10-28 11:01:25 UTC
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>
Comment 19 David Nind 2021-10-28 11:14:57 UTC
Testing notes (using koha-testing-docker):

- Set default values for items in the framework using 952 subfields
Comment 20 Martin Renvoize 2021-11-12 11:18:54 UTC
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>
Comment 21 Martin Renvoize 2021-11-12 11:18:58 UTC
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>
Comment 22 Martin Renvoize 2021-11-12 11:19:01 UTC
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>
Comment 23 Martin Renvoize 2021-11-12 11:20:21 UTC
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.
Comment 24 Marcel de Rooy 2021-11-12 12:27:10 UTC
(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 :)
Comment 25 Martin Renvoize 2021-11-14 10:50:43 UTC
By all means start with a question.. but amend it once a conclusion has been reached perhaps?
Comment 26 Jonathan Druart 2021-11-15 11:39:36 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 27 Kyle M Hall 2021-11-19 16:30:08 UTC
Does not apply to 21.05.x. Please rebase and let me know if this is needed!
Comment 28 Marcel de Rooy 2021-11-29 14:55:15 UTC
See follow-up report bug 29592 and discussion about impact on importing records from Z39.50 into basic editor.
Comment 29 Aleisha Amohia 2022-03-08 21:28:23 UTC
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.
Comment 30 Fridolin Somers 2022-09-23 02:46:38 UTC
Also behavior is now different between biblio and authorities forms.
See method create_input().