Bug 5010 - Fix OPACBaseURL to include protocol
Summary: Fix OPACBaseURL to include protocol
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: M. Tompsett
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 6146 7770 (view as bug list)
Depends on:
Blocks: 6874 10988 13774 14111 14120 16031
  Show dependency treegraph
 
Reported: 2010-07-15 13:38 UTC by Owen Leonard
Modified: 2017-06-14 22:11 UTC (History)
18 users (show)

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


Attachments
Patch to fix and use OPACBaseURL on all send list and send basket pages (5.18 KB, patch)
2013-11-14 18:51 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Bug 5010: Send list emails with complete links back into the catalog (1.47 KB, patch)
2013-12-11 17:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5010: Fix incomplete links in carts and lists sent from the staff client (6.54 KB, patch)
2013-12-11 18:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 5010: Send list emails with complete links back into the catalog (1.60 KB, patch)
2014-03-06 00:48 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 5010: Send list emails with complete links back into the catalog (1.60 KB, patch)
2014-03-06 00:49 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 5010: Fix incomplete links in carts and lists sent from the staff client (6.90 KB, patch)
2014-03-06 00:49 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: Fix incomplete links in carts and lists sent from the staff client (9.80 KB, patch)
2014-04-12 01:01 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: Fix incomplete links in carts and lists sent from the staff client (10.56 KB, patch)
2014-04-14 05:33 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: Send list emails with complete links back into the catalog (1.58 KB, patch)
2014-04-22 17:48 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: Fix incomplete links in carts and lists sent from the staff client (5.96 KB, patch)
2014-04-22 17:48 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: Fix incomplete links in carts and lists sent from the staff client (10.29 KB, patch)
2014-04-22 17:48 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5010: Send list emails with complete links back into the catalog (1.67 KB, patch)
2014-04-22 18:50 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5010: Fix incomplete links in carts and lists sent from the staff client (6.04 KB, patch)
2014-04-22 18:50 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5010: (2) Fix incomplete links in carts and lists sent from the staff client (10.43 KB, patch)
2014-04-22 18:50 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 5010 - Follow up: Case typographical error (1.29 KB, patch)
2014-04-26 23:39 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5010 - Follow up: Case typographical error (1.52 KB, patch)
2014-04-29 18:31 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 5010: Send list emails with complete links back into the catalog (1.66 KB, patch)
2014-05-06 02:57 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: Fix incomplete links in carts and lists sent from the staff client (6.03 KB, patch)
2014-05-06 02:58 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: (2) Fix incomplete links in carts and lists sent from the staff client (10.41 KB, patch)
2014-05-06 02:58 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Follow up: Case typographical error (1.51 KB, patch)
2014-05-06 02:58 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (20.84 KB, patch)
2014-09-19 05:00 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (21.47 KB, patch)
2014-09-19 05:13 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (21.47 KB, patch)
2014-09-19 05:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (28.78 KB, patch)
2014-09-20 01:54 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (29.45 KB, patch)
2014-11-20 06:12 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (29.38 KB, patch)
2015-02-04 06:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (29.42 KB, patch)
2015-02-10 00:01 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (8.52 KB, patch)
2015-02-11 14:15 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.25 KB, patch)
2015-02-11 15:54 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.25 KB, patch)
2015-02-11 17:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.25 KB, patch)
2015-02-11 17:27 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.25 KB, patch)
2015-02-11 17:33 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.20 KB, patch)
2015-02-26 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.46 KB, patch)
2015-04-12 02:59 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.53 KB, patch)
2015-04-19 10:06 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.59 KB, patch)
2015-04-29 11:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5010: Fix - replace tab with spaces (1.14 KB, patch)
2015-04-29 11:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.41 KB, patch)
2015-05-05 00:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: Fix - replace tab with spaces (1.14 KB, patch)
2015-05-05 00:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010 - Fix OPACBaseURL to include protocol (35.31 KB, patch)
2015-05-10 00:49 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 5010: Fix - replace tab with spaces (1.14 KB, patch)
2015-05-10 00:49 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5010: Fix OPACBaseURL to include protocol (35.42 KB, patch)
2015-05-12 14:23 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5010: Fix - replace tab with spaces (1.33 KB, patch)
2015-05-12 14:24 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 5010: Fix OPACBaseURL to include protocol (35.51 KB, patch)
2015-05-21 11:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 5010: Fix - replace tab with spaces (1.69 KB, patch)
2015-05-21 11:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2010-07-15 13:38:48 UTC
If you send a cart or a list to an email address from the staff client, each record contains an incomplete link:

In the online catalog:
/cgi-bin/koha/opac-detail.pl?biblionumber=306

Assuming a link back to the OPAC is correct*, the scripts should use the OPACBaseURL variable to form a complete link.

* One could argue that a cart/list sent from the staff client should link back to the staff client, but I think there is enough of a potential that staff will be sending carts/lists to patrons that it should be a link to the OPAC.
Comment 1 Nicole C. Engard 2010-08-05 13:43:05 UTC
I agree - however many libraries don't fill that variable out.  Is there a way to grab it from the server?  I also agree that the staff cart links should also go to the opac.

If we want it to use the sys pref I can make the change.
Comment 2 Owen Leonard 2011-10-05 20:07:34 UTC
My fix for Bug 6957 will address this issue by hiding the link if the OPACBaseURL preference is not filled out.
Comment 3 Katrin Fischer 2012-04-07 18:18:21 UTC
This is still a problem on current master:

Mailing a cart from the staff client I my URLs look like:
/cgi-bin/koha/opac-detail.pl?biblionumber=11

Mailing a cart from the OPAC my URLs look like:
http://localhost/cgi-bin/koha/opac-detail.pl?biblionumber=11

staffClientBaseURL = localhost:8080
OpacBaseURL = localost
Comment 4 Maxime Pelletier 2013-11-14 18:07:49 UTC
A note that since ug 8368 the link is also broken in the OPAC for send cart and send list. "http://" was removed before the OPACBaseURL variable, so the link is now "localhost/cgi-bin/koha/opac-detail.pl?biblionumber=11"
Comment 5 Maxime Pelletier 2013-11-14 18:51:23 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2013-12-09 18:39:57 UTC
I have looked into this a bit, it seems that for the OPAC cart the problem does not exist and I think I found the reason in Auth.pm:

            OPACBaseURL               => ($in->{'query'}->https() ? "https://" : "http://") . $ENV{'SERVER_NAME'} .
                   ($ENV{'SERVER_PORT'} eq ($in->{'query'}->https() ? "443" : "80") ? '' : ":$ENV{'SERVER_PORT'}"),

I will try to provide an alternative patch trying to fix it using that logic, so we don't need to hardcode http:// or https:// into the templates.
Comment 7 Katrin Fischer 2013-12-09 22:00:08 UTC
Hm ok, as discussed on IRC: The solution in Auth.pm is not quite right either because it totally ignores the syspref setting. I think a combination of the http/https bit + checking the syspref could do it. For multiple OPACs you can now overwrite the OpacBaseURL in your apache configuration.
Comment 8 Katrin Fischer 2013-12-11 17:52:46 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-12-11 17:57:59 UTC
The first patch makes sure the URL in the list email is generated like 

everywhere else in the OPAC. The same method won't work for the intranet and the OPACBaseURL is not taken into account at all. 

This is what creates the OpacBaseURL in OPAC from Auth.pm:
(($in->{'query'}->https() ? "https://" : "http://") . $ENV{'SERVER_NAME'} . ($ENV{'SERVER_PORT'} eq ($in->{'query'}->https() ? "443" : "80") ? '' : ":$ENV{'SERVER_PORT'}"))),

Feedback and ideas welcome... the attached patch could still be a small improvement as it adds consistency and makes the link work.
Comment 10 Katrin Fischer 2013-12-11 18:42:51 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2013-12-11 18:47:12 UTC
NOTE: 2 last patches here are alternatives to the first patch, trying to avoid hardcoding the http:// into the templates.
Comment 12 M. Tompsett 2014-01-28 06:19:04 UTC
Comment on attachment 22945 [details] [review]
Patch to fix and use OPACBaseURL on all send list and send basket pages

Second set of patches looks more thorough. Testing them.
Comment 13 M. Tompsett 2014-03-06 00:08:23 UTC
I just realized that I would prefer the system preference to be right beside (above preferably) the OPACBaseURL system preference. Because, the text of the OPACBaseURL probably needs tweaking too.
Comment 14 M. Tompsett 2014-03-06 00:39:22 UTC
mtompset@ubuntu:~/kohaclone$ git bz apply 5010
Bug 5010 - Incomplete links in carts and lists sent from the staff client

23436 - Bug 5010: Send list emails with complete links back into the catalog
23457 - Bug 5010: Fix incomplete links in carts and lists sent from the staff client

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 5010: Send list emails with complete links back into the catalog
Applying: Bug 5010: Fix incomplete links in carts and lists sent from the staff client
/home/mtompset/kohaclone/.git/rebase-apply/patch:58: trailing whitespace.
    $dbh->do(q{INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES
/home/mtompset/kohaclone/.git/rebase-apply/patch:80: trailing whitespace.
              choices:
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
<stdin>:58: trailing whitespace.
    $dbh->do(q{INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES
<stdin>:80: trailing whitespace.
              choices:
warning: 2 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging basket/sendbasket.pl

Ugly but it applies. :)
Comment 15 M. Tompsett 2014-03-06 00:48:23 UTC Comment hidden (obsolete)
Comment 16 M. Tompsett 2014-03-06 00:49:20 UTC Comment hidden (obsolete)
Comment 17 M. Tompsett 2014-03-06 00:49:40 UTC Comment hidden (obsolete)
Comment 18 M. Tompsett 2014-03-06 03:51:58 UTC
Don't forget to read comment #13. :)
Comment 19 Katrin Fischer 2014-03-06 08:21:27 UTC
Afaik the prefs sort alphabetically within the groups, so putting it right above OPACBaseURL would not be possible without renaming.
Comment 20 M. Tompsett 2014-03-06 12:56:50 UTC
Okay, but you put it in a completely different section.
Comment 21 Kyle M Hall 2014-04-11 12:11:21 UTC
I have a few suggestions for this:
1) Use the Preference method of the Koha TT plugin instead of passing them in from the perl script. No real processing is done.
2) Change the new sys pref PublicCatalogUsesSSL to OpacProtocol and give it the two options with the values "http://" and "https://" instead of it being a yes/no. That way in the template we can use do [% Koha.Preference( 'OPACProtocol' ) %][% OPACBaseURL %] which will mean far fewer changes.
Comment 22 Katrin Fischer 2014-04-11 12:40:29 UTC
Hi Kyle,
good suggestions, thank you for your review :) - I hope I will find some time to improve the patches.
Comment 23 M. Tompsett 2014-04-11 12:50:26 UTC
I'll make the patches for you in a few hours, Katrin, if you would like.
Comment 24 Katrin Fischer 2014-04-11 18:30:35 UTC
Thx for the offer! I'd not be too unhappy if you took that item off my to do list :) Maybe leave a quick note here, when you get to it? So we don't try to fix it at the same time.
Comment 25 M. Tompsett 2014-04-11 19:00:43 UTC
Actually, I've started on it, but it may take me another 9 hours before I finish, test, and post it up.
Comment 26 M. Tompsett 2014-04-11 19:12:29 UTC
Comment on attachment 25878 [details] [review]
[SIGNED OFF] Bug 5010: Send list emails with complete links back into the catalog

Review of attachment 25878 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-sendshelf.pl
@@ -109,5 @@
>  
> -    if (C4::Context->preference('OPACBaseURL')){
> -          $template2->param( OPACBaseURL => C4::Context->preference('OPACBaseURL') );
> -    }
> -

Unfortunately, koha-tmpl/opac-tmpl/prog/en/includes/doc-head-close.inc is affected by the removal: <link rel="unapi-server" type="application/xml" title="unAPI" href="[% OPACBaseURL %]/cgi-bin/koha/unapi" />
Comment 27 M. Tompsett 2014-04-12 01:01:59 UTC Comment hidden (obsolete)
Comment 28 M. Tompsett 2014-04-12 01:24:12 UTC
Comment on attachment 27049 [details] [review]
Bug 5010: Fix incomplete links in carts and lists sent from the staff client

Review of attachment 27049 [details] [review]:
-----------------------------------------------------------------

::: basket/sendbasket.pl
@@ +102,5 @@
>  
> +    if (C4::Context->preference('OPACBaseURL')){
> +        my $protocol = C4::Context->preference('OpacProtocol') // 'http://';
> +        $template2->param( OPACBaseURL => $protocol . C4::Context->preference('OPACBaseURL') );
> +    }

koha-tmpl/intranet-tmpl/prog/en/modules/basket/sendbasketform.tt does not have OPACBaseURL in it. However, doc-head-close.inc does. This is a short-term fix, as the problem in doc-head-close.inc affects much more than just carts and lists.

::: koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/sendshelf.tt
@@ +35,4 @@
>  LCCN: [% BIBLIO_RESULT.lccn %][% END %][% IF ( BIBLIO_RESULT.url ) %]
>  URL : [% BIBLIO_RESULT.url %][% END %][% IF ( OPACBaseURL ) %]
>  In the online catalog:
> +    [% Koha.Preference( 'OPACProtocol' ) %][% OPACBaseURL %]/cgi-bin/koha/opac-detail.pl?biblionumber=[% BIBLIO_RESULT.biblionumber |url %][% END %]

Took Kyle's second suggestion from comment #21.

::: opac/opac-sendshelf.pl
@@ +109,5 @@
>  
> +    if (C4::Context->preference('OPACBaseURL')){
> +          $template2->param( OPACBaseURL => C4::Context->preference('OPACBaseURL') );
> +    }
> +

Oops. I think I let a bad piece in.

::: virtualshelves/sendshelf.pl
@@ +103,2 @@
>    if (C4::Context->preference('OPACBaseURL')){
> +        $template2->param( OPACBaseURL => C4::Context->preference('OPACBaseURL') );

This represents changing it back to what it was initially.

@@ -104,2 @@
>    if (C4::Context->preference('OPACBaseURL')){
> -        $template2->param( OPACBaseURL => $protocol . C4::Context->preference('OPACBaseURL') );

The protocol logic is moved to the template now. Only need the OPACBaseURL, which is not available by default for staff client pages.
Comment 29 M. Tompsett 2014-04-12 01:27:39 UTC
Comment on attachment 27049 [details] [review]
Bug 5010: Fix incomplete links in carts and lists sent from the staff client

Review of attachment 27049 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-sendshelf.pl
@@ +109,5 @@
>  
> +    if (C4::Context->preference('OPACBaseURL')){
> +          $template2->param( OPACBaseURL => C4::Context->preference('OPACBaseURL') );
> +    }
> +

Nope. Triple check shows this is what it was before. Removing probably does correct a problem, but it is unrelated to this bug (carts and lists in staff).
Comment 30 M. Tompsett 2014-04-14 05:33:57 UTC Comment hidden (obsolete)
Comment 31 Bernardo Gonzalez Kriegel 2014-04-22 10:52:35 UTC
Question:

On opac-sendbasket.pl you send 

if (C4::Context->preference('OPACBaseURL')){
  $template2->param( OPACBaseURL => C4::Context->preference('OPACBaseURL') );
}

and in virtualshelves/sendshelf.tt

[% Koha.Preference( 'OPACProtocol' ) %][% OPACBaseURL %]/cgi-bin/koha/opac...

Why not 

[% Koha.Preference('OPACProtocol') %][% Koha.Preference('OPACBaseURL') %]

?
Comment 32 M. Tompsett 2014-04-22 15:50:58 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #31)
> Question:
> 
> On opac-sendbasket.pl you send 
> 
> if (C4::Context->preference('OPACBaseURL')){
>   $template2->param( OPACBaseURL => C4::Context->preference('OPACBaseURL') );
> }
> 
> and in virtualshelves/sendshelf.tt
> 
> [% Koha.Preference( 'OPACProtocol' ) %][% OPACBaseURL %]/cgi-bin/koha/opac...
> 
> Why not 
> 
> [% Koha.Preference('OPACProtocol') %][% Koha.Preference('OPACBaseURL') %]
> 
> ?

Because the include file requires OPACBaseURL defined for the opac ones:
$ grep -l -i opacbaseurl `find . | grep en/.*inc$`
./koha-tmpl/opac-tmpl/prog/en/includes/masthead.inc
./koha-tmpl/opac-tmpl/prog/en/includes/doc-head-close.inc
./koha-tmpl/opac-tmpl/ccsr/en/includes/masthead.inc
./koha-tmpl/opac-tmpl/ccsr/en/includes/doc-head-close.inc
./koha-tmpl/opac-tmpl/bootstrap/en/includes/doc-head-close.inc
$ grep -i opacbaseurl ./koha-tmpl/opac-tmpl/prog/en/includes/doc-head-close.inc
<link rel="unapi-server" type="application/xml" title="unAPI" href="[% OPACBaseURL %]/cgi-bin/koha/unapi" />

This would explode into a wider scale fix required (lots more testing!). That's beyond the scope of this bug.

I believe the second one has potential merit, but my goal was to minimize impact without having to look for other areas affected. I have found the OPACBaseURL logic was added by bug 6957, but I haven't looked much further. Since I was leaving OPACBaseURL intact to minimize potential impact, accessing the Template variable is likely faster than the Preference call and it is shorter to type, I didn't change that. The goal of this bug is to add OpacProtocol.
Comment 33 Bernardo Gonzalez Kriegel 2014-04-22 16:33:51 UTC
Patch 2/3 has fixable conflicts
Patch 3/3 no, needs rebasing
Comment 34 M. Tompsett 2014-04-22 16:45:04 UTC
Bug 11248 triggered the need for a rebase.
Comment 35 M. Tompsett 2014-04-22 17:48:10 UTC Comment hidden (obsolete)
Comment 36 M. Tompsett 2014-04-22 17:48:30 UTC Comment hidden (obsolete)
Comment 37 M. Tompsett 2014-04-22 17:48:43 UTC Comment hidden (obsolete)
Comment 38 Bernardo Gonzalez Kriegel 2014-04-22 18:50:09 UTC Comment hidden (obsolete)
Comment 39 Bernardo Gonzalez Kriegel 2014-04-22 18:50:25 UTC Comment hidden (obsolete)
Comment 40 Bernardo Gonzalez Kriegel 2014-04-22 18:50:41 UTC Comment hidden (obsolete)
Comment 41 M. Tompsett 2014-04-26 23:39:52 UTC Comment hidden (obsolete)
Comment 42 Galen Charlton 2014-04-29 14:55:35 UTC
Marking as a candidate for 3.16.0.  Not a guarantee, however.
Comment 43 Bernardo Gonzalez Kriegel 2014-04-29 18:31:18 UTC Comment hidden (obsolete)
Comment 44 Bernardo Gonzalez Kriegel 2014-04-29 18:33:11 UTC
Solved small conflicts on updatedatabse.pl, but only upload last patch signed.
Comment 45 Jonathan Druart 2014-05-02 11:18:30 UTC
2 questions:
1/ Shouldn't we use this pref wherever OPACBaseURL is used?
2/ Shouldn't we put a warning in the updatedb entry?
Comment 46 Katrin Fischer 2014-05-02 11:46:25 UTC
I think the problem mostly is where we try to build the URL in staff, because there you can't get the information if the OPAC is run as https. There are other options used for links in the OPAC. Maybe this could be a first step?
Comment 47 M. Tompsett 2014-05-02 14:53:32 UTC
This is a first step. We fix one place, and if this is deemed sufficient, we go on to the rest. Ideally, this could be integrated into a function that will get used everywhere.

Yes, it would be a good idea to put a warning: "If you use HTTPS for your OPAC, then please set OpacProtocol to 'use' in the OPAC system preferences' Policy section." However, I don't have time to add that.
Comment 48 Galen Charlton 2014-05-05 22:11:48 UTC
I won't be including this in 3.16.0, as I think it needs to bake for a bit longer:

- do we /really/ need a new system preference when we could enforce making OPACBaseURL contain the protocol?
- is it clear that somebody email a cart from the staff interface wants the results to point to the OPAC, rather than the staff interface?
Comment 49 M. Tompsett 2014-05-06 02:09:13 UTC
(In reply to Galen Charlton from comment #48)
> - do we /really/ need a new system preference when we could enforce making
> OPACBaseURL contain the protocol?

Valid point. Just that if one only wants to fix a tiny piece at a time, this solution works. Enforcing the protocol is a much larger patch.


> - is it clear that somebody email a cart from the staff interface wants the
> results to point to the OPAC, rather than the staff interface?

One could email it to someone without staff access. I think it is best to email OPAC links, and not staff links. That is, I think the current behaviour is best. 

Now, that doesn't mean that they won't want the staff interface link, but since we do not know if the email address has staff access (we don't check, though I suppose we could), OPAC is quickest solution.

Perhaps a logged in user should have the staff URL in their OPAC details pages, if they have access?
Comment 50 M. Tompsett 2014-05-06 02:57:44 UTC Comment hidden (obsolete)
Comment 51 M. Tompsett 2014-05-06 02:58:10 UTC Comment hidden (obsolete)
Comment 52 M. Tompsett 2014-05-06 02:58:26 UTC Comment hidden (obsolete)
Comment 53 M. Tompsett 2014-05-06 02:58:42 UTC Comment hidden (obsolete)
Comment 54 M. Tompsett 2014-05-06 03:04:46 UTC
I have rebased, but the discussion on comment 48 and comment 49 should continue. Also, Katrin, I've bounced this back to you, as my development time available for Koha has drastically reduced.
Comment 55 Marcel de Rooy 2014-05-07 14:43:02 UTC
I would favor resolving this issue via OPACBaseURL and not create a new preference that only tells you about the protocol. The protocol should just be in OPACBaseURL. Smart code could assume that it is HTTP if this pref does not start with a protocol.

Changing status to In Discussion
Comment 56 Katrin Fischer 2014-05-07 15:16:47 UTC
Maybe we could take a step by step approach here? 

Currently we are sending out incomplete emails and making the way we create the links consistent, will take a lot more work.

We have code in Auth.pm that produces working links for the opac cart email, but doesn't use OpacBaseUrl. And the links in the opac list emails are broken.

The first patch was just trying to copy the logic from the cart email for the opac list email, so both would create the links the same way using the same lines of code. Maybe this could be still be a valid first step?
Comment 57 Marcel de Rooy 2014-05-08 07:25:16 UTC
(In reply to Katrin Fischer from comment #56)
> Maybe we could take a step by step approach here? 
> 
> Currently we are sending out incomplete emails and making the way we create
> the links consistent, will take a lot more work.
> 
> We have code in Auth.pm that produces working links for the opac cart email,
> but doesn't use OpacBaseUrl. And the links in the opac list emails are
> broken.
> 
> The first patch was just trying to copy the logic from the cart email for
> the opac list email, so both would create the links the same way using the
> same lines of code. Maybe this could be still be a valid first step?

I would not mind passing QA on the first patch, overriding opacbaseurl with the stuff from Auth.pm as a temporary measure. (Maybe adding a comment somewhere?)
But the lines in Auth.pm:
            OPACBaseURL               => ($in->{'query'}->https() ? "https://" : "http://") . $ENV{'SERVER_NAME'} .
                   ($ENV{'SERVER_PORT'} eq ($in->{'query'}->https() ? "443" : "80") ? '' : ":$ENV{'SERVER_PORT'}"),

should actually be adjusted when resolving the various OpacBaseURL bugs. Note that there was an issue too with the https function and Plack..
Comment 58 Katrin Fischer 2014-05-08 07:34:27 UTC
I totally agree that we need to figure out a proper way to generate the URLs that works in the various possible situations. I'd also not mind if we changed the code in Auth.pm - I think in the OPAC it should be possible to determine if https is being used. 

Maybe ist would be best to split off the OPAC problem into a separate bug now?
Comment 59 Marcel de Rooy 2014-05-08 07:36:41 UTC
(In reply to Katrin Fischer from comment #58)
> I totally agree that we need to figure out a proper way to generate the URLs
> that works in the various possible situations. I'd also not mind if we
> changed the code in Auth.pm - I think in the OPAC it should be possible to
> determine if https is being used. 
> 
> Maybe ist would be best to split off the OPAC problem into a separate bug
> now?

Looking forward to your proposal..
Comment 60 Katrin Fischer 2014-05-08 08:09:39 UTC
I am not sure I will be able to provide an idea for the final solution, but I can try and split out a bug for the OPAC change - but probably won't be today.
Comment 61 M. Tompsett 2014-05-08 12:09:24 UTC
(In reply to M. de Rooy from comment #57)
> But the lines in Auth.pm:
>             OPACBaseURL               => ($in->{'query'}->https() ?
> "https://" : "http://") . $ENV{'SERVER_NAME'} .
>                    ($ENV{'SERVER_PORT'} eq ($in->{'query'}->https() ? "443"
> : "80") ? '' : ":$ENV{'SERVER_PORT'}"),
> 
> should actually be adjusted when resolving the various OpacBaseURL bugs.
> Note that there was an issue too with the https function and Plack..

One could run their staff and OPAC clients on different ports. I'm not saying every combination is sane, but every combination is not accounted for with that code. This code does not account for the sane combination of https (staff) and http (OPAC), nor the strange combination of http (staff) and https (OPAC).

The best way, without the OpacProtocol system preference, is a full-out across the board fix the hard coded 'http' 'https' messes, and require the protocol in OPACBaseURL.

The problem with this solution is that it is much larger to implement, because if you put http(s) into your OPACBaseURL, you'll get http://http(s):// in locations you haven't fixed yet.

If Katrin can fix all the OPACBaseURLs, that would be great. :)
Comment 62 Marcel de Rooy 2014-05-08 12:26:08 UTC
(In reply to M. Tompsett from comment #61)
> One could run their staff and OPAC clients on different ports. I'm not
> saying every combination is sane, but every combination is not accounted for
> with that code. This code does not account for the sane combination of https
> (staff) and http (OPAC), nor the strange combination of http (staff) and
> https (OPAC).
I think that she wants to concentrate first on fixing the opac side of this problem: so only opac-sendshelf. In that case the protocol will hopefully be correct in most cases. Sending a wrong shelf list from staff may be a much smaller issue..

> The best way, without the OpacProtocol system preference, is a full-out
> across the board fix the hard coded 'http' 'https' messes, and require the
> protocol in OPACBaseURL.
Agreed. Like "adjust when resolving the various OpacBaseURL bugs"..
Comment 63 Katrin Fischer 2014-05-08 12:31:18 UTC
Marcel is right, I was offering to try the 'cheaper' solution, as I probably won't have time for the bigger fix. I think the suggested solution to include the protocol sounds good.
Comment 64 David Cook 2014-07-16 05:14:15 UTC
It's worth noting that bug 11575 has been pushed to master, so that might affect this bug.

The "OpacProtocol" system preference is probably a good idea. Heck, that might remove the need to do more fancy checking for HTTPS when using reverse proxies and/or Plack.
Comment 65 M. Tompsett 2014-07-16 15:16:48 UTC
Also should be noted a minor rebase is required before fixing.

I think it would be optimal if all the OPACBaseURLs passed to the templates were full paths including the https or http accordingly.

I am not so sure I think the OpacProtocol system preference idea is the best way to do it, but rather it should be in the OPACBaseURL system preference itself. We shouldn't be hardcoding http or https anywhere. We shouldn't need to check for http or https anywhere.

I feel the best solution is the most painful to do: fix every instance of OPACBaseURL use or hardcoded protocols to just use the OPACBaseURL. However, once done, it will just work. That is the way Koha should work.

Sadly, there are partial links in some of the included header templates which would cascade to a larger than simpler solution, as far as I recall from last time I checked. Honestly, someone needs to bite the bullet. I don't have time.
Comment 66 Katrin Fischer 2014-09-08 20:01:45 UTC
While I got distracted, Robin wrote a patch to fix the list email in OPAC - but we still need the real once and for all solution.
Comment 67 M. Tompsett 2014-09-08 21:01:22 UTC
Okay, okay. When I finally get around to upgrading our system, I'll take a whack at changing everything to expect the protocol in the OPACBaseURL, like it should be. So, unless someone beats me to it, I'll get on it.
Comment 68 Robin Sheat 2014-09-08 22:34:44 UTC
(In reply to M. Tompsett from comment #67)
> Okay, okay. When I finally get around to upgrading our system, I'll take a
> whack at changing everything to expect the protocol in the OPACBaseURL, like
> it should be. So, unless someone beats me to it, I'll get on it.

That is the way to do it, imo. There's technically no reason you couldn't have your opac at gopher://example.com, and it'd be good to handle that sort of craziness. Most things shouldn't require any changes, as they already expect that Auth.pm will attach the http(s) to the start anyway.
Comment 69 M. Tompsett 2014-09-19 01:55:11 UTC
Scope broadened from the original, "Incomplete links in carts and lists sent from the staff client"
Comment 70 M. Tompsett 2014-09-19 05:00:20 UTC Comment hidden (obsolete)
Comment 71 M. Tompsett 2014-09-19 05:13:15 UTC Comment hidden (obsolete)
Comment 72 M. Tompsett 2014-09-19 05:16:37 UTC Comment hidden (obsolete)
Comment 73 M. Tompsett 2014-09-20 01:54:58 UTC Comment hidden (obsolete)
Comment 74 M. Tompsett 2014-10-16 15:55:16 UTC
The _get_uri function will need to be amended in bug 8446, if this passes first, or a patch added afterwards if passes later.
Comment 75 Owen Leonard 2014-10-23 15:59:11 UTC
(In reply to M. Tompsett from comment #74)
> The _get_uri function will need to be amended in bug 8446, if this passes
> first, or a patch added afterwards if passes later.

So an additional patch is required here?
Comment 76 M. Tompsett 2014-10-24 00:01:56 UTC
(In reply to Owen Leonard from comment #75)
> So an additional patch is required here?

Yes, an additional patch is required here. I'm working on it.
Comment 77 M. Tompsett 2014-11-20 06:12:25 UTC Comment hidden (obsolete)
Comment 78 M. Tompsett 2015-01-13 07:20:11 UTC
Follow the test plan in comment #77. Though, I do hope to post instructions on how to install Test::DBIx::Class in Debian Jessie (yes, I said Jessie) for full testing.
Comment 79 M. Tompsett 2015-02-04 06:32:06 UTC Comment hidden (obsolete)
Comment 80 M. Tompsett 2015-02-10 00:01:23 UTC Comment hidden (obsolete)
Comment 81 M. Tompsett 2015-02-11 14:15:56 UTC Comment hidden (obsolete)
Comment 82 M. Tompsett 2015-02-11 15:54:04 UTC Comment hidden (obsolete)
Comment 83 M. Tompsett 2015-02-11 17:25:56 UTC Comment hidden (obsolete)
Comment 84 M. Tompsett 2015-02-11 17:27:15 UTC Comment hidden (obsolete)
Comment 85 M. Tompsett 2015-02-11 17:33:43 UTC Comment hidden (obsolete)
Comment 86 Jonathan Druart 2015-02-26 14:54:14 UTC Comment hidden (obsolete)
Comment 87 Jonathan Druart 2015-02-26 14:54:53 UTC
Last patch fixed an easy conflict on updatedatabase.pl
Comment 88 Jonathan Druart 2015-03-31 09:01:02 UTC
*** Bug 7770 has been marked as a duplicate of this bug. ***
Comment 89 M. Tompsett 2015-04-12 02:59:55 UTC Comment hidden (obsolete)
Comment 90 Brendan Gallagher 2015-04-19 04:04:24 UTC
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging catalogue/detail.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 5010 - Fix OPACBaseURL to include protocol
Comment 91 M. Tompsett 2015-04-19 10:06:53 UTC Comment hidden (obsolete)
Comment 92 Jonathan Druart 2015-04-29 11:32:04 UTC Comment hidden (obsolete)
Comment 93 Jonathan Druart 2015-04-29 11:32:13 UTC Comment hidden (obsolete)
Comment 94 Jonathan Druart 2015-04-29 11:33:37 UTC
Signing off but it needs another one, I did not really follow the test plan.
Comment 95 M. Tompsett 2015-05-05 00:46:26 UTC Comment hidden (obsolete)
Comment 96 M. Tompsett 2015-05-05 00:46:34 UTC Comment hidden (obsolete)
Comment 97 M. Tompsett 2015-05-10 00:49:38 UTC Comment hidden (obsolete)
Comment 98 M. Tompsett 2015-05-10 00:49:46 UTC Comment hidden (obsolete)
Comment 99 Bernardo Gonzalez Kriegel 2015-05-12 14:23:54 UTC Comment hidden (obsolete)
Comment 100 Bernardo Gonzalez Kriegel 2015-05-12 14:24:03 UTC Comment hidden (obsolete)
Comment 101 M. Tompsett 2015-05-12 20:06:02 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #100)
> Tested what I can, not plack/shibboleth/cas

For plack: https://github.com/digibib/kohadevbox
(Because I don't understand how to get it installed by default either)

For shibboleth: I figure the tests are good enough, but you need Test::DBIx::Class (which I believe is in the kohadevbox :) )

For cas: Again, I just assumed the tests were good enough.


> Perhaps this can pass and we can fix any problem later (for 3.22)

Yes, please. :)
Comment 102 M. Tompsett 2015-05-13 03:02:48 UTC
(In reply to M. Tompsett from comment #101)
> For shibboleth: I figure the tests are good enough, but you need
> Test::DBIx::Class (which I believe is in the kohadevbox :) )

Debug and make sense of this to get it in.

sudo apt-get install dh-make-perl libdata-visitor-perl libdbix-class-timestamp-perl libdatetime-format-sqlite-perl libdatetimex-easy-perl libtype-tiny-perl libyaml-tiny-perl libmoox-handlesvia-perl libtest-fatal-perl libclass-dbi-perl libmoosex-types-path-class-perl libmoosex-types-json-perl libmoosex-types-loadableclass-perl libtime-piece-mysql-perl libmath-base36-perl libcpanel-json-xs-perl libclass-accessor-lite-perl libfile-copy-recursive-perl libtest-sharedfork-perl libtest-deep-perl libdata-uuid-perl libdbix-class-uuidcolumns-perl libtest-differences-perl libdbd-mysql mysql-server libdatetime-format-mysql-perl

cpan2deb MooseX::Attribute::ENV
cpan2deb DBD::SQLite
cpan2deb MooX::HandlesVia
cpan2deb DBIx::Class::Schema::PopulateMore
cpan2deb Test::mysqld
cpan2deb Test::DBIx::Class
Comment 103 M. Tompsett 2015-05-13 16:22:15 UTC
I may have gotten the Depends on direction wrong for the:
prove t/Auth_with_shibboleth.t

I retested all this yesterday on the kohadev box.
I ended up making the same sorts of changes that are in bug 14111 in order for it to pass the t/Auth_with_shibboleth.t tests.
Comment 104 M. Tompsett 2015-05-16 00:04:56 UTC
(In reply to M. Tompsett from comment #103)
> I ended up making the same sorts of changes that are in bug 14111 in order
> for it to pass the t/Auth_with_shibboleth.t tests.

Specifically, I applied the second patch of bug 14111 in order to ensure prove t/Auth_with_shibboleth.t did not fail.
Comment 105 Marcel de Rooy 2015-05-21 11:36:50 UTC
Created attachment 39358 [details] [review]
Bug 5010: Fix OPACBaseURL to include protocol

First, it is strongly recommended to set the OPACBaseURL. But
this patch allows the inclusion of the protocol and not just
a site.

Next, C4/Auth now puts OPACBaseURL into the template parameters
regardless of OPAC or Staff clients.  t/db_dependent/Auth.t was
tweaked to add a check for confirming that get_template_and_user
adds OPACBaseURL to both OPAC and Staff templates.

In the staff client, once the OPACBaseURL is set, you get a nice
OPAC View link when viewing a biblio's detail. It should reflect
the protocol used now.

Hard coded 'http://' strings were removed from the
sample_notices.sql files. This is what required also updating
the letters table in the updatedatabase.pl script.

The explanation text in the sysprefs.sql needed updating too to
reflect the inclusion of the protocol. And this was the other
update done in the updatedatabase.pl script. The opac.pref file
was similarly changed as well.

catalogue/detail.pl had no need to pass a custom OpacUrl value,
since C4/Auth passes the required OPACBaseURL, so it and the
corresponding template were modified.

Both the MARC21 and NORMARC intranet details files had 'http://'
hard coded in them. This was removed.

Both the bootstrap and prog theme opac-detail template had a
protocol parameter that was used. The logic for the parameter
was not removed, because it is used extensively in one template.
Perhaps it should be used to simplify the other. However, the
calculated current_url parameter had references to the protocol
removed, because of the changes to OPACBaseURL.

opac/opac-shareshelf.pl had a hard coded 'http://' which was
removed.

t/db_dependent/Auth_with_cas.t had 'http://' added to the value
set for OPACBaseURL.

In virtualshelves/sendshelf.pl explicit code which sent the
OPACBaseURL preference was removed, since C4/Auth sends it all
the time now.

C4::Context::set_preference was tweaked to ensure that
OPACBaseURL would always start with http.
t/db_dependent/Context.t was tweaked to specifically test this.

The Shibboleth authentication needs OPACBaseURL set, and that
it be https protocol. The _get_uri routine was tweaked to always
pass back https:// as the protocol on the OPACBaseURL.
t/Auth_with_shibboleth.t was tweaked to specifically test the
changes.

TEST PLAN
---------
This is not an easy patch to test. Difficulties include:
- configuring Koha to run under https
    (tweaking apache2 isn't so hard, just tricky)
- configuring Koha to run OPAC and Staff with Plak
    (since code with comments about plak were sliced out)
- configuring Koha to use CAS
    (may be requires for the CAS test)

 1) Apply patch
 2) Make sure OPACBaseURL is set without the protocol included.

UPDATEDATABASE
 3) back up your DB
 4) ./installer/data/mysql/updatedatabase.pl
    -- It should run without errors.
 5) Look up the OPACBaseURL system preference in the staff
    client
    -- It should have http:// prepended.
 6) Run the mysqlclient from your koha git directory
      USE koha_library;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
    -- There should be no prepended http:// on the
       <<OPACBaseURL>>.
 7) restore your DB
 8) Make sure OPACBaseURL is set with the protocol included,
    preferably https.
    -- Using https requires a bunch of apache2 tweaks.

AUTH
 9) Call up staff client.
10) Call up OPAC.
    -- C4/Auth.pm doesn't barf.
11) Call up Plack staff client
12) Call up Plack OPAC.
    -- C4/Auth.pm doesn't barf.
13) prove -v t/db_dependent/Auth.t

CONTEXT
14) Home -> Koha administration -> Global System Preferences
         -> OPAC
15) Modify and save OPACBaseURL to not have http:// or https://
    on it.
    -- It should be modified to include http://
16) Modify and save another system preference.
    -- It should save normally
17) prove -v t/db_dependent/Context.t

CATALOGUE/DETAIL (tt & pl)
18) Confirm the OPACBaseURL is set
19) Navigate to any biblio details in the staff client
    -- There should be a "OPAC view" link which has the
       correct http:// or https:// in it.

SQL (sample notices and sysprefs)
20) Run the mysqlclient from your koha git directory
      USE koha_library;
      DELETE FROM letter;
      source installer/data/mysql/de-DE/mandatory/sample_notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
      DELETE FROM letter;
      source installer/data/mysql/en/mandatory/sample_notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
      DELETE FROM letter;
      source installer/data/mysql/nb-NO/1-Obligatorisk/sample_notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
      DELETE FROM letter;
      source installer/data/mysql/es-ES/mandatory/sample_notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
      DELETE FROM letter;
      source installer/data/mysql/fr-FR/1-Obligatoire/sample_notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
      DELETE FROM letter;
      source installer/data/mysql/it-IT/necessari/notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
      DELETE FROM letter;
      source installer/data/mysql/pl-PL/mandatory/sample_notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
      DELETE FROM letter;
      source installer/data/mysql/ru-RU/mandatory/sample_notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
      DELETE FROM letter;
      source installer/data/mysql/uk-UA/mandatory/sample_notices.sql;
      SELECT content FROM letter WHERE content LIKE "%<<OPACBaseURL>>%";
    -- Each of the selects should should lines that have
       <<OPACBaseURL>> starting them, but no hard-coded http://
      DELETE FROM systempreferences;
      source installer/data/mysql/sysprefs.sql;
      SELECT * FROM systempreferences WHERE variable='OPACBaseURL';
    -- The explanation should reflect the new explanation.
      QUIT
21) restore your DB
22) Make sure OPACBaseURL is set with the protocol included,
    preferably https.
    -- Using https requires a bunch of apache2 tweaks.

SLIM2INTRANETDETAIL
23) Set 'XSLTDetailsDisplay' system preference to default.
24) Set 'marcflavour' system preference to MARC21.
25) View any biblio's details.
    -- the URL beside 'OPAC View' should have the appropriate
       http:// or https://
26) Set 'marcflavour' system preference to NORMARC.
27) View any biblio's details.
    -- the URL beside 'OPAC View' should have the appropriate
       http:// or https://

OPAC-DETAIL
28) Set 'opacthemes' to bootstrap.
29) Set 'SocialNetworks' to enabled.
30) In OPAC, view any biblio's details.
    -- the Share links should have the appropriate protocol on
       the OPACBaseURL.
31) Set 'opacthemes' to prog.
32) In OPAC, view any biblio's details.
    -- the Share links should have the appropriate protocol on
       the OPACBaseURL.

AUTH_WITH_CAS
33) prove -v t/db_dependent/Auth_with_cas.t

OPAC-SHARESHELF
34) Set 'OpacAllowSharingPrivateLists' to allow.
35) In OPAC, 'Save to Lists' a search result.
36) Save it to a new private list.
37) Click the Lists button, and select the new list.
38) Click the Share button.

AUTH_WITH_SHIBBOLETH
39) prove -v t/Auth_with_shibboleth.t
    -- needs to be tests on Debian, because I can't get
       the Test::DBIx::Class installed in Ubuntu. :(

Rebased again on kohadevbox...

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 106 Marcel de Rooy 2015-05-21 11:36:57 UTC
Created attachment 39359 [details] [review]
Bug 5010: Fix - replace tab with spaces

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
No errors.

Tested what I can, not plack/shibboleth/cas

Perhaps this can pass and we can fix any problem later (for 3.22)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 107 Marcel de Rooy 2015-05-21 11:43:40 UTC
QA Comment:
Great work, Mark. This took some time :)
Code looks good to me. No complaints from qa tools.
I understood that Martin tested the shibboleth case.
I did not test cas or plack. Altough I do not expect something there when looking at the code, I leave it to RM to request additional tests. A very early push of this patch in 3.22 could perhaps resolve that btw..

Some small remarks:
+        if ($oldprotocol ne 'https') {
+            $debug
+                and warn
+                  'Shibboleth requires OPACBaseURL to use the https protocol!';
+        }
Perhaps we could argue here that the debug flag is not needed. Just warn.

C4/Context.pm:
+    # force explicit protocol on OPACBaseURL
+    if ($var eq 'opacbaseurl' && substr($value,0,4) !~ /http/) {
+        $value = 'http://' . $value;
+    }
This is bit more of a philosophical nature: Should we enforce it and make users lazy? Or should we enforce it differently: Perhaps with a select combo with protocol? Or in javascript? Note that if you now save the pref, you still have a wrong value. Should we depend on modules to correct pref values? I would be more in favor of correcting it earlier in the process. (No blocker)

What about staffClientBaseURL now?  :-)
svc/import_bib:    my $url = 'http://'. C4::Context->preference('staffClientBaseURL') .'/cgi-bin/koha/catalogue/detail.pl?biblionumber='. $biblionumber;
[This pref seems to be the easier case.]

Passed QA
Comment 108 Marcel de Rooy 2015-05-21 11:49:09 UTC
Additionally: I git grepped on several case combinations of OPACBaseUrl and found this exception:

t/db_dependent/Auth_with_cas.t contains OpacBaseURL (NOK)
misc/release_notes/release_notes_3_10_0.txt contains OPACBaseurl (OK)

No big deal, noted for the record..
Comment 109 Tomás Cohen Arazi 2015-06-04 13:18:01 UTC
Patches pushed to master.

Thanks Mark and everyone involved in QA, this one has been a big effort for everyone involved!
Comment 110 M. Tompsett 2015-06-04 15:53:18 UTC
(In reply to Tomás Cohen Arazi from comment #109)
> Patches pushed to master.
> 
> Thanks Mark and everyone involved in QA, this one has been a big effort for
> everyone involved!

*whew* with Bug 13967 getting pushed, I was worried I'd need to rebase this. :)
Comment 111 Chris Cormack 2015-06-07 07:50:51 UTC
Pushed to 3.20.x will be in 3.20.1
Comment 112 Liz Rea 2015-06-08 04:41:42 UTC
Hi, 

This patch does not apply cleanly to 3.18.x, if you feel it should be included, please rebase for 3.18.x.

Cheers,
Liz
Comment 113 M. Tompsett 2015-10-19 22:55:42 UTC
*** Bug 6146 has been marked as a duplicate of this bug. ***