Bug 16889 - Move the ::columns subroutines to Koha::Objects->columns
Summary: Move the ::columns subroutines to Koha::Objects->columns
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 16917
Blocks: 16846
  Show dependency treegraph
 
Reported: 2016-07-09 13:46 UTC by Jonathan Druart
Modified: 2017-12-07 22:15 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16889: Add Koha::Objects->columns (1.90 KB, patch)
2016-07-09 13:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead (3.12 KB, patch)
2016-07-09 13:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead (2.38 KB, patch)
2016-07-09 13:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead (2.57 KB, patch)
2016-07-09 13:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16889: Add Koha::Objects->columns (1.95 KB, patch)
2016-07-12 01:38 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead (3.17 KB, patch)
2016-07-12 01:38 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead (2.43 KB, patch)
2016-07-12 01:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead (2.62 KB, patch)
2016-07-12 01:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16889: Add Koha::Objects->columns (2.00 KB, patch)
2016-07-13 15:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16889: Add Koha::Objects->columns (2.00 KB, patch)
2016-07-13 15:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16889: Remove C4::Members::columns and use Koha::Patrons->columns instead (3.22 KB, patch)
2016-07-13 15:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16889: Remove C4::Items::columns and use Koha::Items->columns instead (2.49 KB, patch)
2016-07-13 15:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16889: Remove C4::Items::biblioitems_columns and use Koha::Biblioitems->columns instead (2.67 KB, patch)
2016-07-13 15:45 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. 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 M. 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 M. 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 M. Tompsett 2016-07-12 01:38:40 UTC Comment hidden (obsolete)
Comment 9 M. Tompsett 2016-07-12 01:38:56 UTC Comment hidden (obsolete)
Comment 10 M. Tompsett 2016-07-12 01:39:08 UTC Comment hidden (obsolete)
Comment 11 M. 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 M. 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.