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.
Created attachment 61645 [details] [review] 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 its uses in the codebase like this: $attribute->opac_editable for $attribute->type->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
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.
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 [];
Are you sure you restarted plack?
(In reply to Tomás Cohen Arazi from comment #4) > Are you sure you restarted plack? Yes. Several times. I even restarted the server.
(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.
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!
I forgot to mention that I can NOT reproduce without patches from this bug. Repeatable attribute saves fine from OPAC.
Created attachment 62403 [details] [review] 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 its uses in the codebase like this: $attribute->opac_editable for $attribute->type->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
Created attachment 62405 [details] [review] 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.
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?
Created attachment 62419 [details] [review] 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 its uses in the codebase like this: $attribute->opac_editable for $attribute->type->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: Marc Véron <veron@veron.ch>
Created attachment 62420 [details] [review] 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>
Created attachment 62628 [details] [review] 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
Created attachment 62629 [details] [review] 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>
I just re-did this patch following what we discussed with Jonathan. Please sign again if it works for you!
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>
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>
I feel that there is something wrong in all the logic, but cannot found something better. 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.
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.