|
Description
Kyle M Hall (khall)
2017-06-12 12:30:46 UTC
Created attachment 64202 [details] [review] Bug 18786 - Update database Created attachment 64203 [details] [review] Bug 18786 - Update schema Created attachment 64204 [details] [review] Bug 18786 - Add ability to create custom payment types Test plan was missing. I tested with following steps: Following test is OK: - Apply patches - Update database - Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' - Go to the fines section of a patron who has several fines - Tab 'Fines' - Click on 'Pay' for an individual fine - Verified that in form 'Pay an individual fine' a dropdown appears with paxment types - Selected a payment type, confirm payment - Verify that payment type appears in "Description of charges" in tab 'Account' Following is missing: - Go to 'Fines' tab - Use 'Pay amount' or 'Pay selected' for payment Fail: There is no dropdown list with payment types Ack! Thanks for the test plan! I'll get the patch set fixed up asap! (In reply to Marc Véron from comment #4) > Test plan was missing. > > I tested with following steps: > Following test is OK: > > - Apply patches > - Update database > - Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit > card' > - Go to the fines section of a patron who has several fines > - Tab 'Fines' > - Click on 'Pay' for an individual fine > - Verified that in form 'Pay an individual fine' a dropdown appears with > paxment types > - Selected a payment type, confirm payment > - Verify that payment type appears in "Description of charges" in tab > 'Account' > > > Following is missing: > - Go to 'Fines' tab > - Use 'Pay amount' or 'Pay selected' for payment > Fail: There is no dropdown list with payment types Created attachment 64288 [details] [review] Bug 18786 - Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Created attachment 64300 [details] [review] Bug 18786 - Update database Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64301 [details] [review] Bug 18786 - Update schema Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64302 [details] [review] Bug 18786 - Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Signed-off-by: Marc Véron <veron@veron.ch> Got the following error when trying to pay a lost item charge, but fine is gone from the account. No indication of payment on Account page or in account_offsets table.
Software error:
{UNKNOWN}: Can't call method "unblessed" on an undefined value at /var/lib/koha/hcpl/kohaclone/C4/Circulation.pm line 2178. at /var/lib/koha/hcpl/kohaclone/C4/Circulation.pm line 2198
For help, please send mail to the webmaster ([no address given]), giving this error message and the time and date of the error.
Everything else works fine.
Test Plan:
1) Apply patches
2) Update database
3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card'
4) Go to the fines section of a patron who has several fines
5) Tab 'Fines'
6) Click on 'Pay' for an individual fine
7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types - YES (also appears with all "Pay")
8) Select a payment type, confirm payment
9) Verify that payment type appears in "Description of charges" in tab 'Account'
Please rebase. Created attachment 67476 [details] [review] Bug 18786 - Update database Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 67477 [details] [review] Bug 18786 - Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 69155 [details] [review] Bug 18786 - Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 69156 [details] [review] Bug 18786 - Update database Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 69157 [details] [review] Bug 18786 - Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Signed-off-by: Marc Véron <veron@veron.ch> I think this is a duplicate of the older bug 5630 which is also signed off at the moment. There was some discussion about using an authorised value there and because we already have a list of payment types used by SIP and translatability some hardcoded values were added. Maybe we could make it so that the list can be extended by an authorised value? (... and make authorised values translatable for display one day...) Can we pick one implementation? (In reply to Katrin Fischer from comment #17) > I think this is a duplicate of the older bug 5630 which is also signed off > at the moment. Katrin, which bug did you have in mind? This bug number is not related to account either. > There was some discussion about using an authorised value there and because > we already have a list of payment types used by SIP and translatability some > hardcoded values were added. Maybe we could make it so that the list can be > extended by an authorised value? (... and make authorised values > translatable for display one day...) > > Can we pick one implementation? +1 ;) Found it - it was bug 5620! (In reply to Katrin Fischer from comment #19) > Found it - it was bug 5620! That bug doesn't do what this one does. That one is specifically about SIP payment types. Moreover, I see some issues with it since it hardcodes a bunch of payment type values that aren't specified by the SIP2 protocol : \ If this one gets pushed I'd be willing to assist in fixing up 5620. Created attachment 71020 [details] [review] Bug 18786 - Update database Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 71021 [details] [review] Bug 18786 - Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> I agree with you Kyle, better to not have more hardcoded values in Koha... Passed QA, note for RM: on boraccount.pl, the payment type is not shown now, but should be fixed by bug 12001 I think, with GetMemberAccountRecords refactoring What I am not sure about is the relation to the already existing SIP payment types - will both work together nicely for reports etc? (In reply to Katrin Fischer from comment #24) > What I am not sure about is the relation to the already existing SIP payment > types - will both work together nicely for reports etc? Yes they will! Created attachment 72015 [details] [review] Bug 18786 - Update database Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 72016 [details] [review] Bug 18786 - Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 72114 [details] [review] Bug 18786 - Update database Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 72115 [details] [review] Bug 18786 - Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> We are going to add yet another AV that cannot be translatable. Moreover this patch set does not create some default ones for existing or new installations. I'd like to get more QA pov on that. Cannot we try to be exhaustive and list all (or at least several to start) payment types in a separate table? We could have a FK at DB level and make them translatable. (In reply to Jonathan Druart from comment #31) > Cannot we try to be exhaustive and list all (or at least several to start) > payment types in a separate table? We could have a FK at DB level and make > them translatable. These are not hard coded payment types, they are meant to be added by the librarians, just as we already use a AV category for librarian defined fee types. Really, what we need is for authorised values to be translatable, but that's a bit outside the scope of this bug ; ) If you would like me to add an example payment type to the sample data, I'd be happy to do so! As the payment types are ok as key value pairs and you wouldn't need more, I think having them as an authorised value is ok. But we should start to make AV translatable one by one (or all at once - wouldn't mind that!). It is the next logical step, especially for values exposed in the OPAC like location and collection. Created attachment 72282 [details] [review] Bug 18786: Remove use of Koha::Patron::Images Why did you added the cleaning of the code in C4/SIP/ILS.pm? It looks OK, but I think it's not related to this report. Created attachment 73628 [details] [review] Bug 18786: Remove use of Koha::Patron::Images (In reply to Josef Moravec from comment #35) > Why did you added the cleaning of the code in C4/SIP/ILS.pm? > > It looks OK, but I think it's not related to this report. No idea how the snuck in. This new version of the followup should have that fixed! Josef, do you have time to finish here? (In reply to Katrin Fischer from comment #38) > Josef, do you have time to finish here? Sorry, I missed this somehow, I'll take a look on this Created attachment 74147 [details] [review] Bug 18786: Update database Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74148 [details] [review] Bug 18786: Add ability to create custom payment types Some libraries would like to be able to specify more specific payment types such as cash, check, credit card, or even canned food ( for food drives ). This feature will allow a library to specify payment types as authorised values of the type PAYMENT_TYPE. Test Plan: 1) Apply patches 2) Update database 3) Add authorised value PAYMENT_TYPE with values 'Cash', 'Check', 'Credit card' 4) Go to the fines section of a patron who has several fines 5) Tab 'Fines' 6) Click on 'Pay' for an individual fine 7) Verify that in the form a 'Pay an individual fine' dropdown appears with payment types 8) Select a payment type, confirm payment 9) Verify that payment type appears in "Description of charges" in tab 'Account' Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 74149 [details] [review] Bug 18786: Remove use of Koha::Patron::Images Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Hum, PQA, sure? Template process failed: undef error - The method count is not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 122. Created attachment 74172 [details] [review] Bug 18786: Add PAYMENT_TYPE to the AV categories (In reply to Jonathan Druart from comment #43) > Hum, PQA, sure? > > Template process failed: undef error - The method count is not covered by > tests! at /home/vagrant/kohaclone/C4/Templates.pm line 122. Why is this failed qa now? From this error? How do you generate it? Just try to pay a fine. (In reply to Jonathan Druart from comment #46) > Just try to pay a fine. Thanks! Probably due to a recent change to master. (In reply to Jonathan Druart from comment #46) > Just try to pay a fine. I am completely unable to reproduce your error. Can you try again? Did you create AVs for PAYMENT_TYPE? (In reply to Jonathan Druart from comment #49) > Did you create AVs for PAYMENT_TYPE? Yes, I tested every payment option with and without PAYMENT_TYPEs. In both cases the av category itself existed. Take a look at the patch, the problem is obvious: the [% IF payment_types %] is duplicated. One has a call to ->count, the others not. (In reply to Jonathan Druart from comment #51) > Take a look at the patch, the problem is obvious: the [% IF payment_types %] > is duplicated. One has a call to ->count, the others not. Ah, I *still* don't get an error, but that is clearly a duplicate block. I'll get that taken care of! Created attachment 74436 [details] [review] Bug 18786: Remove duplicate block with bad IF Pushed to master for 18.05, thanks to everybody involved! Created attachment 75401 [details] [review] Bug 18786: Add PAYMENT_TYPE to the installer file (In reply to Jonathan Druart from comment #55) > Created attachment 75401 [details] [review] [review] > Bug 18786: Add PAYMENT_TYPE to the installer file Pushed to master for 18.05 *** Bug 5620 has been marked as a duplicate of this bug. *** |