Description
Frédérick Capovilla
2014-02-28 20:26:35 UTC
Comment hidden (obsolete)
Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Wanted to test. Patch does not apply. Created attachment 25922 [details] [review] New version of the patch. Rebase on current master. Patch updated. Created attachment 25941 [details] [review] Bug 11879 [QA Followup] - Fix patch to follow modern coding guidelines The second patch (from Kyle) has a problem: $ git bz apply 11879 Bug 11879 - Add a new borrower field : main contact method. 25922 - New version of the patch. Rebase on current master. 25941 - Bug 11879 [QA Followup] - Fix patch to follow modern coding guidelines Apply? [(y)es, (n)o, (i)nteractive] y Applying: Add the "Main contact method" field in borrower informations. Applying: Bug 11879 [QA Followup] - Fix patch to follow modern coding guidelines fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 11879 [QA Followup] - Fix patch to follow modern coding guidelines The copy of the patch that failed is found in: /home/magnus/scripts/kohaclone/.git/rebase-apply/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-11879-QA-Followup---Fix-patch-to-follow-modern-D5v_Ji.patch Created attachment 31300 [details] [review] New version of the patch. Rebase on current master Created attachment 31301 [details] [review] New version of the patch. Rebase on current master Main contact select box displays as expected in borrower modification, but value is not written to database. It seems that changes to members/memberentry.pl are not contained in this patch. Created attachment 31320 [details] [review] Version rebased on current master - fixed columm name Created attachment 34351 [details] [review] Rebased on current master for testing Main contact select box displays as expected in borrower modification and the value is written to database. However I found following issue: Edit syspref BorrowerUnwantedField and hide e.g. phonepro (= Secondary phone) Result: Secondary phone can still be selected as Main contact. Expected Result: Secondary phone (or other unwanted fields) should be hidden in select box as well. Created attachment 34482 [details] [review] Fix syspref BorrowerUnwantedField Syspref BorrowerUnwantedField problem was fixed Created attachment 34502 [details] [review] [Signed-off] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 34503 [details] [review] [Signed-off] Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 34571 [details] [review] [PASSED QA] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 34572 [details] [review] [PASSED QA] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 34573 [details] [review] [PASSED QA] Bug 11879 - Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> The only issue I have is with the language. I think 'preferred' or 'primary' would probably serve better than 'main', but that is only my opinion. After applying t/db_dependent/Members.t fails at lin 102 with: # Failed test 'Confirm MoveMemberToDeleted.' # at t/db_dependent/Members.t line 102. # Structures begin differing at: # $got->{primary_contact_method} = Does not exist # $expected->{primary_contact_method} = undef The reason is that the field primary_contact_method is added to the table borrowers but not to the table deletedborrowers. Created attachment 41209 [details] [review] Bug 11879 - adds primary_contact_method column to deletedborrowers table Created attachment 41220 [details] [review] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 41221 [details] [review] Bug 11879 - Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 41222 [details] [review] Bug 11879 - adds primary_contact_method column to deletedborrowers table (In reply to Rémi Mayrand-Provencher from comment #24) > Created attachment 41222 [details] [review] [review] > Bug 11879 - adds primary_contact_method column to deletedborrowers table I transferred the changes from the patch "Bug 11879 - adds primary_contact_method column to deletedborrowers table" on the first two patches so that they can be both be applied without any conflicts. (In reply to Rémi Mayrand-Provencher from comment #24) > Created attachment 41222 [details] [review] [review] > Bug 11879 - adds primary_contact_method column to deletedborrowers table I transferred the changes from the patch "Bug 11879 - adds primary_contact_method column to deletedborrowers table" on the first two patches so that they can be both be applied without any conflicts. Where can a librarian see the patron's chosen main contact method? I feel like it should be under their contact information on the Details page Created attachment 46101 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Created attachment 46284 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Created attachment 46285 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Created attachment 46286 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Applying: Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) fatal: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) Created attachment 48073 [details] [review] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 48074 [details] [review] Bug 11879 - Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 48075 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Created attachment 48076 [details] [review] [SIGNED-OFF] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 48077 [details] [review] [SIGNED-OFF] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 48078 [details] [review] [SIGNED-OFF] Bug 11879 - Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 48079 [details] [review] [SIGNED-OFF] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Note: This is just for display purpose, this new field is never used for sending notices to patrons. QA comment: - The second patch modifies "Other phone" with "Mobile phone". "Other phone" is used in master atm - The deletedborrowers table structure should be modified too - It would be good to let the user modify this value from the OPAC, don't you think? Created attachment 48556 [details] [review] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 48557 [details] [review] Bug 11879 - Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 48558 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 48559 [details] [review] Bug 11879 QA follow-up Changed "Mobile Phone" to Other Phone, modified deletedborrowers table structute and added the field in opac When ALTER'ing it is a good idea to put "AFTER" as part of the SQL statement so that upgrades and fresh installs are guaranteed to have the same field order. I'm guessing this is 'Failed QA' based on Comment 45. Created attachment 50433 [details] [review] Bug 11879 - QA Follow-up Added "after" in the atomic update, changed status to needs signoff! Thank you! Sorry, this does not apply anymore! Created attachment 52607 [details] [review] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 52608 [details] [review] Bug 11879 - Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 52614 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 52615 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 52616 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 52617 [details] [review] Bug 11879 QA follow-up Changed "Mobile Phone" to Other Phone, modified deletedborrowers table structute and added the field in opac Created attachment 52618 [details] [review] Bug 11879 - QA Follow-up Created attachment 52619 [details] [review] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 52620 [details] [review] Bug 11879 - Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> installer/data/mysql/updatedatabase.pl adds a field primary_contact_method to borrowers That makes borrowers_primary_contact_method.sql fail (adding fields to borrowers, deletedborrowers, borrower_modifications). Created attachment 53229 [details] [review] Bug 11879 - updatedatabase.pl no longer adds column primary_contact_method to table borrowers Signed-off-by: mehdi <mehdi@inlibro.com> (In reply to Jonathan Druart from comment #40) > QA comment: > - The second patch modifies "Other phone" with "Mobile phone". "Other phone" > is used in master atm This is still true. Also, in both the OPAC and staff client the "main contact method" label is incorrectly capitalized. Template guidelines say that only the first letter should be upper case. There should also be a ":" after the label: "Main contact method:" Is there a plan for some functionality which is based on this setting? Created attachment 53688 [details] [review] Bug 11879 - QA Follow-up Fixed tests, they now work without any fails Please discard my last comment. Created attachment 53695 [details] [review] Bug 11879 - QA Follow-up updatedatabase.pl no longer adds column primary_contact_method to table borrowers Fixed the incorrectly capitalized label and added the ':' in front of it(opac and intranet). Changed mobile phone to other phone in intranet and added a switch case in moremember.tt to display the correct label. Created attachment 53705 [details] [review] Bug 11879 - Add a new borrower field : main contact method Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 53707 [details] Bug 11879 - Add a new borrower field : main contact method Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 53708 [details] [review] Bug 11879 - Update BDIx schema and show main contact method in moremember.pl Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 53709 [details] [review] Bug 11879 QA follow-up Changed "Mobile Phone" to Other Phone, modified deletedborrowers table structute and added the field in opac Created attachment 53710 [details] [review] Bug 11879 - QA Follow-up Created attachment 53711 [details] [review] Bug 11879 - Add a new borrower field : main contact method. (Was: Rebased on current master for testing) http://bugs.koha-community.org/show_bug.cgi?id=11879 To test this patch: Update database Edit a borrower's informations. The "Main Contact Method" selectbox appears in the form and its value is correctly. Works as expected. For handling of syspref BorrowerUnwantedField use second patch- Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 53712 [details] [review] Bug 11879 - Fix syspref BorrowerUnwantedField How I tested: Apply 1st patch Apply this patch Edit syspref BorrowerUnwantedField to exclude fields Edit patron's contact information Make sure that fields do not longer appear in drop down for Main contact Wors as expected http://bugs.koha-community.org/show_bug.cgi?id=11879 Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Aleisha <aleishaamohia@hotmail.com> Created attachment 53713 [details] [review] Bug 11879 - QA Follow-up updatedatabase.pl no longer adds column primary_contact_method to table borrowers Fixed the incorrectly capitalized label and added the ':' in front of it(opac and intranet). Changed mobile phone to other phone in intranet and added a switch case in moremember.tt to display the correct label. Should be an easy to solve conflict, just a conflict on memberentrygen.tt to do with the CSRF protection. Happy to test again when it is sorted. Created attachment 55946 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Created attachment 56285 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Removed a $branch variable from patch, since it has been suppressed in Bug 17284. Created attachment 56286 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Removed a $branch variable from patch, since it has been suppressed in Bug 17284. The patch does not apply, please rebase. Created attachment 58580 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Comment on attachment 58580 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. Review of attachment 58580 [details] [review]: ----------------------------------------------------------------- I don't believe the Borrower.pm, BorrowerModification.pm, and Deletedborrower.pm were generated using the appropriate scripts. - Schema files should be moved to a separate patch - Changes to kohastructure are wrong, please double check. Mark, when you say the schema files were not generated correctly, do you mean that you want to see the file hash change in the patch? I know you can't override it afterwards if you decide to regenerate them again because the script doesn't allow it, but I thought these files were generated again by the RM upon release. Anyway, I'll submit a separate patch for the schemas with the output of the script _as is_. And sorry for that messy rebase, that borrower table appearing a thousand lines before caught me by surprise. Created attachment 58591 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Created attachment 58592 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Created attachment 58593 [details] [review] Bug 11879: Changes to Koha's schemas This patch does not apply. Created attachment 63238 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Created attachment 63239 [details] [review] Bug 11879: Changes to Koha's schemas I found some issues with this patchset: 1) It removes field overdrive_auth_token field from tables borrowers and deletedborrowers 2) Automic update and kohastructure has to be consistent in field order, in atomic, the new field is added after 'privacy', in kohastructure at the end - it is bad Created attachment 65811 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Created attachment 65812 [details] [review] Bug 11879: Changes to Koha's schemas Hi Philippe Sorry but when applying your patch I got the following merge errors (see below), the issues are in moremember.tt and moremember.pl. Can you please rebase against master and then I will be more than happy to test again: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11879: Add the "Main contact method" field in borrower informations. Using index info to reconstruct a base tree... M installer/data/mysql/kohastructure.sql M installer/data/mysql/updatedatabase.pl M koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt M members/memberentry.pl M members/moremember.pl M opac/opac-memberentry.pl Falling back to patching base and 3-way merge... Auto-merging opac/opac-memberentry.pl Auto-merging members/moremember.pl CONFLICT (content): Merge conflict in members/moremember.pl Auto-merging members/memberentry.pl Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Auto-merging installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/kohastructure.sql Failed to merge in the changes. Patch failed at 0001 Bug 11879: Add the "Main contact method" field in borrower informations. The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/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-11879-Add-the-Main-contact-method-field-in-bor-uOjbTv.patch Created attachment 69028 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Created attachment 69029 [details] [review] Bug 11879: Changes to Koha's schemas Rebased patches. Test plan is same as before: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) Save the form. The selected field should appear in the patron's detail page. Created attachment 69050 [details] [review] Bug 11879: Follow-up - Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Comment on attachment 69028 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. Review of attachment 69028 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/borrowers_primary_contact_method.sql @@ +1,2 @@ > +ALTER TABLE `borrowers` ADD COLUMN `primary_contact_method` VARCHAR(45) NULL DEFAULT NULL AFTER `lastseen`; > +ALTER TABLE `deletedborrowers` ADD COLUMN `primary_contact_method` VARCHAR(45) NULL DEFAULT NULL AFTER `overdrive_auth_token`; deletedborrowers and borrowers should maintain the same field order. ::: installer/data/mysql/kohastructure.sql @@ +1682,4 @@ > `lang` varchar(25) NOT NULL default 'default', -- lang to use to send notices to this patron > `login_attempts` int(4) default 0, -- number of failed login attemps > `overdrive_auth_token` text default NULL, -- persist OverDrive auth token > + `lastseen` datetime default NULL, -- last time a patron has been seed (connected at the OPAC or staff interface) This adds lastseen, but the borrowers_primary_contact_method did not add it at all. ::: installer/data/mysql/updatedatabase.pl @@ +14968,4 @@ > } > } > > + Totally not necessary to touch this. ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ +538,5 @@ > > [% UNLESS nophone && nophonepro && nomobile && noemail && noemailpro && nofax %] > <fieldset class="rows" id="memberentry_contact"> > + <legend id="contact_lgd">Contact</legend> > +<ol> Keep whitespace changes separate. @@ +602,4 @@ > [% IF ( mandatoryemailpro ) %]<span class="required">Required</span>[% END %] > </li> > [% END %] > + [% UNLESS nofax %] Keep whitespace changes separate. @@ +664,5 @@ > + [% END %] > + [% END %] > + </select> > + </li> > + </ol> This is the only place where you can really get away with these minor whitespace changes on the END and /ol. ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ +275,5 @@ > + [% ELSE %] > + [% guarantor.firstname | html %] [% guarantor.surname | html %] > + [% END %] > + </li> > + [% END %] Where did this ELSIF clause come from?! This has nothing to do with primary_contact_method. ::: opac/opac-memberentry.pl @@ +93,5 @@ > libraries => \@libraries, > OPACPatronDetails => C4::Context->preference('OPACPatronDetails'), > ); > +my $check_BorrowerMandatoryField=C4::Context->preference("BorrowerMandatoryField"); > + my @field_check=split(/\|/,$check_BorrowerMandatoryField); Something is strange about this indentation. I would expect the same indentation level. Comment on attachment 69029 [details] [review] Bug 11879: Changes to Koha's schemas Review of attachment 69029 [details] [review]: ----------------------------------------------------------------- None of these have lastseen, as mentioned in the other patch. Comment on attachment 69050 [details] [review] Bug 11879: Follow-up - Streamline markup and make label corrections Review of attachment 69050 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ +293,5 @@ > + Secondary email > + [% CASE 'fax' %] > + Fax > + [% END %] > + </li> This is nicer and friendlier on the po files, I'm sure. I believe it was done the other way because of a misunderstanding of HTML 1 (https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML_Templates). The SWITCH isn't allowed inside HTML tags, that is <tag [% SWITCH ... %]> is really bad. Created attachment 71955 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Created attachment 71956 [details] [review] Bug 11879: Changes to Koha's schemas Created attachment 71957 [details] [review] Bug 11879: Follow-up - Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Created attachment 71958 [details] [review] Bug 11879: Follow-up - code review Much tighter patches this time, thanks to Mark's comments. I'll review the changes one by one: 1) members/moremember.pl was adapted to reduce the diff's length. In previous iterations, many additional variables were passed to the templates -- borrowernumber, othernames, etc. -- but these are not necessary anymore, thanks to the patron objects. The template moremember-brief.tt was adapted to reflect these changes. 2) borrowers_primary_contact_method.sql is now adding columns at the very end of the table, in order to keep the existing columns in the same order. 3) `lastseen` had no reason whatsoever to be here. Kept the field the way it already was in the tables where we added primary_contact_method. 4) whitespace changes were rolled back, according to the minimum diff policy(TM) 5) I'm definitely responsible for the moremember.tt screwup code duplication; it appeared in patch #69028 when I must've rebased. It's now removed from this patch. 6) Indentation was corrected in opac-memberentry.pl This code should also pass qa -c 4. Needs signoff. Patch tested with a sandbox, by Lucie Gay <lucie.gay@ens-paris-saclay.fr> Created attachment 72632 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Created attachment 72633 [details] [review] Bug 11879: Changes to Koha's schemas Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Created attachment 72634 [details] [review] Bug 11879: Follow-up - Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Created attachment 72635 [details] [review] Bug 11879: Follow-up - code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Lucie et Aurélie : first test during Hackfest 2018 for us! A new dropdown select to chose the main contact method = OK The selected field appears in the patron's detail page = OK Almost finished, but will have to continue later! Created attachment 73987 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 73988 [details] [review] Bug 11879: Schema updates Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 73989 [details] [review] Bug 11879: (follow-up) Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 73990 [details] [review] Bug 11879: (QA follow-up) Fix problems found in code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Tried rebasing moremember.tt over the work done in Bug 19129, but I was struck by this message: Applying: Bug 11879: Follow-up - Streamline markup and make label corrections Applying: Bug 11879: Follow-up - code review fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). error: could not build fake ancestor Patch failed at 0001 Bug 11879: Follow-up - code review The copy of the patch that failed is found in: .git/rebase-apply/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-11879-Follow-up---code-review-S0MhGZ.patch Have the same conflict. It's unfortunate, but I'll just reapply an obsoleted set of patches to make this work. Changes coming in the next few hours. Really at lost about what happened here. Created attachment 74123 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74124 [details] [review] Bug 11879: (follow-up) Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74125 [details] [review] Bug 11879: (QA follow-up) Fix problems found in code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74126 [details] [review] Bug 11879: Schema updates Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Digging a bit further, the problem stemmed from patch ordering, and what looks like patch duplication after signoff. Changing the patch titles to adhere to the new naming convention must have confused git-bz attach. If you're unsure, you can diff the previous patch 1 against patch 5, idem for patch 2 and 6. I've submitted a new set of four patches, keeping those with the newest naming convention for clarity's sake, all the while making sure no changes were dropped. Should work exactly as before. Sorry, that was my fault then, didn't spot it! Checked the patches, QA tools still pass, no differences found. Created attachment 74818 [details] [review] Bug 11879: Remove unecessary code in controller - adds the field required is defined in BorrowerMandatoryField (staff only) - removes code from controller (not needed, we can follow existing pattern) - does not send patron's attribute separately (we have the 'patron' variable already) Need to be done: - same changes for the OPAC - remove extra changes made to moremember-brief.tt (if bugfix it needs to be moved on a separate bug report) - deal with PatronSelfModificationBorrowerUnwantedField and PatronSelfRegistrationBorrowerUnwantedField I first thought it would have replaced AutoEmailPrimaryAddress, but it actually only needed for display purpose, how/where can we explain that? Are release notes enough? Sorry but I found several issues, I started to fix them but cannot fix all. Please take a look at the last patch and the "todo" section. Created attachment 75652 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes I corrected the OPAC's member info form's behavior and removed the unrelated code in moremember-brief.tt Created attachment 83757 [details] [review] Bug 21753: DBRev 18.12.00.007 Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 83758 [details] Bug 21753: DBRev 18.12.00.007 Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 84017 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 84018 [details] [review] Bug 11879: (follow-up) Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 84019 [details] [review] Bug 11879: (QA follow-up) Fix problems found in code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 84020 [details] [review] Bug 11879: Schema updates Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 84021 [details] [review] Bug 11879: Remove unecessary code in controller - adds the field required is defined in BorrowerMandatoryField (staff only) - removes code from controller (not needed, we can follow existing pattern) - does not send patron's attribute separately (we have the 'patron' variable already) Need to be done: - same changes for the OPAC - remove extra changes made to moremember-brief.tt (if bugfix it needs to be moved on a separate bug report) - deal with PatronSelfModificationBorrowerUnwantedField and PatronSelfRegistrationBorrowerUnwantedField I first thought it would have replaced AutoEmailPrimaryAddress, but it actually only needed for display purpose, how/where can we explain that? Are release notes enough? Created attachment 84022 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes Rebased to master, patch should apply again. Patch doesn't apply. If this can be sorted I am keen to test this patch Applying: Bug 11879: Add the "Main contact method" field in borrower informations. Using index info to reconstruct a base tree... M installer/data/mysql/kohastructure.sql M installer/data/mysql/updatedatabase.pl M koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt M members/memberentry.pl M members/moremember.pl M opac/opac-memberentry.pl Falling back to patching base and 3-way merge... Auto-merging opac/opac-memberentry.pl Auto-merging members/moremember.pl Auto-merging members/memberentry.pl Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Auto-merging installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/kohastructure.sql Applying: Bug 11879: (follow-up) Streamline markup and make label corrections Applying: Bug 11879: (QA follow-up) Fix problems found in code review error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt). error: could not build fake ancestor Patch failed at 0001 Bug 11879: (QA follow-up) Fix problems found in code review Created attachment 88358 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 88359 [details] [review] Bug 11879: (follow-up) Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 88360 [details] [review] Bug 11879: (QA follow-up) Fix problems found in code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 88361 [details] [review] Bug 11879: Schema updates Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 88362 [details] [review] Bug 11879: Remove unecessary code in controller - adds the field required is defined in BorrowerMandatoryField (staff only) - removes code from controller (not needed, we can follow existing pattern) - does not send patron's attribute separately (we have the 'patron' variable already) Need to be done: - same changes for the OPAC - remove extra changes made to moremember-brief.tt (if bugfix it needs to be moved on a separate bug report) - deal with PatronSelfModificationBorrowerUnwantedField and PatronSelfRegistrationBorrowerUnwantedField I first thought it would have replaced AutoEmailPrimaryAddress, but it actually only needed for display purpose, how/where can we explain that? Are release notes enough? Created attachment 88363 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes (In reply to Hayley Mapley from comment #138) I've rebased it, if you want to take a look at it ! Created attachment 88394 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 88395 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Created attachment 88396 [details] [review] Bug 11879: (follow-up) Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Created attachment 88397 [details] [review] Bug 11879: (QA follow-up) Fix problems found in code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Created attachment 88398 [details] [review] Bug 11879: Schema updates Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Created attachment 88399 [details] [review] Bug 11879: Remove unecessary code in controller - adds the field required is defined in BorrowerMandatoryField (staff only) - removes code from controller (not needed, we can follow existing pattern) - does not send patron's attribute separately (we have the 'patron' variable already) Need to be done: - same changes for the OPAC - remove extra changes made to moremember-brief.tt (if bugfix it needs to be moved on a separate bug report) - deal with PatronSelfModificationBorrowerUnwantedField and PatronSelfRegistrationBorrowerUnwantedField I first thought it would have replaced AutoEmailPrimaryAddress, but it actually only needed for display purpose, how/where can we explain that? Are release notes enough? Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Created attachment 88400 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Started QAing: 1) QA test tools have complains about missing filters FAIL koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-brief.tt FAIL filters missing_filter at line 22 ( [% UNLESS ( I ) %][% IF ( othernames ) %]“[% othernames %]”[% END %]) missing_filter at line 64 ( <li><span class="label">Card number: </span>[% cardnumber %]</li>) missing_filter at line 65 ( <li><span class="label">Borrowernumber: </span> [% borrowernumber %]</li>) missing_filter at line 66 ( <li><span class="label">Category: </span>[% categoryname %] ([% categorycode %])</li>) missing_filter at line 66 ( <li><span class="label">Category: </span>[% categoryname %] ([% categorycode %])</li>) 2) The database update is not consistent, in atomic file you add columns after other columns then in kohastructure.sql Also, you should use .perl atomic update here, to check for column existence before adding it - see skeleton.perl for example *** Bug 9082 has been marked as a duplicate of this bug. *** Created attachment 93850 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 93851 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 93852 [details] [review] Bug 11879: (follow-up) Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 93853 [details] [review] Bug 11879: (QA follow-up) Fix problems found in code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 93854 [details] [review] Bug 11879: Schema updates Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 93855 [details] [review] Bug 11879: Remove unecessary code in controller - adds the field required is defined in BorrowerMandatoryField (staff only) - removes code from controller (not needed, we can follow existing pattern) - does not send patron's attribute separately (we have the 'patron' variable already) Need to be done: - same changes for the OPAC - remove extra changes made to moremember-brief.tt (if bugfix it needs to be moved on a separate bug report) - deal with PatronSelfModificationBorrowerUnwantedField and PatronSelfRegistrationBorrowerUnwantedField I first thought it would have replaced AutoEmailPrimaryAddress, but it actually only needed for display purpose, how/where can we explain that? Are release notes enough? Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 93856 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 93900 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 93901 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes modified: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-brief.tt modified: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt Created attachment 94456 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94457 [details] [review] Bug 11879: (follow-up) Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94458 [details] [review] Bug 11879: (QA follow-up) Fix problems found in code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94459 [details] [review] Bug 11879: Schema updates Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94460 [details] [review] Bug 11879: Remove unecessary code in controller - adds the field required is defined in BorrowerMandatoryField (staff only) - removes code from controller (not needed, we can follow existing pattern) - does not send patron's attribute separately (we have the 'patron' variable already) Need to be done: - same changes for the OPAC - remove extra changes made to moremember-brief.tt (if bugfix it needs to be moved on a separate bug report) - deal with PatronSelfModificationBorrowerUnwantedField and PatronSelfRegistrationBorrowerUnwantedField I first thought it would have replaced AutoEmailPrimaryAddress, but it actually only needed for display purpose, how/where can we explain that? Are release notes enough? Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94461 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes modified: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-brief.tt modified: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt Created attachment 94463 [details] [review] Bug 11879: Add the "Main contact method" field in borrower informations. This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Following Nicolas' fix, removed a $branch variable from patch, since it has been suppressed in Bug 17284. Test case : * Edit a borrower's informations. The "Main Contact Method" selectbox should appear in the form and its value should be saved correctly. Sponsored-by: CCSR <http://www.ccsr.qc.ca> Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94464 [details] [review] Bug 11879: (follow-up) Streamline markup and make label corrections This patch makes minor markup corrections to the patron detail page template: - Simplify the SWITCH markup - Add "primary" to primary phone label - Correct case on label - Add IF statement to hide label if there is no primary contact method Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94465 [details] [review] Bug 11879: (QA follow-up) Fix problems found in code review Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94466 [details] [review] Bug 11879: Schema updates Signed-off-by: Lucie Gay <lucie.gay@ens-paris-saclay.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94467 [details] [review] Bug 11879: Remove unecessary code in controller - adds the field required is defined in BorrowerMandatoryField (staff only) - removes code from controller (not needed, we can follow existing pattern) - does not send patron's attribute separately (we have the 'patron' variable already) Need to be done: - same changes for the OPAC - remove extra changes made to moremember-brief.tt (if bugfix it needs to be moved on a separate bug report) - deal with PatronSelfModificationBorrowerUnwantedField and PatronSelfRegistrationBorrowerUnwantedField I first thought it would have replaced AutoEmailPrimaryAddress, but it actually only needed for display purpose, how/where can we explain that? Are release notes enough? Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 94468 [details] [review] Bug 11879: (follow-up) Corrected OPAC and removed unrelated changes modified: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-brief.tt modified: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt Signed-off-by: Bin Wen <bin.wen@inlibro.com> Created attachment 96432 [details] [review] Bug 11879 - Add a new field to patron record: main contact method Fixed the problem mentioned above. Test plan is same as before: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) In the opac page, edit the personal details.There should be a new dropdown select to choose the main contact method. 4) Save the form. It should work. Patch no longer applies 8-( (Not sure of the correct protocol here - paste the message from git bz, or just say the patch no longer applies). Created attachment 100817 [details] [review] Bug 11879 - Add a new field to patron record: main contact method Fixed the problem mentioned above. Test plan is same as before: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) In the opac page, edit the personal details.There should be a new dropdown select to choose the main contact method. 4) Save the form. It should work. Created attachment 100843 [details] [review] Bug 11879: Add a new field to patron record: main contact method Fixed the problem mentioned above. Test plan is same as before: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) In the opac page, edit the personal details.There should be a new dropdown select to choose the main contact method. 4) Save the form. It should work. Signed-off-by: David Nind <david@davidnind.com> > 2) In the staff client, edit a patron's contact info. There should be a new
> dropdown select to choose the main contact method
I see this during the edit process but I don't see the information on the patron detail page (moremember.pl). It looks like only the template was changed.
You squashed the patches and lost the authors? You squashed the patches and lost the authors? Or did Bin Wen do that before? I'm trying to figure if everything is in the single patch currently presented. Ok, the patches were squashed due to the difficulties to apply them separately. Some dependencies issues. Someone here will work on it. Created attachment 112062 [details] [review] Bug 11879: Add a new field to patron record: main contact method This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test plan: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) In the opac page, edit the personal details.There should be a new dropdown select to choose the main contact method. 4) Save the form. It should work. Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: David Nind <david@davidnind.com> Created attachment 112063 [details] [review] Bug 11879: Schema update for Borrower tables Created attachment 112064 [details] [review] Bug 11879: (follow up) Some fixes and code cleanup - Display the value in the patron detail page - Adds support for BorrowerUnwantedField (staff) - Removes unecessary code in opac/opac-memberentry.pl Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) I've rebased the patch and added a follow up to fix the problem concerning display on the detail page and other code cleaning. The schema update has also been extracted from the first patch and the commit message adjusted to be a bit more telling. Created attachment 112557 [details] [review] Bug 11879: Add a new field to patron record: main contact method This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test plan: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) In the opac page, edit the personal details.There should be a new dropdown select to choose the main contact method. 4) Save the form. It should work. Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: David Nind <david@davidnind.com> Created attachment 112559 [details] [review] Bug 11879: Schema update for Borrower tables Signed-off-by: David Nind <david@davidnind.com> Created attachment 112560 [details] [review] Bug 11879: (follow up) Some fixes and code cleanup - Display the value in the patron detail page - Adds support for BorrowerUnwantedField (staff) - Removes unecessary code in opac/opac-memberentry.pl Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: David Nind <david@davidnind.com> There is one small glitch here: It allows you to hide the field using the "*unwanted*" preferences, but the pulldown itself does not change according to the hidden fields. I feel that if a library decides to hide "Fax" in the OPAC, we should also remove this entry from the pull down. I think it should not be too hard to do (I hope I am right), can you please check? I am not sure if we will need changes to the API Patron/Patrons routes too. Something else to look into. Created attachment 118959 [details] [review] Bug 11879: Add a new field to patron record: main contact method This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test plan: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) In the opac page, edit the personal details.There should be a new dropdown select to choose the main contact method. 4) Save the form. It should work. Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Created attachment 118960 [details] [review] Bug 11879: Schema update for Borrower tables Created attachment 118961 [details] [review] Bug 11879: (follow up) Some fixes and code cleanup - Display the value in the patron detail page - Adds support for BorrowerUnwantedField (staff) - Removes unecessary code in opac/opac-memberentry.pl Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Created attachment 118962 [details] [review] Bug 11879: Remove the 'unwanted' preferences from the main contact method pull-down menu in the OPAC Created attachment 118985 [details] [review] Bug 11879: Add a new field to patron record: main contact method This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test plan: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) In the opac page, edit the personal details.There should be a new dropdown select to choose the main contact method. 4) Save the form. It should work. Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: David Nind <david@davidnind.com> Created attachment 118986 [details] [review] Bug 11879: Schema update for Borrower tables Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118987 [details] [review] Bug 11879: (follow up) Some fixes and code cleanup - Display the value in the patron detail page - Adds support for BorrowerUnwantedField (staff) - Removes unecessary code in opac/opac-memberentry.pl Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 118988 [details] [review] Bug 11879: Remove the 'unwanted' preferences from the main contact method pull-down menu in the OPAC Signed-off-by: David Nind <david@davidnind.com> Created attachment 120947 [details] [review] Bug 11879: Add a new field to patron record: main contact method This patch adds the "Main Contact Method" field in the borrower modification form. This field is useful for reporting purposes, or to know which contact method to use first when trying to contact a borrower. Test plan: 0) Apply patch 1) updatedatabase.pl 2) In the staff client, edit a patron's contact info. There should be a new dropdown select to choose the main contact method 3) In the opac page, edit the personal details.There should be a new dropdown select to choose the main contact method. 4) Save the form. It should work. Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 120948 [details] [review] Bug 11879: Schema update for Borrower tables Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 120949 [details] [review] Bug 11879: (follow up) Some fixes and code cleanup - Display the value in the patron detail page - Adds support for BorrowerUnwantedField (staff) - Removes unecessary code in opac/opac-memberentry.pl Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 120950 [details] [review] Bug 11879: Remove the 'unwanted' preferences from the main contact method pull-down menu in the OPAC Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> (In reply to Katrin Fischer from comment #191) > I am not sure if we will need changes to the API Patron/Patrons routes too. > Something else to look into. Yes, I think we should. Tomas can you confirm? (In reply to Jonathan Druart from comment #204) > (In reply to Katrin Fischer from comment #191) > > I am not sure if we will need changes to the API Patron/Patrons routes too. > > Something else to look into. > > Yes, I think we should. Tomas can you confirm? Yes. We need to add it or remove it explicitly in to_api_mapping. I would remove it if you ask me. We can add /patrons/:patron_id/default_contact_method to deal with it. If you prefer to keep it, I vote for making it an enum with valid values. I would do the same on the DB. Created attachment 122778 [details] [review] Bug 11879: Remove primary_contact_method from REST API I will push as it but it would be great to have it part of the REST API spec. See bug 28701. Pushed to master for 21.11, thanks to everybody involved! Enhancement not backported for 21.05 It looks like there's no way to set a default for this field... |