[Bug 18339] New: Koha::Patron::Attribute ->opac_editable and -> opac_display should be removed
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Bug ID: 18339 Summary: Koha::Patron::Attribute ->opac_editable and ->opac_display should be removed Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Assignee|gmcharlt@gmail.com |tomascohen@gmail.com Depends on| |17828 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17828 [Bug 17828] Koha::Patron::Attribute->store should raise an exception if unique_id is being broken -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 61645 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61645&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Change sponsored?|--- |Sponsored -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch --- Comment #2 from Marc Véron <veron@veron.ch> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #3 from Marc Véron <veron@veron.ch> --- 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 []; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Are you sure you restarted plack? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #5 from Marc Véron <veron@veron.ch> --- (In reply to Tomás Cohen Arazi from comment #4)
Are you sure you restarted plack?
Yes. Several times. I even restarted the server. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Bug 18339 depends on bug 17828, which changed state. Bug 17828 Summary: Koha::Patron::Attribute->store should raise an exception if unique_id is being broken https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17828 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #6 from Owen Leonard <oleonard@myacpl.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #7 from Marc Véron <veron@veron.ch> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #8 from Marc Véron <veron@veron.ch> --- I forgot to mention that I can NOT reproduce without patches from this bug. Repeatable attribute saves fine from OPAC. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #61645|0 |1 is obsolete| | --- Comment #9 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 62403 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62403&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 62405 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62405&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #11 from Marc Véron <veron@veron.ch> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62403|0 |1 is obsolete| | --- Comment #12 from Marc Véron <veron@veron.ch> --- Created attachment 62419 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62419&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62405|0 |1 is obsolete| | --- Comment #13 from Marc Véron <veron@veron.ch> --- Created attachment 62420 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62420&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18461 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62419|0 |1 is obsolete| | --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 62628 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62628&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62420|0 |1 is obsolete| | --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 62629 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62629&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |18182 Status|Signed Off |Needs Signoff --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I just re-did this patch following what we discussed with Jonathan. Please sign again if it works for you! Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18182 [Bug 18182] TestBuilder should be able to return Koha::Object objects -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|Trivial patch |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62628|0 |1 is obsolete| | Attachment #62629|0 |1 is obsolete| | --- Comment #17 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 62662 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62662&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #18 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 62663 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62663&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18339 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org