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.
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.
Created attachment 42254 [details] [review] Bug 13757 - Make patron attributes editable in the OPAC (1/2) This patch adds an opac_editable property of borrower attribute types that can be set in the interface. 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.
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).
Created attachment 42626 [details] [review] Bug 13757 - (followup) fix creation of patron attributes
Created attachment 42627 [details] [review] Bug 13757 - Make patron attributes editable in the OPAC (2/2) This patch makes attributes marked as OPAC-editable editable from the OPAC. NOTE: This needs to be extended to work with the modifications system, and thus only works for new patrons. Test plan: 1) Create attributes that cover the spectrum of possible attribute types; repeatable, unique, dropdown, etc. 2) Verify that all of these attributes are correctly validated and saved.
Created attachment 47625 [details] [review] Bug 13757 - Make patron attributes editable in the OPAC (2/2) This patch makes attributes marked as OPAC-editable editable from the OPAC. NOTE: This needs to be extended to work with the modifications system, and thus only works for new patrons. Test plan: 1) Create attributes that cover the spectrum of possible attribute types; repeatable, unique, dropdown, etc. 2) Verify that all of these attributes are correctly validated and saved. Aleisha's update: Fixed a merge conflict and a warn "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."
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.
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?
Created attachment 48040 [details] [review] [SIGNED-OFF] Bug 13757 - Make patron attributes editable in the OPAC (2/2) This patch makes attributes marked as OPAC-editable editable from the OPAC. NOTE: This needs to be extended to work with the modifications system, and thus only works for new patrons. Test plan: 1) Create attributes that cover the spectrum of possible attribute types; repeatable, unique, dropdown, etc. 2) Verify that all of these attributes are correctly validated and saved. Aleisha's update: Fixed a merge conflict and a warn "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." Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Sorry forgot to run updatedatabase.pl!
Created attachment 48041 [details] [review] [SIGNED-OFF] Bug 13757 - Make patron attributes editable in the OPAC (1/2) This patch adds an opac_editable property of borrower attribute types that can be set in the interface. 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>
Created attachment 48042 [details] [review] [SIGNED-OFF] Bug 13757 - Make patron attributes editable in the OPAC (1/2) This patch adds an opac_editable property of borrower attribute types that can be set in the interface. 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>
Created attachment 48043 [details] [review] [SIGNED-OFF] Bug 13757 - (followup) fix creation of patron attributes Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Ah sorry for all the comments git kept timing out, but I am signing off
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.
Created attachment 48379 [details] [review] Bug 13757: Make patron attributes editable in the OPAC (1/2) This patch adds an opac_editable property of borrower attribute types that can be set in the interface. 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>
Created attachment 48380 [details] [review] Bug 13757: Make patron attributes editable in the OPAC (2/2) This patch makes attributes marked as OPAC-editable editable from the OPAC. NOTE: This needs to be extended to work with the modifications system, and thus only works for new patrons. Test plan: 1) Create attributes that cover the spectrum of possible attribute types; repeatable, unique, dropdown, etc. 2) Verify that all of these attributes are correctly validated and saved. Aleisha's update: Fixed a merge conflict and a warn "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." Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Sorry forgot to run updatedatabase.pl!
Created attachment 48381 [details] [review] Bug 13757: (followup) fix creation of patron attributes Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Created attachment 48382 [details] [review] Bug 13757: (QA followup) fix QA issues, add small niceties Remove debug warns, and hide patron attributes in the OPAC if PatronSelfRegistrationVerifyByEmail is enabled (as this enhancement currently does not work with Koha::Borrower::Modifications). Also, slightly improve appearance of attributes with passwords.
It does not seem to work correctly, I'd like to get another signoff on this one.
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.
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.
Created attachment 56924 [details] [review] Bug 13757: Make patron attributes editable in the OPAC (1/2) This patch adds an opac_editable property of borrower attribute types that can be set in the interface. 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>
Created attachment 56925 [details] [review] Bug 13757: Make patron attributes editable in the OPAC (2/2) This patch makes attributes marked as OPAC-editable editable from the OPAC. NOTE: This needs to be extended to work with the modifications system, and thus only works for new patrons. Test plan: 1) Create attributes that cover the spectrum of possible attribute types; repeatable, unique, dropdown, etc. 2) Verify that all of these attributes are correctly validated and saved. Aleisha's update: Fixed a merge conflict and a warn "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." Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Sorry forgot to run updatedatabase.pl!
Created attachment 56926 [details] [review] Bug 13757: (followup) fix creation of patron attributes Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Created attachment 56927 [details] [review] Bug 13757: (QA followup) fix QA issues, add small niceties Remove debug warns, and hide patron attributes in the OPAC if PatronSelfRegistrationVerifyByEmail is enabled (as this enhancement currently does not work with Koha::Borrower::Modifications). Also, slightly improve appearance of attributes with passwords.
Created attachment 56928 [details] [review] Bug 13757: (followup) Rebase against master
Created attachment 56929 [details] [review] Bug 13757: DBIC update
I rebased them, they need some work, tough. Input from people interested is welcome.
Created attachment 57041 [details] [review] Bug 13757: Make patron attributes editable in the OPAC (1/2) This patch adds an opac_editable property of borrower attribute types that can be set in the interface. 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>
Created attachment 57042 [details] [review] Bug 13757: Make patron attributes editable in the OPAC (2/2) This patch makes attributes marked as OPAC-editable editable from the OPAC. NOTE: This needs to be extended to work with the modifications system, and thus only works for new patrons. Test plan: 1) Create attributes that cover the spectrum of possible attribute types; repeatable, unique, dropdown, etc. 2) Verify that all of these attributes are correctly validated and saved. Aleisha's update: Fixed a merge conflict and a warn "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." Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Sorry forgot to run updatedatabase.pl! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 57043 [details] [review] Bug 13757: (followup) fix creation of patron attributes Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 57044 [details] [review] Bug 13757: (QA followup) fix QA issues, add small niceties Remove debug warns, and hide patron attributes in the OPAC if PatronSelfRegistrationVerifyByEmail is enabled (as this enhancement currently does not work with Koha::Borrower::Modifications). Also, slightly improve appearance of attributes with passwords. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 57045 [details] [review] Bug 13757: (followup) Rebase against master Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 57046 [details] [review] Bug 13757: DBIC update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 58252 [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>
Created attachment 58253 [details] [review] Bug 13757: DB update
Created attachment 58254 [details] [review] Bug 13757: OPAC changes
Created attachment 58255 [details] [review] Bug 13757: DBIC update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 58351 [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>
Created attachment 58352 [details] [review] Bug 13757: DB update
Created attachment 58353 [details] [review] Bug 13757: OPAC changes
Created attachment 58354 [details] [review] Bug 13757: DBIC update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 58355 [details] [review] Bug 13757: Add extended attributes to the patron modification
Created attachment 58356 [details] [review] Bug 13757: (followup) Remove warnings
Created attachment 58357 [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).
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
Thanks for testing! I'll fix it tomorrow!
Created attachment 58428 [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.
Created attachment 58429 [details] [review] Bug 13757: (followup) Regression tests for ->approve changes
(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.
Created attachment 58581 [details] [review] Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable
Created attachment 58703 [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>
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>
Created attachment 58707 [details] [review] Bug 13757: DB update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 58708 [details] [review] Bug 13757: OPAC changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 58709 [details] [review] Bug 13757: DBIC update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 58710 [details] [review] Bug 13757: Add extended attributes to the patron modification Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 58711 [details] [review] Bug 13757: (followup) Remove warnings Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
Created attachment 58714 [details] [review] Bug 13757: (followup) Regression tests for ->approve changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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
Created attachment 59376 [details] [review] Bug 13757: (QA followup) Check DB structure before altering table Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Please use column_exists in the update DB entry.
(In reply to Jonathan Druart from comment #65) > Please use column_exists in the update DB entry. Done later, sorry!
(In reply to Jonathan Druart from comment #65) > Please use column_exists in the update DB entry. Last patch?
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?
(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
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.
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
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
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
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).
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.
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>
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>
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.
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
Tomas, there are too many patches without a signoff, we need a signoff on the whole stack.
It would be awesome to get a quick signoff on this one!
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>
Created attachment 60360 [details] [review] Bug 13757: Attribute with value 0 should be stored Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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!
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>
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).
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Pushed to master for 17.05, thanks Tomas!
This won't get ported back to 16.11.x as it is an enhancement. But congrats, Tomas!
*** Bug 18679 has been marked as a duplicate of this bug. ***