[Koha-bugs] [Bug 9016] Multi transport types for notices

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

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9016

--- Comment #88 from Jonathan Druart <jonathan.druart at biblibre.com> ---
(In reply to M. de Rooy from comment #87)
> First QA comment:

\o/ Thanks Marcel!

>   * General: Feature looks good (although not complete yet). Seems to be a
> welcome addition.
>     At this moment in the process, it is not clear what is used (probably
> only email) and what is not. That makes a push not very obvious?

I tried to explain the complete feature in a previous email to koha-devel: all
patches should be reviewed together and pushed together, in order to keep thing
consistent (for the review, not the same day :))

>   * Patch 1
>   * Functionality: I can imagine that I have filled in multiple transport
> types but want to activate or deactivate some of them. Would you need some
> status on each of them? 

I am sorry but I don't understand what you mean here.

>   * POD of GetMessageTransportTypes says: returns a list of hashes?? But it
> returns an arrayref! (See also Patch 3, yes.)
>   * BTW why not use my $mtts = $dbh->selectcol_arrayref.. instead of the
> more complex map { } .. in GetMessageTransportTypes?

Yes, will be fixed.

>   * Just noting that we still have lots of SQL code in tools/letter.pl. Too
> bad you didn't move it ;) No blocker for me.

Not too much at the same time :)

>   * Textual: You must specify a title and a content -> Please specify title
> and content [a content sounds funny to me as non-native speaker]
>   * IMPORTANT: The Insert button does no longer surround the fieldname with
> << and >>. 

Good catch, will be fixed!

>     When resolving bugs crosses the boundaries of a patch set, it could be
> hard to perform QA. (I simply cannot qa 30 patches at 8 reports at one time
> [numbers at random, no offense]..)

I tried to fix issues on the same report, all linked reports do something
different.
Except if it is a bug on master, I open a new report.

>   * Patch 2
>   * Functionality clash? For Overdue notice (phone notice): I can select all
> transport types. Confusing..

I don't find this notice in sql files. Normally, with this feature, all notices
created for a specific transport type, should be moved "into" the "main"
notice. For instance, "Overdue notice (phone notice)" should be moved into the
"phone" template for the "Overdue" notice.

>   * From commit: [Currently, only email, sms and print are relevant.] Note
> that you could hide what is not relevant now?

Maybe someone else would like to implement phone, etc.
So I did not restrict the mtts to display on the interface.

>   * Glancing through tools/overduerules.pl: If you move First, Second, etc.
> from template to script, you do not translate them anymore?

Yes, they are translatable:
    var tab_map = { "1" : _("First"), "2" : _("Second"), "3" : _("Third")};

>   * Patch 4
>     The dbrev prints: Upgrade done (Bug 9016: Adds the association table
> overduerules_transport_types)
>     The db rev does a lot more than that (or more important things..)
>     It also adds e.g. message_transport_type to letter which is closer to
> the 'general theme'.
>     Please change the print message to a more generic one. 

Will be fixed.

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