Bug 20443 - Move C4::Members::Attributes to Koha namespace
Summary: Move C4::Members::Attributes to Koha namespace
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 20445 (view as bug list)
Depends on: 12159 20444
Blocks: 8326 15449 21083
  Show dependency treegraph
 
Reported: 2018-03-19 08:35 UTC by Josef Moravec
Modified: 2019-10-24 13:47 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2018-03-19 08:35:09 UTC

    
Comment 1 Jonathan Druart 2018-07-17 18:45:46 UTC
Here is a first draft of that work:
  https://gitlab.com/joubu/Koha/commits/bug_20443

It's quite a funky move:
 41 files changed, 713 insertions(+), 2017 deletions(-)

However some tests are missing for trivial methods added to Koha::Patrons.

Any remarks / suggestions?
Comment 2 Jonathan Druart 2018-08-08 13:41:29 UTC
*** Bug 20445 has been marked as a duplicate of this bug. ***
Comment 3 Martin Renvoize 2018-08-09 14:23:41 UTC
First QA comment.. and it's possibly a little mean..

You've introduced three 'filter_by_' methods: Awesome++.

However, there's an inconsistency with their chainability.. I personally feel such a method should always allow chaining and as such would request, although currently, you don't use them within a chain, that `filter_by_attribute_type` and `filter_by_attribute_value` are altered such that they act upon self rather than instantiating a new object to pass as the return.
Comment 4 Martin Renvoize 2018-08-09 14:32:52 UTC
I don't see the above as a blocker however and I'm happy to submit it as a qa followup myself once there are patches here ;)
Comment 5 Jonathan Druart 2018-08-09 14:55:23 UTC
Remote branch has been rebased against master.
Comment 6 Tomás Cohen Arazi 2018-08-09 15:13:35 UTC
I don't think we can break GetPatronInfo (ILS-DI) like this. We should keep the current behaviour and propose a behaviour change on a separate bug.
Comment 7 Tomás Cohen Arazi 2018-08-09 15:34:27 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I don't think we can break GetPatronInfo (ILS-DI) like this. We should keep
> the current behaviour and propose a behaviour change on a separate bug.

This:

my $attrs = $patron->extended_attributes->search({ opac_display => 1 })->unblessed;

should be:

my $attrs = grep { $_->type->opac_display } @{ $patron->extended_attributes->search };
my @attrs_unblessed = map { $_->unblessed } @{ $attrs };

Highlighted by:
k$ prove t/db_dependent/ILSDI_Services.t
t/db_dependent/ILSDI_Services.t .. 1/4 DBD::mysql::st execute failed: Unknown column 'opac_display' in 'where clause'...
Comment 9 Jonathan Druart 2018-08-09 22:56:30 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I don't think we can break GetPatronInfo (ILS-DI) like this. We should keep
> the current behaviour and propose a behaviour change on a separate bug.

I have pushed a follow-up to fix that.

NOTES:
1. I do not think we should display info from the attribute's type
2. We should test the value of the description if linked with AV (value_description). Are we sure we want "attribute" vs "value". And "value_description"? Let's stick to current behaviours for now.
Comment 10 Josef Moravec 2019-03-07 07:26:16 UTC
I just rebased this on top of current code in bug 12159, see https://gitlab.com/josef.moravec/Koha/tree/bug_20443
Comment 11 Josef Moravec 2019-03-07 07:27:33 UTC
Tests not passing (at least):
t/db_dependent/Koha/Patrons/Import.t
t/db_dependent/Auth_with_ldap.t
Comment 12 Jonathan Druart 2019-04-29 16:20:30 UTC
Hi Josef, thanks for the rebase.
I have also rebased them against master (so hard!) and added a fix for the Auth_with_ldap.t failure.
I am stuck with the other failure.

I think we need to know if we (dev team) want this patchset. If so we will need to have a task force to have it cleaned and pushed at the beginning of the next release cycle. It will be impossible to rebase this branch for months.

Who's in?
Comment 13 Tomás Cohen Arazi 2019-04-29 18:07:16 UTC
(In reply to Jonathan Druart from comment #12)
> Hi Josef, thanks for the rebase.
> I have also rebased them against master (so hard!) and added a fix for the
> Auth_with_ldap.t failure.
> I am stuck with the other failure.
> 
> I think we need to know if we (dev team) want this patchset. If so we will
> need to have a task force to have it cleaned and pushed at the beginning of
> the next release cycle. It will be impossible to rebase this branch for
> months.
> 
> Who's in?

I'm in
Comment 14 Katrin Fischer 2019-04-29 20:24:59 UTC
Do we have a choice? 

We decided on the move to the new namespace as a community... so I think we don't. I'll help where I can.
Comment 15 Jonathan Druart 2019-08-05 14:56:45 UTC
Remote branch rebased against master.
Comment 16 Martin Renvoize 2019-10-24 13:47:01 UTC
Lets schedule this for the beginning of month two in the next cycle.. that way it's early enough to push it and catch any fallout during the cycle, but late enough that we're not unduly making pain for the next stable maintainer whose trying to backport bugfixes ;)