Bug 23980

Summary: [18.11] Non existent include prevents template display
Product: Koha Reporter: Andrew Isherwood <bugzilla>
Component: ILLAssignee: Andrew Isherwood <bugzilla>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: bgkriegel, hayleypelham, jonathan.druart, katrin.fischer, lucas
Version: 18.11   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
18.11.15
Attachments: Bug 23980: Remove non-existent include
Revert "Bug 23229: (QA follow-up) Fix indentation"
Revert "Bug 23229: Only load ILL table JS when needed"
Bug 23980: Revert "Bug 23229: (QA follow-up) Fix indentation"
Bug 23980: Revert "Bug 23229: Only load ILL table JS when needed"
Bug 23980: Revert "Bug 23229: (QA follow-up) Fix indentation"
Bug 23980: Revert "Bug 23229: Only load ILL table JS when needed"

Description Andrew Isherwood 2019-11-06 15:57:11 UTC
One of the patches done during the QA process on Bug 18589 moved the strings into a separate file "ill-list-table-strings.inc", an include for this file was added to "ill-requests.tt". This bug, being an enhancement, was not backported to 18.11.x.

Some months later, Bug 23529 made some changes to that template, which contained the include, which was by this time in master. Bug 23529 *was* backported to 18.11.x, along with the include. This breaks since the include is referring to a file that never made it into 18.11.x.

The original bug, 18589, was definitely an enhancement, so there's no point in trying to backport it. The fix is to just remove the include.
Comment 1 Andrew Isherwood 2019-11-06 15:58:17 UTC
Created attachment 95112 [details] [review]
Bug 23980: Remove non-existent include

This patch removes the erroneous include
Comment 2 Andrew Isherwood 2019-11-06 16:01:34 UTC
Test plan:
- Do not apply the patch
- Go to /cgi-bin/koha/ill/ill-requests.pl
=> TEST: Observe that you receive the error:
   "Template process failed: file error - ill-list-table-strings.inc: not found at ..."
- Apply the patch
- Refresh the page
=> TEST: Observe that you no longer receive the error
Comment 3 Andrew Isherwood 2019-11-06 16:33:49 UTC
Looking at this further, I have noticed that the request for:
[% Asset.js("js/ill-list-table.js") | $raw %]
is also incorrect since, in 18.11.x, the JS has not been moved out of the template. I will create another patch to address this
Comment 4 Andrew Isherwood 2019-11-06 16:43:22 UTC
OK, I *think* I know what's going on. Bug 23229 should never have been backported to 18.11.x since it contains patches that don't correspond with the ILL template in that version and they add file includes to files that don't exist in 18.11.x.

Fortunately, there have been no commits to ill-requests.tt since this was backported, so it should be just a case of reverting the commits.

I'll add Lucas to this, since he did the backport of 23229
Comment 5 Jonathan Druart 2019-12-11 09:42:41 UTC
Andrew, Rmaint for 18.11 is no longer Lucas.

I suggest you to provide the revert commits here with a test plan, and follow the usual workflow for integration.
Better than letting the RMaint dealing with that.
Comment 6 Andrew Isherwood 2019-12-11 10:58:46 UTC
Thanks for the heads up Jonathan, I'd actually forgotten about this as Lucas said he would revert, I guess he never got a chance.

"...follow the usual workflow for integration." - what would that be, apart from me creating the revert patches?
Comment 7 Andrew Isherwood 2019-12-11 11:11:32 UTC
Created attachment 96191 [details] [review]
Revert "Bug 23229: (QA follow-up) Fix indentation"

This reverts commit d4676ff4532f41abec87ba2d1128e3f219d9c920.
Comment 8 Andrew Isherwood 2019-12-11 11:11:35 UTC
Created attachment 96192 [details] [review]
Revert "Bug 23229: Only load ILL table JS when needed"

This reverts commit f95749a66bb5dad53fa2cf00224fc38f52c1e788.
Comment 9 Andrew Isherwood 2019-12-11 11:12:20 UTC
Test plan:
- Do not apply the patches
- Go to /cgi-bin/koha/ill/ill-requests.pl
=> TEST: Observe that you receive the error:
   "Template process failed: file error - ill-list-table-strings.inc: not found at ..."
- Apply the patches
- Refresh the page
=> TEST: Observe that you no longer receive the error
Comment 10 Jonathan Druart 2019-12-11 11:15:22 UTC
(In reply to Andrew Isherwood from comment #6)
> "...follow the usual workflow for integration." - what would that be, apart
> from me creating the revert patches?

Patch, submit, SO, ... pushed! :)
Comment 11 Andrew Isherwood 2019-12-11 11:16:42 UTC
Riiiiight, I assumed there was more to it for cases like this :-D
Comment 12 Bernardo Gonzalez Kriegel 2020-02-23 14:29:13 UTC
Hi, is this still valid?
I tested using 18.11.14 and error message from test plan does not show.
Comment 13 Andrew Isherwood 2020-02-24 11:32:00 UTC
Some weirdness going on here. The error is a result of the "jsinclude" block (which should not be there in 18.11, and which this bug removed) being parsed by TT. jsinclude blocks should only be parsed if the "footerjs" flag is set prior to the "jsinclude" block in the mark up. As far as I can see, the 18.11 version of this template has *never* included a footerjs flag, so I don't see how the jsinclude block can ever have been parsed and the resulting error displayed (scratches head).

So, you're right Bernardo, the error is not being displayed. However, the jsinclude block should not be present in the template because it references files that do not exist in 18.11. So we still need to get the patches in this bug merged.

I think the only test plan I can offer in that case is:

- Do not apply the patch
- View koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
- TEST: Verify that it includes a "jsinclude" block near the bottom of the template
- Apply the patch
- Re-open the template
- TEST: Verify that the "jsinclude" block is now gone
Comment 14 Bernardo Gonzalez Kriegel 2020-02-25 10:55:16 UTC
Created attachment 99567 [details] [review]
Bug 23980: Revert "Bug 23229: (QA follow-up) Fix indentation"

This reverts commit d4676ff4532f41abec87ba2d1128e3f219d9c920.

https://bugs.koha-community.org/show_bug.cgi?id=23980

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Fixed commit title
Comment 15 Bernardo Gonzalez Kriegel 2020-02-25 10:55:20 UTC
Created attachment 99568 [details] [review]
Bug 23980: Revert "Bug 23229: Only load ILL table JS when needed"

This reverts commit f95749a66bb5dad53fa2cf00224fc38f52c1e788.

https://bugs.koha-community.org/show_bug.cgi?id=23980

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Fixed commit title, no errors

Revised test plan:
- Do not apply the patch
- View koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
- TEST: Verify that it includes a "jsinclude" block near the bottom of the template
- Apply the patch
- Re-open the template
- TEST: Verify that the "jsinclude" block is now gone
Comment 16 Nick Clemens 2020-03-04 12:26:44 UTC
Created attachment 100109 [details] [review]
Bug 23980: Revert "Bug 23229: (QA follow-up) Fix indentation"

This reverts commit d4676ff4532f41abec87ba2d1128e3f219d9c920.

https://bugs.koha-community.org/show_bug.cgi?id=23980

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2020-03-04 12:26:47 UTC
Created attachment 100110 [details] [review]
Bug 23980: Revert "Bug 23229: Only load ILL table JS when needed"

This reverts commit f95749a66bb5dad53fa2cf00224fc38f52c1e788.

https://bugs.koha-community.org/show_bug.cgi?id=23980

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Fixed commit title, no errors

Revised test plan:
- Do not apply the patch
- View koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
- TEST: Verify that it includes a "jsinclude" block near the bottom of the template
- Apply the patch
- Re-open the template
- TEST: Verify that the "jsinclude" block is now gone

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Hayley Pelham 2020-03-09 22:13:25 UTC
Backported to 18.11.x for 18.11.15