Bug 32046 - When adding a new records from a staged files, there are style issues
Summary: When adding a new records from a staged files, there are style issues
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Owen Leonard
QA Contact: Martin Renvoize
URL: /cgi-bin/koha/acqui/addorderiso2709.p...
Keywords:
: 32235 (view as bug list)
Depends on: 30952
Blocks: 32231
  Show dependency treegraph
 
Reported: 2022-10-30 12:31 UTC by Katrin Fischer
Modified: 2023-06-08 22:32 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Proposed layout change (122.14 KB, image/png)
2022-11-04 17:11 UTC, Owen Leonard
Details
Bug 32046: When adding a new records from a staged files, there are style issues (67.49 KB, patch)
2022-11-16 12:54 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32046: When adding a new records from a staged files, there are style issues (67.54 KB, patch)
2022-11-16 14:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32046: (follow-up) Accommodate the inclusion of item information (38.39 KB, patch)
2022-11-16 16:16 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32046: (follow-up) Accommodate the inclusion of item information (48.71 KB, patch)
2022-11-17 12:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32046: (follow-up) Accommodate the inclusion of item information (48.79 KB, patch)
2022-11-17 12:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32046: When adding a new records from a staged files, there are style issues (67.57 KB, patch)
2022-11-17 18:36 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32046: (follow-up) Accommodate the inclusion of item information (48.79 KB, patch)
2022-11-17 18:36 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32046: When adding a new records from a staged files, there are style issues (67.57 KB, patch)
2022-11-17 18:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32046: (follow-up) Accommodate the inclusion of item information (48.79 KB, patch)
2022-11-17 18:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32046: When adding a new records from a staged files, there are style issues (67.63 KB, patch)
2022-11-18 10:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32046: (follow-up) Accommodate the inclusion of item information (48.85 KB, patch)
2022-11-18 10:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32046: When adding a new records from a staged files, there are style issues (67.69 KB, patch)
2022-11-23 10:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32046: (follow-up) Accommodate the inclusion of item information (48.91 KB, patch)
2022-11-23 10:13 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2022-10-30 12:31:41 UTC
To test:
* Create order
* Stage file of MARC records
* Order from staged file
* Select file -> now you are on the right page listing records

Found issues:
- Toolbar has a shadow and background color?
- Labels and pulldowns/checkboxes are misaligned
- Item form layout on Item tab is broken!
- counting information tab: Statistics fields are misaligned
- can't save due to JS erorr, filed separately as: bug 32045
Comment 1 Owen Leonard 2022-11-04 17:11:04 UTC
Created attachment 143258 [details]
Proposed layout change

How does this look as an alternative layout?
Comment 2 Katrin Fischer 2022-11-10 16:31:35 UTC
The layout would work for me - maybe some more white background?
Comment 3 Katrin Fischer 2022-11-15 16:43:53 UTC
Owen, will you have time for this one? I could try to have a go at the misaligned statistics and the item form (looking at the other places we fixed them)
Comment 4 Owen Leonard 2022-11-16 12:54:51 UTC
Created attachment 143952 [details] [review]
Bug 32046: When adding a new records from a staged files, there are style issues

This patch modifies the template for adding a record from a staged file
in order to resolve some layout problems introduced by the staff
redesign.

This patch also corrects an problem with addorderiso2709.js: It included
some template markup. The template variables are converted to a JS
variable to prevent a JS error.

Note that the QA tool's js_in_body error is invalid for this template
which requires that the JS stay inside <head>.

To test you must have at least one MARC file staged for import.

- Apply the patch and go to Acquisitions.
- Locate a vendor and create a basket if necessary.
- Add to the basket and choose the option of adding an order from a
  staged file.
- Click "Add orders" next to one of the staged files.
- On the Add orders from MARC file page, test that everything looks
  correct and works correctly:
  - In the first tab, expanding and collapsing order details by checking
    the checkbox or clicking the title.
  - Test the "MARC" and "Card" preview buttons.
  - Under the Item information tab, confirm that the layout looks
    correct and that the form works correctly, including plugin
    operations like auto barcode fill.
  - Test that, under the Default accounting details tab, changing the
    fund automatically sets the fund for each title in the first tab.
Comment 5 Katrin Fischer 2022-11-16 14:15:52 UTC
Created attachment 143956 [details] [review]
Bug 32046: When adding a new records from a staged files, there are style issues

This patch modifies the template for adding a record from a staged file
in order to resolve some layout problems introduced by the staff
redesign.

This patch also corrects an problem with addorderiso2709.js: It included
some template markup. The template variables are converted to a JS
variable to prevent a JS error.

Note that the QA tool's js_in_body error is invalid for this template
which requires that the JS stay inside <head>.

To test you must have at least one MARC file staged for import.

- Apply the patch and go to Acquisitions.
- Locate a vendor and create a basket if necessary.
- Add to the basket and choose the option of adding an order from a
  staged file.
- Click "Add orders" next to one of the staged files.
- On the Add orders from MARC file page, test that everything looks
  correct and works correctly:
  - In the first tab, expanding and collapsing order details by checking
    the checkbox or clicking the title.
  - Test the "MARC" and "Card" preview buttons.
  - Under the Item information tab, confirm that the layout looks
    correct and that the form works correctly, including plugin
    operations like auto barcode fill.
  - Test that, under the Default accounting details tab, changing the
    fund automatically sets the fund for each title in the first tab.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Owen Leonard 2022-11-16 16:16:03 UTC
Created attachment 143958 [details] [review]
Bug 32046: (follow-up) Accommodate the inclusion of item information

If you have item information configured in the MarcItemFieldsToOrder
system preference, the "Select to import" tab will display a basic item
information form which needs to use the same style as other item edit
forms.

In order to make the CSS in addbiblio.css more generic, I've changed
some styles to use a class, .item_edit_form, instead of an ID. This lets
us have multiple <div class="item_edit_form"> in this template without
duplicating IDs. Other instances of <div
id="cataloguing_additem_newitem"> have haa the .item_edit_form class
added.

To test you must have a staged MARC file which includes items and you
must configure the MarcItemFieldsToOrder preference to map those items
in the ordering interface.

As you follow the previous patch's test plan you'll find that when you
click a checkbox or title to expand the information about that title
you'll see "Item record X" headings following by a properly-styled form.

To confirm that the changes to addbiblio.css are not too far-reaching,
test the other affected pages:

 - Item add/edit
 - Batch item modification
 - Batch item deletion
Comment 7 David Nind 2022-11-16 19:02:02 UTC
I think I tested this correctly, but have a couple of comments:

1. Could we make an exception to the add/edit item form so that the width for form labels is wider? In my view, making it narrow and then wrapping the text:
   - makes it harder to read/scan
   - makes the input field height longer
   - which makes the alignment for the tick boxes and RegEx links look "weird" (Batch item modification form)
   - particularly when there is enough space on the page, even for smaller screen widths

2. For the acquisition screens, I wasn't sure why on these that the dropdown lists have a grey background.
Comment 8 Katrin Fischer 2022-11-16 19:03:43 UTC
The first tab looks great now, the third tab too, but I got some issues with the item form:

https://snipboard.io/QHUtIW.jpg

The longer descriptions now blow up the input fields, this was not the case in previous iterations.

In general I'd love if we could give labels more space in multiple locations. We always need to add additional CSS there for German and I'd so love to get rid of it. Also the problem seems prominent now also for the English templates.
Comment 9 Katrin Fischer 2022-11-16 19:03:58 UTC
Great minds think alike ;)
Comment 10 Owen Leonard 2022-11-17 12:35:24 UTC
Created attachment 143996 [details] [review]
Bug 32046: (follow-up) Accommodate the inclusion of item information

If you have item information configured in the MarcItemFieldsToOrder
system preference, the "Select to import" tab will display a basic item
information form which needs to use the same style as other item edit
forms.

In order to make the CSS in addbiblio.css more generic, I've changed
some styles to use a class, .item_edit_form, instead of an ID. This lets
us have multiple <div class="item_edit_form"> in this template without
duplicating IDs. Other instances of <div
id="cataloguing_additem_newitem"> have haa the .item_edit_form class
added.

To test you must have a staged MARC file which includes items and you
must configure the MarcItemFieldsToOrder preference to map those items
in the ordering interface.

As you follow the previous patch's test plan you'll find that when you
click a checkbox or title to expand the information about that title
you'll see "Item record X" headings following by a properly-styled form.

To confirm that the changes to addbiblio.css are not too far-reaching,
test the other affected pages:

 - Item add/edit
 - Batch item modification
 - Batch item deletion
Comment 11 Owen Leonard 2022-11-17 12:57:32 UTC
Created attachment 143997 [details] [review]
Bug 32046: (follow-up) Accommodate the inclusion of item information

If you have item information configured in the MarcItemFieldsToOrder
system preference, the "Select to import" tab will display a basic item
information form which needs to use the same style as other item edit
forms.

In order to make the CSS in addbiblio.css more generic, I've changed
some styles to use a class, .item_edit_form, instead of an ID. This lets
us have multiple <div class="item_edit_form"> in this template without
duplicating IDs. Other instances of <div
id="cataloguing_additem_newitem"> have haa the .item_edit_form class
added.

To test you must have a staged MARC file which includes items and you
must configure the MarcItemFieldsToOrder preference to map those items
in the ordering interface.

As you follow the previous patch's test plan you'll find that when you
click a checkbox or title to expand the information about that title
you'll see "Item record X" headings following by a properly-styled form.

To confirm that the changes to addbiblio.css are not too far-reaching,
test the other affected pages:

 - Item add/edit
 - Batch item modification
 - Batch item deletion
Comment 12 Katrin Fischer 2022-11-17 18:10:31 UTC
I am sorry, conflicts... :(
Comment 13 Owen Leonard 2022-11-17 18:36:12 UTC
Created attachment 144022 [details] [review]
Bug 32046: When adding a new records from a staged files, there are style issues

This patch modifies the template for adding a record from a staged file
in order to resolve some layout problems introduced by the staff
redesign.

This patch also corrects an problem with addorderiso2709.js: It included
some template markup. The template variables are converted to a JS
variable to prevent a JS error.

Note that the QA tool's js_in_body error is invalid for this template
which requires that the JS stay inside <head>.

To test you must have at least one MARC file staged for import.

- Apply the patch and go to Acquisitions.
- Locate a vendor and create a basket if necessary.
- Add to the basket and choose the option of adding an order from a
  staged file.
- Click "Add orders" next to one of the staged files.
- On the Add orders from MARC file page, test that everything looks
  correct and works correctly:
  - In the first tab, expanding and collapsing order details by checking
    the checkbox or clicking the title.
  - Test the "MARC" and "Card" preview buttons.
  - Under the Item information tab, confirm that the layout looks
    correct and that the form works correctly, including plugin
    operations like auto barcode fill.
  - Test that, under the Default accounting details tab, changing the
    fund automatically sets the fund for each title in the first tab.
Comment 14 Owen Leonard 2022-11-17 18:36:17 UTC
Created attachment 144023 [details] [review]
Bug 32046: (follow-up) Accommodate the inclusion of item information

If you have item information configured in the MarcItemFieldsToOrder
system preference, the "Select to import" tab will display a basic item
information form which needs to use the same style as other item edit
forms.

In order to make the CSS in addbiblio.css more generic, I've changed
some styles to use a class, .item_edit_form, instead of an ID. This lets
us have multiple <div class="item_edit_form"> in this template without
duplicating IDs. Other instances of <div
id="cataloguing_additem_newitem"> have haa the .item_edit_form class
added.

To test you must have a staged MARC file which includes items and you
must configure the MarcItemFieldsToOrder preference to map those items
in the ordering interface.

As you follow the previous patch's test plan you'll find that when you
click a checkbox or title to expand the information about that title
you'll see "Item record X" headings following by a properly-styled form.

To confirm that the changes to addbiblio.css are not too far-reaching,
test the other affected pages:

 - Item add/edit
 - Batch item modification
 - Batch item deletion
Comment 15 Katrin Fischer 2022-11-17 18:55:17 UTC
Created attachment 144027 [details] [review]
Bug 32046: When adding a new records from a staged files, there are style issues

This patch modifies the template for adding a record from a staged file
in order to resolve some layout problems introduced by the staff
redesign.

This patch also corrects an problem with addorderiso2709.js: It included
some template markup. The template variables are converted to a JS
variable to prevent a JS error.

Note that the QA tool's js_in_body error is invalid for this template
which requires that the JS stay inside <head>.

To test you must have at least one MARC file staged for import.

- Apply the patch and go to Acquisitions.
- Locate a vendor and create a basket if necessary.
- Add to the basket and choose the option of adding an order from a
  staged file.
- Click "Add orders" next to one of the staged files.
- On the Add orders from MARC file page, test that everything looks
  correct and works correctly:
  - In the first tab, expanding and collapsing order details by checking
    the checkbox or clicking the title.
  - Test the "MARC" and "Card" preview buttons.
  - Under the Item information tab, confirm that the layout looks
    correct and that the form works correctly, including plugin
    operations like auto barcode fill.
  - Test that, under the Default accounting details tab, changing the
    fund automatically sets the fund for each title in the first tab.
Comment 16 Katrin Fischer 2022-11-17 18:55:22 UTC
Created attachment 144028 [details] [review]
Bug 32046: (follow-up) Accommodate the inclusion of item information

If you have item information configured in the MarcItemFieldsToOrder
system preference, the "Select to import" tab will display a basic item
information form which needs to use the same style as other item edit
forms.

In order to make the CSS in addbiblio.css more generic, I've changed
some styles to use a class, .item_edit_form, instead of an ID. This lets
us have multiple <div class="item_edit_form"> in this template without
duplicating IDs. Other instances of <div
id="cataloguing_additem_newitem"> have haa the .item_edit_form class
added.

To test you must have a staged MARC file which includes items and you
must configure the MarcItemFieldsToOrder preference to map those items
in the ordering interface.

As you follow the previous patch's test plan you'll find that when you
click a checkbox or title to expand the information about that title
you'll see "Item record X" headings following by a properly-styled form.

To confirm that the changes to addbiblio.css are not too far-reaching,
test the other affected pages:

 - Item add/edit
 - Batch item modification
 - Batch item deletion
Comment 17 Owen Leonard 2022-11-17 19:49:53 UTC
*** Bug 32235 has been marked as a duplicate of this bug. ***
Comment 18 Marcel de Rooy 2022-11-18 09:12:29 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorderiso2709.tt
   OK     filters
   OK     forbidden patterns
   OK     git manipulation
   FAIL   js_in_body
                A <script> tag found inside head, must be moved to the body (see bug 17858)
Comment 19 Katrin Fischer 2022-11-18 09:49:18 UTC
It's a false positive, I checked with Owen when I signed off and he also noted in the commit:

> Note that the QA tool's js_in_body error is invalid for this template
> which requires that the JS stay inside <head>.
Comment 20 Marcel de Rooy 2022-11-18 10:13:02 UTC
Ah, I see:
[%# As long as cataloging plugins rely on 'script' tags added inline, JS must be in the header %]
<script>
    const template_path = "[% interface | html %]/[% theme | html %]";
</script>

Katrin: You switched status. But you did not add signoff lines ?
Comment 21 Marcel de Rooy 2022-11-18 10:15:55 UTC
Actually, there are such large changes in a template that I cant oversee if we somewhere add string changes at this time before release..
Comment 22 Katrin Fischer 2022-11-18 10:18:31 UTC
(In reply to Marcel de Rooy from comment #21)
> Actually, there are such large changes in a template that I cant oversee if
> we somewhere add string changes at this time before release..

Ah, will fix. It's my usual mistake when I forget to git stash the yarn. The git so then fails a bit too silently.
Comment 23 Katrin Fischer 2022-11-18 10:20:57 UTC
Created attachment 144052 [details] [review]
Bug 32046: When adding a new records from a staged files, there are style issues

This patch modifies the template for adding a record from a staged file
in order to resolve some layout problems introduced by the staff
redesign.

This patch also corrects an problem with addorderiso2709.js: It included
some template markup. The template variables are converted to a JS
variable to prevent a JS error.

Note that the QA tool's js_in_body error is invalid for this template
which requires that the JS stay inside <head>.

To test you must have at least one MARC file staged for import.

- Apply the patch and go to Acquisitions.
- Locate a vendor and create a basket if necessary.
- Add to the basket and choose the option of adding an order from a
  staged file.
- Click "Add orders" next to one of the staged files.
- On the Add orders from MARC file page, test that everything looks
  correct and works correctly:
  - In the first tab, expanding and collapsing order details by checking
    the checkbox or clicking the title.
  - Test the "MARC" and "Card" preview buttons.
  - Under the Item information tab, confirm that the layout looks
    correct and that the form works correctly, including plugin
    operations like auto barcode fill.
  - Test that, under the Default accounting details tab, changing the
    fund automatically sets the fund for each title in the first tab.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2022-11-18 10:21:02 UTC
Created attachment 144053 [details] [review]
Bug 32046: (follow-up) Accommodate the inclusion of item information

If you have item information configured in the MarcItemFieldsToOrder
system preference, the "Select to import" tab will display a basic item
information form which needs to use the same style as other item edit
forms.

In order to make the CSS in addbiblio.css more generic, I've changed
some styles to use a class, .item_edit_form, instead of an ID. This lets
us have multiple <div class="item_edit_form"> in this template without
duplicating IDs. Other instances of <div
id="cataloguing_additem_newitem"> have haa the .item_edit_form class
added.

To test you must have a staged MARC file which includes items and you
must configure the MarcItemFieldsToOrder preference to map those items
in the ordering interface.

As you follow the previous patch's test plan you'll find that when you
click a checkbox or title to expand the information about that title
you'll see "Item record X" headings following by a properly-styled form.

To confirm that the changes to addbiblio.css are not too far-reaching,
test the other affected pages:

 - Item add/edit
 - Batch item modification
 - Batch item deletion

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Martin Renvoize 2022-11-23 10:13:30 UTC
Created attachment 144193 [details] [review]
Bug 32046: When adding a new records from a staged files, there are style issues

This patch modifies the template for adding a record from a staged file
in order to resolve some layout problems introduced by the staff
redesign.

This patch also corrects an problem with addorderiso2709.js: It included
some template markup. The template variables are converted to a JS
variable to prevent a JS error.

Note that the QA tool's js_in_body error is invalid for this template
which requires that the JS stay inside <head>.

To test you must have at least one MARC file staged for import.

- Apply the patch and go to Acquisitions.
- Locate a vendor and create a basket if necessary.
- Add to the basket and choose the option of adding an order from a
  staged file.
- Click "Add orders" next to one of the staged files.
- On the Add orders from MARC file page, test that everything looks
  correct and works correctly:
  - In the first tab, expanding and collapsing order details by checking
    the checkbox or clicking the title.
  - Test the "MARC" and "Card" preview buttons.
  - Under the Item information tab, confirm that the layout looks
    correct and that the form works correctly, including plugin
    operations like auto barcode fill.
  - Test that, under the Default accounting details tab, changing the
    fund automatically sets the fund for each title in the first tab.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2022-11-23 10:13:35 UTC
Created attachment 144194 [details] [review]
Bug 32046: (follow-up) Accommodate the inclusion of item information

If you have item information configured in the MarcItemFieldsToOrder
system preference, the "Select to import" tab will display a basic item
information form which needs to use the same style as other item edit
forms.

In order to make the CSS in addbiblio.css more generic, I've changed
some styles to use a class, .item_edit_form, instead of an ID. This lets
us have multiple <div class="item_edit_form"> in this template without
duplicating IDs. Other instances of <div
id="cataloguing_additem_newitem"> have haa the .item_edit_form class
added.

To test you must have a staged MARC file which includes items and you
must configure the MarcItemFieldsToOrder preference to map those items
in the ordering interface.

As you follow the previous patch's test plan you'll find that when you
click a checkbox or title to expand the information about that title
you'll see "Item record X" headings following by a properly-styled form.

To confirm that the changes to addbiblio.css are not too far-reaching,
test the other affected pages:

 - Item add/edit
 - Batch item modification
 - Batch item deletion

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2022-11-23 10:15:06 UTC
Nice improvement, QA scripts generally happy and all works as expected.

Passing QA.

Warning: This patch does introduce two new strings as far as I can tell, labels only.  I'll leave it to RM's discretion whether as a major it's worth the pain or not to push now or just after 22.11.00 release.
Comment 28 Tomás Cohen Arazi 2022-11-23 12:48:11 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!