Description
Zeno Tajoli
2016-04-30 06:04:23 UTC
Created attachment 66025 [details] [review] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' Test plan: 1) Insert some value to staffBaseURL preference, without starting http:// 2) Apply the patch 3) Update database 4) Go to system preferences adminsitration, find the staffBaseURL preference, the inserted value should be prepended with 'http://' and the comment should be "This should be a complete URL, starting with http:// or https://. Do not include a trailing slash in the URL. (This must be filled in correctly for CAS, svc, and load_testing to work.)" 5) prove t/db_dependent/Auth_with_cas.t 6) prove t/db_dependent/selenium/basic_workflow.t 7) prove t/db_dependent/check_sysprefs.t Glad to see this patch - When using CAS for example it's already required that you add https:// in order to make things work right. Created attachment 66143 [details] [review] [SIGNED OFF] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' Test plan: 1) Insert some value to staffBaseURL preference, without starting http:// 2) Apply the patch 3) Update database 4) Go to system preferences adminsitration, find the staffBaseURL preference, the inserted value should be prepended with 'http://' and the comment should be "This should be a complete URL, starting with http:// or https://. Do not include a trailing slash in the URL. (This must be filled in correctly for CAS, svc, and load_testing to work.)" 5) prove t/db_dependent/Auth_with_cas.t 6) prove t/db_dependent/selenium/basic_workflow.t 7) prove t/db_dependent/check_sysprefs.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 66712 [details] [review] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' Test plan: 1) Insert some value to staffBaseURL preference, without starting http:// 2) Apply the patch 3) Update database 4) Go to system preferences adminsitration, find the staffBaseURL preference, the inserted value should be prepended with 'http://' and the comment should be "This should be a complete URL, starting with http:// or https://. Do not include a trailing slash in the URL. (This must be filled in correctly for CAS, svc, and load_testing to work.)" 5) prove t/db_dependent/Auth_with_cas.t 6) prove t/db_dependent/selenium/basic_workflow.t 7) prove t/db_dependent/check_sysprefs.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> What about the following query instead? UPDATE systempreferences SET value = CONCAT('http://', value) WHERE variable = 'staffClientBaseURL' AND value <> "" AND value NOT LIKE '^http%'; staffClientBaseURL is not supposed to be empty, but it will avoid to have it set to "http://" Created attachment 67268 [details] [review] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' Test plan: 1) Insert some value to staffBaseURL preference, without starting http:// 2) Apply the patch 3) Update database 4) Go to system preferences adminsitration, find the staffBaseURL preference, the inserted value should be prepended with 'http://' and the comment should be "This should be a complete URL, starting with http:// or https://. Do not include a trailing slash in the URL. (This must be filled in correctly for CAS, svc, and load_testing to work.)" 5) prove t/db_dependent/Auth_with_cas.t 6) prove t/db_dependent/selenium/basic_workflow.t 7) prove t/db_dependent/check_sysprefs.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 67269 [details] [review] Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference Adds a condition to check that the staffClientBaseUrl is not empty before updating it with 'http://'. Created attachment 67270 [details] [review] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' Test plan: 1) Insert some value to staffBaseURL preference, without starting http:// 2) Apply the patch 3) Update database 4) Go to system preferences adminsitration, find the staffBaseURL preference, the inserted value should be prepended with 'http://' and the comment should be "This should be a complete URL, starting with http:// or https://. Do not include a trailing slash in the URL. (This must be filled in correctly for CAS, svc, and load_testing to work.)" 5) prove t/db_dependent/Auth_with_cas.t 6) prove t/db_dependent/selenium/basic_workflow.t 7) prove t/db_dependent/check_sysprefs.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 67271 [details] [review] Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference Adds a condition to check that the staffClientBaseUrl is not empty before updating it with 'http://'. I like Jonathan's idea and have written a follow-up for it. The change is tiny, so pushing it back to QA. C4::Context has a OPACBaseURL change to make sure a protocol is prepended. Probably should do something similar for staffBaseUrl. Also, are there any letters/noticed which might be affected? I don't see how this change would affect notices or letters. When I tested this initially I checked all occurrences of the preference had been taken into account. Created attachment 67918 [details] [review] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' Test plan: 1) Insert some value to staffBaseURL preference, without starting http:// 2) Apply the patch 3) Update database 4) Go to system preferences adminsitration, find the staffBaseURL preference, the inserted value should be prepended with 'http://' and the comment should be "This should be a complete URL, starting with http:// or https://. Do not include a trailing slash in the URL. (This must be filled in correctly for CAS, svc, and load_testing to work.)" 5) prove t/db_dependent/Auth_with_cas.t 6) prove t/db_dependent/selenium/basic_workflow.t 7) prove t/db_dependent/check_sysprefs.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 67919 [details] [review] Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference Adds a condition to check that the staffClientBaseUrl is not empty before updating it with 'http://'. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (In reply to Katrin Fischer from comment #12) > I don't see how this change would affect notices or letters. When I tested > this initially I checked all occurrences of the preference had been taken > into account. Are there any letters with something like <<staffClientBaseURL>>? Because OPACBaseURL had some with http:// in front for some letters/notices. I vaguely remember checking and not finding anything, but that is how. So Passed QA is fine with me. (In reply to M. Tompsett from comment #15) > (In reply to Katrin Fischer from comment #12) > Are there any letters with something like <<staffClientBaseURL>>? I didn't see anything in my check, doesn't mean a library isn't using it in their custom notices, but I think it is a minor local adjustment if needed Created attachment 67967 [details] [review] Bug 16401: Add a warning if letter.content matches staffClientBaseURL Pushed to master for 17.11, thanks to everybody involved! |