Bug 18339 - Koha::Patron::Attribute ->opac_editable and ->opac_display should be removed
Summary: Koha::Patron::Attribute ->opac_editable and ->opac_display should be removed
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 17828 18182
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-27 15:58 UTC by Tomás Cohen Arazi
Modified: 2017-04-26 19:28 UTC (History)
2 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18339: Remove ->opac_editable ->opac_display from Koha::Patron::Attribute (5.97 KB, patch)
2017-03-27 16:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18339: Remove ->opac_editable ->opac_display from Koha::Patron::Attribute (6.93 KB, patch)
2017-04-19 17:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18339: (followup) Remove warnings on adding a patron modification (1.37 KB, patch)
2017-04-19 17:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18339: Remove ->opac_editable ->opac_display from Koha::Patron::Attribute (7.06 KB, patch)
2017-04-19 19:47 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18339: (followup) Remove warnings on adding a patron modification (1.50 KB, patch)
2017-04-19 19:48 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18339: Remove ->opac_editable ->opac_display from Koha::Patron::Attribute (12.58 KB, patch)
2017-04-24 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18339: (followup) Remove warnings on adding a patron modification (1.50 KB, patch)
2017-04-24 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18339: Remove ->opac_editable ->opac_display from Koha::Patron::Attribute (12.64 KB, patch)
2017-04-25 12:29 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18339: (followup) Remove warnings on adding a patron modification (1.56 KB, patch)
2017-04-25 12:29 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2017-03-27 15:58:41 UTC
Now that bug 17828 introduces the ->type method, returning the related Koha::Patron::Attribute::Type object, there's no need for such specific method.

It should be called $patron->type->opac_editable instead.
Comment 1 Tomás Cohen Arazi 2017-03-27 16:12:21 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2017-03-27 19:44:16 UTC
Tests pass fine, but if I make an attribute editable and try to edit /submit changes in OPAC, I get an Internal server error.
Plack log says:
The method opac_editable is not covered by tests!

Without 17828 and 18339 applied, I can edit and submit the attribute from OPAC.
Comment 3 Marc Véron 2017-03-27 19:56:42 UTC
Maybe the following, with the change below I can save without getting an Internal Server error:

diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl
index 9f2a607..bc54508 100755
--- a/opac/opac-memberentry.pl
+++ b/opac/opac-memberentry.pl
@@ -535,7 +535,7 @@ sub GeneratePatronAttributesForm {
     my ( $borrowernumber, $entered_attributes ) = @_;
 
     # Get all attribute types and the values for this patron (if applicable)
-    my @types = grep { $_->opac_editable() or $_->opac_display }
+    my @types = grep { $_->opac_editable or $_->opac_display }
         Koha::Patron::Attribute::Types->search()->as_list();
     if ( scalar(@types) == 0 ) {
         return [];
Comment 4 Tomás Cohen Arazi 2017-03-27 19:58:37 UTC
Are you sure you restarted plack?
Comment 5 Marc Véron 2017-03-27 20:46:07 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> Are you sure you restarted plack?

Yes. Several times. I even restarted the server.
Comment 6 Owen Leonard 2017-04-18 14:15:16 UTC
(In reply to Marc Véron from comment #2)
> Tests pass fine, but if I make an attribute editable and try to edit /submit
> changes in OPAC, I get an Internal server error.
> Plack log says:
> The method opac_editable is not covered by tests!

I get the same thing.
Comment 7 Marc Véron 2017-04-19 16:25:01 UTC
Re-tested on a brand new install, only mandatory db data installed, created a superlibrarian user and minimum of patron related data.

- Added non repeatable patron atribute, show/edit in OPAC allowed
Edited in OPAC, works fine

- Added repeatable patron atribute, show/edit in OPAC allowed

Try to edit in OPAC, after clicking 'Submit update request':
Internal server error
Plack error log: The method opac-editable is ot covered by tests!
Comment 8 Marc Véron 2017-04-19 16:30:00 UTC
I forgot to mention that I can NOT reproduce without patches from this bug. Repeatable attribute saves fine from OPAC.
Comment 9 Tomás Cohen Arazi 2017-04-19 17:39:15 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2017-04-19 17:49:41 UTC Comment hidden (obsolete)
Comment 11 Marc Véron 2017-04-19 19:40:45 UTC
No more internal server error when editing in OPAC. I tested repeatable and non repeatable text fields and a YES_NO field.

However I get an internal server error in staff client (approving). I can trigger it by entering text with äöü etc.

E.g. with a text like 111ä222 I get the following in plack error log:

malformed UTF-8 character in JSON string, at character offset 14 (before "\x{fffd}2","code":"T...") at /home/marc/koha/Koha/Patron/Modifications.pm line 100.

I can reproduce it with and without patches. Should that go to a separate Bug?
Comment 12 Marc Véron 2017-04-19 19:47:14 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2017-04-19 19:48:31 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2017-04-24 18:19:20 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2017-04-24 18:19:27 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2017-04-24 18:21:30 UTC
I just re-did this patch following what we discussed with Jonathan.
Please sign again if it works for you!
Comment 17 Owen Leonard 2017-04-25 12:29:40 UTC
Created attachment 62662 [details] [review]
[SIGNED-OFF] Bug 18339: Remove ->opac_editable ->opac_display from Koha::Patron::Attribute

Trying to simplify the implementation of 13757, I replaced some legacy code
in favour of a Koha::Object-based implementation for the OPAC controller scripts.
On doing so I went too far with this methods, as what was needed was a way to retrieve
the type of the attribute and ask it about such properties.

This patch removes those methods, its tests, and replaces them for new methods in the
Koha::Patron::Attributes class:
- K:P:A->search_opac_displayable
- K:P:A->search_opac_editable

To test:
- Run
  $ prove t/db_dependent/Koha/Patron/Attributes.t
=> SUCCESS: Tests still pass
- Try changing the opac_editable and opac_display properties and play in the OPAC with those attributes.
Verify that everything works as expected.
- Sign off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 18 Owen Leonard 2017-04-25 12:29:43 UTC
Created attachment 62663 [details] [review]
[SIGNED-OFF] Bug 18339: (followup) Remove warnings on adding a patron modification

This patch removes warnings raised because hash values existence is not
checked before comparing them.

As the sub is deleting fields that are similar from the modification, I considered
the following use cases
- Both fields are defined, need to compare values, should delete if they match
- One of the fields is defined, the other isn't, discrepancy, should not delete.
- Both fields are not defined, they match, should delete.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Jonathan Druart 2017-04-26 19:24:57 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2017-04-26 19:28:06 UTC
I feel that there is something wrong in the controller code, but cannot find something better.
It sounds like there is too much logic and the template could receive the whole list of attributes and check opac_display to know if we display it.
But out of the scope for sure :)

Anyway, I found a bug. Edit your detail (OPAC), do not change anything, submit: it will create an empty change request.
I do not recreate on master.