|
Description
Maryse Simard
2020-10-02 04:14:41 UTC
Created attachment 111054 [details] [review] Bug 26597: Transfer informations from guarantor when adding a guarantor to an existing patron When creating a new guarantee from the guarantor, the preference PrefillGuaranteeField dictates some fields to be transfered from guarantor to guarantee. This patch makes it so those informations are also transfered when adding a new guarantor relationship to an existing patron. To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click "Select" 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Created attachment 111319 [details] [review] Bug 26597: Transfer informations from guarantor when adding a guarantor to an existing patron When creating a new guarantee from the guarantor, the preference PrefillGuaranteeField dictates some fields to be transfered from guarantor to guarantee. This patch makes it so those informations are also transfered when adding a new guarantor relationship to an existing patron. To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click "Select" 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Signed-off-by: David Nind <david@davidnind.com> Created attachment 112196 [details] [review] Bug 26597: Transfer informations from guarantor when adding a guarantor to an existing patron When creating a new guarantee from the guarantor, the preference PrefillGuaranteeField dictates some fields to be transfered from guarantor to guarantee. This patch makes it so those informations are also transfered when adding a new guarantor relationship to an existing patron. To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click "Select" 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> RM Note: We might possibly want a way to *not* transfer info to existing patrons when guaranteeing them ( possibly another syspref, or a checkbox when adding a guarantor ). The feature does leave existing fields untouched. I will leave that decision to the RM. I have the feeling that the path you chose is way too complicated for what you tried to implement. The "PrefillGuaranteeField" notion should not appear in the DT patron search module (C4:Utils::DataTables::Members). A more elegant implementation would be to retrieve the patron's data using the REST API route and keep the logic of "prefilling" JS-side. What do you think? What can we do to take this out of the "In Discussion" limbo? It was already signed off twice. (In reply to Caroline Cyr La Rose from comment #6) > What can we do to take this out of the "In Discussion" limbo? It was already > signed off twice. An answer to my comment? :) Your comment is to redo the patch completely. I was asking because it was already signed off and PQA as it is. Sorry, not PQA, I was mistaking it for another one Created attachment 142443 [details] [review] Bug 26597: Transfer informations from guarantor when adding a guarantor to an existing patron When creating a new guarantee from the guarantor, the preference PrefillGuaranteeField dictates some fields to be transfered from guarantor to guarantee. This patch makes it so those informations are also transfered when adding a new guarantor relationship to an existing patron. To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click "Select" 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 144475 [details] [review] Bug 26597: Transfer informations from guarantor when adding a guarantor to an existing patron When creating a new guarantee from the guarantor, the preference PrefillGuaranteeField dictates some fields to be transfered from guarantor to guarantee. This patch makes it so those informations are also transfered when adding a new guarantor relationship to an existing patron. To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on 'Search to add' in 'Patron guarantor' fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click 'Select' 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten For me this is not working. In PrefillGuaranteeField I am adding every Alternate address field. Then I make sure my gaurantor has each of those fields filled out. If I then "Add gaurantee" the normal way it works. But when I go into the Guarantee record, search for a gaurantor , and add them. None of the fields are populated. Created attachment 144560 [details] [review] Bug 26597: Fixing the populate of fields when using the REST API route This attachment correct the populate of fields by using the api mapping. Now All fields are populated following the selected PrefillGuaranteeField options For me this is not working either. In PrefillGuaranteeField I am adding every Alternate address field. When I search for a guarantor, and then add them to the guarantee, the fields do not populate. Created attachment 148564 [details] [review] Bug 26597: (follow-up) Add disabling / enabled the save button while waiting from api to respond I've updated the test plan since the REST API route takes a little time to respond To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click Select 7) Wait until save button is enabled 8) Confirm guarantee's information is filled from the guarantor's record 9) Check that any preexisting information is not overwritten (In reply to Laura Escamilla from comment #14) > For me this is not working either. In PrefillGuaranteeField I am adding > every Alternate address field. When I search for a guarantor, and then add > them to the guarantee, the fields do not populate. Hi Laura, thank you for testing. I've updated the test plan since the guarantee's information take a little time to be filled Created attachment 148597 [details] [review] Bug 26597: Transfer informations from guarantor when adding a guarantor to an existing patron When creating a new guarantee from the guarantor, the preference PrefillGuaranteeField dictates some fields to be transfered from guarantor to guarantee. This patch makes it so those informations are also transfered when adding a new guarantor relationship to an existing patron. To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on 'Search to add' in 'Patron guarantor' fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click 'Select' 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 148598 [details] [review] Bug 26597: Fixing the populate of fields when using the REST API route This attachment correct the populate of fields by using the api mapping. Now All fields are populated following the selected PrefillGuaranteeField options Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 148599 [details] [review] Bug 26597: (follow-up) Add disabling / enabled the save button while waiting from api to respond I've updated the test plan since the REST API route takes a little time to respond To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click Select 7) Wait until save button is enabled 8) Confirm guarantee's information is filled from the guarantor's record 9) Check that any preexisting information is not overwritten Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Works as described, thanks for the patch. Looking here FAIL koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
FAIL filters
missing_filter at line 1921 ( guarantor_attributes = [% To.json(guarantor_attributes) %];)
missing_filter at line 1924 ( to_api_mapping = [% To.json(to_api_mapping) %];)
Please fix.
+ error:function(){
+ $('#saverecord').prop('disabled', false);
Shouldnt we do more than that ?
koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc: select_user( borrowernumber, JSON.parse(borrower_data) );
koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc: function select_user(borrowernumber, data) {
koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc: p.select_user(borrowernumber, data);
koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgets.tt: function select_user(borrowernumber, borrower) {
It seems that we are overloading this function name already. So no blocker for this report. Just noting. But might be a chance to rename in members.js? The name is quite obscure for copying guarantor information.
koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: select_user( '[% g.patron.borrowernumber | html %]', [% To.json( g.patron.unblessed ) | $raw %], '[% g.relationship | html %]' );
koha-tmpl/intranet-tmpl/prog/js/members.js:function select_user(borrowernumber, borrower, relationship) {
[% IF new_guarantors %]
[% FOREACH g IN new_guarantors %]
select_user( '[% g.patron.borrowernumber | html %]', [% To.json( g.patron.unblessed ) | $raw %], '[% g.relationship | html %]' );
[% END %]
[% END %]
Please explain why whe should do an ajax GET in select_user when we pass the borrower unblessed hash to this function?
This does not look good.
Created attachment 151850 [details] [review] Bug 26597: (Fix) remove REST API call and retrieve the patron's data from borrower_data input This patch remove the REST API call in select_user function and retrieve the patron's data from borrower_data To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click Select 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Created attachment 151851 [details] [review] Bug 26597: (QA follow-up) Remane select_user and add missing filters (In reply to Marcel de Rooy from comment #22) > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt > FAIL filters > missing_filter at line 1921 ( > guarantor_attributes = [% To.json(guarantor_attributes) %];) > missing_filter at line 1924 ( to_api_mapping = [% > To.json(to_api_mapping) %];) > Please fix. Missing_filters added > for this report. Just noting. But might be a chance to rename in members.js? > The name is quite obscure for copying guarantor information. I've rename the function to add_guarantor > Please explain why whe should do an ajax GET in select_user when we pass the > borrower unblessed hash to this function? > This does not look good. It was a suggestion in the comment 5 but you are right, there is no need to use the REST API to retrieve data from the guarantor since we can get it from the borrower hidden input, it is fixed in this patch. (In reply to Hammat wele from comment #24) > Created attachment 151851 [details] [review] [review] > Bug 26597: (QA follow-up) Remane select_user and add missing filters This is wrong. You are mixing different things. select_user in patron-search.inc cannot be renamed, it's a generic function that is used in other places. If you rename it you are going to break other patron searches. If you are removing the REST API fetch I don't see why you are still passing and using to_api_mapping. (In reply to Jonathan Druart from comment #27) > If you are removing the REST API fetch I don't see why you are still passing > and using to_api_mapping. With borrower, I get all the guarantor's data i need but it is not associated with the name of the fields of the form. the to_api_mapping allows me to retrieve the name of the fields to be filled. the data received from borrower is like { ... "altaddress_address": "addressvalue", "altaddress_address2": "address2value", "altaddress_city": "addressCityvalue", "altaddress_country": "addresscontryvalue", "altaddress_email": "email@example.com", ... } to be able to fill the form with these data i need to get the name of the fields to be filled in and the to_api_mapping help me to get it { B_address => 'altaddress_address', } Created attachment 151878 [details] [review] Bug 26597: (QA follow-up) Add missing filters Created attachment 154501 [details] [review] Bug 26597: Transfer informations from guarantor when adding a guarantor to an existing patron When creating a new guarantee from the guarantor, the preference PrefillGuaranteeField dictates some fields to be transfered from guarantor to guarantee. This patch makes it so those informations are also transfered when adding a new guarantor relationship to an existing patron. To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on 'Search to add' in 'Patron guarantor' fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click 'Select' 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Created attachment 154503 [details] [review] Bug 26597: Fixing the populate of fields when using the REST API route This attachment correct the populate of fields by using the api mapping. Now All fields are populated following the selected PrefillGuaranteeField options Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Created attachment 154504 [details] [review] Bug 26597: (follow-up) Add disabling / enabled the save button while waiting from api to respond I've updated the test plan since the REST API route takes a little time to respond To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click Select 7) Wait until save button is enabled 8) Confirm guarantee's information is filled from the guarantor's record 9) Check that any preexisting information is not overwritten Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Created attachment 154505 [details] [review] Bug 26597: (Fix) remove REST API call and retrieve the patron's data from borrower_data input This patch remove the REST API call in select_user function and retrieve the patron's data from borrower_data To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click Select 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Created attachment 154506 [details] [review] Bug 26597: (QA follow-up) Add missing filters Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> This looks good but the system preference description ( and possibly the name ) needs to be updated to reflect that the information copying will now work in both directions. Created attachment 158119 [details] [review] Bug 26597: Update the description of the PrefillGuaranteeField system preference I updated the system preference description so that we now know this option is working from both the guarantee and guarantor forms. Created attachment 158300 [details] [review] Bug 26597: Update the description of the PrefillGuaranteeField system preference I updated the system preference description so that we now know this option is working from both the guarantee and guarantor forms. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Created attachment 161510 [details] [review] Bug 26597: Transfer informations from guarantor when adding a guarantor to an existing patron When creating a new guarantee from the guarantor, the preference PrefillGuaranteeField dictates some fields to be transfered from guarantor to guarantee. This patch makes it so those informations are also transfered when adding a new guarantor relationship to an existing patron. To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on 'Search to add' in 'Patron guarantor' fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click 'Select' 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 161511 [details] [review] Bug 26597: Fixing the populate of fields when using the REST API route This attachment correct the populate of fields by using the api mapping. Now All fields are populated following the selected PrefillGuaranteeField options Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 161512 [details] [review] Bug 26597: (follow-up) Add disabling / enabled the save button while waiting from api to respond I've updated the test plan since the REST API route takes a little time to respond To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click Select 7) Wait until save button is enabled 8) Confirm guarantee's information is filled from the guarantor's record 9) Check that any preexisting information is not overwritten Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 161513 [details] [review] Bug 26597: (Fix) remove REST API call and retrieve the patron's data from borrower_data input This patch remove the REST API call in select_user function and retrieve the patron's data from borrower_data To test: 1) Apply patch 2) Search PrefillGuaranteeField preference and make sure some fields are selected 3) Select a user that can have a guarantor 4) In the edit form, click on "Search to add" in "Patron guarantor" fieldset 5) Choose a patron who has at least one of the fields in 1) set 6) Click Select 7) Confirm guarantee's information is filled from the guarantor's record 8) Check that any preexisting information is not overwritten Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 161514 [details] [review] Bug 26597: (QA follow-up) Add missing filters Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 161515 [details] [review] Bug 26597: Update the description of the PrefillGuaranteeField system preference I updated the system preference description so that we now know this option is working from both the guarantee and guarantor forms. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 161516 [details] [review] Bug 26597: (QA follow-up) Tidy code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I am not sure why, but I am seeing sha1 errors much more often recently. If someone has an idea how to help avoid them, I am all ears. Right now it's only the last patch here that has the issue: Patch failed at 0001 Bug 26597: (QA follow-up) Tidy code hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-26597-QA-follow-up-Tidy-code-ol81ve_8.patch I tidied the patch inline. Pushed for 24.05! Well done everyone, thank you! Enhancement not pushed to 23.11.x |