Bug 25852 - If a layout is edited, the layout type will revert to barcode
Summary: If a layout is edited, the layout type will revert to barcode
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15019
  Show dependency treegraph
 
Reported: 2020-06-23 14:58 UTC by Kelly McElligott
Modified: 2020-09-20 11:07 UTC (History)
5 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.11.00, 20.05.03, 19.11.09, 19.05.15


Attachments
Bug 25852: Improve C4::Creators::Lib reliability under plack (3.31 KB, patch)
2020-07-30 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25852: Add tests (1.22 KB, patch)
2020-07-30 10:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25852: Improve C4::Creators::Lib reliability under plack (3.37 KB, patch)
2020-07-30 11:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25852: Add tests (1.28 KB, patch)
2020-07-30 11:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25852: Add tests (1.34 KB, patch)
2020-07-30 20:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25852: Improve C4::Creators::Lib reliability under plack (3.42 KB, patch)
2020-07-30 20:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25852: Add tests (1.39 KB, patch)
2020-07-30 20:43 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 Kelly McElligott 2020-06-23 14:58:52 UTC
If a label is set to have the layout as Barcode/Biblio - and edits are made, the layout type will change to just Barcode.

To Recreate:
1. Create a layout type for Barcode/Biblio
2. Choose fields to print and size of font.
3. Save and Exit.
4. Test print this label and realize that font is too large.
5. Edit existing Layout and change font.
6. Save and then see that the layout type is now set to Barcode.

This is extremely confusing and frustrating since one might not notice that the type has changed and prepared to print a sheet of labels and they are now wrong as they are only creating barcode labels instead of barcode/biblio labels.
Comment 1 Jonathan Druart 2020-07-30 10:11:40 UTC
Created attachment 107571 [details] [review]
Bug 25852: Improve C4::Creators::Lib reliability under plack

This is certainly a major issue that leads to many side-effects.
Under plack, the structure of the default values are not handled
correctly.
Package variables are used to store stuff like the "layout type". They
are complex structures (arrays of hashes) and returned without being
copied.
When the caller (the controller script) retrieve them then modify the
returned structures, it actually modifies the package's variables.

One of the issue is:
Create a new layout
The script retrieve a structure with all "selected" flags are set to 0
It select the first one as default (BAR as selected => 1)
The user creates the new layout and will selected BIBBAR (for instance)
If you then edit this new layout, the script will retrieve the
"label_types" and set "selected" for BIBBAR. However BAR is still
selected!
The UI receives 2 selected and display the first selected one that has
the selected option.

Test plan:
1. Create a layout type for Barcode/Biblio
2. Choose fields to print and size of font
3. Save
4. Edit existing Layout
=> Withtout this patch "Barcode" is the preselected option
=> With this patch applied, the correct "Barcode/Biblio" option is
selected
Comment 2 Jonathan Druart 2020-07-30 10:12:10 UTC
Created attachment 107572 [details] [review]
Bug 25852: Add tests
Comment 3 ByWater Sandboxes 2020-07-30 11:06:40 UTC
Created attachment 107573 [details] [review]
Bug 25852: Improve C4::Creators::Lib reliability under plack

This is certainly a major issue that leads to many side-effects.
Under plack, the structure of the default values are not handled
correctly.
Package variables are used to store stuff like the "layout type". They
are complex structures (arrays of hashes) and returned without being
copied.
When the caller (the controller script) retrieve them then modify the
returned structures, it actually modifies the package's variables.

One of the issue is:
Create a new layout
The script retrieve a structure with all "selected" flags are set to 0
It select the first one as default (BAR as selected => 1)
The user creates the new layout and will selected BIBBAR (for instance)
If you then edit this new layout, the script will retrieve the
"label_types" and set "selected" for BIBBAR. However BAR is still
selected!
The UI receives 2 selected and display the first selected one that has
the selected option.

Test plan:
1. Create a layout type for Barcode/Biblio
2. Choose fields to print and size of font
3. Save
4. Edit existing Layout
=> Withtout this patch "Barcode" is the preselected option
=> With this patch applied, the correct "Barcode/Biblio" option is
selected

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 4 ByWater Sandboxes 2020-07-30 11:06:43 UTC
Created attachment 107574 [details] [review]
Bug 25852: Add tests

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 5 Katrin Fischer 2020-07-30 20:42:38 UTC
Created attachment 107607 [details] [review]
Bug 25852: Add tests

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2020-07-30 20:43:11 UTC
Created attachment 107608 [details] [review]
Bug 25852: Improve C4::Creators::Lib reliability under plack

This is certainly a major issue that leads to many side-effects.
Under plack, the structure of the default values are not handled
correctly.
Package variables are used to store stuff like the "layout type". They
are complex structures (arrays of hashes) and returned without being
copied.
When the caller (the controller script) retrieve them then modify the
returned structures, it actually modifies the package's variables.

One of the issue is:
Create a new layout
The script retrieve a structure with all "selected" flags are set to 0
It select the first one as default (BAR as selected => 1)
The user creates the new layout and will selected BIBBAR (for instance)
If you then edit this new layout, the script will retrieve the
"label_types" and set "selected" for BIBBAR. However BAR is still
selected!
The UI receives 2 selected and display the first selected one that has
the selected option.

Test plan:
1. Create a layout type for Barcode/Biblio
2. Choose fields to print and size of font
3. Save
4. Edit existing Layout
=> Withtout this patch "Barcode" is the preselected option
=> With this patch applied, the correct "Barcode/Biblio" option is
selected

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2020-07-30 20:43:15 UTC
Created attachment 107609 [details] [review]
Bug 25852: Add tests

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2020-07-30 20:43:59 UTC
I am a little worried that other parts of the layout creator/patron card creator follow similar patterns and have similar issues...
Comment 9 Jonathan Druart 2020-07-31 08:56:40 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 Lucas Gass 2020-08-13 16:30:27 UTC
backported to 20.05.x for 20.05.03
Comment 11 Aleisha Amohia 2020-08-16 20:40:31 UTC
backported to 19.11.x for 19.11.09
Comment 12 David Cook 2020-08-17 00:30:58 UTC
(In reply to Katrin Fischer from comment #8)
> I am a little worried that other parts of the layout creator/patron card
> creator follow similar patterns and have similar issues...

After reviewing this code/issue, I'm actually more concerned for other parts of Koha. I wonder how many related issues are sleeping throughout the codebase...
Comment 13 Victor Grousset/tuxayo 2020-09-20 11:07:41 UTC
Backported to 19.05.x branch for 19.05.15