The field opac_news.new is very confusing and should be renamed. If you want to access it via Koha::NewsItem you will have trouble: use Koha::News; my $news_item = Koha::News->next; say $news_item->new; => Attempt to bless into a reference at /home/vagrant/kohaclone/Koha/Object.pm line 78.
Created attachment 59406 [details] [review] Bug 17960: Add a test to highlight the issue With only this patch applied, proving t/db_dependent/Koha/News.t will return "Attempt to bless into a reference at /home/vagrant/kohaclone/Koha/Object.pm line 78."
Created attachment 59407 [details] [review] Bug 17960: Rename opac_news.new with opac_news.content The field opac_news.new is very confusing and should be renamed. If you want to access it via Koha::NewsItem you will have trouble: use Koha::News; my $news_item = Koha::News->next; say $news_item->new; => Attempt to bless into a reference at /home/vagrant/kohaclone/Koha/Object.pm line 78. This patchset is going to rename this DB field to opac_news_content instead. Since the opac_news.new can be used in notice templates, we need to warn the user during the update DB process that some templates must be updated. Test plan: 0/ Apply the first patch "Add a test to highlight the issue" and confirm that the test fail 1/ Apply this second patch 2/ Execute the DB entry 3/ Confirm that you get a warning if at least one of your notice templates is using opac_news.new 4/ Confirm that the test new pass 5/ Add/update and delete a news 6/ Confirm that the RSS new feed still works as expected
Created attachment 59408 [details] [review] Bug 17960: Update installed files git grep opac_news.new installer should not return any occurrences in sql files
Created attachment 59409 [details] [review] Bug 17960: DBIC Schema changes for opac_news.content
On my non-kohadevbox I drop'd the db and created it, applied all the patches, ran a web install only doing mandatory default stuff, and then I tried this: mtompset@debian:~/kohaclone$ prove t/db_dependent/Koha/News.t t/db_dependent/Koha/News.t .. 1/5 # Failed test 'The content method return the content of the news' # at t/db_dependent/Koha/News.t line 50. # got: '' # expected: undef # Looks like you failed 1 test of 5. t/db_dependent/Koha/News.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/5 subtests Test Summary Report ------------------- t/db_dependent/Koha/News.t (Wstat: 256 Tests: 5 Failed: 1) Failed test: 4 Non-zero exit status: 1 Files=1, Tests=5, 1 wallclock secs ( 0.02 usr 0.00 sys + 0.72 cusr 0.01 csys = 0.75 CPU) Result: FAIL While I would have expected a failure (and I did get one) before applying the last three on an older DB, I was not expect a failure after the upgrade and all patches applied. Nor after a drop and recreate web install.
Created attachment 59488 [details] [review] Bug 17960: Add opac_news.content values in tests
There is missing content in opac-maint.tt, nad intranet-main.tt see git grep koha_new.new
Created attachment 59768 [details] [review] Bug 17960: Replace missing occurrences
Created attachment 59772 [details] [review] [SIGNED-OFF] Bug 17960: Add a test to highlight the issue With only this patch applied, proving t/db_dependent/Koha/News.t will return "Attempt to bless into a reference at /home/vagrant/kohaclone/Koha/Object.pm line 78." Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59773 [details] [review] [SIGNED-OFF] Bug 17960: Rename opac_news.new with opac_news.content The field opac_news.new is very confusing and should be renamed. If you want to access it via Koha::NewsItem you will have trouble: use Koha::News; my $news_item = Koha::News->next; say $news_item->new; => Attempt to bless into a reference at /home/vagrant/kohaclone/Koha/Object.pm line 78. This patchset is going to rename this DB field to opac_news_content instead. Since the opac_news.new can be used in notice templates, we need to warn the user during the update DB process that some templates must be updated. Test plan: 0/ Apply the first patch "Add a test to highlight the issue" and confirm that the test fail 1/ Apply this second patch 2/ Execute the DB entry 3/ Confirm that you get a warning if at least one of your notice templates is using opac_news.new 4/ Confirm that the test new pass 5/ Add/update and delete a news 6/ Confirm that the RSS new feed still works as expected Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59774 [details] [review] [SIGNED-OFF] Bug 17960: Update installed files git grep opac_news.new installer should not return any occurrences in sql files Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59775 [details] [review] [SIGNED-OFF] Bug 17960: DBIC Schema changes for opac_news.content Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59776 [details] [review] [SIGNED-OFF] Bug 17960: Add opac_news.content values in tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59777 [details] [review] [SIGNED-OFF] Bug 17960: Replace missing occurrences Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59861 [details] [review] Bug 17960: Add a test to highlight the issue With only this patch applied, proving t/db_dependent/Koha/News.t will return "Attempt to bless into a reference at /home/vagrant/kohaclone/Koha/Object.pm line 78." Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59862 [details] [review] Bug 17960: Rename opac_news.new with opac_news.content The field opac_news.new is very confusing and should be renamed. If you want to access it via Koha::NewsItem you will have trouble: use Koha::News; my $news_item = Koha::News->next; say $news_item->new; => Attempt to bless into a reference at /home/vagrant/kohaclone/Koha/Object.pm line 78. This patchset is going to rename this DB field to opac_news_content instead. Since the opac_news.new can be used in notice templates, we need to warn the user during the update DB process that some templates must be updated. Test plan: 0/ Apply the first patch "Add a test to highlight the issue" and confirm that the test fail 1/ Apply this second patch 2/ Execute the DB entry 3/ Confirm that you get a warning if at least one of your notice templates is using opac_news.new 4/ Confirm that the test new pass 5/ Add/update and delete a news 6/ Confirm that the RSS new feed still works as expected Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59863 [details] [review] Bug 17960: Update installed files git grep opac_news.new installer should not return any occurrences in sql files Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59864 [details] [review] Bug 17960: DBIC Schema changes for opac_news.content Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59865 [details] [review] Bug 17960: Add opac_news.content values in tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59866 [details] [review] Bug 17960: Replace missing occurrences Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 17.05, thanks Jonathan!
I can see that this change makes sense, but do we strictly need it for 16.11.x? I'd like to avoid db changes if possible and not sure if this fixes a current bug. It will also require to update notice templates.
(In reply to Katrin Fischer from comment #22) > I can see that this change makes sense, but do we strictly need it for > 16.11.x? I'd like to avoid db changes if possible and not sure if this fixes > a current bug. It will also require to update notice templates. No, I do not think we need to backport it.
Thx for the quick answer! This won't be included in 16.11.x.
Was wondering - why not have a database update fix this automatically? Should only be a find&replace kind of thing? ) NOTICE - If you are using opac_news.new in your notice templates, you should update it to opac_news.content
(In reply to Katrin Fischer from comment #25) > Was wondering - why not have a database update fix this automatically? > Should only be a find&replace kind of thing? > ) > > NOTICE - If you are using opac_news.new in your notice templates, you should > update it to opac_news.content I would prefer to add things like bug 17898
For SQL it's hard, I think for notice templates it shoudl work alright with a database update, because the placeholders are easy to select. Something like this might work: UPDATE letter SET content = REPLACE(content, '<<opac_news.new>>', '<<opac_news.content>>');
Yes, I was thinking about reports.
(In reply to Katrin Fischer from comment #25) > Was wondering - why not have a database update fix this automatically? > Should only be a find&replace kind of thing? See bug 18121.