Bug 6636

Summary: a syspref to display 'powered by Koha' text to OPAC footer
Product: Koha Reporter: Mason James <mtj>
Component: OPACAssignee: Mason James <mtj>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: enhancement    
Priority: PATCH-Sent (DO NOT USE) CC: chris, katrin.fischer, m.de.rooy, nengard, oleonard, paul.poulain, robin
Version: 3.6   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: patch
a little formatting tweak...
screenshot
new patch
revised patch, with updatedatabase.pl SQL added.
revised patch, with koha-version.pl edit removed
revised patch
patch
patch
Bug 6636 - add 'powered by Koha' text to OPAC footer
Bug 6636 - add 'powered by Koha' text to OPAC footer
Bug 6636 - add 'powered by Koha' text to OPAC footer

Description Mason James 2011-07-27 02:01:07 UTC
a syspref to add 'powered by Koha' text to OPAC footer
Comment 1 Mason James 2011-07-27 02:12:05 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2011-07-27 02:19:41 UTC
Mason,

I have some questions:

#1 where will this appear - above or below opacfooter?
#2 why not allow for choosing to show the koha logo or the text? 

Nicole
Comment 3 Mason James 2011-07-27 02:39:01 UTC
patch applied to commit 68f99cdf980222dcb557667925bd6024640cda2a
Comment 4 Mason James 2011-07-27 03:53:39 UTC Comment hidden (obsolete)
Comment 5 Mason James 2011-07-27 03:59:07 UTC
Created attachment 4749 [details]
screenshot
Comment 6 Mason James 2011-07-27 04:00:55 UTC
(In reply to comment #2)
> Mason,
> 
> I have some questions:
> 
> #1 where will this appear - above or below opacfooter?

below - take a look at my screenshot

> #2 why not allow for choosing to show the koha logo or the text? 

that will be the next patch, after this patch is accepted ;)
Comment 7 Katrin Fischer 2011-07-27 19:53:29 UTC
Hi Mason, 

I like this idea!
I tested with the new syspref off/on and languagechooser off/on.

Some things are missing from the patch:

- updatedatabase for existing installatons
- translates systempreferences sql files

I am marking failed QA because of those 2 problems.

I also have some suggestions:

- make the link open in a new window
- make 'powered by Koha' a link, instead of only Koha

I am not sure about having the css directly in the anchor tag. This way it's hard to customize it, like changing the font size or adding a picture. 
Perhaps having another spot for cutomizable HTML code (like opaccredits) with a default value would work too?
Comment 8 Mason James 2011-07-27 22:32:37 UTC
> Some things are missing from the patch:
> 
> - updatedatabase for existing installatons

fyi: an updatedatabase.pl entry is not needed here - 
the default behaviour for this feature is OFF. when the feature set to ON, the 'OpacKohaUrl' row is created in the systempreferences table

so... no updatedatabase.pl is needed ;)

> - translates systempreferences sql files

my new patch adds these...
Comment 9 Mason James 2011-07-27 22:39:34 UTC Comment hidden (obsolete)
Comment 10 Mason James 2011-07-27 22:41:45 UTC
applied to patch d4b397feb41fe6b0cb8672d8c002b1b0f709b097

http://git.kohaaloha.com/?p=head-dev/.git;a=commit;h=a8f0a96f7cda17fb6cb00460b4166c9fe697e262
Comment 11 Mason James 2011-07-27 22:56:38 UTC
(In reply to comment #7)
> Hi Mason, 
> 
> I like this idea!
> I also have some suggestions:
> - make 'powered by Koha' a link, instead of only Koha

my patch does this already...

> 
> I am not sure about having the css directly in the anchor tag. This way it's
> hard to customize it, like changing the font size or adding a picture. 
> Perhaps having another spot for cutomizable HTML code (like opaccredits) with a
> default value would work too?

these are great ideas, lets do them after this patch gets accepted...
Comment 12 Katrin Fischer 2011-07-28 08:12:51 UTC
I brought this up on IRC, because I was not sure about the updatedatabase. Like you said it will be added to the systempreferences table on first change, but consensus was that updating the database makes sense for a couple of reasons:

(09:53:02) paul_p: kf : I agree with rangi = the problem with "no updatedatabase for syspref" is also that it's not created until you explicitly save something. It will work, but may cause trouble sometimes (SQL request to see what's the value of the syspref for example)

(09:47:16) rangi: he is right, the way the code works, if it is in the .pref file, it will get created when changed, but
(09:47:40) rangi: the update is good because it tells people a new syspref has been added
Comment 13 Mason James 2011-07-31 23:37:23 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2011-08-04 07:51:09 UTC
I am very sorry, but your patch is changing kohaversion.pl
If you are adding a syspref, please add a section with subversion XXX to updatedatabase.pl.
The RM updates kohaversion.pl later on.
Comment 15 Mason James 2011-08-04 22:19:29 UTC Comment hidden (obsolete)
Comment 16 Mason James 2011-08-04 22:21:49 UTC
(In reply to comment #14)
> I am very sorry, but your patch is changing kohaversion.pl
this is now corrected in a newer patch

> If you are adding a syspref, please add a section with subversion XXX to
> updatedatabase.pl.
this is now corrected in a newer patch

> The RM updates kohaversion.pl later on.
ok, thanks for this info
Comment 17 Nicole C. Engard 2011-08-05 11:51:27 UTC
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: add 'powered by Koha' text to OPAC footer
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Auth.pm
Auto-merging installer/data/mysql/de-DE/mandatory/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/de-DE/mandatory/sysprefs.sql
Auto-merging installer/data/mysql/en/mandatory/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/sysprefs.sql
Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql
Auto-merging installer/data/mysql/it-IT/necessari/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/it-IT/necessari/sysprefs.sql
Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/nb-NO/1-Obligatorisk/sysprefs.sql
Auto-merging installer/data/mysql/pl-PL/mandatory/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/pl-PL/mandatory/sysprefs.sql
Auto-merging installer/data/mysql/ru-RU/mandatory/system_preferences_full_optimal_for_install_only.sql
CONFLICT (content): Merge conflict in installer/data/mysql/ru-RU/mandatory/system_preferences_full_optimal_for_install_only.sql
Auto-merging installer/data/mysql/uk-UA/mandatory/system_preferences_full_optimal_for_install_only.sql
CONFLICT (content): Merge conflict in installer/data/mysql/uk-UA/mandatory/system_preferences_full_optimal_for_install_only.sql
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Failed to merge in the changes.
Patch failed at 0001 add 'powered by Koha' text to OPAC footer
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
nengard@debian:~/kohaclone$
Comment 18 Mason James 2011-08-06 00:15:17 UTC Comment hidden (obsolete)
Comment 19 Mason James 2011-08-06 00:17:01 UTC
(In reply to comment #18)
> Created attachment 4873 [details] [review]
> revised patch

applied to commit 240edb6c0da80d3d932442ccd96f9f98e31260c3
 '5660 Webinstaller should check if KohaOpacLanguage exists in staff'
Comment 20 Mason James 2011-08-06 00:21:55 UTC
(In reply to comment #18)
> Created attachment 4873 [details] [review]
> revised patch

patch emailed to patches@kc.org
Comment 21 Marcel de Rooy 2011-08-10 11:52:21 UTC
Owen, 
I added you to the CC list of this report.
Patch looks good to me, except from the CSS related issues on the Powered by Koha footer. I guess it would be better to move the hardcoded style stuff to the css. I also noticed that the horizontal line above the language bar disappeared when opaclanguagesdisplay pref was disabled. 

Could you have a look?
Thanks,
Marcel
Comment 22 Marcel de Rooy 2011-08-10 12:09:58 UTC
Just two small extra remarks:
1) www.koha-community.org should be: koha-community.org
2) the <a> link should open in a new window; add target="_blank" attribute
Comment 23 Owen Leonard 2011-08-10 19:59:30 UTC
I agree that the CSS should be handled in opac.css. The <span> already has an id, you can use that. Doing this will eliminate the other error I see, an all-caps STYLE attribute. It has to be lower-case in order to be valid XHTML.

I disagree that the link should open a new window. If we really want to get technical about it, we could wrap it in a check of OPACURLOpenInNewWindow. Otherwise I say leave it.
Comment 24 Mason James 2011-08-23 03:31:03 UTC
(In reply to comment #21)
> Owen, 
> I added you to the CC list of this report.
> Patch looks good to me, except from the CSS related issues on the Powered by
> Koha footer. I guess it would be better to move the hardcoded style stuff to
> the css. 

yeah, i agree - i'll resumbit with that fix

I also noticed that the horizontal line above the language bar
> disappeared when opaclanguagesdisplay pref was disabled. 

yep, this was a deliberate 'feature' :) 
i'll removed it, as it introduces some unnecessary duplication to the template file
Comment 25 Mason James 2011-08-23 03:34:32 UTC Comment hidden (obsolete)
Comment 26 Mason James 2011-08-23 03:37:22 UTC
(In reply to comment #22)
> Just two small extra remarks:
> 1) www.koha-community.org should be: koha-community.org

i agree :) fixed in my latest patch 

fyi, it looks like the canonical url for kc.org has ?recently? changed from 
www.koha-community.org to koha-community.org (which is a good thing, to me)
Comment 27 Mason James 2011-08-23 04:00:40 UTC
(In reply to comment #23)
> I agree that the CSS should be handled in opac.css. 

yep, this is now done

> Doing this will eliminate the other error I see, an
> all-caps STYLE attribute. It has to be lower-case in order to be valid XHTML.

fyi: the new patch tested ok as 'XHTML 1.0 Transitional' at http://validator.w3.org
Comment 28 Robin Sheat 2011-08-29 05:53:35 UTC
Attempted to sign off, however can't convince it to apply :(
Comment 29 Mason James 2011-09-12 06:08:23 UTC Comment hidden (obsolete)
Comment 30 Mason James 2011-09-12 06:10:43 UTC
(In reply to comment #29)
> Created attachment 5402 [details] [review]
> patch

newly attached patch is rebased from today's master/HEAD

commit c849f7a90d4eed248bd53a00b8021c07e856e022
    Adding the 155th developer to the history
Comment 31 Mason James 2011-09-12 06:15:51 UTC
> newly attached patch is rebased from today's master/HEAD

... and emailed to kc.org patches list ;)
Comment 32 Robin Sheat 2011-09-13 01:03:17 UTC Comment hidden (obsolete)
Comment 33 Paul Poulain 2011-10-06 16:38:42 UTC
Comment on attachment 5402 [details] [review]
patch

this one is obsolete if i'm not wrong (the signed-off version is the other)
Comment 34 Paul Poulain 2011-10-06 16:40:18 UTC
mason, sorry, does not apply anymore :((

Could you rebase & resubmit (keeping "signed-off")
If you do it this night, i'll try to QA it tomorrow, to have it for 3.6, it's a small but very usefull ENH !
Comment 35 Robin Sheat 2011-10-06 22:57:31 UTC Comment hidden (obsolete)
Comment 36 Robin Sheat 2011-10-06 22:58:50 UTC
It was just a conflict in the CSS, I've fixed it against current master.
Comment 37 Mason James 2011-10-06 23:01:57 UTC
(In reply to comment #36)
> It was just a conflict in the CSS, I've fixed it against current master.

yep... and a 'single line of whitespace' conflict against master, too....
Comment 38 Paul Poulain 2011-10-07 08:44:47 UTC
Created attachment 5762 [details] [review]
Bug 6636 - add 'powered by Koha' text to OPAC footer

Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
  QA control: added () inside 2 [% IF %]
Comment 39 Paul Poulain 2011-10-07 08:47:33 UTC
QA comment:
  * () were missing in a [% IF %]
  * also does some un-announced changes from TAB to 4 spaces, that's OK


Marking passed QA
Comment 40 Chris Cormack 2011-10-09 07:14:04 UTC
Pushed, please test
Comment 41 Mason James 2011-10-09 13:51:46 UTC
(In reply to comment #40)
> Pushed, please test

tested OK - marking resolved ;)