Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory
Summary: Increase the size of opacnote and librariannote for table subscriptionhistory
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Martin Renvoize
QA Contact: Josef Moravec
URL:
Keywords: Academy
: 8668 10116 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-05-07 16:48 UTC by Kyle M Hall
Modified: 2019-06-12 22:40 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.01


Attachments
Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory (1.84 KB, patch)
2013-05-07 16:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory (2.31 KB, patch)
2013-05-07 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory (3.47 KB, patch)
2013-06-26 17:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory (3.38 KB, patch)
2013-07-02 11:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory (2.87 KB, patch)
2019-02-20 17:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory (2.90 KB, patch)
2019-02-20 17:56 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory (2.90 KB, patch)
2019-04-10 10:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory (2.90 KB, patch)
2019-06-05 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10215: (follow-up) Correct existing data (1.29 KB, patch)
2019-06-05 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10215: (follow-up) Fix the failing test (1.53 KB, patch)
2019-06-05 11:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory (2.95 KB, patch)
2019-06-06 12:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10215: (follow-up) Correct existing data (1.35 KB, patch)
2019-06-06 12:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 10215: (follow-up) Fix the failing test (1.58 KB, patch)
2019-06-06 12:10 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 Kyle M Hall 2013-05-07 16:48:58 UTC
We have librarians that are regularly hitting the maximum number of characters for the note fields in subscription history. They would like to be able to store at least 1000 characters here. Converting the varchar(150) to text should do it.
Comment 1 Kyle M Hall 2013-05-07 16:49:48 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-05-07 16:53:55 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-05-11 12:24:37 UTC
I think this has potential as a bug.
Comment 4 Jared Camins-Esakov 2013-05-12 17:03:43 UTC
*** Bug 10116 has been marked as a duplicate of this bug. ***
Comment 5 Galen Charlton 2013-05-31 18:03:37 UTC
Alas, it can't be quite that simple -- MySQL doesn't allow default values for text and blob columns:

mysql> ALTER TABLE subscriptionhistory CHANGE librariannote librariannote TEXT NOT NULL DEFAULT '';
Query OK, 1 row affected, 1 warning (0.15 sec)
Records: 1  Duplicates: 0  Warnings: 0

mysql> show warnings;
+---------+------+-------------------------------------------------------------+
| Level   | Code | Message                                                     |
+---------+------+-------------------------------------------------------------+
| Warning | 1101 | BLOB/TEXT column 'librariannote' can't have a default value |
+---------+------+-------------------------------------------------------------+
1 row in set (0.00 sec)

Admittedly, by default this is just a warning; MySQL will blithely supply a default value (of '') for text columns.  However, if we ever feel inspired to turn on stricter modes like STRICT_TRANS_TABLES (which I believe MariaDB does by default), problems will occur.

I think making those two columns TEXT (and nullable) is the way to go.  There might be a bit of additional code warranted to make sure that the Apache error log doesn't display spurious warnings about undef opacnote and librariannote.
Comment 6 Kyle M Hall 2013-06-26 17:42:08 UTC Comment hidden (obsolete)
Comment 7 Galen Charlton 2013-06-26 18:48:14 UTC
Comment on attachment 19247 [details] [review]
Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory

Review of attachment 19247 [details] [review]:
-----------------------------------------------------------------

::: C4/Serials.pm
@@ +483,4 @@
>          $subs->{startdate}     = format_date( $subs->{startdate} );
>          $subs->{histstartdate} = format_date( $subs->{histstartdate} );
>          $subs->{histenddate}   = format_date( $subs->{histenddate} );
> +        $subs->{opacnote} . q{}     =~ s/\n/\<br\/\>/g;

$ perl -c C4/Serials.pm
Can't modify constant item in substitution (s///) at C4/Serials.pm line 486, near "s/\n/\<br\/\>/g;"
C4/Serials.pm had compilation errors.

A patch that causes a module to fail to compile is an automatic Failed QA.
Comment 8 Kyle M Hall 2013-07-02 11:34:28 UTC
Created attachment 19335 [details] [review]
Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory

We have librarians that are regularly hitting the maximum number of
characters for the note fields in subscription history. They would like
to be able to store at least 1000 characters here. Converting the
varchar(150) to text should do it.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Add public and private notes to a subscription history entry
   where each note is greater than 150 characters.
4) Save and view the notes, the notes should not be truncated.
Comment 9 Galen Charlton 2013-07-02 14:44:09 UTC
Comment on attachment 19335 [details] [review]
Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory

Review of attachment 19335 [details] [review]:
-----------------------------------------------------------------

::: C4/Serials.pm
@@ +483,4 @@
>          $subs->{startdate}     = format_date( $subs->{startdate} );
>          $subs->{histstartdate} = format_date( $subs->{histstartdate} );
>          $subs->{histenddate}   = format_date( $subs->{histenddate} );
> +        $subs->{opacnote} ||= q{};

Consider what happens if the opacnote happens to "0".  The note entered by the user would not be displayed.

Since 5.10 is the minimum version of Perl required by Koha, we can freely using the //= operator, and I recommend that you do that when you want to conditionally convert an undefined value to the empty string.
Comment 10 Katrin Fischer 2014-03-26 23:04:01 UTC
*** Bug 8668 has been marked as a duplicate of this bug. ***
Comment 11 Katrin Fischer 2014-03-26 23:04:38 UTC
Still valid in current master.
Comment 12 Katrin Fischer 2014-11-09 23:30:37 UTC
Would be nice to see this fixed!
Comment 13 Marjorie Barry-Vila 2016-04-08 14:23:07 UTC
Hi,
Still valid in 3.22.

Anyone to fix it?

Regards,
Marjorie
Comment 14 Marjorie Barry-Vila 2019-02-19 15:53:14 UTC
Still valid in 18.05...

Marjorie
Comment 15 Martin Renvoize 2019-02-20 17:20:51 UTC
Created attachment 85395 [details] [review]
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory

We have librarians that are regularly hitting the maximum number of
characters for the note fields in subscription history. They would like
to be able to store at least 1000 characters here. Converting the
varchar(150) to text should do it.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Add public and private notes to a subscription history entry
   where each note is greater than 150 characters.
4) Save and view the notes, the notes should not be truncated.

Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2019-02-20 17:25:54 UTC
Patch rescued.. it should apply now and is ready for testing.
Comment 17 Owen Leonard 2019-02-20 17:56:24 UTC
Created attachment 85400 [details] [review]
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory

We have librarians that are regularly hitting the maximum number of
characters for the note fields in subscription history. They would like
to be able to store at least 1000 characters here. Converting the
varchar(150) to text should do it.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Add public and private notes to a subscription history entry
   where each note is greater than 150 characters.
4) Save and view the notes, the notes should not be truncated.

Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 18 Josef Moravec 2019-02-25 11:14:30 UTC
Failed because:
t/db_dependent/Serials.t does not pass
atomic update and kohastructure are not consistant (NULL vs NOT NULL default '')
Comment 19 Martin Renvoize 2019-04-10 10:52:08 UTC
Created attachment 87705 [details] [review]
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory

We have librarians that are regularly hitting the maximum number of
characters for the note fields in subscription history. They would like
to be able to store at least 1000 characters here. Converting the
varchar(150) to text should do it.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Add public and private notes to a subscription history entry
   where each note is greater than 150 characters.
4) Save and view the notes, the notes should not be truncated.

Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 20 Martin Renvoize 2019-04-10 10:52:56 UTC
Patch corrected and re-based, back to SO.
Comment 21 Katrin Fischer 2019-04-10 20:36:16 UTC
Tests are still failing for me:

t/db_dependent/Serials.t .. 2/46 
#   Failed test 'NewSubscription should not set subscriptionhistory opacnotes'
#   at t/db_dependent/Serials.t line 95.
#          got: undef
#     expected: ''

#   Failed test 'NewSubscription should not set subscriptionhistory librariannotes'
#   at t/db_dependent/Serials.t line 96.
#          got: undef
#     expected: ''
t/db_dependent/Serials.t .. 14/46 # Looks like you failed 2 tests of 46.
t/db_dependent/Serials.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/46 subtests 

Wondering, should we correct the old data? I know have a mix of '' and NULL in my db.
Comment 22 Martin Renvoize 2019-06-05 11:55:43 UTC
Created attachment 90323 [details] [review]
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory

We have librarians that are regularly hitting the maximum number of
characters for the note fields in subscription history. They would like
to be able to store at least 1000 characters here. Converting the
varchar(150) to text should do it.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Add public and private notes to a subscription history entry
   where each note is greater than 150 characters.
4) Save and view the notes, the notes should not be truncated.

Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 23 Martin Renvoize 2019-06-05 11:55:46 UTC
Created attachment 90324 [details] [review]
Bug 10215: (follow-up) Correct existing data

This patch adds the correction of data ('' => NULL) during the DB
update.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2019-06-05 11:56:12 UTC
Added a followup for Katrins request.
Comment 25 Martin Renvoize 2019-06-05 11:59:28 UTC
Created attachment 90325 [details] [review]
Bug 10215: (follow-up) Fix the failing test

We now handle NULL values at the database end and do not expect empty
strings.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Nick Clemens 2019-06-06 12:10:34 UTC
Created attachment 90355 [details] [review]
Bug 10215: Increase the size of opacnote and librariannote for table subscriptionhistory

We have librarians that are regularly hitting the maximum number of
characters for the note fields in subscription history. They would like
to be able to store at least 1000 characters here. Converting the
varchar(150) to text should do it.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Add public and private notes to a subscription history entry
   where each note is greater than 150 characters.
4) Save and view the notes, the notes should not be truncated.

Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2019-06-06 12:10:38 UTC
Created attachment 90356 [details] [review]
Bug 10215: (follow-up) Correct existing data

This patch adds the correction of data ('' => NULL) during the DB
update.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens 2019-06-06 12:10:41 UTC
Created attachment 90357 [details] [review]
Bug 10215: (follow-up) Fix the failing test

We now handle NULL values at the database end and do not expect empty
strings.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens 2019-06-06 12:11:27 UTC
Just noting: If you save a note, then blank it, the DB value is '' instead of NULL -I don't think that is an issue
Comment 30 Martin Renvoize 2019-06-06 14:27:49 UTC
Nice work!

Pushed to master for 19.11.00
Comment 31 Fridolin SOMERS 2019-06-11 15:13:55 UTC
Pushed to 19.05.x for 19.05.01
Comment 32 Lucas Gass 2019-06-12 22:40:18 UTC
backported to 18.11.x for 18.11.07