Currently we have 3 subroutines to retrieve the columns of tables: C4::Members::columns, C4::Items::columns and C4::Items::biblioitems_columns. It will be easier to have a Koha::Objects->columns method to provide the different classes to retrieve their related column names.
Created attachment 53240 [details] [review] Bug 16889: Add Koha::Objects->columns Currently we have 3 subroutines to retrieve the columns of tables: C4::Members::columns, C4::Items::columns and C4::Items::biblioitems_columns. It is easier to have a Koha::Objects->columns method to provide the different classes to retrieve their related column names.
Created attachment 53241 [details] [review] Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead Test plan: Import some patrons (tools/import_borrowers.pl) and make sure it imports the patrons correctly.
Created attachment 53243 [details] [review] Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead Test plan: Add new rule to the "Automatic item modifications by age" tools (tools/automatic_item_modification_by_age.pl) and make sure the columns of the items table are correctly displayed.
Created attachment 53244 [details] [review] Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead Test plan: Add new rule to the "Automatic item modifications by age" tools (tools/automatic_item_modification_by_age.pl) and make sure the columns of the biblioitems table are correctly displayed in the 'Conditions' dropdown list.
Comment on attachment 53241 [details] [review] Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead Review of attachment 53241 [details] [review]: ----------------------------------------------------------------- ::: C4/Members.pm @@ -583,5 @@ > - else { > - @data = (); > - } > - return @data; > -} Farewell, my baby.
Comment on attachment 53243 [details] [review] Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead Review of attachment 53243 [details] [review]: ----------------------------------------------------------------- ::: C4/Items.pm @@ +3067,4 @@ > > my $dbh = C4::Context->dbh; > my @errors; > + my @item_columns = map { "items.$_" } Koha::Items->columns; This should be triggered by t/db_dependent/Items/AutomaticItemModificationByAge.t as a test.
Commit #1: Test in patch is sufficient to test. Commit #2: Yes, I can get into Tools->Import Patrons just fine. Couldn't find any other instance of C4::Members::columns, so this patch has it covered as far as I can tell. Commit #3: The Items.pm change is tested enough by prove t/db_dependent/Items/AutomaticItemModificationByAge.t The automatic item modification by age tool removing a rule is unintuitive. Code eyeballs well enough.
Created attachment 53292 [details] [review] Bug 16889: Add Koha::Objects->columns Currently we have 3 subroutines to retrieve the columns of tables: C4::Members::columns, C4::Items::columns and C4::Items::biblioitems_columns. It is easier to have a Koha::Objects->columns method to provide the different classes to retrieve their related column names. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 53293 [details] [review] Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead Test plan: Import some patrons (tools/import_borrowers.pl) and make sure it imports the patrons correctly. NOTE: *sniff sniff* Good bye. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 53294 [details] [review] Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead Test plan: Add new rule to the "Automatic item modifications by age" tools (tools/automatic_item_modification_by_age.pl) and make sure the columns of the items table are correctly displayed. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 53295 [details] [review] Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead Test plan: Add new rule to the "Automatic item modifications by age" tools (tools/automatic_item_modification_by_age.pl) and make sure the columns of the biblioitems table are correctly displayed in the 'Conditions' dropdown list. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
(In reply to M. Tompsett from comment #7) > Commit #1: Test in patch is sufficient to test. I don't understand, what do you suggest? The method is covered by the test in Objects.t. I have arbitrary chosen to test the Patron module, but I don't see the point to add the same test for each module based on Koha::Object.
(In reply to Jonathan Druart from comment #12) > (In reply to M. Tompsett from comment #7) > > Commit #1: Test in patch is sufficient to test. > > I don't understand, what do you suggest? I was just stating what I did to test the patches. Yes, it was arbitrary that you chose Patrons, but anything would be sufficient to trigger the columns code, which is the point of the test. Nothing wrong. Just a statement. I'm hoping this moves ahead to QA without those sorts of misunderstandings. Sorry I was unclear.
Created attachment 53379 [details] [review] Bug 16889: Add Koha::Objects->columns Currently we have 3 subroutines to retrieve the columns of tables: C4::Members::columns, C4::Items::columns and C4::Items::biblioitems_columns. It is easier to have a Koha::Objects->columns method to provide the different classes to retrieve their related column names. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 53380 [details] [review] Bug 16889: Add Koha::Objects->columns Currently we have 3 subroutines to retrieve the columns of tables: C4::Members::columns, C4::Items::columns and C4::Items::biblioitems_columns. It is easier to have a Koha::Objects->columns method to provide the different classes to retrieve their related column names. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 53381 [details] [review] Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead Test plan: Import some patrons (tools/import_borrowers.pl) and make sure it imports the patrons correctly. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 53382 [details] [review] Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead Test plan: Add new rule to the "Automatic item modifications by age" tools (tools/automatic_item_modification_by_age.pl) and make sure the columns of the items table are correctly displayed. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 53383 [details] [review] Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead Test plan: Add new rule to the "Automatic item modifications by age" tools (tools/automatic_item_modification_by_age.pl) and make sure the columns of the biblioitems table are correctly displayed in the 'Conditions' dropdown list. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 16.11, thanks Jonathan!
When adding a new member, pressing save, this returns to the same page. This broke master. Not sure what the status should be.
Sorry. Wrong bug, meant 16909.
Wrote to bug 16889 by mistake. This patch broke add new member. When pressing save, we cycle back to the page. Not sure what to do with the status and importance, but put in "blocker" since, well, it is kind of a big problem.
... I'm an idiot. Sorry for polluting this bug again. Going to 16909 now.