Bug 16889

Summary: Move the ::columns subroutines to Koha::Objects->columns
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: kyle, mtompset, philippe.blouin
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16917    
Bug Blocks: 16846    
Attachments: Bug 16889: Add Koha::Objects->columns
Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead
Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead
Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead
Bug 16889: Add Koha::Objects->columns
Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead
Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead
Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead
Bug 16889: Add Koha::Objects->columns
Bug 16889: Add Koha::Objects->columns
Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead
Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead
Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead

Description Jonathan Druart 2016-07-09 13:46:32 UTC
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.
Comment 1 Jonathan Druart 2016-07-09 13:54:26 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-07-09 13:54:29 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-07-09 13:54:32 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-07-09 13:54:37 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2016-07-12 01:12:34 UTC
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 6 Mark Tompsett 2016-07-12 01:12:49 UTC
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.
Comment 7 Mark Tompsett 2016-07-12 01:36:20 UTC
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&#4: 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.
Comment 8 Mark Tompsett 2016-07-12 01:38:40 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2016-07-12 01:38:56 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2016-07-12 01:39:08 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2016-07-12 01:39:16 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-07-12 17:22:57 UTC
(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.
Comment 13 Mark Tompsett 2016-07-13 02:15:56 UTC
(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.
Comment 14 Kyle M Hall 2016-07-13 15:44:46 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2016-07-13 15:44:58 UTC
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>
Comment 16 Kyle M Hall 2016-07-13 15:45:04 UTC
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>
Comment 17 Kyle M Hall 2016-07-13 15:45:07 UTC
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>
Comment 18 Kyle M Hall 2016-07-13 15:45:10 UTC
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>
Comment 19 Kyle M Hall 2016-07-15 18:13:26 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 20 Blou 2016-07-21 12:51:07 UTC
When adding a new member, pressing save, this returns to the same page.

This broke master.  Not sure what the status should be.
Comment 21 Blou 2016-07-21 12:55:08 UTC
Sorry. Wrong bug, meant 16909.
Comment 22 Blou 2016-07-21 12:57:35 UTC
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.
Comment 23 Blou 2016-07-21 13:55:41 UTC
...  I'm an idiot.  Sorry for polluting this bug again.  Going to 16909 now.