Bug 13757 - Make patron attributes editable in the opac if set to 'editable in OPAC'
Summary: Make patron attributes editable in the opac if set to 'editable in OPAC'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 14659 17767 17792
Blocks: 17828 18461
  Show dependency treegraph
 
Reported: 2015-02-24 15:27 UTC by David Roberts
Modified: 2017-04-24 16:54 UTC (History)
12 users (show)

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


Attachments
Bug 13757 - Make patron attributes editable in the OPAC (1/2) (10.44 KB, patch)
2015-09-02 23:57 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 13757 - (followup) fix creation of patron attributes (1.09 KB, patch)
2015-09-17 00:42 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 13757 - Make patron attributes editable in the OPAC (2/2) (16.65 KB, patch)
2015-09-17 00:42 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 13757 - Make patron attributes editable in the OPAC (2/2) (18.15 KB, patch)
2016-02-03 22:29 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13757 - Make patron attributes editable in the OPAC (2/2) (18.26 KB, patch)
2016-02-14 19:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13757 - Make patron attributes editable in the OPAC (1/2) (10.52 KB, patch)
2016-02-14 20:00 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13757 - Make patron attributes editable in the OPAC (1/2) (10.52 KB, patch)
2016-02-14 20:01 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13757 - (followup) fix creation of patron attributes (1.16 KB, patch)
2016-02-14 20:01 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 13757: Make patron attributes editable in the OPAC (1/2) (10.49 KB, patch)
2016-02-25 22:31 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 13757: Make patron attributes editable in the OPAC (2/2) (17.92 KB, patch)
2016-02-25 22:31 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 13757: (followup) fix creation of patron attributes (1.14 KB, patch)
2016-02-25 22:31 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 13757: (QA followup) fix QA issues, add small niceties (6.65 KB, patch)
2016-02-25 22:32 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 13757: Make patron attributes editable in the OPAC (1/2) (10.46 KB, patch)
2016-10-27 14:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Make patron attributes editable in the OPAC (2/2) (16.81 KB, patch)
2016-10-27 14:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) fix creation of patron attributes (1.12 KB, patch)
2016-10-27 14:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (QA followup) fix QA issues, add small niceties (5.51 KB, patch)
2016-10-27 14:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Rebase against master (5.00 KB, patch)
2016-10-27 14:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: DBIC update (1.61 KB, patch)
2016-10-27 14:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Make patron attributes editable in the OPAC (1/2) (10.46 KB, patch)
2016-11-01 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Make patron attributes editable in the OPAC (2/2) (16.86 KB, patch)
2016-11-01 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) fix creation of patron attributes (1.17 KB, patch)
2016-11-01 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (QA followup) fix QA issues, add small niceties (5.57 KB, patch)
2016-11-01 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Rebase against master (4.93 KB, patch)
2016-11-01 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: DBIC update (1.67 KB, patch)
2016-11-01 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Add the option to set patron attributes editable in the OPAC (10.01 KB, patch)
2016-12-16 14:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: DB update (2.26 KB, patch)
2016-12-16 14:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: OPAC changes (16.08 KB, patch)
2016-12-16 14:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: DBIC update (1.67 KB, patch)
2016-12-16 14:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Add the option to set patron attributes editable in the OPAC (9.98 KB, patch)
2016-12-21 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: DB update (2.25 KB, patch)
2016-12-21 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: OPAC changes (16.07 KB, patch)
2016-12-21 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: DBIC update (1.67 KB, patch)
2016-12-21 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Add extended attributes to the patron modification (2.15 KB, patch)
2016-12-21 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Remove warnings (1.61 KB, patch)
2016-12-21 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Staff interface changes (10.49 KB, patch)
2016-12-21 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Only touch opac_editable attributes (4.37 KB, patch)
2016-12-26 14:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Regression tests for ->approve changes (2.39 KB, patch)
2016-12-26 14:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable (1.37 KB, patch)
2017-01-03 20:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Staff interface changes (10.57 KB, patch)
2017-01-09 17:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Add the option to set patron attributes editable in the OPAC (9.98 KB, patch)
2017-01-09 18:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: DB update (2.30 KB, patch)
2017-01-09 18:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: OPAC changes (16.12 KB, patch)
2017-01-09 18:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: DBIC update (1.67 KB, patch)
2017-01-09 18:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Add extended attributes to the patron modification (2.21 KB, patch)
2017-01-09 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Remove warnings (1.67 KB, patch)
2017-01-09 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Staff interface changes (10.57 KB, patch)
2017-01-09 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Only touch opac_editable attributes (4.41 KB, patch)
2017-01-09 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Regression tests for ->approve changes (2.44 KB, patch)
2017-01-09 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable (1.42 KB, patch)
2017-01-09 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (QA followup) Check DB structure before altering table (2.07 KB, patch)
2017-01-20 14:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (regression tests) Empty attributes should delete existing (2.34 KB, patch)
2017-02-01 13:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Make Koha::Patron::Modification->store del empty attrs (1.56 KB, patch)
2017-02-01 13:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (QA followup) Make opac-memberentry.pl handle attrs deletion (2.42 KB, patch)
2017-02-01 13:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (QA followup) Fix non-editable attrs on failed save (10.53 KB, patch)
2017-02-02 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (QA followup) Fix non-editable attrs on failed save (14.91 KB, patch)
2017-02-03 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Make K::P::Modifications->pending return K::P::Attribute (6.46 KB, patch)
2017-02-03 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Better display for attr changes in members-update.pl (9.14 KB, patch)
2017-02-03 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (regression test) Attribute with value 0 should be stored (1.92 KB, patch)
2017-02-16 19:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Attribute with value 0 should be stored (2.51 KB, patch)
2017-02-16 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Add the option to set patron attributes editable in the OPAC (10.04 KB, patch)
2017-03-15 20:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: DB update (2.36 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: OPAC changes (16.18 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: DBIC update (1.72 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: Add extended attributes to the patron modification (2.26 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (followup) Remove warnings (1.72 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (followup) Staff interface changes (10.63 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (followup) Only touch opac_editable attributes (4.47 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (followup) Regression tests for ->approve changes (2.50 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable (1.47 KB, patch)
2017-03-15 20:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (QA followup) Check DB structure before altering table (2.12 KB, patch)
2017-03-15 20:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (regression tests) Empty attributes should delete existing (2.39 KB, patch)
2017-03-15 20:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: Make Koha::Patron::Modification->store del empty attrs (1.61 KB, patch)
2017-03-15 20:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (QA followup) Make opac-memberentry.pl handle attrs deletion (2.47 KB, patch)
2017-03-15 20:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (QA followup) Fix non-editable attrs on failed save (14.95 KB, patch)
2017-03-15 20:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: Make K::P::Modifications->pending return K::P::Attribute (6.50 KB, patch)
2017-03-15 20:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: Better display for attr changes in members-update.pl (9.17 KB, patch)
2017-03-15 20:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (regression test) Attribute with value 0 should be stored (1.96 KB, patch)
2017-03-15 20:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: Attribute with value 0 should be stored (2.56 KB, patch)
2017-03-15 20:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13757: (QA followup) Exclude empty attributes from rendering if non-editable (2.36 KB, patch)
2017-03-22 19:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: (QA followup) Filter out non-editable params before storing (2.47 KB, patch)
2017-03-22 21:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13757: Add the option to set patron attributes editable in the OPAC (10.12 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: DB update (2.43 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: OPAC changes (16.25 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: DBIC update (1.79 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: Add extended attributes to the patron modification (2.33 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (followup) Remove warnings (1.79 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (followup) Staff interface changes (10.70 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (followup) Only touch opac_editable attributes (4.54 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (followup) Regression tests for ->approve changes (2.57 KB, patch)
2017-03-23 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable (1.54 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (QA followup) Check DB structure before altering table (2.19 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (regression tests) Empty attributes should delete existing (2.46 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: Make Koha::Patron::Modification->store del empty attrs (1.69 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (QA followup) Make opac-memberentry.pl handle attrs deletion (2.54 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (QA followup) Fix non-editable attrs on failed save (15.02 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: Make K::P::Modifications->pending return K::P::Attribute (6.58 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: Better display for attr changes in members-update.pl (9.25 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (regression test) Attribute with value 0 should be stored (2.04 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: Attribute with value 0 should be stored (2.63 KB, patch)
2017-03-23 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (QA followup) Exclude empty attributes from rendering if non-editable (2.42 KB, patch)
2017-03-23 16:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13757: (QA followup) Filter out non-editable params before storing (2.53 KB, patch)
2017-03-23 16:00 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Roberts 2015-02-24 15:27:46 UTC
Following on from Bug 12718 where Patron Extended Attributes were made visible in the My Details page of the OPAC, it would be useful if these could be editable by the patron, instead of just viewable. 

This should only apply to any attributes that are configured to display in the OPAC.
Comment 1 Marc Véron 2015-02-24 17:43:36 UTC
This would be very useful, e.g. for managing newsletters.

I think we should have two settings in /cgi-bin/koha/admin/patron-attr-types.pl

- Viewable but not not editable:
Display in OPAC: [] Check to display this attribute on a patron's details page in the OPAC. 

- Viewable and editable:
Edit in OPAC: [] Check to display and allow to edit this attribute on a patron's details page in the OPAC.
Comment 2 Jesse Weaver 2015-09-02 23:57:13 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2015-09-03 13:35:57 UTC
Hi Jesse,

I tested this first patch because I'm looking forward for this enhancement.

After applying I can no longer create new attributes. 
This is what I get in intranet-error.log after I hit "Save":

[Thu Sep 03 15:24:22 2015] [error] [client 10.0.0.85] [Thu Sep  3 15:24:22 2015] patron-attr-types.pl: DBD::mysql::st bind_param failed: Illegal parameter number at /usr/share/kohaclone/C4/Members/AttributeTypes.pm line 243., referer: http://10.0.0.31:8080/cgi-bin/koha/admin/patron-attr-types.pl?op=add_attribute_type
[Thu Sep 03 15:24:22 2015] [error] [client 10.0.0.85] [Thu Sep  3 15:24:22 2015] patron-attr-types.pl: DBD::mysql::st execute failed: Column count doesn't match value count at row 1 at /usr/share/kohaclone/C4/Members/AttributeTypes.pm line 244., referer: http://10.0.0.31:8080/cgi-bin/koha/admin/patron-attr-types.pl?op=add_attribute_type
[Thu Sep 03 15:24:22 2015] [error] [client 10.0.0.85] [Thu Sep  3 15:24:22 2015] patron-attr-types.pl: Argument "" isn't numeric in numeric gt (>) at /usr/share/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt line 336., referer: http://10.0.0.31:8080/cgi-bin/koha/admin/patron-attr-types.pl?op=add_attribute_type
[Thu Sep 03 15:24:22 2015] [error] [client 10.0.0.85] [Thu Sep  3 15:24:22 2015] patron-attr-types.pl: Argument "" isn't numeric in numeric gt (>) at /usr/share/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt line 336., referer: http://10.0.0.31:8080/cgi-bin/koha/admin/patron-attr-types.pl?op=add_attribute_type

However, I can set and save the value "Display in OPAC" on existing entries (and I see them in the table borrower_attribute_types).
Comment 4 Jesse Weaver 2015-09-17 00:42:07 UTC Comment hidden (obsolete)
Comment 5 Jesse Weaver 2015-09-17 00:42:19 UTC Comment hidden (obsolete)
Comment 6 Aleisha Amohia 2016-02-03 22:29:47 UTC Comment hidden (obsolete)
Comment 7 Aleisha Amohia 2016-02-03 22:31:54 UTC
Hi, I can't sign-off on this

Any changes I make to my patron attribute type (ie unchecking the display box or checking the editing box) don't actually change. I'll check or uncheck the box and click save and it will say it's been modified, but if I go in to edit it again my changes were forgotten.
Comment 8 Jesse Weaver 2016-02-12 23:05:49 UTC
Could I ask you to check your error log after editing/saving? And, since I neglected to mention it in the test plan, did you run updatedatabase.pl?
Comment 9 Aleisha Amohia 2016-02-14 19:20:11 UTC Comment hidden (obsolete)
Comment 10 Aleisha Amohia 2016-02-14 20:00:42 UTC Comment hidden (obsolete)
Comment 11 Aleisha Amohia 2016-02-14 20:01:23 UTC Comment hidden (obsolete)
Comment 12 Aleisha Amohia 2016-02-14 20:01:32 UTC Comment hidden (obsolete)
Comment 13 Aleisha Amohia 2016-02-14 20:02:58 UTC
Ah sorry for all the comments git kept timing out, but I am signing off
Comment 14 Jonathan Druart 2016-02-16 10:46:37 UTC
 FAIL   C4/Members/AttributeTypes.pm
   FAIL   pod
                *** ERROR: Spurious =cut command  in file C4/Members/AttributeTypes.pm
 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
   FAIL   forbidden patterns
                forbidden pattern: merge marker (=======) (line 953)
                forbidden pattern: merge marker (<<<<<<<) (line 937)
                forbidden pattern: merge marker (>>>>>>>) (line 971)

There are also debug warns, please remove them.
Comment 15 Jesse Weaver 2016-02-25 22:31:39 UTC Comment hidden (obsolete)
Comment 16 Jesse Weaver 2016-02-25 22:31:47 UTC Comment hidden (obsolete)
Comment 17 Jesse Weaver 2016-02-25 22:31:54 UTC Comment hidden (obsolete)
Comment 18 Jesse Weaver 2016-02-25 22:32:54 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-03-01 14:11:06 UTC
It does not seem to work correctly, I'd like to get another signoff on this one.
Comment 20 Owen Leonard 2016-04-06 13:41:44 UTC
When I submit a change in the OPAC the page is redisplayed with a "No changes were made" message and the extended attribute fields have disappeared from the form.
Comment 21 Marc Véron 2016-06-01 20:51:18 UTC
Patch does not apply:
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/css/staff-global.css
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging admin/patron-attr-types.pl
Auto-merging C4/Members/AttributeTypes.pm
CONFLICT (content): Merge conflict in C4/Members/AttributeTypes.pm
Failed to merge in the changes.
Comment 22 Tomás Cohen Arazi 2016-10-27 14:53:14 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2016-10-27 14:53:22 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2016-10-27 14:53:33 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2016-10-27 14:53:42 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2016-10-27 14:53:50 UTC Comment hidden (obsolete)
Comment 27 Tomás Cohen Arazi 2016-10-27 14:53:57 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2016-10-27 16:54:01 UTC
I rebased them, they need some work, tough. Input from people interested is welcome.
Comment 29 Tomás Cohen Arazi 2016-11-01 15:30:01 UTC Comment hidden (obsolete)
Comment 30 Tomás Cohen Arazi 2016-11-01 15:30:09 UTC Comment hidden (obsolete)
Comment 31 Tomás Cohen Arazi 2016-11-01 15:30:29 UTC Comment hidden (obsolete)
Comment 32 Tomás Cohen Arazi 2016-11-01 15:30:37 UTC Comment hidden (obsolete)
Comment 33 Tomás Cohen Arazi 2016-11-01 15:30:45 UTC Comment hidden (obsolete)
Comment 34 Tomás Cohen Arazi 2016-11-01 15:30:54 UTC Comment hidden (obsolete)
Comment 35 Tomás Cohen Arazi 2016-12-16 14:31:38 UTC Comment hidden (obsolete)
Comment 36 Tomás Cohen Arazi 2016-12-16 14:31:48 UTC Comment hidden (obsolete)
Comment 37 Tomás Cohen Arazi 2016-12-16 14:31:55 UTC Comment hidden (obsolete)
Comment 38 Tomás Cohen Arazi 2016-12-16 14:32:08 UTC Comment hidden (obsolete)
Comment 39 Tomás Cohen Arazi 2016-12-21 15:59:27 UTC Comment hidden (obsolete)
Comment 40 Tomás Cohen Arazi 2016-12-21 15:59:36 UTC Comment hidden (obsolete)
Comment 41 Tomás Cohen Arazi 2016-12-21 15:59:43 UTC Comment hidden (obsolete)
Comment 42 Tomás Cohen Arazi 2016-12-21 15:59:51 UTC Comment hidden (obsolete)
Comment 43 Tomás Cohen Arazi 2016-12-21 16:00:05 UTC Comment hidden (obsolete)
Comment 44 Tomás Cohen Arazi 2016-12-21 16:00:15 UTC Comment hidden (obsolete)
Comment 45 Tomás Cohen Arazi 2016-12-21 16:00:24 UTC Comment hidden (obsolete)
Comment 46 Owen Leonard 2016-12-21 20:13:42 UTC
I tested this with a patron record which had two attributes with data already stored: One editable in the OPAC, the other not.

When I edited the one, it cleared out the other. So the patron modification request in the staff client looked like this:

Old value
Attr1: OldValue1 <-- not editable in OPAC
Attr2: OldValue2 <-- editable in OPAC

New value

Attr2: NewValue2
Comment 47 Tomás Cohen Arazi 2016-12-22 00:09:21 UTC
Thanks for testing! I'll fix it tomorrow!
Comment 48 Tomás Cohen Arazi 2016-12-26 14:37:01 UTC Comment hidden (obsolete)
Comment 49 Tomás Cohen Arazi 2016-12-26 14:37:07 UTC Comment hidden (obsolete)
Comment 50 Owen Leonard 2017-01-03 15:51:58 UTC
(In reply to Owen Leonard from comment #46)
> I tested this with a patron record which had two attributes with data
> already stored: One editable in the OPAC, the other not.

Editing is working correctly now, but now I don't see the value for my non-editable attribute. It only shows the label.
Comment 51 Tomás Cohen Arazi 2017-01-03 20:22:34 UTC Comment hidden (obsolete)
Comment 52 Tomás Cohen Arazi 2017-01-09 17:29:35 UTC Comment hidden (obsolete)
Comment 53 Tomás Cohen Arazi 2017-01-09 18:01:39 UTC
Created attachment 58706 [details] [review]
Bug 13757: Add the option to set patron attributes editable in the OPAC

Note: this is a squashed version of the original patchset, because it was needed

This patch adds an opac_editable property of borrower attribute types
that can be set in the interface. I'm removing work on OPAC and will
refactor it, keeping the author attribution.

Test plan:
  1. Repeat the following with a new and existing borrower attribute
     type:
  2. Verify that "Editable in OPAC" can only be checked if "Display in
     OPAC" is checked.
  3. Verify that this new property is correctly saved.

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 54 Tomás Cohen Arazi 2017-01-09 18:01:45 UTC
Created attachment 58707 [details] [review]
Bug 13757: DB update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 55 Tomás Cohen Arazi 2017-01-09 18:01:52 UTC
Created attachment 58708 [details] [review]
Bug 13757: OPAC changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 56 Tomás Cohen Arazi 2017-01-09 18:01:58 UTC
Created attachment 58709 [details] [review]
Bug 13757: DBIC update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 57 Tomás Cohen Arazi 2017-01-09 18:02:04 UTC
Created attachment 58710 [details] [review]
Bug 13757: Add extended attributes to the patron modification

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 58 Tomás Cohen Arazi 2017-01-09 18:02:12 UTC
Created attachment 58711 [details] [review]
Bug 13757: (followup) Remove warnings

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 59 Tomás Cohen Arazi 2017-01-09 18:02:19 UTC
Created attachment 58712 [details] [review]
Bug 13757: (followup) Staff interface changes

This patch adds proper extended attributes display and handling on the
patron modifications moderation page (members-update.pl).

It also adds changes checking to the opac-memberentry.pl page so it
only saves a modification request if there are changes (it only checked
regular fields and not the extended ones).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 60 Tomás Cohen Arazi 2017-01-09 18:02:26 UTC
Created attachment 58713 [details] [review]
Bug 13757: (followup) Only touch opac_editable attributes

As reported by Owen, the members-update.pl was showing every attributes
the patron has (display issue) instead of showing only those affected by
the changes.

This patch fixes this by filtering the patron's attributes by opac
editability.

It also fixes Koha::Patron::Modification->approve so it only clears the
attributes with the updating 'code' and leaves the others untouched.
As its been coded so far (until someone refactors it all) the
Koha::Patron::Modification object needs to contain all the attributes
for a specific code. And it comes from parsing the UI's input.

Tests for Koha::Patron::Modification->approve to come.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 61 Tomás Cohen Arazi 2017-01-09 18:02:31 UTC
Created attachment 58714 [details] [review]
Bug 13757: (followup) Regression tests for ->approve changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 62 Tomás Cohen Arazi 2017-01-09 18:02:39 UTC
Created attachment 58715 [details] [review]
Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 63 Hugo Agud 2017-01-13 10:44:03 UTC
I have tested the patch and it works, BUT I am still learning and I do not know how to create the signed patch, sorry
Comment 64 Tomás Cohen Arazi 2017-01-20 14:00:24 UTC
Created attachment 59376 [details] [review]
Bug 13757: (QA followup) Check DB structure before altering table

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 65 Jonathan Druart 2017-01-26 13:02:46 UTC Comment hidden (obsolete)
Comment 66 Jonathan Druart 2017-01-26 13:04:07 UTC Comment hidden (obsolete)
Comment 67 Tomás Cohen Arazi 2017-01-26 13:04:37 UTC Comment hidden (obsolete)
Comment 68 Jonathan Druart 2017-01-26 13:23:36 UTC
I was expecting to find some calls to Koha::Object::Limit::Library->library_limits in this patchset, but I actually do not see any.
Is it unused code or I missed a bug report?
Comment 69 Tomás Cohen Arazi 2017-01-26 13:31:36 UTC
(In reply to Jonathan Druart from comment #68)
> I was expecting to find some calls to
> Koha::Object::Limit::Library->library_limits in this patchset, but I
> actually do not see any.
> Is it unused code or I missed a bug report?

It is unused code. The idea was to introduce the new code and then migrate page by page to using the new class. On separate bug reports
Comment 70 Jonathan Druart 2017-01-27 11:08:08 UTC
Found a bug:
If a attr is editable at the opac and the patron clear the field, it appears in the patron modifications to approve/deny. But if you approve it, it's not cleared.
Comment 71 Tomás Cohen Arazi 2017-02-01 13:09:42 UTC
Created attachment 59737 [details] [review]
Bug 13757: (regression tests) Empty attributes should delete existing

This patch introduces tests for the required functionality.

To test:
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> FAIL: The current code doesn't work like that

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=13737
Comment 72 Tomás Cohen Arazi 2017-02-01 13:09:49 UTC
Created attachment 59738 [details] [review]
Bug 13757: Make Koha::Patron::Modification->store del empty attrs

This patch makes Koha::Patron::Modification->store delete the passed
attributes that contain empty values.

This is the way it currently works, as all opac-editable attributes are
presented to the end-user and they are allowed to delete them, and the
best way I found to represent the deletion on the modification request
is by setting it to the empty string. I chose this way because it is how
the staff interface handles it, so it is consistent.

To test:
- Apply this patch
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> SUCCESS: This time tests pass!
- Verify comment #70 on the bug is fixed now
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=13737
Comment 73 Tomás Cohen Arazi 2017-02-01 13:09:57 UTC
Created attachment 59739 [details] [review]
Bug 13757: (QA followup) Make opac-memberentry.pl handle attrs deletion

The original code on this bug skipped empty-valued attributes. But
emptying attributes is the only way to tell the controller script that
the user wants to delete them.

This patch makes opac-memberentry.pl check the existence of attributes
sharing the code of the empty for the given patron, and it stores the
deletion on the Koha::Patron::Modification as needed. Otherwise
deletions got skipped.

To test:
- Verify setting/deleting attributes that are opac-editable and verify
the results are sound.

https://bugs.koha-community.org/show_bug.cgi?id=13737
Comment 74 Jonathan Druart 2017-02-01 14:04:12 UTC
I do not know if it comes from the last patches but I found something else.
if I do not make any changes to the editable attrs, they will appear as modified anyway (entry in borrower_modifications).
Comment 75 Jonathan Druart 2017-02-01 14:07:06 UTC
Another one:
1/ Set an attr as displayed at the OPAC but not editable
2/ At the OPAC, fill the edit patron form but miss a mandatory field, submit
3/ You get the form again but the attr is not displayed.
Comment 76 Tomás Cohen Arazi 2017-02-02 13:16:35 UTC
Created attachment 59813 [details] [review]
Bug 13757: (QA followup) Fix non-editable attrs on failed save

When a field is not editable but displayable in the OPAC, and you submit
an incomplete/wrong update, those attributes are displayed as empty.

This patch fixes that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 77 Tomás Cohen Arazi 2017-02-03 18:35:11 UTC
Created attachment 59896 [details] [review]
Bug 13757: (QA followup) Fix non-editable attrs on failed save

When a field is not editable but displayable in the OPAC, and you submit
an incomplete/wrong update, those attributes are displayed as empty.

This patch fixes that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 78 Tomás Cohen Arazi 2017-02-03 18:35:22 UTC
Created attachment 59897 [details] [review]
Bug 13757: Make K::P::Modifications->pending return K::P::Attribute

This patch makes Koha::Patron::Modifications->pending return
Koha::Patron:Attribute objects. They are not stored on the DB but only
live in memory on runtime.

members-update.pl is the only place this is used, and this way we have
all we need for better rendering the UI.

Tests are added for the changed API.
Comment 79 Tomás Cohen Arazi 2017-02-03 18:35:32 UTC
Created attachment 59898 [details] [review]
Bug 13757: Better display for attr changes in members-update.pl

This patch changes the way changed attributes are displayed for the
staff user to make the decision to approve (or not) the changes.

Regards
Comment 80 Jonathan Druart 2017-02-14 11:12:32 UTC
Tomas, there are too many patches without a signoff, we need a signoff on the whole stack.
Comment 81 Jonathan Druart 2017-02-16 13:23:33 UTC
It would be awesome to get a quick signoff on this one!
Comment 82 Tomás Cohen Arazi 2017-02-16 19:04:56 UTC
Created attachment 60359 [details] [review]
Bug 13757: (regression test) Attribute with value 0 should be stored

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Tomás Cohen Arazi 2017-02-16 19:05:02 UTC
Created attachment 60360 [details] [review]
Bug 13757: Attribute with value 0 should be stored

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Nick Clemens 2017-03-15 20:29:56 UTC
Created attachment 61132 [details] [review]
Bug 13757: Add the option to set patron attributes editable in the OPAC

Note: this is a squashed version of the original patchset, because it was needed

This patch adds an opac_editable property of borrower attribute types
that can be set in the interface. I'm removing work on OPAC and will
refactor it, keeping the author attribution.

Test plan:
  1. Repeat the following with a new and existing borrower attribute
     type:
  2. Verify that "Editable in OPAC" can only be checked if "Display in
     OPAC" is checked.
  3. Verify that this new property is correctly saved.

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 85 Nick Clemens 2017-03-15 20:30:04 UTC
Created attachment 61133 [details] [review]
Bug 13757: DB update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 86 Nick Clemens 2017-03-15 20:30:11 UTC
Created attachment 61134 [details] [review]
Bug 13757: OPAC changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 87 Nick Clemens 2017-03-15 20:30:18 UTC
Created attachment 61135 [details] [review]
Bug 13757: DBIC update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 88 Nick Clemens 2017-03-15 20:30:25 UTC
Created attachment 61136 [details] [review]
Bug 13757: Add extended attributes to the patron modification

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 89 Nick Clemens 2017-03-15 20:30:32 UTC
Created attachment 61137 [details] [review]
Bug 13757: (followup) Remove warnings

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 90 Nick Clemens 2017-03-15 20:30:39 UTC
Created attachment 61138 [details] [review]
Bug 13757: (followup) Staff interface changes

This patch adds proper extended attributes display and handling on the
patron modifications moderation page (members-update.pl).

It also adds changes checking to the opac-memberentry.pl page so it
only saves a modification request if there are changes (it only checked
regular fields and not the extended ones).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 91 Nick Clemens 2017-03-15 20:30:47 UTC
Created attachment 61139 [details] [review]
Bug 13757: (followup) Only touch opac_editable attributes

As reported by Owen, the members-update.pl was showing every attributes
the patron has (display issue) instead of showing only those affected by
the changes.

This patch fixes this by filtering the patron's attributes by opac
editability.

It also fixes Koha::Patron::Modification->approve so it only clears the
attributes with the updating 'code' and leaves the others untouched.
As its been coded so far (until someone refactors it all) the
Koha::Patron::Modification object needs to contain all the attributes
for a specific code. And it comes from parsing the UI's input.

Tests for Koha::Patron::Modification->approve to come.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 92 Nick Clemens 2017-03-15 20:30:53 UTC
Created attachment 61140 [details] [review]
Bug 13757: (followup) Regression tests for ->approve changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 93 Nick Clemens 2017-03-15 20:30:59 UTC
Created attachment 61141 [details] [review]
Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 94 Nick Clemens 2017-03-15 20:31:05 UTC
Created attachment 61142 [details] [review]
Bug 13757: (QA followup) Check DB structure before altering table

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 95 Nick Clemens 2017-03-15 20:31:11 UTC
Created attachment 61143 [details] [review]
Bug 13757: (regression tests) Empty attributes should delete existing

This patch introduces tests for the required functionality.

To test:
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> FAIL: The current code doesn't work like that

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 96 Nick Clemens 2017-03-15 20:31:18 UTC
Created attachment 61144 [details] [review]
Bug 13757: Make Koha::Patron::Modification->store del empty attrs

This patch makes Koha::Patron::Modification->store delete the passed
attributes that contain empty values.

This is the way it currently works, as all opac-editable attributes are
presented to the end-user and they are allowed to delete them, and the
best way I found to represent the deletion on the modification request
is by setting it to the empty string. I chose this way because it is how
the staff interface handles it, so it is consistent.

To test:
- Apply this patch
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> SUCCESS: This time tests pass!
- Verify comment #70 on the bug is fixed now
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 97 Nick Clemens 2017-03-15 20:31:24 UTC
Created attachment 61145 [details] [review]
Bug 13757: (QA followup) Make opac-memberentry.pl handle attrs deletion

The original code on this bug skipped empty-valued attributes. But
emptying attributes is the only way to tell the controller script that
the user wants to delete them.

This patch makes opac-memberentry.pl check the existence of attributes
sharing the code of the empty for the given patron, and it stores the
deletion on the Koha::Patron::Modification as needed. Otherwise
deletions got skipped.

To test:
- Verify setting/deleting attributes that are opac-editable and verify
the results are sound.

https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 98 Nick Clemens 2017-03-15 20:31:38 UTC
Created attachment 61146 [details] [review]
Bug 13757: (QA followup) Fix non-editable attrs on failed save

When a field is not editable but displayable in the OPAC, and you submit
an incomplete/wrong update, those attributes are displayed as empty.

This patch fixes that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 99 Nick Clemens 2017-03-15 20:31:44 UTC
Created attachment 61147 [details] [review]
Bug 13757: Make K::P::Modifications->pending return K::P::Attribute

This patch makes Koha::Patron::Modifications->pending return
Koha::Patron:Attribute objects. They are not stored on the DB but only
live in memory on runtime.

members-update.pl is the only place this is used, and this way we have
all we need for better rendering the UI.

Tests are added for the changed API.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 100 Nick Clemens 2017-03-15 20:31:52 UTC
Created attachment 61148 [details] [review]
Bug 13757: Better display for attr changes in members-update.pl

This patch changes the way changed attributes are displayed for the
staff user to make the decision to approve (or not) the changes.

Regards

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 101 Nick Clemens 2017-03-15 20:31:58 UTC
Created attachment 61149 [details] [review]
Bug 13757: (regression test) Attribute with value 0 should be stored

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 102 Nick Clemens 2017-03-15 20:32:05 UTC
Created attachment 61150 [details] [review]
Bug 13757: Attribute with value 0 should be stored

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 103 Jonathan Druart 2017-03-16 19:31:27 UTC
Just small things:
1. I think we should not display the attributes at the opac if they are empty and not editable (it's especially weird for self registration)
2. 
[% IF !pm.extended_attributes and pm.size > 3 or
       pm.extended_attributes and pm.size > 4  %]
I am sure you can make it more readable :D

3. Typo 'FilterUnchagedAttributes'

Please fix ASAP and I PQA it!
Comment 104 Tomás Cohen Arazi 2017-03-22 19:26:02 UTC
Created attachment 61492 [details] [review]
Bug 13757: (QA followup) Exclude empty attributes from rendering if non-editable

In self registration opac displayable (and not editable) attributes are
displayed as empty. This an empty value is passed to the template for
creating an empty input and it shouldn't when the attribute is not
editable.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 105 Jonathan Druart 2017-03-22 19:53:12 UTC
Tomas, please do not hate me!

I manage to edit a non opac editable field hijacking the value of one of the "patron_attribute_value" hidden input at the OPAC.
I then see the requested changes in the "Patron modification" list at the staff interface.

You should check if the attribute is editable before storing it (server-side check).
Comment 106 Tomás Cohen Arazi 2017-03-22 21:54:21 UTC
Created attachment 61493 [details] [review]
Bug 13757: (QA followup) Filter out non-editable params before storing

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 107 Jonathan Druart 2017-03-23 15:58:02 UTC
Created attachment 61555 [details] [review]
Bug 13757: Add the option to set patron attributes editable in the OPAC

Note: this is a squashed version of the original patchset, because it was needed

This patch adds an opac_editable property of borrower attribute types
that can be set in the interface. I'm removing work on OPAC and will
refactor it, keeping the author attribution.

Test plan:
  1. Repeat the following with a new and existing borrower attribute
     type:
  2. Verify that "Editable in OPAC" can only be checked if "Display in
     OPAC" is checked.
  3. Verify that this new property is correctly saved.

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 108 Jonathan Druart 2017-03-23 15:58:08 UTC
Created attachment 61556 [details] [review]
Bug 13757: DB update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 109 Jonathan Druart 2017-03-23 15:58:14 UTC
Created attachment 61557 [details] [review]
Bug 13757: OPAC changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 110 Jonathan Druart 2017-03-23 15:58:20 UTC
Created attachment 61558 [details] [review]
Bug 13757: DBIC update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 111 Jonathan Druart 2017-03-23 15:58:26 UTC
Created attachment 61559 [details] [review]
Bug 13757: Add extended attributes to the patron modification

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 112 Jonathan Druart 2017-03-23 15:58:32 UTC
Created attachment 61560 [details] [review]
Bug 13757: (followup) Remove warnings

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 113 Jonathan Druart 2017-03-23 15:58:38 UTC
Created attachment 61561 [details] [review]
Bug 13757: (followup) Staff interface changes

This patch adds proper extended attributes display and handling on the
patron modifications moderation page (members-update.pl).

It also adds changes checking to the opac-memberentry.pl page so it
only saves a modification request if there are changes (it only checked
regular fields and not the extended ones).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 114 Jonathan Druart 2017-03-23 15:58:46 UTC
Created attachment 61562 [details] [review]
Bug 13757: (followup) Only touch opac_editable attributes

As reported by Owen, the members-update.pl was showing every attributes
the patron has (display issue) instead of showing only those affected by
the changes.

This patch fixes this by filtering the patron's attributes by opac
editability.

It also fixes Koha::Patron::Modification->approve so it only clears the
attributes with the updating 'code' and leaves the others untouched.
As its been coded so far (until someone refactors it all) the
Koha::Patron::Modification object needs to contain all the attributes
for a specific code. And it comes from parsing the UI's input.

Tests for Koha::Patron::Modification->approve to come.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 115 Jonathan Druart 2017-03-23 15:58:54 UTC
Created attachment 61563 [details] [review]
Bug 13757: (followup) Regression tests for ->approve changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 116 Jonathan Druart 2017-03-23 15:59:01 UTC
Created attachment 61564 [details] [review]
Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 117 Jonathan Druart 2017-03-23 15:59:07 UTC
Created attachment 61565 [details] [review]
Bug 13757: (QA followup) Check DB structure before altering table

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 118 Jonathan Druart 2017-03-23 15:59:13 UTC
Created attachment 61566 [details] [review]
Bug 13757: (regression tests) Empty attributes should delete existing

This patch introduces tests for the required functionality.

To test:
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> FAIL: The current code doesn't work like that

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 119 Jonathan Druart 2017-03-23 15:59:19 UTC
Created attachment 61567 [details] [review]
Bug 13757: Make Koha::Patron::Modification->store del empty attrs

This patch makes Koha::Patron::Modification->store delete the passed
attributes that contain empty values.

This is the way it currently works, as all opac-editable attributes are
presented to the end-user and they are allowed to delete them, and the
best way I found to represent the deletion on the modification request
is by setting it to the empty string. I chose this way because it is how
the staff interface handles it, so it is consistent.

To test:
- Apply this patch
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> SUCCESS: This time tests pass!
- Verify comment #70 on the bug is fixed now
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 120 Jonathan Druart 2017-03-23 15:59:25 UTC
Created attachment 61568 [details] [review]
Bug 13757: (QA followup) Make opac-memberentry.pl handle attrs deletion

The original code on this bug skipped empty-valued attributes. But
emptying attributes is the only way to tell the controller script that
the user wants to delete them.

This patch makes opac-memberentry.pl check the existence of attributes
sharing the code of the empty for the given patron, and it stores the
deletion on the Koha::Patron::Modification as needed. Otherwise
deletions got skipped.

To test:
- Verify setting/deleting attributes that are opac-editable and verify
the results are sound.

https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 121 Jonathan Druart 2017-03-23 15:59:31 UTC
Created attachment 61569 [details] [review]
Bug 13757: (QA followup) Fix non-editable attrs on failed save

When a field is not editable but displayable in the OPAC, and you submit
an incomplete/wrong update, those attributes are displayed as empty.

This patch fixes that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 122 Jonathan Druart 2017-03-23 15:59:37 UTC
Created attachment 61570 [details] [review]
Bug 13757: Make K::P::Modifications->pending return K::P::Attribute

This patch makes Koha::Patron::Modifications->pending return
Koha::Patron:Attribute objects. They are not stored on the DB but only
live in memory on runtime.

members-update.pl is the only place this is used, and this way we have
all we need for better rendering the UI.

Tests are added for the changed API.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 123 Jonathan Druart 2017-03-23 15:59:44 UTC
Created attachment 61571 [details] [review]
Bug 13757: Better display for attr changes in members-update.pl

This patch changes the way changed attributes are displayed for the
staff user to make the decision to approve (or not) the changes.

Regards

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 124 Jonathan Druart 2017-03-23 15:59:50 UTC
Created attachment 61572 [details] [review]
Bug 13757: (regression test) Attribute with value 0 should be stored

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 125 Jonathan Druart 2017-03-23 15:59:56 UTC
Created attachment 61573 [details] [review]
Bug 13757: Attribute with value 0 should be stored

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 126 Jonathan Druart 2017-03-23 16:00:04 UTC
Created attachment 61574 [details] [review]
Bug 13757: (QA followup) Exclude empty attributes from rendering if non-editable

In self registration opac displayable (and not editable) attributes are
displayed as empty. This an empty value is passed to the template for
creating an empty input and it shouldn't when the attribute is not
editable.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 127 Jonathan Druart 2017-03-23 16:00:11 UTC
Created attachment 61575 [details] [review]
Bug 13757: (QA followup) Filter out non-editable params before storing

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 128 Kyle M Hall 2017-03-24 18:50:16 UTC
Pushed to master for 17.05, thanks Tomas!
Comment 129 Katrin Fischer 2017-03-26 21:13:01 UTC
This won't get ported back to 16.11.x as it is an enhancement.


But congrats, Tomas!