Bug 21243 - Regression: SRU mapping popup for bibliographic records is unstyled
Summary: Regression: SRU mapping popup for bibliographic records is unstyled
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 21151
Blocks: 20743
  Show dependency treegraph
 
Reported: 2018-08-17 14:18 UTC by Owen Leonard
Modified: 2018-09-21 10:11 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21243: Regression: SRU mapping popup for bibliographic records is unstyled (1.24 KB, patch)
2018-08-17 14:27 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21243: Regression: SRU mapping popup for bibliographic records is unstyled (1.30 KB, patch)
2018-08-21 07:21 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 21243: Regression: SRU mapping popup for bibliographic records is unstyled (1.36 KB, patch)
2018-08-21 12:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21243: (follow-up) Remove extra '</div>' tag (1.05 KB, patch)
2018-08-21 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2018-08-17 14:18:08 UTC
Bug 21151 removed [% INCLUDE 'doc-head-close.inc' %] from sru_modmapping.tt. It means the page loads without any CSS at all. I don't think this was intentional?
Comment 1 Owen Leonard 2018-08-17 14:27:19 UTC
Created attachment 77982 [details] [review]
Bug 21243: Regression: SRU mapping popup for bibliographic records is unstyled

This patch corrects a regression caused by Bug 21151: The include file
which adds CSS to the page was removed from the template.

To test, apply the patch and go to Administration -> Z39.50/SRU servers
-> New SRU server.

- Click the "Modify" button next to "SRU Search fields mapping"
- Verify that the pop-up window is correctly styled.
Comment 2 Katrin Fischer 2018-08-17 16:28:02 UTC
Hm, my patch on bug 21552 didn't remove it, only added a parameter - wonder what happened there? 

+[% INCLUDE 'doc-head-close.inc' popup => 1 %]
Comment 3 Claire Gravely 2018-08-21 07:21:28 UTC
Created attachment 78026 [details] [review]
Bug 21243: Regression: SRU mapping popup for bibliographic records is unstyled

This patch corrects a regression caused by Bug 21151: The include file
which adds CSS to the page was removed from the template.

To test, apply the patch and go to Administration -> Z39.50/SRU servers
-> New SRU server.

- Click the "Modify" button next to "SRU Search fields mapping"
- Verify that the pop-up window is correctly styled.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 4 Kyle M Hall 2018-08-21 12:01:04 UTC
Created attachment 78041 [details] [review]
Bug 21243: Regression: SRU mapping popup for bibliographic records is unstyled

This patch corrects a regression caused by Bug 21151: The include file
which adds CSS to the page was removed from the template.

To test, apply the patch and go to Administration -> Z39.50/SRU servers
-> New SRU server.

- Click the "Modify" button next to "SRU Search fields mapping"
- Verify that the pop-up window is correctly styled.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Katrin Fischer 2018-08-21 12:02:37 UTC
Is reverting bug 21151 and repushing it correctly also an option? It adds a missing div as well.
Comment 6 Nick Clemens 2018-08-21 13:17:19 UTC
(In reply to Katrin Fischer from comment #5)
> Is reverting bug 21151 and repushing it correctly also an option? It adds a
> missing div as well.

The div does seem to have been added, it was a typo during rebase that I deleted the line, I think we can just add it here

bug 13618 would make revert more difficult I think
Comment 7 Nick Clemens 2018-08-21 13:35:04 UTC
(In reply to Nick Clemens from comment #6)
> (In reply to Katrin Fischer from comment #5)
> > Is reverting bug 21151 and repushing it correctly also an option? It adds a
> > missing div as well.
> 
> The div does seem to have been added, it was a typo during rebase that I
> deleted the line, I think we can just add it here
> 
> bug 13618 would make revert more difficult I think

Looking again, I think that extra closing div is incorrect. We have a closing div in intranet-bottom.inc - with 21151 we have an extra closing tag (run the generated html through the wc3 validator)
Comment 8 Nick Clemens 2018-08-21 13:43:22 UTC
Created attachment 78044 [details] [review]
Bug 21243: (follow-up) Remove extra '</div>' tag

To test:
1 - Go to Admin->Z39/SRU servers
2 - Pick a server or add one
3 - Click 'Modify' button for mappings
4 - Copy the HTML of the popup into w3c validator
https://validator.w3.org/#validate_by_input
5 - Note extra closing div
6 - Apply patch
7 - Repeat
8 - Success
Comment 9 Katrin Fischer 2018-08-21 13:47:19 UTC
Ok, probably also fixed by adding SRU for authorities then. I had checked the validator at the time, but the other patches altered page structure.
Comment 10 Nick Clemens 2018-08-21 13:59:02 UTC
Awesome work all!

Pushed to master for 18.11
Comment 11 Martin Renvoize 2018-08-24 10:27:00 UTC
Pushed to 18.05.x for 18.05.03
Comment 12 Fridolin SOMERS 2018-09-21 10:11:30 UTC
Depends on Bug 21151 not in 17.11.x