Bug 24276 - Add functionality to apply defaults from the ACQ framework for mandatory fields when adding records from external sources
Summary: Add functionality to apply defaults from the ACQ framework for mandatory fiel...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-19 13:42 UTC by Marcel de Rooy
Modified: 2020-05-18 13:02 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24276: First change indent (1.49 KB, patch)
2019-12-20 08:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: Add FillWithDefaultValues call in neworderempty (2.69 KB, patch)
2019-12-20 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues (2.66 KB, patch)
2019-12-20 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: First change indent (1.43 KB, patch)
2019-12-20 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: Add FillWithDefaultValues call in neworderempty (2.69 KB, patch)
2019-12-20 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues (2.66 KB, patch)
2019-12-20 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues (2.66 KB, patch)
2019-12-20 10:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: First change indent (1.43 KB, patch)
2020-03-09 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: Add FillWithDefaultValues call in neworderempty (2.69 KB, patch)
2020-03-09 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues (2.72 KB, patch)
2020-03-09 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24276: First change indent (1.40 KB, patch)
2020-03-23 12:42 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 24276: Add FillWithDefaultValues call in neworderempty (2.79 KB, patch)
2020-03-23 12:42 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues (2.69 KB, patch)
2020-03-23 12:42 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 24276: First change indent (1.46 KB, patch)
2020-03-24 08:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24276: Add FillWithDefaultValues call in neworderempty (2.85 KB, patch)
2020-03-24 08:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues (2.75 KB, patch)
2020-03-24 08:21 UTC, Katrin Fischer
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 2019-12-19 13:42:51 UTC

    
Comment 1 Marcel de Rooy 2019-12-20 08:30:56 UTC
If we import a record via Acquisition Z3950 search, the default itemtype (942c) is NULL although it is mandatory. Do not confuse with itype in 952.
We apply defaults if we add a new empty record; we should at least apply the mandatory defaults to external records.
Comment 2 Marcel de Rooy 2019-12-20 08:40:58 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2019-12-20 08:41:03 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2019-12-20 08:41:07 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2019-12-20 08:44:15 UTC
Created attachment 96510 [details] [review]
Bug 24276: First change indent

No other changes than whitespace, guys.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2019-12-20 08:44:19 UTC
Created attachment 96511 [details] [review]
Bug 24276: Add FillWithDefaultValues call in neworderempty

Apply mandatory defaults from the ACQ framework to records from external
source in Acquisition.

Test plan:
[1] Add 'BK' as mandatory default in ACQ framework for 942$c.
[2] Add order to basket via external source.
[3] Check 942$c on detail page of new record, MARC tab.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2019-12-20 08:44:23 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2019-12-20 10:16:35 UTC
Created attachment 96513 [details] [review]
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues

Test plan:
Run t/db_dependent/Acquisition/FillWithDefaultValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2020-03-09 13:46:27 UTC
Created attachment 100376 [details] [review]
Bug 24276: First change indent

No other changes than whitespace, guys.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2020-03-09 13:46:31 UTC
Created attachment 100377 [details] [review]
Bug 24276: Add FillWithDefaultValues call in neworderempty

Apply mandatory defaults from the ACQ framework to records from external
source in Acquisition.

Test plan:
[1] Add 'BK' as mandatory default in ACQ framework for 942$c.
[2] Add order to basket via external source.
[3] Check 942$c on detail page of new record, MARC tab.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2020-03-09 13:46:35 UTC
Created attachment 100378 [details] [review]
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues

Test plan:
Run t/db_dependent/Acquisition/FillWithDefaultValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2020-03-09 13:46:55 UTC
Rebased. Please test!
Comment 13 Bernardo Gonzalez Kriegel 2020-03-21 21:42:43 UTC
Hi Marcel,
current ACQ framework does not have tag 942.
Do you think we need to add it and it's subfields?

Patches applies cleanly, test pass.
Comment 14 Marcel de Rooy 2020-03-23 10:59:35 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #13)
> Hi Marcel,
> current ACQ framework does not have tag 942.
> Do you think we need to add it and it's subfields?
> 
> Patches applies cleanly, test pass.

Thx for testing, Bernardo.
No I do not think we should add it on this report per se. Surely, it could be done on its own. But it is a just an example of how we can apply mandatories. Another existing field could be used too.
Comment 15 Bernardo Gonzalez Kriegel 2020-03-23 12:42:44 UTC
Created attachment 101437 [details] [review]
Bug 24276: First change indent

No other changes than whitespace, guys.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 16 Bernardo Gonzalez Kriegel 2020-03-23 12:42:49 UTC
Created attachment 101438 [details] [review]
Bug 24276: Add FillWithDefaultValues call in neworderempty

Apply mandatory defaults from the ACQ framework to records from external
source in Acquisition.

Test plan:
[1] Add 'BK' as mandatory default in ACQ framework for 942$c.
[2] Add order to basket via external source.
[3] Check 942$c on detail page of new record, MARC tab.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
First added 942 field and subfields to ACQ fw, then followed test plan.
wo/patch 942 is not added, w/patch it is.
Work as described.
Comment 17 Bernardo Gonzalez Kriegel 2020-03-23 12:42:52 UTC
Created attachment 101439 [details] [review]
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues

Test plan:
Run t/db_dependent/Acquisition/FillWithDefaultValues.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass
Comment 18 Katrin Fischer 2020-03-24 08:20:42 UTC
+If the parameter only_mandatory => 1 is passed via $params, only the mandatory
+defaults are being applied to the record.

For the cataloguing editor it does not matter if the field is mandatory or not. As soon as you set a default, the value will appear in the record. So we are handling things differently here - but we also don't open the record for editing, so this makes sense to me.

I have another issue tho: We apply the default from the ACQ framework, while we can specifically choose a framework to import the record in on the Z39.50/SRU form - this is not caused by this bug, but doesn't make sense to me. Especially for 942$c it would make a lot of sense to me to set different default for different frameworks and use them. What do you think, Marcel?

Still passing as this is pre-bug.
Comment 19 Katrin Fischer 2020-03-24 08:21:18 UTC
Created attachment 101505 [details] [review]
Bug 24276: First change indent

No other changes than whitespace, guys.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2020-03-24 08:21:21 UTC
Created attachment 101506 [details] [review]
Bug 24276: Add FillWithDefaultValues call in neworderempty

Apply mandatory defaults from the ACQ framework to records from external
source in Acquisition.

Test plan:
[1] Add 'BK' as mandatory default in ACQ framework for 942$c.
[2] Add order to basket via external source.
[3] Check 942$c on detail page of new record, MARC tab.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
First added 942 field and subfields to ACQ fw, then followed test plan.
wo/patch 942 is not added, w/patch it is.
Work as described.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2020-03-24 08:21:25 UTC
Created attachment 101507 [details] [review]
Bug 24276: Add unit tests for mandatory parameter of FillWithDefaultValues

Test plan:
Run t/db_dependent/Acquisition/FillWithDefaultValues.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Martin Renvoize 2020-03-24 11:33:46 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 23 Marcel de Rooy 2020-03-24 13:25:00 UTC
(In reply to Katrin Fischer from comment #18)
> I have another issue tho: We apply the default from the ACQ framework, while
> we can specifically choose a framework to import the record in on the
> Z39.50/SRU form - this is not caused by this bug, but doesn't make sense to
> me. Especially for 942$c it would make a lot of sense to me to set different
> default for different frameworks and use them. What do you think, Marcel?

Sounds valid, have to check. Getting back to it..
Comment 24 Marcel de Rooy 2020-03-24 15:50:51 UTC
Katrin,
I think we should probably open a new report for some of the inconsistencies between the various acquisitions routes and the use of the ACQ framework.

It is rather trivial to pass the frameworkcode parameter of acqui/z3950_search to FillWithDefaults to overrule ACQ. But are we bitten later on by the preference UseACQFrameworkForBiblioRecords ? The code in Acquisition is not completely clear as to that preference?
Comment 25 Katrin Fischer 2020-03-24 15:55:08 UTC
(In reply to Marcel de Rooy from comment #24)
> Katrin,
> I think we should probably open a new report for some of the inconsistencies
> between the various acquisitions routes and the use of the ACQ framework.
> 
> It is rather trivial to pass the frameworkcode parameter of
> acqui/z3950_search to FillWithDefaults to overrule ACQ. But are we bitten
> later on by the preference UseACQFrameworkForBiblioRecords ? The code in
> Acquisition is not completely clear as to that preference?

Hi Marcel, certainly a new bug. The handling of frameworks (ACQ vs the one selected on import) is not very clear.
Comment 26 Joy Nelson 2020-04-03 21:47:41 UTC
enhancement not backported to 19.11.x