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

Julian Maurice <julian.maurice@biblibre.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Failed QA                   |Signed Off

--- Comment #249 from Julian Maurice <julian.maurice@biblibre.com> ---
(In reply to Jonathan Druart from comment #245)
> 1. aqbasketgroups.closedate is set to NOW, maybe better to use
> basket.closedate (the latest)?
Fixed

> 2. ->bookseller should be ->vendor (not blocker as we already have both in
> Koha::*...)
Fixed

> 3. a. I would not "cache" ->baskets, we already deal with the context in
> Koha::Objects->search
I don't know what is the context you are talking about, but I removed the
"caching"

> b. ->baskets_count - no need to have it, it should be replaced with
> baskets->count
> => Maybe you want to keep them for performance purpose, but is it really a
> performance improvement?
It's used only in templates, because TT seems to force the list context even
with a directive like this: [% basketgroup.baskets.count %]
It tries to call count on the first item of @{ $basketgroup->baskets }
I'd happily remove this sub if you have a better idea.

> 5. basketgroup.tt has a [% IF booksellerid %], it should not be useful now.
Fixed

> We may want to display a friendly message if called without a [valid] id
Is returning a 404 error friendly ? Because it's probably what should be done
if
the id is invalid.

> 6. basketgroups.tt should have JS at the bottom
Fixed

> 7. Some links still point to basketgroup.pl?booksellerid=X without the
> basketgroupid, they should be replaced right?
> i.e. code related to 'sub displaybasketgroups' in basketgroup.pl should be
> removed (?)
Fixed

> 8. Are you sure you need the changes done to C4::Acquisition?
No. They're not needed anymore. I removed them.

> 9. acqui/basketgroup.pl
> + use List::MoreUtils qw/uniq/;
> => Not needed
Fixed

> 10. "kohaDataTable": We already have KohaTable. You are adding a 3rd way to
> init a table.
We already have 3 ways to initialize DT:
1. The "legacy API" way:
   $("#table_id").dataTable($.extend(true, {}, dataTablesDefaults, { ... });
2. The "legacy API + column_settings" way:
   KohaTable('table_id', ...);
3. The "1.10+ API" way
   $('#table_id').DataTable(...)

kohaDataTable is just a wrapper around the 3rd that provides the same defaults
than the 1st and 2nd methods.
I don't see how this is bad.

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