Description
Chris Cormack
2011-03-28 21:02:42 UTC
Created attachment 3480 [details] [review] Proposed Patch This patch alters the scripts but lacks any change to preferences or the database. This bug is mentioned in: Bug 5979 : OPAC : using OPACISBD systempreference to display link http://lists.koha-community.org/pipermail/koha-patches/2011-March/014296.html The idea behind this patch is correct: there are some problems with isbd when you do some advanced things, as links are supposed to be replaced on the fly depending on you're on opac or staff. Also, one may want to have a different ISBD on staff and on opac. This patch is quite incomplete though. Affecting it to hdl again Created attachment 10029 [details] [review] Bug 5979 - Follow up: using OPACISBD systempreference to display link This follow up adds the OPACISBD syspref Created attachment 10326 [details] [review] [SIGNED-OFF] Bug 5979 : OPAC : using OPACISBD systempreference to display link The OPACISBD system preference is used in order to be able to have one ISBD for OPAC another one for intranet. But the link ISBD at OPAC was depending on the presence or absence of value in ISBD which is for intranet. This patch fixes the behaviour to depend on the correct system preference at OPAC Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 10327 [details] [review] [SIGNED-OFF] Bug 5979 - Follow up: using OPACISBD systempreference to display link Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Verified that new system preference is added cleanly and ISBD view can be configured independently for staff and OPAC using OPACISBD and viewISBD system preferences. OPACISBD exists in ru-RU and uk-UA language sysprefs only... very odd. In that context, it's a text area, rather than a boolean. It looks like this perference was added back around 3.00.05.001, but is not longer part of the default installation. The first patch is fine; it changes the logic to use OPACISBD in the opac. But getting that preference properly installed where it may not already be is something else. If OPACISBD syspref is not present in newer installs, we should create one that is congruent with the original intent. A YesNo type is not appropriate in this context Failed QA. Seems like OPACISBD was a syspref to configure an OPAC specific ISBD view, that's why it is a textarea. Why not using 2 syspref in OPAC like in intranet : ISBD and OPACISBD : ISBD config for intranet/OPAC viewISBD and OPACviewISBD : allow ISBD view in intranet/OPAC Can someone clarify the point of this bug? I'm not sure I understand, and it has been sitting Failed QA for nearly 3 years. Is the ISBD view even used today ? Since the XSLT are largely used for a customised view. I confirm that ISBD view (opac and intranet) is wide used here in Italy. The main user are librarians. Created attachment 44123 [details] [review] Bug 5979 - Follow up: using OPACISBD systempreference to display link Created attachment 44126 [details] [review] Bug 5979 - OPAC ISBD view has its own syspref Syspref OPACISBD is now a textarea so it's possible to have a different template in staff and opac. I didn't add a "OPACviewISBD", empty OPACISBD is enough for disable ISBD view in opac. Created attachment 44208 [details] [review] [SIGNED-OFF] Bug 5979 : OPAC : using OPACISBD systempreference to display link The OPACISBD system preference is used in order to be able to have one ISBD for OPAC another one for intranet. But the link ISBD at OPAC was depending on the presence or absence of value in ISBD which is for intranet. This patch fixes the behaviour to depend on the correct system preference at OPAC Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Verified that new system preference is added cleanly and ISBD view can be configured independently for staff and OPAC using OPACISBD and viewISBD system preferences. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 44209 [details] [review] [SIGNED-OFF] Bug 5979 - Follow up: using OPACISBD systempreference to display link Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Created attachment 44210 [details] [review] [SIGNED-OFF] Bug 5979 - OPAC ISBD view has its own syspref Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works well, empty OPACISBD removes link. Test pass. Small koha-qa error fixed in followup Created attachment 44211 [details] [review] Bug 5979: follow to fix spelling Changed Montains -> Mountains koha-qa complains!! Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> 1) Please copy the current ISBD into OPACISBD to keep the same behaviour as before the patch for an installation with a modified ISBD view. 2) Please amend the ISBD preference description to include "staff", so it's more clear about the new difference. 3) The tests output some warnings, please take a look to see if they can be fixed: ok 1 - use C4::Biblio; Use of uninitialized value $template in string eq at /home/katrin/kohaclone/C4/Biblio.pm line 926. Use of uninitialized value $template in string eq at /home/katrin/kohaclone/C4/Biblio.pm line 998. Use of uninitialized value $template in string eq at /home/katrin/kohaclone/C4/Biblio.pm line 1010. Use of uninitialized value $template in string eq at /home/katrin/kohaclone/C4/Biblio.pm line 998. Use of uninitialized value $template in string eq at /home/katrin/kohaclone/C4/Biblio.pm line 1010. Hm, also a question: if we have separate preferences, do we really still need 4 preferences? Could we have only 2 preferences and deactivate the feature by emptying them out? (In reply to Katrin Fischer from comment #22) > Hm, also a question: if we have separate preferences, do we really still > need 4 preferences? Could we have only 2 preferences and deactivate the > feature by emptying them out? Hello Katrin, We have 3 preferences. 2 for staff (ISBD, viewISBD) and 1 for OPAC (OPACISBD). I agree that we don't need 2 preferences for staff as we do the same job with 1 on opac. Hi Alex, you are right, sorry - I think I misread last night. I think deduplicating the 2 staff preferences would be good, but on a separate bug :) Does anyone known why this bug failed QA ? (In reply to Katrin Fischer from comment #21) > 1) Please copy the current ISBD into OPACISBD to keep the same behaviour as > before the patch for an installation with a modified ISBD view. Katrin, i think this is done in installer/data/mysql/atomicupdate/bug_5979-add_opacisbd_syspref.sql Are you ok with that ? Looks ok! Created attachment 48295 [details] [review] Bug 5979 - Follow up: Change ISBD syspref description and fix warning in t/Biblio/Isbd.t Comment on attachment 44210 [details] [review] [SIGNED-OFF] Bug 5979 - OPAC ISBD view has its own syspref Review of attachment 44210 [details] [review]: ----------------------------------------------------------------- ::: t/Biblio/Isbd.t @@ +26,5 @@ > + use_ok('C4::Biblio'); > +} > + > +my $dbh = C4::Context->dbh; > +# Start transaction Perhaps this is better suited for t/db_dependent? I was thinking perhaps INSERT IGNORE INTO ... not just INSERT INTO `systempreferences` (variable,value,options,explanation,type) SELECT 'OPACISBD', value, '70|10', 'Allows to define ISBD view in OPAC', 'Textarea' FROM `systempreferences` WHERE variable = 'ISBD'; Created attachment 49873 [details] [review] [SIGNED-OFF] Bug 5979: follow to fix spelling Changed Montains -> Mountains koha-qa complains!! Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 49874 [details] [review] [SIGNED-OFF] Bug 5979 - Follow up: Change ISBD syspref description and fix warning in t/Biblio/Isbd.t Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 49898 [details] [review] Bug 5979 : OPAC : using OPACISBD systempreference to display link The OPACISBD system preference is used in order to be able to have one ISBD for OPAC another one for intranet. But the link ISBD at OPAC was depending on the presence or absence of value in ISBD which is for intranet. This patch fixes the behaviour to depend on the correct system preference at OPAC Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Verified that new system preference is added cleanly and ISBD view can be configured independently for staff and OPAC using OPACISBD and viewISBD system preferences. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 49899 [details] [review] Bug 5979 - Follow up: using OPACISBD systempreference to display link Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 49900 [details] [review] Bug 5979 - OPAC ISBD view has its own syspref Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works well, empty OPACISBD removes link. Test pass. Small koha-qa error fixed in followup Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 49901 [details] [review] Bug 5979: follow to fix spelling Changed Montains -> Mountains koha-qa complains!! Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 49902 [details] [review] Attachment to Bug 5979 - Add separate OPACISBD system preference Bug 5979 - Follow up: Change ISBD syspref description and fix warning in t/Biblio/Isbd.t Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 49903 [details] [review] Bug 5979: Update INSERT IGNORE INTO statement Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Maybe this should be an enh? (In reply to Katrin Fischer from comment #40) > Maybe this should be an enh? I agree, because it adds a new system preference. Pushed to Master - Should be in the May 2016 Release. Thanks! (In reply to M. Tompsett from comment #41) > (In reply to Katrin Fischer from comment #40) > > Maybe this should be an enh? > > I agree, because it adds a new system preference. So be it ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '('OPACISBD','#100||{ 100a }{ 100b }{ 100c }{ 100d }{ 110a }{ 110b }{ 110c }{ 110' at line 536 (In reply to Brendan Gallagher from comment #44) > ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; check > the manual that corresponds to your MySQL server version for the right > syntax to use near '('OPACISBD','#100||{ 100a }{ 100b }{ 100c }{ 100d }{ > 110a }{ 110b }{ 110c }{ 110' at line 536 That's doing a fresh install... If anyone spots the problem before I do - please let me know - This is a candidate to get reverted... Thanks, Brendan Created attachment 50591 [details] [review] Bug 5979: Follow up on systempreferences.sql Missing comma, extra comma, and bad placement Pushed last patch from Mtompsett (thanks!) |