[Koha-bugs] [Bug 23049] Replace MANUAL_INV authorised value with a dedicated table

[bugzilla-daemon at bugs.koha-community.org]

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

--- Comment #203 from Martin Renvoize <martin.renvoize at ptfs-europe.com> ---
(In reply to Marcel de Rooy from comment #199)
> Tested on top of commit 480434bbf4a32750c5e47a3600b6a386d9732296 with your
> branch.
> 
> Thanks for your hard work.
> I have noted comments 64 and 85 (thx Kyle). I made my preference clear, and
> continued on the current stand.
> 
> Comments below are in order of time noted, not priority.
> 
> User experience:
> During startup of admin/debit_types.pl I am seeing the 15 records for a
> short moment and end up with 3 records (filtered).

I agree, I not a fan of the "flash of unstyled content" effect this has but as
yet I've not managed to work around it.. I'll have another try.

> It would be nice to see them all and only allow editing of the non-system
> types ?

This is actually how I had it originally but updated the approach after
feedback from Nick and Severine.  Perhaps just sorted the system and archived
ones to the bottom would be the best approach.. though I liked them being
interleaved so one could see why you could not create a new type with a code
that matched a system debit type.

> Same when saving a record (no blocker, but does not look good). When you do
> not want to show the records, do not fetch them at all? 
> I saw that I now have a debit type F (Unexpected type found during upgrade).

I will take a look at adding some additional handling into the db update to
catch more cases of unrecognised types.. 'F' is obviously missed in certain
cases in previous bugs and should be caught and converted to proper 'OVERDUE'
types in my opinion. (I'm going to apply the patchset to a number of clones of
customers from various versions and try to spot some patterns to see what other
types may be being missed).

> Obviously this had to do with old fines stuff. I understand that bug 22521
> should have dealt with those, but somehow I still had such a record in
> accountlines. I am wondering if this will happen to more people and if we
> should anticipate on that by giving this F another description than
> unexpected ;)
> 
> $dbh->do(
>         qq{
>           INSERT IGNORE INTO account_debit_types (
>             code,
>             description,
>             can_be_added_manually,
>             default_amount,
>             is_system
>           )
>           SELECT
>             SUBSTR(accounttype, 1, 80),
>             "Unexpected type found during upgrade",
>             1,
>             NULL,
>             0
>           FROM
>             accountlines
>           WHERE
>             amount > 0
>         }
>     );
> No need for the SUBSTR anymore. Please add a DISTINCT accounttype to this
> query to eliminate a lof of ignored inserts.

Good catch, I'll update the  update statement

> And a question: If this type is unexpected, why do we enable Add manually ?

Also a good catch, thanks :).

> 
> sub UpdateFine
> You touch the following line, obviously only changing the type codes. But
> this is a condition for finding the overdues. Why type M in the first place,
> and why not yet another debit code?
> debit_type_code   => [ 'OVERDUE', 'M' ],
> This needs fixing somehow but I agree that it should be done on its own
> report..

Totally agree, I've never been entirely sure why 'M' was in that list and have
been slowly working towards removing it by clarifying it's use in bugs like
this.

> 
> sub GetFine
> +    WHERE debit_type_code LIKE 'OVERDUE'
> Shouldnt that be = ?

Also agree, I remember coming accross that one and thinking the same but
forgetting to actually change it. Thanks :). (I think historically it comes
from when fines were 'F' and 'FU' so it was a `LIKE "F%"`.)

> 
> flexability [a.o. :) ]
> No, not so flexible.
> 
> sub adjust
> -    my $account_type   = $self->accounttype;
> [..]
> +    my $debit_type_code = $self->debit_type_code;
> As I understand, the only allowed code is now overdue so debit. But this
> change does not look very solid to me. Hopefully, we will not adjust too
> much ;)

Not sure I follow.. I'll double check the code but I believe it checks against
'OVERDUE' + 'UNRETURNED' (mix of type + status)

> 
> Copier Fees
> Database: | Copier Fees      | Copier Fees                          |       
> 1 |       0.250000 |         0 |
> -INSERT INTO `authorised_values` (category, authorised_value, lib) VALUES
> ('MANUAL_INV','Copier Fees','.25');
> git grep 'Copier Fees'
> installer/data/mysql/es-ES/optional/auth_val.sql:INSERT INTO
> `authorised_values` (category, authorised_value, lib) VALUES
> ('MANUAL_INV','Copier Fees','.25');
> Oops, you should remove that one above.
> Since we previously installed Copier Fees as a manual invoice, I think you
> should do now too. So add it too account_debit_types.sql.
> And Copier Fees should also have a new code. Now the description and code
> are the same. It is inconsistent with the other debit types. You also
> changed lots of other codes, or not?

I spotted that one this morning too, it certainly needs cleaning up here :).

> Does this hold for some other debit types too that were formerly hardcoded?
> Stuff like sundry etc. ? Which actually is a horrible category..
> +       [%- SWITCH account.debit_type_code -%]
> +           [%- CASE 'ACCOUNT'          -%]Account creation fee
> +           [%- CASE 'ACCOUNT_RENEW'    -%]Account renewal fee
> +           [%- CASE 'LOST'             -%]Lost item
> +           [%- CASE 'M'                -%]Sundry
> +           [%- CASE 'NEW_CARD'         -%]New card
> +           [%- CASE 'OVERDUE'          -%]Fine
> +           [%- CASE 'PROCESSING'       -%]Lost item processing fee
> +           [%- CASE 'RENT'             -%]Rental fee
> +           [%- CASE 'RENT_DAILY'       -%]Daily rental fee
> +           [%- CASE 'RENT_RENEW'       -%]Renewal of rental item
> +           [%- CASE 'RENT_DAILY_RENEW' -%]Rewewal of daily rental item
> +           [%- CASE 'RESERVE'          -%]Hold fee
> +           [%- CASE 'RESERVE_EXPIRED'  -%]Hold waiting too long
> +           [%- CASE                    -%][% account.debit_type.description
> | html %]
> +       [%- END -%]
> Note that koha-tmpl/intranet-tmpl/prog/en/includes/accounts.inc contains
> debit code M for sundry while account_debit_types.sql install M as Manual
> fee !
> This is a BLOCKER. Please correct.

Good catch :)

> At the credit side, I am seeing code FORW (and FOR and W and WO !) and
> having doubts about it, leaving it outside the scope for now..

I've submitted a bug for the conversion of credit type accounttypes now too
which should take care of those.. Keeping them separate for now in an attempt
to not drown QA in one superbug ;).

> 
> Submitting another comment for the failing tests.

Thanks :)

-- 
You are receiving this mail because:
You are watching all bug changes.