|
Description
Kyle M Hall
2013-05-07 16:48:58 UTC
Created attachment 17998 [details] [review] Bug 10215 - Increase the size of opacnote and librariannote for table subscriptionhistory Created attachment 17999 [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. I think this has potential as a bug. *** Bug 10116 has been marked as a duplicate of this bug. *** 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. Created attachment 19247 [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 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. 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 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. *** Bug 8668 has been marked as a duplicate of this bug. *** Still valid in current master. Would be nice to see this fixed! Hi, Still valid in 3.22. Anyone to fix it? Regards, Marjorie Still valid in 18.05... Marjorie 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> Patch rescued.. it should apply now and is ready for testing. 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> Failed because: t/db_dependent/Serials.t does not pass atomic update and kohastructure are not consistant (NULL vs NOT NULL default '') 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> Patch corrected and re-based, back to SO. 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. 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> 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> Added a followup for Katrins request. 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> 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> 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> 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> 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 Nice work! Pushed to master for 19.11.00 Pushed to 19.05.x for 19.05.01 backported to 18.11.x for 18.11.07 |