Libraries would like more flexibility in the virtual card shown to patrons via the OPAC. Making that virtual card use a notice template would make is highly customizable.
Created attachment 187695 [details] [review] Bug 40659: Add new notice VIRTUALCARD
Created attachment 187696 [details] [review] Bug 40659: Add ability to customize OPACVirtualCard
Created attachment 187697 [details] [review] Bug 40659: Add ability to customize OPACVirtualCard To test: 1. Turn on the OPACVirtualCard. 2. Log in to the OPAC as a patron and go look at your Library card ( opac-virtual-card.pl ) 3. APPLY PATCH, updatedatabase, restart_all 4. Go again at your patron's card on the OPAC ( opac-virtual-card.pl ), it should look exactly the same. 5. Go to Tools > Notices > VIRTUALCARD 6. You should see the default content for the library card. 7. Play around with different variables from borrower, like [% borrower.firstname%] and branches, like [% branch.branchcode %] 8. You can also move around the barcode by using the placeholder value [% my_barcode %]. 9. Put the [% my_barcode %] variable anywhere you want in the notice then go to the sys pref "OPACVirtualCardBarcode". 10. Change to different barcode types and relaod the library card page ( opac-virtual-card.pl ). You should notice the barcode type is being honored. 11. Turn on OPACpatronimages AND patronimages, then upload a patron image for your patron. 12. Use the [% my_image %] variable anywhere you want in the notice to place the patron image. 13. Leaving the [% my_image %] variable as is, turn off OPACpatronimages. 14. Reload the OPAC page, no image should display. 15. Delete the VIRTUALCARD notice. 16. Go back to the OPAC page and with the notice deleted you should see the hardcoded content. ie: the page should look exactly the same as it does without this patch.
Created attachment 187706 [details] [review] Bug 40659: Close two divs in sample notice
Created attachment 187894 [details] [review] Bug 40659: Add new notice VIRTUALCARD Signed-off-by: Marie Martino <martinom43@morainevalley.edu>
Created attachment 187895 [details] [review] Bug 40659: Add ability to customize OPACVirtualCard To test: 1. Turn on the OPACVirtualCard. 2. Log in to the OPAC as a patron and go look at your Library card ( opac-virtual-card.pl ) 3. APPLY PATCH, updatedatabase, restart_all 4. Go again at your patron's card on the OPAC ( opac-virtual-card.pl ), it should look exactly the same. 5. Go to Tools > Notices > VIRTUALCARD 6. You should see the default content for the library card. 7. Play around with different variables from borrower, like [% borrower.firstname%] and branches, like [% branch.branchcode %] 8. You can also move around the barcode by using the placeholder value [% my_barcode %]. 9. Put the [% my_barcode %] variable anywhere you want in the notice then go to the sys pref "OPACVirtualCardBarcode". 10. Change to different barcode types and relaod the library card page ( opac-virtual-card.pl ). You should notice the barcode type is being honored. 11. Turn on OPACpatronimages AND patronimages, then upload a patron image for your patron. 12. Use the [% my_image %] variable anywhere you want in the notice to place the patron image. 13. Leaving the [% my_image %] variable as is, turn off OPACpatronimages. 14. Reload the OPAC page, no image should display. 15. Delete the VIRTUALCARD notice. 16. Go back to the OPAC page and with the notice deleted you should see the hardcoded content. ie: the page should look exactly the same as it does without this patch. Signed-off-by: Marie Martino <martinom43@morainevalley.edu>
Created attachment 187896 [details] [review] Bug 40659: Close two divs in sample notice Signed-off-by: Marie Martino <martinom43@morainevalley.edu>
(In reply to Andrew Fuerste-Henry from comment #0) > Libraries would like more flexibility in the virtual card shown to patrons > via the OPAC. Making that virtual card use a notice template would make is > highly customizable. It would also introduce a security vulnerability. I was weighing up whether to use "Failed QA" or "In Discussion", but I figured I'd opt for the former. I think an argument could be made that other Notes & Slips might already have vulnerabilities in them. I haven't reviewed them in depth. However, for new code, I think that we want to do better - in any case. -- On a related note, I think "HTML Customizations" would make more sense than Notes & Slips for this type of change. That might then dovetail in work that Lucas has done for adding templating to show extra MARC fields on the detail page. I can't recall the bug number but I'm sure Lucas will know it. (Of course, with the "HTML Customizations" we're in a bit of a bind where you can't use the WYSIWYG, you have to use the text editor, but that would be the same thing as Notes & Slips anyway.)
(In reply to David Cook from comment #8) > I can't recall the bug number but I'm sure Lucas will know it. It's bug 39860 which I've added as a See Also
(In reply to David Cook from comment #8) > It would also introduce a security vulnerability. > > I was weighing up whether to use "Failed QA" or "In Discussion", but I > figured I'd opt for the former. > > I think an argument could be made that other Notes & Slips might already > have vulnerabilities in them. I haven't reviewed them in depth. > > However, for new code, I think that we want to do better - in any case. Can you provide some further detail on what "do better" would mean to you in this situation? What is it about the approach on 39860 that meets your needs better than the work here?
(In reply to Andrew Fuerste-Henry from comment #10) > (In reply to David Cook from comment #8) > > However, for new code, I think that we want to do better - in any case. > > Can you provide some further detail on what "do better" would mean to you in > this situation? What is it about the approach on 39860 that meets your needs > better than the work here? Yeah for sure. I can DM you with more in-depth info, but here I'll say that bug 39860 uses the HTML::Scrubber to allow a safe range of HTML to be added. I'd add it's not about my needs, but rather the security of Koha as a whole. I'm just quite active in the security space. At some point, I hope to either add some additional coding guidelines or a separate "Safe Coding Guidelines" to complement the existing coding guidelines, which should then make things clearer. Thanks for the question!
(In reply to David Cook from comment #11) > (In reply to Andrew Fuerste-Henry from comment #10) > > (In reply to David Cook from comment #8) > > > However, for new code, I think that we want to do better - in any case. > > > > Can you provide some further detail on what "do better" would mean to you in > > this situation? What is it about the approach on 39860 that meets your needs > > better than the work here? > > Yeah for sure. I can DM you with more in-depth info, but here I'll say that > bug 39860 uses the HTML::Scrubber to allow a safe range of HTML to be added. > > I'd add it's not about my needs, but rather the security of Koha as a whole. > I'm just quite active in the security space. At some point, I hope to either > add some additional coding guidelines or a separate "Safe Coding Guidelines" > to complement the existing coding guidelines, which should then make things > clearer. Thanks for the question! Is there any reason those details cannot be shared here fro transparency? If scrubbing the HTML is the only issue then I think this should be left in Notices/Slips where it can be scrubbed. The reason is simple, notices/slips already has a pattern for using placeholder/substitutions which will be need in this bug. I advocate to leave this in notice/slips for that reason.
(In reply to Lucas Gass (lukeg) from comment #12) > Is there any reason those details cannot be shared here fro transparency? Typically, we'd talk about it in the Koha Security project. I thought folks might like to keep this bug in Koha, but we could move it to Koha Security for a fuller discussion? > If scrubbing the HTML is the only issue then I think this should be left in > Notices/Slips where it can be scrubbed. The reason is simple, notices/slips > already has a pattern for using placeholder/substitutions which will be need > in this bug. I advocate to leave this in notice/slips for that reason. That's a compelling point. It does have a better UI for that for sure. What about translatability? I was thinking HTML Customizations would be better as people could add in custom labels for each of their different language interfaces?
> What about translatability? I was thinking HTML Customizations would be > better as people could add in custom labels for each of their different > language interfaces? Notices are translatable. There is a TranslateNotices system preference to allow them to be.
(In reply to Lucas Gass (lukeg) from comment #14) > > What about translatability? I was thinking HTML Customizations would be > > better as people could add in custom labels for each of their different > > language interfaces? > > Notices are translatable. There is a TranslateNotices system preference to > allow them to be. Ahh right. That's good. Hmm, conceptually I think it still fits best in HTML Customizations, as it's an HTML Customization. But... I do take your point. Maybe "Notices and slips" needs a re-brand, and to move out of the "Patrons and circulation" area of "Tools". After all, we have the OAI_HARVEST_REPORT and Report templates in "Notices and slips" as well. Huh, SFTP_FAILURE and SFTP_SUCCESS as well. That's interesting. Yeah, all right. I think "Notices and slips" is far enough gone now that it's fine to use it for this purpose. -- So yeah I think if scrubbing is added then it should be all good.
(In reply to David Cook from comment #15) > (In reply to Lucas Gass (lukeg) from comment #14) > > > What about translatability? I was thinking HTML Customizations would be > > > better as people could add in custom labels for each of their different > > > language interfaces? > > > > Notices are translatable. There is a TranslateNotices system preference to > > allow them to be. > > Ahh right. That's good. > > Hmm, conceptually I think it still fits best in HTML Customizations, as it's > an HTML Customization. But... I do take your point. > > Maybe "Notices and slips" needs a re-brand, and to move out of the "Patrons > and circulation" area of "Tools". > > After all, we have the OAI_HARVEST_REPORT and Report templates in "Notices > and slips" as well. Huh, SFTP_FAILURE and SFTP_SUCCESS as well. That's > interesting. > > Yeah, all right. I think "Notices and slips" is far enough gone now that > it's fine to use it for this purpose. > > -- > > So yeah I think if scrubbing is added then it should be all good. I did find this already open bug: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36525
After speaking with Martin I am convinced that this should go in HTML customization, not notice/slips. Going forward notice/slips should only be used for things that are considered "communications". All other customization belong in additional contents.
(In reply to Lucas Gass (lukeg) from comment #17) > After speaking with Martin I am convinced that this should go in HTML > customization, not notice/slips. > > Going forward notice/slips should only be used for things that are > considered "communications". All other customization belong in additional > contents. I changed my mind after reviewing Bug 27334. It is not close to ready for inclusion. It has pretty glaring security issues and doesn't deal with CodeMirrror vs TinyMCE at all. So this needs to stay in Notice/Slips until there is a viable alternative. At which point we'd already have a bunch of things to migrate out of notice/slips.
Yeah it's a really tough one. Some of the things I've found in bug 11583 also make bug 27334 even harder to implement than one might think.
(In reply to David Cook from comment #19) > Yeah it's a really tough one. Some of the things I've found in bug 11583 > also make bug 27334 even harder to implement than one might think. I mean... in our defence... there are whole apps like Wordpress which focus on this sort of thing and show it's easier said than done. Actually even in the case of the block editor... I don't know if Wordpress can even do what we're trying to do here.
Created attachment 188357 [details] [review] Bug 40659: Add Scrubber profile and scrub the contents of VIRTUALCARD template
I am making this dependent on Bug 39860 because the 2 scrubber profiles in C4/Scrubber might conflict. The profile called 'opac' is extremely limited. I am wondering if we need some sort of hint/tip on the VIRTUALCARD template page telling the user what HTML are allowed. If I was working without knowledge of what tags are allowed it would be frustrating.
Created attachment 188358 [details] [review] Bug 40659: Unit tests for new Scrubber profile
Created attachment 188359 [details] [review] Bug 40659: Unit tests for new Scrubber profile
(In reply to Lucas Gass (lukeg) from comment #22) > The profile called 'opac' is extremely limited. I am wondering if we need > some sort of hint/tip on the VIRTUALCARD template page telling the user what > HTML are allowed. If I was working without knowledge of what tags are > allowed it would be frustrating. That's a really good point.
Hm. Looking at this: When we remove the default template in favor our a notice template, we cause a regression for updating libraries that are not English users. Especially when handling a lot of libraries with multiple languages that can mean a huge extra work on update. Also: Not all libraries use TranslateNotices and many have this turned off. I feel it would be cleaner to keep the default template, that makes this available for any language installed without putting extra work on the library and have the customization as an optional extra that replaces the default.
> I feel it would be cleaner to keep the default template, that makes this > available for any language installed without putting extra work on the > library and have the customization as an optional extra that replaces the > default. We could think of other solutions, but I am a bit tired of the clean-up we have to do with each update, just because our default is not English and we do provide lots of translations for users in the OPAC.
(In reply to Katrin Fischer from comment #26) > Hm. Looking at this: > > When we remove the default template in favor our a notice template, we cause > a regression for updating libraries that are not English users. Especially > when handling a lot of libraries with multiple languages that can mean a > huge extra work on update. Also: Not all libraries use TranslateNotices and > many have this turned off. > > I feel it would be cleaner to keep the default template, that makes this > available for any language installed without putting extra work on the > library and have the customization as an optional extra that replaces the > default. So, I should remove the DB update and sample notice? And folks will have to add it themselves?
Right now, all you need to do is switch a system preference to make the feature work. If this is added non-English libraries will have to also add translations. This feels backwards for a template that should be provided translated out of the box. Could we handle it like OpacMaintenance? We have a default in the template, that can be overwritten if needed with your own text/styling. Nothing would change for updating libraries, no disruptions, no extra work. But it would still be customizable if needed. So yes, I suggest removing the default notices and add the standard template back.
I didn't quite understand the conversation for a little while there, but now I think that I agree with Katrin. Keeping the status quo would be good. So the idea would be that this feature allows an opt-in way to customize the format? That sounds good to me. It would probably be useful to provide a sample template somewhere though. Do we put one in the wiki and then link to it from the system preference description? Or maybe we could put a static file or modal somewhere that could be linked to from the system preference description?
(In reply to David Cook from comment #30) > I didn't quite understand the conversation for a little while there, but now > I think that I agree with Katrin. > > Keeping the status quo would be good. So the idea would be that this feature > allows an opt-in way to customize the format? That sounds good to me. > > It would probably be useful to provide a sample template somewhere though. > Do we put one in the wiki and then link to it from the system preference > description? Or maybe we could put a static file or modal somewhere that > could be linked to from the system preference description? Makes sense to me. I believe all we have to do is drop the first patch.
I have obsoleted the first patch, so this should again be ready for testing.
Hmm could we get a rebase? Looks like "Close two divs in sample notice" is causing a problem. -- $ git bz apply 40659 Bug 40659 - Allow "My virtual card" format and content to be customizable 187895 - Bug 40659: Add ability to customize OPACVirtualCard 187896 - Bug 40659: Close two divs in sample notice 188357 - Bug 40659: Add Scrubber profile and scrub the contents of VIRTUALCARD template 188359 - Bug 40659: Unit tests for new Scrubber profile Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 40659: Add ability to customize OPACVirtualCard Applying: Bug 40659: Close two divs in sample notice error: sha1 information is lacking or useless (installer/data/mysql/en/mandatory/sample_notices.yml). error: could not build fake ancestor Patch failed at 0001 Bug 40659: Close two divs in sample notice hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-40659-Close-two-divs-in-sample-notice-ko46zg6c.patch
(In reply to David Cook from comment #33) > Hmm could we get a rebase? Looks like "Close two divs in sample notice" is > causing a problem. Done!
It's mostly working but it looks like there might be a problem when using TranslateNotices. I can't get a custom English or Spanish translation to appear. The only thing that works is Default. Also, there's nothing in Koha to tell you how to customize this. I think it would still be a good idea to have a DB update to insert an empty notice for Patrons > VIRTUALCARD. Then a reference to that in the OPACVirtualCard syspref would be a good idea I think.
Comment on attachment 188357 [details] [review] Bug 40659: Add Scrubber profile and scrub the contents of VIRTUALCARD template Review of attachment 188357 [details] [review]: ----------------------------------------------------------------- ::: C4/Scrubber.pm @@ +88,4 @@ > }, > ], > }, > + opac => { Might be a good idea to call this something other than "opac" for the time being. Even "opac_virtual_card" might be good for now.
Created attachment 189633 [details] [review] Bug 40659: Change Scrubber profile name to opac_virtual_card