|
Description
Kyle M Hall (khall)
2020-05-13 14:26:13 UTC
Created attachment 104849 [details] [review] Bug 25485: TinyMCE broken in Debian package installs When installing the Debian package of Koha, we install the Debian tinymce package and symlink over our own tinymce libs that are in the Koha codebase. This version of tinymce does not have the same files and because of this, tinymce does not get loaded. We should remove the use of the tinymce Debian package, and just use our own version of TinyMCE. Test Plan: 1) Install Koha 19.11 from package 2) Browse to the news editor 4) Note the WYSIWYG editor is missing 5) Build a new Koha package with this patch applied, install it 6) Reload the news editor 7) Note the WYSIWYG editor is back! Hi Kyle, I am seeing a 'partial' breakage in 18.11, could this be related? TinyMCE loads, but when I click on 'link' for example, the pop-up is not formatted nicely and the dev tools report at least 2 files missing, one being: /cgi-bin/koha/tools/themes/advanced/skins/default/dialog.css If you don't have the same Koha and/or Debian version we can expect variations. Like only the CSS file missing while the rest is not broken. This is amazing. (In reply to Tomás Cohen Arazi from comment #4) > This is amazing. I know, right? I've often wondered why we used Debian's tinymce package. I think we used to do the same thing with the yui Javascript. And we're requiring Debian's libjs-jquery package too, although I don't think that actually gets used anywhere? (In reply to Kyle M Hall from comment #1) > Test Plan: > 1) Install Koha 19.11 from package > 2) Browse to the news editor > 4) Note the WYSIWYG editor is missing > 5) Build a new Koha package with this patch applied, install it > 6) Reload the news editor > 7) Note the WYSIWYG editor is back! I don't think that this test plan is 100% sufficient, but it is a hard one to explain well... Comment on attachment 104849 [details] [review] Bug 25485: TinyMCE broken in Debian package installs Review of attachment 104849 [details] [review]: ----------------------------------------------------------------- ::: debian/rules @@ +23,3 @@ > rm $(TMP)/var/log/koha/README > rm -r \ > $(TMP)/usr/share/koha/intranet/htdocs/intranet-tmpl/lib/tiny_mce At a glance, it looks like this should cause a problem? But maybe I'm misreading the rules file. (In reply to David Cook from comment #7) > Comment on attachment 104849 [details] [review] [review] > Bug 25485: TinyMCE broken in Debian package installs > > Review of attachment 104849 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: debian/rules > @@ +23,3 @@ > > rm $(TMP)/var/log/koha/README > > rm -r \ > > $(TMP)/usr/share/koha/intranet/htdocs/intranet-tmpl/lib/tiny_mce > > At a glance, it looks like this should cause a problem? But maybe I'm > misreading the rules file. So I removed that rule in my test, but that also meant the existing symlink didn't get removed... so I had to unlink the symlink and then reinstall the koha-common package... (In reply to Kyle M Hall from comment #1) > Test Plan: > 1) Install Koha 19.11 from package > 2) Browse to the news editor > 4) Note the WYSIWYG editor is missing > 5) Build a new Koha package with this patch applied, install it > 6) Reload the news editor > 7) Note the WYSIWYG editor is back! As I noted above, I found this wasn't a sufficient test plan. I did something like this: 0) Applied Kyle's patch + manually updated debian/control and removed a questionable line in debian/rules 1) Built the koha-common package 2) Unlinked /usr/share/koha/intranet/htdocs/intranet-tmpl/lib/tiny_mce in my target environment 3) Installed (ie upgraded) Koha package 4) Noted the WYSIWYG editor is back! So... not sure this one is quite there yet. Actually, I'll do more testing without my changes... going back to "Needs Signoff" (In reply to David Cook from comment #5) > And we're requiring Debian's libjs-jquery package too, although I don't > think that actually gets used anywhere? git grep "/usr/share/" | grep -vE "/usr/share/koha|/usr/share/perl5|/usr/share/fonts|/usr/share/debconf|/usr/share/doc/koha" IIUC, nothing to worry about :D Makefile.PL: push @version, ('/usr/share/perl/5.10') if !$version[0]; debian/copyright: /usr/share/common-licenses/GPL-3 . debian/koha.config:if [ -f /usr/share/dbconfig-common/dpkg/config.mysql ]; then debian/koha.config: . /usr/share/dbconfig-common/dpkg/config.mysql debian/koha.postinst:. /usr/share/dbconfig-common/dpkg/postinst.mysql debian/koha.postrm:if [ -f /usr/share/dbconfig-common/dpkg/postrm.mysql ]; then debian/koha.postrm: . /usr/share/dbconfig-common/dpkg/postrm.mysql debian/koha.prerm:. /usr/share/dbconfig-common/dpkg/prerm.mysql debian/rules: ln -s /usr/share/tinymce/www \ debian/rules: /usr/share/xml/docbook/stylesheet/docbook-xsl/manpages/docbook.xsl \ I think this one needs more testing, but on my initial tests it doesn't work when upgrading Koha. I imagine it would work for installing Koha initially though. Ok when I removed everything tiny_mce related from debian/rules, it installed the bundled tinymce overtop of /usr/share/tinymce/www through the existing symlink for my already existing Koha installation. Yikes. I should've tested with Kyle's original patch, but it's overtime on Friday night, so will have to leave that to someone else, or I can revisit on Monday... Hm, we tested an 18.11.16 package with Kyle's patch but the Editor is now gone: ReferenceError: tinyMCE is not defined tinyMCE.baseURL = "/intranet-tmpl/lib/tiny_mce"; Not the result I was hoping for to fix our issues :( (In reply to Katrin Fischer from comment #14) > Hm, we tested an 18.11.16 package with Kyle's patch but the Editor is now > gone: > > ReferenceError: tinyMCE is not defined > tinyMCE.baseURL = "/intranet-tmpl/lib/tiny_mce"; > > Not the result I was hoping for to fix our issues :( So you rebuilt the package? (In reply to Tomás Cohen Arazi from comment #15) > (In reply to Katrin Fischer from comment #14) > > Hm, we tested an 18.11.16 package with Kyle's patch but the Editor is now > > gone: > > > > ReferenceError: tinyMCE is not defined > > tinyMCE.baseURL = "/intranet-tmpl/lib/tiny_mce"; > > > > Not the result I was hoping for to fix our issues :( > > So you rebuilt the package? Yes, we built a new package and used it to update an existing 18.11.11 installation. It could be the fix doesn't apply to 18.11 or that someting is not quite right yet. (In reply to Kyle M Hall from comment #1) > When installing the Debian package of Koha, we install the Debian tinymce > package and symlink over our own tinymce libs that are in the Koha codebase. Why do we do this? (In reply to David Cook from comment #5) > I've often wondered why we used Debian's tinymce package. I think we used to > do the same thing with the yui Javascript. We used to offer the option of loading YUI from Yahoo's CDN. I came to the conclusion that the TinyMCE problem on 18.11.16 is a different problem - I found a workaround I will attach to bug 25519. Sorry for adding to the confusion here. > 1) Install Koha 19.11 from package > 5) Build a new Koha package with this patch applied, install it IIUC from various IRC discussions the cleanest way[1] to test this while being simple enough be less like likely to be messed up is: - make packages with https://hub.docker.com/r/koha/koha-dpkg - spin a standard Debian (9?) however one wants (container, VM, VPS) - install the packages following https://wiki.koha-community.org/wiki/Koha_on_Debian Anything missing? [1] for someone without experience with building packages and installing Koha from them *enough to be less likely to mess things up Hoping to work on this more today. Getting the following on master: cp: cannot stat 'debian/tmp/debian/tmp/etc/koha/searchengine': No such file or directory dh_install: cp -a debian/tmp/debian/tmp/etc/koha/searchengine debian/koha-common//etc/koha/ returned exit code 1 debian/rules:9: recipe for target 'binary' failed But I don't think it relates to this patch. (In reply to David Cook from comment #22) > Getting the following on master: > > cp: cannot stat 'debian/tmp/debian/tmp/etc/koha/searchengine': No such file > or directory > dh_install: cp -a debian/tmp/debian/tmp/etc/koha/searchengine > debian/koha-common//etc/koha/ returned exit code 1 > debian/rules:9: recipe for target 'binary' failed > > But I don't think it relates to this patch. Looks like Bug 24735 got rid of etc/searchengine/queryparser.yml, but Bugzilla says that bug was pushed to master at the start of March... I guess I probably haven't been building master packages since then but https://dashboard.koha-community.org/ says the master build is still passing on D9? (In reply to David Cook from comment #23) > Looks like Bug 24735 got rid of etc/searchengine/queryparser.yml, but > Bugzilla says that bug was pushed to master at the start of March... > > I guess I probably haven't been building master packages since then but > https://dashboard.koha-community.org/ says the master build is still passing > on D9? (In reply to David Cook from comment #22) > Getting the following on master: > > cp: cannot stat 'debian/tmp/debian/tmp/etc/koha/searchengine': No such file > or directory > dh_install: cp -a debian/tmp/debian/tmp/etc/koha/searchengine > debian/koha-common//etc/koha/ returned exit code 1 > debian/rules:9: recipe for target 'binary' failed > > But I don't think it relates to this patch. See Bug 25068 Ok with Bug 25068's patch and this one (plus a manual update to debian/control), I was able to create a package. Next, I inspected the files in the package: dpkg-deb -c koha-common_19.12.00-test+20200518031703.33335a5d-1_all.deb As I anticipated, it looks like /usr/share/koha/intranet/htdocs/intranet-tmpl/lib/tiny_mce has been removed (as the debian/rules file removes it). I'll add a follow-up patch... Created attachment 104995 [details] [review] Bug 25485: Remove all mention of tiny_mce in debian/rules This patch removes the removal of the following during the build process: /usr/share/koha/intranet/htdocs/intranet-tmpl/lib/tiny_mce The package now includes the tiny_mce files, but it's not removing the symlink, which is a problem. See Bug 14055 for the YUI related issue. Created attachment 104997 [details] [review] Bug 25485: Remove tiny_mce symlink during pre install of deb package This patch removes the tiny_mce symlink before Koha package installation using the same method that we used when we removed the YUI symlink in 2015. Created attachment 104999 [details] [review] Bug 25485: TinyMCE broken in Debian package installs When installing the Debian package of Koha, we install the Debian tinymce package and symlink over our own tinymce libs that are in the Koha codebase. This version of tinymce does not have the same files and because of this, tinymce does not get loaded. We should remove the use of the tinymce Debian package, and just use our own version of TinyMCE. Test Plan: 1) Install Koha 19.11 from package 2) Browse to the news editor 4) Note the WYSIWYG editor is missing 5) Build a new Koha package with this patch applied, install it 6) Reload the news editor 7) Note the WYSIWYG editor is back! Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 105000 [details] [review] Bug 25485: TinyMCE broken in Debian package installs When installing the Debian package of Koha, we install the Debian tinymce package and symlink over our own tinymce libs that are in the Koha codebase. This version of tinymce does not have the same files and because of this, tinymce does not get loaded. We should remove the use of the tinymce Debian package, and just use our own version of TinyMCE. Test Plan: 1) Install Koha 19.11 from package 2) Browse to the news editor 4) Note the WYSIWYG editor is missing 5) Build a new Koha package with this patch applied, install it 6) Reload the news editor 7) Note the WYSIWYG editor is back! Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 105001 [details] [review] Bug 25485: Remove all mention of tiny_mce in debian/rules This patch removes the removal of the following during the build process: /usr/share/koha/intranet/htdocs/intranet-tmpl/lib/tiny_mce Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 105002 [details] [review] Bug 25485: Remove tiny_mce symlink during pre install of deb package This patch removes the tiny_mce symlink before Koha package installation using the same method that we used when we removed the YUI symlink in 2015. Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 105004 [details] [review] Bug 25485: [Do not push] remove tiny_mce from debian/control for testing Ok, using all these patches, you can get the correct behaviour. I've signed off Kyle's original patch, and I'm leaving "Needs Signoff" so that someone (maybe Kyle?) can sign off my patches. Note that this also affects 19.11 and not just master... (In reply to David Cook from comment #23) > I guess I probably haven't been building master packages since then but > https://dashboard.koha-community.org/ says the master build is still passing > on D9? The CI is not an actual build unfortunately. It's spinning up koha-testing-docker which is a gitified install (if I got the terminology right) like the DevBox and then runs all the tests. But no tests for package build yet. Fortunately these two projects (including yours :)) are good bases to move it that direction. https://gitlab.com/tomascohen/koha-dpkg-docker https://gitlab.com/minusdavid/koha-deb-builder-docker Created attachment 105048 [details] [review] Bug 25485: Remove all mention of tiny_mce in debian/rules This patch removes the removal of the following during the build process: /usr/share/koha/intranet/htdocs/intranet-tmpl/lib/tiny_mce Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 105049 [details] [review] Bug 25485: Remove tiny_mce symlink during pre install of deb package This patch removes the tiny_mce symlink before Koha package installation using the same method that we used when we removed the YUI symlink in 2015. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 105050 [details] [review] Bug 25485: [Do not push] remove tiny_mce from debian/control for testing Created attachment 105127 [details] [review] Bug 25485: TinyMCE broken in Debian package installs When installing the Debian package of Koha, we install the Debian tinymce package and symlink over our own tinymce libs that are in the Koha codebase. This version of tinymce does not have the same files and because of this, tinymce does not get loaded. We should remove the use of the tinymce Debian package, and just use our own version of TinyMCE. Test Plan: 1) Install Koha 19.11 from package 2) Browse to the news editor 4) Note the WYSIWYG editor is missing 5) Build a new Koha package with this patch applied, install it 6) Reload the news editor 7) Note the WYSIWYG editor is back! Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 105128 [details] [review] Bug 25485: Remove all mention of tiny_mce in debian/rules This patch removes the removal of the following during the build process: /usr/share/koha/intranet/htdocs/intranet-tmpl/lib/tiny_mce Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 105130 [details] [review] Bug 25485: Remove tiny_mce symlink during pre install of deb package This patch removes the tiny_mce symlink before Koha package installation using the same method that we used when we removed the YUI symlink in 2015. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Good job. I tested an upgrade to verify the symlink is removed and doesn't break the upgrade. All good. Thanks and sorry for the delay! Will be trying to test all the packages bugs together tomorrow on a new VPS. Nice work everyone! Pushed to master for 20.05 Not sure the dependency affect this in 19.11.x - it wasn't pushed. This fix for TinyMCE was pushed to 19.11.x for 19.11.06 (In reply to Joy Nelson from comment #45) > Not sure the dependency affect this in 19.11.x - it wasn't pushed. This fix > for TinyMCE was pushed to 19.11.x for 19.11.06 That's exactly right. Dependency doesn't affect 19.11.x. Great to have this in 19.11.06! Correct me if i am wrong but I don't think this issue is present in 19.05.x package installs. Unless I'm told otherwise I won't backport |