Bug 29001 - Subfields attributes are not preserved when order is changed in framework
Summary: Subfields attributes are not preserved when order is changed in framework
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 8976
Blocks: 31238 31869
  Show dependency treegraph
 
Reported: 2021-09-12 15:38 UTC by Mathieu Saby
Modified: 2023-06-08 22:27 UTC (History)
6 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.05


Attachments
Bug 29001: Fix framework edition when subfields are reordered (21.06 KB, patch)
2022-07-19 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29001: Fix framework edition when subfields are reordered (21.22 KB, patch)
2022-07-20 06:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29001: Fix framework edition when subfields are reordered (21.28 KB, patch)
2022-07-20 08:56 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 29001: Fix framework edition when subfields are reordered (21.42 KB, patch)
2022-07-21 18:03 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29001: (QA follow-up) Remove code duplication (2.85 KB, patch)
2022-07-21 18:03 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Saby 2021-09-12 15:38:05 UTC
Bug 8976 adds the ability to change the default order of the subfields in MARC framework. But it does not preserve the Repeatable or Mandatory attribute. ​

To test : in a MARC framework, define a subfield as Mandatory. Move the subfield to an other position within the field. The Mandatory flag for that subfield is not preserved. Instead, the subfield occupying the same position is flaged as Mandatory. 

M. Saby
Comment 1 Katrin Fischer 2022-07-17 11:23:23 UTC
I can replicate the issue. 

To test:
- Edit subfields of any field
- Mark the last subfield in the row as mandatory
- Save
- Edit again and verify the field shows as mandatory
- Move the field to the first position and save
- Verify the original field is no longer mandatory, but the last subfield is now

Updating severity as this is a data issue.
Comment 2 Jonathan Druart 2022-07-19 15:15:22 UTC
Created attachment 137874 [details] [review]
Bug 29001: Fix framework edition when subfields are reordered

There is a flaw when subfields are ordered, the inputs are not retrieved
correctly.
We should not rely on the order but use an id instead.
Test plan:
Create, edit subfields
Modify values from the different subfields
Confirm that values are correctly saved
Comment 3 Thibault Keromnès 2022-07-19 23:36:02 UTC
The patch fixes the issue but brings another one, I can't mark a subfield as repeatable now (if i check the 3 attributes and save the subfield will be mandatory and important but not repeatable). 
I don't have this issue with the current master.

I change the status to failed QA, not sure it's the right one, feel free to change it.
Comment 4 Jonathan Druart 2022-07-20 06:33:04 UTC
Created attachment 137893 [details] [review]
Bug 29001: Fix framework edition when subfields are reordered

There is a flaw when subfields are ordered, the inputs are not retrieved
correctly.
We should not rely on the order but use an id instead.
Test plan:
Create, edit subfields
Modify values from the different subfields
Confirm that values are correctly saved
Comment 5 Jonathan Druart 2022-07-20 06:33:40 UTC
(In reply to Thibault Keromnès from comment #3)
> The patch fixes the issue but brings another one, I can't mark a subfield as
> repeatable now (if i check the 3 attributes and save the subfield will be
> mandatory and important but not repeatable). 
> I don't have this issue with the current master.
> 
> I change the status to failed QA, not sure it's the right one, feel free to
> change it.

Indeed, thanks! Should be fixed now.

Yes, Failed QA is the correct status when the patch does not work as expected.
Comment 6 Biblibre Sandboxes 2022-07-20 08:56:57 UTC
Created attachment 137904 [details] [review]
Bug 29001: Fix framework edition when subfields are reordered

There is a flaw when subfields are ordered, the inputs are not retrieved
correctly.
We should not rely on the order but use an id instead.
Test plan:
Create, edit subfields
Modify values from the different subfields
Confirm that values are correctly saved

Signed-off-by: Thibault Kero <thibault.keromnes@univ-paris8.fr>
Comment 7 Joonas Kylmälä 2022-07-21 18:03:46 UTC
Created attachment 137991 [details] [review]
Bug 29001: Fix framework edition when subfields are reordered

There is a flaw when subfields are ordered, the inputs are not retrieved
correctly.
We should not rely on the order but use an id instead.
Test plan:
Create, edit subfields
Modify values from the different subfields
Confirm that values are correctly saved

Signed-off-by: Thibault Kero <thibault.keromnes@univ-paris8.fr>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 8 Joonas Kylmälä 2022-07-21 18:03:53 UTC
Created attachment 137992 [details] [review]
Bug 29001: (QA follow-up) Remove code duplication

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 9 Tomás Cohen Arazi 2022-07-21 19:04:55 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 10 Lucas Gass 2022-09-02 14:52:37 UTC
Backported to 22.05.x for 22.05.05
Comment 11 Arthur Suzuki 2022-09-12 09:47:43 UTC
lots of conflicts on the templates when trying to apply to 21.11.x.
Please provide a backport patch if needed.
Comment 12 Andrii Nugged 2022-09-25 01:33:46 UTC
(In reply to Lucas Gass from comment #10)
> Backported to 22.05.x for 22.05.05

Bug 31238 is also needed in 22.05
because this 29001 brings a bug and fixed in this way:


-        my $authorised_value = $input->param("authorised_values_$id");
+        my $authorised_value = $input->param("authorised_value_$id");

in 31238.