Bug 17960 - Rename opac_news.new with opac_news.content
Summary: Rename opac_news.new with opac_news.content
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 14757
Blocks: 17966 18121
  Show dependency treegraph
 
Reported: 2017-01-23 11:01 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
The database column opac_news.new is renamed to opac_news.content. The notice templates using that placeholder should have been updated automatically (bug 18121).
Version(s) released in:


Attachments
Bug 17960: Add a test to highlight the issue (1.32 KB, patch)
2017-01-23 12:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17960: Rename opac_news.new with opac_news.content (10.12 KB, patch)
2017-01-23 12:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17960: Update installed files (47.79 KB, patch)
2017-01-23 12:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17960: DBIC Schema changes for opac_news.content (1.57 KB, patch)
2017-01-23 12:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17960: Add opac_news.content values in tests (979 bytes, patch)
2017-01-24 08:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17960: Replace missing occurrences (3.70 KB, patch)
2017-02-02 08:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17960: Add a test to highlight the issue (1.39 KB, patch)
2017-02-02 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17960: Rename opac_news.new with opac_news.content (10.19 KB, patch)
2017-02-02 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17960: Update installed files (47.86 KB, patch)
2017-02-02 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17960: DBIC Schema changes for opac_news.content (1.64 KB, patch)
2017-02-02 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17960: Add opac_news.content values in tests (1.02 KB, patch)
2017-02-02 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17960: Replace missing occurrences (3.76 KB, patch)
2017-02-02 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17960: Add a test to highlight the issue (1.43 KB, patch)
2017-02-03 10:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17960: Rename opac_news.new with opac_news.content (10.23 KB, patch)
2017-02-03 10:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17960: Update installed files (47.90 KB, patch)
2017-02-03 10:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17960: DBIC Schema changes for opac_news.content (1.68 KB, patch)
2017-02-03 10:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17960: Add opac_news.content values in tests (1.07 KB, patch)
2017-02-03 10:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17960: Replace missing occurrences (3.81 KB, patch)
2017-02-03 10:32 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-01-23 11:01:59 UTC
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.
Comment 1 Jonathan Druart 2017-01-23 12:58:33 UTC
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."
Comment 2 Jonathan Druart 2017-01-23 12:58:40 UTC
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
Comment 3 Jonathan Druart 2017-01-23 12:58:51 UTC
Created attachment 59408 [details] [review]
Bug 17960: Update installed files

git grep opac_news.new installer
should not return any occurrences in sql files
Comment 4 Jonathan Druart 2017-01-23 12:58:58 UTC
Created attachment 59409 [details] [review]
Bug 17960: DBIC Schema changes for opac_news.content
Comment 5 Mark Tompsett 2017-01-24 02:52:12 UTC
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.
Comment 6 Jonathan Druart 2017-01-24 08:16:33 UTC
Created attachment 59488 [details] [review]
Bug 17960: Add opac_news.content values in tests
Comment 7 Josef Moravec 2017-02-01 22:23:30 UTC
There is missing content in opac-maint.tt, nad intranet-main.tt

see git grep koha_new.new
Comment 8 Jonathan Druart 2017-02-02 08:22:53 UTC
Created attachment 59768 [details] [review]
Bug 17960: Replace missing occurrences
Comment 9 Josef Moravec 2017-02-02 09:46:19 UTC
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>
Comment 10 Josef Moravec 2017-02-02 09:46:26 UTC
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>
Comment 11 Josef Moravec 2017-02-02 09:46:33 UTC
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>
Comment 12 Josef Moravec 2017-02-02 09:46:38 UTC
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>
Comment 13 Josef Moravec 2017-02-02 09:46:43 UTC
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>
Comment 14 Josef Moravec 2017-02-02 09:46:50 UTC
Created attachment 59777 [details] [review]
[SIGNED-OFF] Bug 17960: Replace missing occurrences

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Nick Clemens 2017-02-03 10:31:54 UTC
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>
Comment 16 Nick Clemens 2017-02-03 10:32:00 UTC
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>
Comment 17 Nick Clemens 2017-02-03 10:32:05 UTC
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>
Comment 18 Nick Clemens 2017-02-03 10:32:09 UTC
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>
Comment 19 Nick Clemens 2017-02-03 10:32:14 UTC
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>
Comment 20 Nick Clemens 2017-02-03 10:32:18 UTC
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>
Comment 21 Kyle M Hall 2017-02-06 17:45:24 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 22 Katrin Fischer 2017-02-07 07:15:05 UTC
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.
Comment 23 Jonathan Druart 2017-02-07 07:43:47 UTC
(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.
Comment 24 Katrin Fischer 2017-02-07 09:32:05 UTC
Thx for the quick answer!

This won't be included in 16.11.x.
Comment 25 Katrin Fischer 2017-02-14 06:52:46 UTC
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
Comment 26 Jonathan Druart 2017-02-15 08:21:52 UTC
(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
Comment 27 Katrin Fischer 2017-02-15 08:32:55 UTC
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>>');
Comment 28 Jonathan Druart 2017-02-15 08:36:52 UTC
Yes, I was thinking about reports.
Comment 29 Jonathan Druart 2017-02-15 08:39:17 UTC
(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.