Description
Kyle M Hall (khall)
2014-06-05 16:30:29 UTC
*** Bug 8956 has been marked as a duplicate of this bug. *** Created attachment 28675 [details] [review] Bug 12375 [1] - Update database Created attachment 28676 [details] [review] Bug 12375 [2] - Update Serial schema file Created attachment 28677 [details] [review] Bug 12375 [3] - Store serials enumeration data in separate fields Created attachment 28678 [details] [review] Bug 12375 [4] - Replace use of DBI with Koha::Database in C4::Serials::NewIssue Created attachment 28679 [details] [review] Bug 12375 [4] - Replace use of DBI with Koha::Database in C4::Serials::NewIssue Currently, all serials enumeration data is stored in conglomerated fashion in serial.serialseq. This makes it extremely difficult to do any reporting based on the individual sequence values due to this fact. In addition to the formatted version of the sequence, we should be storing the individual values as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Create a new subscription, and add serials to it. The new x y and z fields should be populated with the same data the replaces {X} {Y} and {Z} in the enumeration field. 4) Inspect some existing serials. The update script will have made a best effort to recover the existing enumeration data to store in separate fields. Created attachment 28680 [details] [review] Bug 12375 [1] - Update database Created attachment 28681 [details] [review] Bug 12375 [2] - Update Serial schema file Created attachment 28682 [details] [review] Bug 12375 [3] - Store serials enumeration data in separate fields Created attachment 28683 [details] [review] Bug 12375 [4] - Replace use of DBI with Koha::Database in C4::Serials::NewIssue Currently, all serials enumeration data is stored in conglomerated fashion in serial.serialseq. This makes it extremely difficult to do any reporting based on the individual sequence values due to this fact. In addition to the formatted version of the sequence, we should be storing the individual values as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Create a new subscription, and add serials to it. The new x y and z fields should be populated with the same data the replaces {X} {Y} and {Z} in the enumeration field. 4) Inspect some existing serials. The update script will have made a best effort to recover the existing enumeration data to store in separate fields. Patch tested with a sandbox, by Paul Landers <paul.landers@ttuhsc.edu> Created attachment 28843 [details] [review] Bug 12375 [1] - Update database Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 28844 [details] [review] Bug 12375 [2] - Update Serial schema file Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 28845 [details] [review] Bug 12375 [3] - Store serials enumeration data in separate fields Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 28846 [details] [review] Bug 12375 [4] - Replace use of DBI with Koha::Database in C4::Serials::NewIssue Currently, all serials enumeration data is stored in conglomerated fashion in serial.serialseq. This makes it extremely difficult to do any reporting based on the individual sequence values due to this fact. In addition to the formatted version of the sequence, we should be storing the individual values as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Create a new subscription, and add serials to it. The new x y and z fields should be populated with the same data the replaces {X} {Y} and {Z} in the enumeration field. 4) Inspect some existing serials. The update script will have made a best effort to recover the existing enumeration data to store in separate fields. Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> If a numbering pattern uses {X}, {X}{Y} or {X}{Y}{Z}, in this order, then the updatedatabase works. But if I have a numbering pattern that uses only {Z} for example, then the Z part will be stored in serialseq_x (and future serials will have the Z part in serialseq_z column) If my numbering pattern uses only {X} and {Z}, the Z part will be in serialseq_y. I think this have to be fixed, so failing QA here. Also the original serialseq column is not removed. Is it intended ? I think we should not keep values in database that can be easily generated on the fly. Created attachment 29928 [details] [review] 28846 - Bug 12375 [5] [QA Followup] - Don't assume X Y and Z are used in order Created attachment 29929 [details] [review] Bug 12375 [5] [QA Followup] - Don't assume X Y and Z are used in order (In reply to Julian Maurice from comment #16) > If a numbering pattern uses {X}, {X}{Y} or {X}{Y}{Z}, in this order, then > the updatedatabase works. > But if I have a numbering pattern that uses only {Z} for example, then the Z > part will be stored in serialseq_x (and future serials will have the Z part > in serialseq_z column) > If my numbering pattern uses only {X} and {Z}, the Z part will be in > serialseq_y. > I think this have to be fixed, so failing QA here. Fixed! The attached qa followup should allow the database update to handle these situations. > Also the original serialseq column is not removed. Is it intended ? I think > we should not keep values in database that can be easily generated on the > fly. We need to keep the serialseq column, at least for the time being. Right now the new columns are for reporting. In the future we can also use them to improve the sorting of serials. In the long run we may want to deprecate serialseq, but each library would need to go through all their serials to verify the sequence values are correct before upgrading ( that is, the values may have been migrated from a previous ILS and need to be updated ). Ok, it works well now, but it generates a lot of warnings Use of uninitialized value $indexes[1] in hash element at installer/data/mysql/updatedatabase.pl line 8605. Use of uninitialized value $indexes[2] in hash element at installer/data/mysql/updatedatabase.pl line 8605. ... line 8605 for me is... ( undef, $enumeration_data{ $indexes[0] }, # <= ... here $enumeration_data{ $indexes[1] }, $enumeration_data{ $indexes[2] } ) = split( /__SPLIT__/, $serialseq ); Could you fix that, please ? Created attachment 30099 [details] [review] Bug 12375 [1] - Update database Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 30100 [details] [review] Bug 12375 [2] - Update Serial schema file Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 30101 [details] [review] Bug 12375 [3] - Store serials enumeration data in separate fields Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 30102 [details] [review] Bug 12375 [4] - Replace use of DBI with Koha::Database in C4::Serials::NewIssue Currently, all serials enumeration data is stored in conglomerated fashion in serial.serialseq. This makes it extremely difficult to do any reporting based on the individual sequence values due to this fact. In addition to the formatted version of the sequence, we should be storing the individual values as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Create a new subscription, and add serials to it. The new x y and z fields should be populated with the same data the replaces {X} {Y} and {Z} in the enumeration field. 4) Inspect some existing serials. The update script will have made a best effort to recover the existing enumeration data to store in separate fields. Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 30103 [details] [review] Bug 12375 [5] [QA Followup] - Don't assume X Y and Z are used in order Created attachment 30104 [details] [review] Bug 12375 [6] [QA Followup] - Supress warnings for undefined indexes This new followup should suppress those warnings. Please give it a try! (In reply to Julian Maurice from comment #20) > Ok, it works well now, but it generates a lot of warnings > > Use of uninitialized value $indexes[1] in hash element at > installer/data/mysql/updatedatabase.pl line 8605. > Use of uninitialized value $indexes[2] in hash element at > installer/data/mysql/updatedatabase.pl line 8605. > ... > > line 8605 for me is... > > ( > undef, > $enumeration_data{ $indexes[0] }, # <= ... here > $enumeration_data{ $indexes[1] }, > $enumeration_data{ $indexes[2] } > ) = split( /__SPLIT__/, $serialseq ); > > Could you fix that, please ? Great! Thanks. koha-qa is only complaining about a trailing whitespace in updatedatabase.pl which I'll fix in a follow-up patch. Passed QA. Created attachment 30108 [details] [review] Bug 12375: QA followup - remove trailing whitespace Created attachment 35155 [details] [review] Bug 12375 [1] - Update database Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 35156 [details] [review] Bug 12375 [2] - Update Serial schema file Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 35157 [details] [review] Bug 12375 [3] - Store serials enumeration data in separate fields Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 35158 [details] [review] Bug 12375 [4] - Replace use of DBI with Koha::Database in C4::Serials::NewIssue Currently, all serials enumeration data is stored in conglomerated fashion in serial.serialseq. This makes it extremely difficult to do any reporting based on the individual sequence values due to this fact. In addition to the formatted version of the sequence, we should be storing the individual values as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Create a new subscription, and add serials to it. The new x y and z fields should be populated with the same data the replaces {X} {Y} and {Z} in the enumeration field. 4) Inspect some existing serials. The update script will have made a best effort to recover the existing enumeration data to store in separate fields. Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 35159 [details] [review] Bug 12375 [5] [QA Followup] - Don't assume X Y and Z are used in order Created attachment 35160 [details] [review] Bug 12375 [6] [QA Followup] - Supress warnings for undefined indexes Created attachment 35161 [details] [review] Bug 12375: QA followup - remove trailing whitespace Kyle, could you please rebase this one? The updatedatabase patch was easy, but C4/Serials.pm got a bit more complicated. Created attachment 40710 [details] [review] Bug 12375 [1] - Update database Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 40711 [details] [review] Bug 12375 [2] - Update Serial schema file Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 40712 [details] [review] Bug 12375 [3] - Store serials enumeration data in separate fields Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 40713 [details] [review] Bug 12375 [4] - Replace use of DBI with Koha::Database in C4::Serials::NewIssue Currently, all serials enumeration data is stored in conglomerated fashion in serial.serialseq. This makes it extremely difficult to do any reporting based on the individual sequence values due to this fact. In addition to the formatted version of the sequence, we should be storing the individual values as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Create a new subscription, and add serials to it. The new x y and z fields should be populated with the same data the replaces {X} {Y} and {Z} in the enumeration field. 4) Inspect some existing serials. The update script will have made a best effort to recover the existing enumeration data to store in separate fields. Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 40714 [details] [review] Bug 12375 [5] [QA Followup] - Don't assume X Y and Z are used in order Created attachment 40715 [details] [review] Bug 12375 [6] [QA Followup] - Supress warnings for undefined indexes Created attachment 40716 [details] [review] Bug 12375: QA followup - remove trailing whitespace Created attachment 46122 [details] [review] Bug 12375 [1] - Update database Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 46123 [details] [review] Bug 12375 [2] - Update Serial schema file Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 46124 [details] [review] Bug 12375 [3] - Store serials enumeration data in separate fields Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 46125 [details] [review] Bug 12375 [4] - Replace use of DBI with Koha::Database in C4::Serials::NewIssue Currently, all serials enumeration data is stored in conglomerated fashion in serial.serialseq. This makes it extremely difficult to do any reporting based on the individual sequence values due to this fact. In addition to the formatted version of the sequence, we should be storing the individual values as well. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Create a new subscription, and add serials to it. The new x y and z fields should be populated with the same data the replaces {X} {Y} and {Z} in the enumeration field. 4) Inspect some existing serials. The update script will have made a best effort to recover the existing enumeration data to store in separate fields. Signed-off-by: Paul Landers <paul.landers@ttuhsc.edu> Created attachment 46126 [details] [review] Bug 12375 [5] [QA Followup] - Don't assume X Y and Z are used in order Created attachment 46127 [details] [review] Bug 12375 [6] [QA Followup] - Supress warnings for undefined indexes Created attachment 46128 [details] [review] Bug 12375 [7] - Update to use Koha::Object classes This code was written before the introduction of Koha::Object(s) Considering the triviality of updating the code, we should go ahead and switch this code to use Koha::Object(s) now. Pushed to master! Created attachment 46319 [details] [review] Bug 12375 - Add new serial fields to kohastructure *** Bug 13420 has been marked as a duplicate of this bug. *** Jenkins raises a warning: No method update! at /srv/jenkins/workspace/Koha_Master_D8/C4/Serials.pm line 1602. It seems that ->update should be replaced with ->store (In reply to Jonathan Druart from comment #55) > Jenkins raises a warning: > > No method update! at /srv/jenkins/workspace/Koha_Master_D8/C4/Serials.pm > line 1602. > > It seems that ->update should be replaced with ->store Will be fixed by bug 16692 |