[Koha-bugs] [Bug 20212] Slowness in receiving in acquisitions

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

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

--- Comment #118 from Tomás Cohen Arazi <tomascohen at gmail.com> ---
(In reply to Jonathan Druart from comment #117)
> 1. We need a delay between the different AJAX request when filtering the
> table, otherwise we face weird behaviour and lag (server and browser)

I will look into that. I understand you're referring to the columns filters.

> 2. Prior to this patch, the "pending orders" table contained orders with
> status (orderstatus) set to "ordered" or "partial". Now we see them all.
> See 'pending' and 'ordered' flag of SearchOrders

That's odd, actually. What we do in the controller:

+        if ( $only_active ) {
+            $orders_rs = Koha::Acquisition::Orders->filter_by_active;
+        }

which in turn

+sub filter_by_active {
+    my ($self) = @_;
+    return $self->search(
+        {
+            orderstatus => [ 'new', 'ordered', 'partial' ]
+        }
+    );
+}

should take care of that. Maybe we are loosing the only_active query param
somewhere. Worth checking.

Also, maybe there's something wrong when it comes to partially received orders.
It would be a problem with bug 25670.

> 3. Not blocker but worth noting, we lost the column filter on "Order cost"

That worked just fine because all data was loaded on the DataTable. If this is
a problem, we could add a special munging to the query parameters so it builds
the multiply on the query. I didn't consider it that important.

> 4. I don't understand how works the column filters. It's the first view
> where we introduce it. It would have been interesting to have it on a
> simpler view (cities?) to understand how it is working.
> 
> There is this code:
> 793             // column filter events handling
> 794             $(".column-filter").each(function () {
> 795                 $(this).on( 'keyup change', function () {
> 
> It's like we are not using the DT standard way of using column filtering.

This is related to (1). Will take a look.

> 5. Code to build the URI is c/p in
> 794             $(".column-filter").each(function () {
> and
> 809             $("#filterform").on("submit", function(e) {

It is not exactly the same. I think it is fine as-is.

> 6. In the schema:
> 
> +__PACKAGE__->has_many(
> +  "biblioitem",
> +  "Koha::Schema::Result::Biblioitem",
> +  { "foreign.biblionumber" => "self.biblionumber" },
> +  { cascade_copy => 0, cascade_delete => 0 },
> +);
> 
> Why has_many? Why cascade_delete => 0?

This is just having an alias for the already existing relationship. I consider
it correct as it is what we have already defined in the schema. I don't mind
changing it anyway.

> 7. Not blocker for now, but I think we need some JS functions to display
> variables coming from the REST API.
> For instance this code appears a lot:
>   if ( data != null ) {
>     return data.escapeHtml().format_price();
>   }
>   else {
>     return "";
>   }
> 
> or
> 
>   if ( data == null ) {
>     return "";
>   }
>   else {
>     return data;
>   }

I agree. But it is small enough anyway. Truth is, we do the same in Perl.
Specially when it comes to rendering undef.

> 8. In the "render" you have
>   if ( type != 'display' ) {
> Why are you testing that?
> The different values are sorting, filtering and display. According to the
> doc 
> https://datatables.net/reference/option/columns.render

Usually, it is to leave the data untouched if is not for display, and tweak it
for display. Not sure about each specific case you're referring to.

> 9. There are several places where strings are not escaped
> 
> At least 2 examples:
> 
> +                            return "<a
> href=\"/cgi-bin/koha/acqui/basketgroup.pl?op=add&booksellerid="
> +                                    + row.basket.vendor_id +
> "&basketgroupid="
> +                                    + row.basket.basket_group_id + "\">"
> +                                    + row.basket.basket_group.name + " (" +
> row.basket.basket_group_id + ")</a>";
> 
> 
> +                        return "<a
> href=\"/cgi-bin/koha/acqui/basket.pl?basketno=" + row.basket.basket_id +
> "\">" + data + " (" + row.basket.basket_id + ")</a>";

I'm not sure why those slipped in, but is probably a bad rebase. I'll fix them.

> Side notes (and my opinion), I don't think we are going into the right
> direction. The JS code we are adding in our template is not robust, hard to
> read, and so hard to maintain.
> Also we will have to rewrite things we have done in TT, like display of
> patron's name for instance (patron-title.inc).
> I really would like to see some efforts in improving the readability of the
> code we have already before adding more.

We really need to adopt a JS framework for dealing with the UI. Modern
frameworks fit well with APIs like the one we are writing.

I find myself (again) in the chicken-egg situation regarding this bug. When the
first routes were pushed, we just built hashrefs on the controllers and
returned them. We've come a long way from that point, with magic stuffs
happening under the hood to prevent working more than needed, and having clean
new code. The quest for optimal, easily reusable code and I'm very happy with
the results. We have a hell of a framework written. With its caveats of course.

But at some point, years are passing, and not being a bit more pragmatic is
holding us from lots of cool stuffs we could be doing.

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