Bug 23166 - Simplify code related to orders in catalogue/*detail.pl
Summary: Simplify code related to orders in catalogue/*detail.pl
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 9780 23084
Blocks: 26139 26365
  Show dependency treegraph
 
Reported: 2019-06-20 02:34 UTC by Jonathan Druart
Modified: 2020-09-04 19:49 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 23166: Replace the count of orders with Koha::Acq::Orders (1.76 KB, patch)
2019-06-20 02:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} (37.12 KB, patch)
2019-06-20 02:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Remove baskets_orders code (5.16 KB, patch)
2019-06-20 02:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Remove baskets_deletedorders code (5.86 KB, patch)
2019-06-20 02:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Replace the count of orders with Koha::Acq::Orders (1.76 KB, patch)
2019-06-20 02:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Remove baskets_orders code (5.17 KB, patch)
2020-05-05 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Remove baskets_deletedorders code (5.86 KB, patch)
2020-05-05 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Replace the count of orders with Koha::Acq::Orders (1.80 KB, patch)
2020-05-05 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Remove filtering from controllers (8.33 KB, patch)
2020-05-05 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods (3.08 KB, patch)
2020-05-05 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Call the methods from the .inc (3.46 KB, patch)
2020-05-05 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Remove baskets_orders code (5.22 KB, patch)
2020-08-24 14:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Remove baskets_deletedorders code (5.92 KB, patch)
2020-08-24 14:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Replace the count of orders with Koha::Acq::Orders (1.86 KB, patch)
2020-08-24 14:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Remove filtering from controllers (8.39 KB, patch)
2020-08-24 14:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods (3.10 KB, patch)
2020-08-24 14:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Call the methods from the .inc (3.53 KB, patch)
2020-08-24 14:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Remove baskets_orders code (5.31 KB, patch)
2020-08-24 15:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Remove baskets_deletedorders code (6.01 KB, patch)
2020-08-24 15:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Replace the count of orders with Koha::Acq::Orders (1.95 KB, patch)
2020-08-24 15:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Remove filtering from controllers (8.48 KB, patch)
2020-08-24 15:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods (3.20 KB, patch)
2020-08-24 15:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Call the methods from the .inc (3.62 KB, patch)
2020-08-24 15:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: (QA follow-up) Add POD text (731 bytes, patch)
2020-08-24 15:36 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Remove baskets_orders code (5.17 KB, patch)
2020-08-24 15:54 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Remove baskets_deletedorders code (5.86 KB, patch)
2020-08-24 15:54 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Replace the count of orders with Koha::Acq::Orders (1.80 KB, patch)
2020-08-24 15:54 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Remove filtering from controllers (8.33 KB, patch)
2020-08-24 15:54 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods (3.05 KB, patch)
2020-08-24 15:54 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Call the methods from the .inc (3.48 KB, patch)
2020-08-24 15:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: (QA follow-up) Add POD text (731 bytes, patch)
2020-08-24 15:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Remove baskets_orders code (5.22 KB, patch)
2020-08-24 20:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Remove baskets_deletedorders code (5.92 KB, patch)
2020-08-24 20:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Replace the count of orders with Koha::Acq::Orders (1.86 KB, patch)
2020-08-24 20:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Remove filtering from controllers (8.39 KB, patch)
2020-08-24 20:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods (3.10 KB, patch)
2020-08-24 20:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Call the methods from the .inc (3.53 KB, patch)
2020-08-24 20:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: (QA follow-up) Add POD text (787 bytes, patch)
2020-08-24 20:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23166: Remove baskets_orders code (5.37 KB, patch)
2020-08-24 20:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Remove baskets_deletedorders code (6.07 KB, patch)
2020-08-24 20:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Replace the count of orders with Koha::Acq::Orders (2.00 KB, patch)
2020-08-24 20:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Remove filtering from controllers (8.53 KB, patch)
2020-08-24 20:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods (3.25 KB, patch)
2020-08-24 20:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Call the methods from the .inc (3.68 KB, patch)
2020-08-24 20:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: (QA follow-up) Add POD text (938 bytes, patch)
2020-08-24 20:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 23166: Early return if no set (863 bytes, patch)
2020-08-25 09:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23166: Early return if no set (1015 bytes, patch)
2020-08-25 09:47 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-06-20 02:34:50 UTC
Since bug 9780 we are the ability to alert the librarian when a record which has been ordered will be deleted.
Then the code to handle the alert has been copied to other catalogue/*detail.pl script, as the JS check is in the toolbar.

This code is really unnecessary complicated and sometime simply not necessary. It could be replaced with Koha::Acquisition::Orders->search and make it more readable.

(found it working on bug 23084, so writing it on top of it).
Comment 1 Jonathan Druart 2019-06-20 02:36:20 UTC
Created attachment 90824 [details] [review]
Bug 23166: Replace the count of orders with Koha::Acq::Orders
Comment 2 Jonathan Druart 2019-06-20 02:36:53 UTC
Created attachment 90825 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!
Comment 3 Jonathan Druart 2019-06-20 02:37:42 UTC
Created attachment 90826 [details] [review]
Bug 23166: Remove baskets_orders code

This is never used.
Comment 4 Jonathan Druart 2019-06-20 02:37:45 UTC
Created attachment 90827 [details] [review]
Bug 23166: Remove baskets_deletedorders code

This is never used.
Comment 5 Jonathan Druart 2019-06-20 02:37:48 UTC
Created attachment 90828 [details] [review]
Bug 23166: Replace the count of orders with Koha::Acq::Orders
Comment 6 Jonathan Druart 2019-06-20 02:39:05 UTC
Martin, any suggestions to make this even more readable? Maybe using filter_by?

I will be waiting for a decision before applying the change to other *detail.pl scripts.
Comment 7 Jonathan Druart 2020-05-05 13:55:00 UTC
Created attachment 104365 [details] [review]
Bug 23166: Remove baskets_orders code

This is never used.
Comment 8 Jonathan Druart 2020-05-05 13:55:04 UTC
Created attachment 104366 [details] [review]
Bug 23166: Remove baskets_deletedorders code

This is never used.
Comment 9 Jonathan Druart 2020-05-05 13:55:07 UTC
Created attachment 104367 [details] [review]
Bug 23166: Replace the count of orders with Koha::Acq::Orders
Comment 10 Jonathan Druart 2020-05-05 13:55:10 UTC
Created attachment 104368 [details] [review]
Bug 23166: Remove filtering from controllers
Comment 11 Jonathan Druart 2020-05-05 13:55:13 UTC
Created attachment 104369 [details] [review]
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods
Comment 12 Jonathan Druart 2020-05-05 13:55:16 UTC
Created attachment 104370 [details] [review]
Bug 23166: Call the methods from the .inc

We do not longer need the order variables to be passed from the
controllers, we can call the methods on the biblio object instead.

There is something wrong with our ->search method and TT behaviours, it
is hard to retrieve object list in a scalar context.
If [% objects.method.count %] is called, objects.method will get the
first object of the list and count will explode (Koha::Object->count
does not exist)

We need to force the call in a scalar context to retrieve an iterator
and prevent to fetch all the objects (we could have called all then
size, but it's not efficient)

If adopted I will move the plugin on a separate bug report to ease
backport
Comment 13 Jonathan Druart 2020-05-05 13:58:01 UTC
Test plan:
Create several orders (on at least 2 bibliographic records)
Cancel some
Try to delete the bibliographic records
=> You should the JS alert about existing orders

The delete button from the different views where the toolbar is displayed should be tested:
- normal detail
- ISBD detail
- MARC detail
- image viewer
- labeled MARC
- More detail

No behaviour changes expected.
Comment 14 Jonathan Druart 2020-05-05 13:58:28 UTC
This is ready for testing (at least receive feedback on the different changes made by this patch set)
Comment 15 Nick Clemens 2020-08-24 14:53:51 UTC
Created attachment 108991 [details] [review]
Bug 23166: Remove baskets_orders code

This is never used.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2020-08-24 14:53:54 UTC
Created attachment 108992 [details] [review]
Bug 23166: Remove baskets_deletedorders code

This is never used.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2020-08-24 14:53:57 UTC
Created attachment 108993 [details] [review]
Bug 23166: Replace the count of orders with Koha::Acq::Orders

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2020-08-24 14:54:00 UTC
Created attachment 108994 [details] [review]
Bug 23166: Remove filtering from controllers

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens 2020-08-24 14:54:03 UTC
Created attachment 108995 [details] [review]
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens 2020-08-24 14:54:06 UTC
Created attachment 108996 [details] [review]
Bug 23166: Call the methods from the .inc

We do not longer need the order variables to be passed from the
controllers, we can call the methods on the biblio object instead.

There is something wrong with our ->search method and TT behaviours, it
is hard to retrieve object list in a scalar context.
If [% objects.method.count %] is called, objects.method will get the
first object of the list and count will explode (Koha::Object->count
does not exist)

We need to force the call in a scalar context to retrieve an iterator
and prevent to fetch all the objects (we could have called all then
size, but it's not efficient)

If adopted I will move the plugin on a separate bug report to ease
backport

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Nick Clemens 2020-08-24 14:54:53 UTC
Qa tool has complaints:
 FAIL	Koha/Template/Plugin/Context.pm
   FAIL	  pod coverage
		POD is missing for 'Scalar'

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc
   FAIL	  valid_template
		Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 43.
Comment 22 Joonas Kylmälä 2020-08-24 15:11:02 UTC Comment hidden (obsolete)
Comment 23 Joonas Kylmälä 2020-08-24 15:11:05 UTC Comment hidden (obsolete)
Comment 24 Joonas Kylmälä 2020-08-24 15:11:08 UTC Comment hidden (obsolete)
Comment 25 Joonas Kylmälä 2020-08-24 15:11:12 UTC Comment hidden (obsolete)
Comment 26 Joonas Kylmälä 2020-08-24 15:11:15 UTC Comment hidden (obsolete)
Comment 27 Joonas Kylmälä 2020-08-24 15:11:18 UTC Comment hidden (obsolete)
Comment 28 Joonas Kylmälä 2020-08-24 15:27:59 UTC
I totally missed running the qa tool, thanks for noticing Nick!
Comment 29 Joonas Kylmälä 2020-08-24 15:36:33 UTC Comment hidden (obsolete)
Comment 30 Joonas Kylmälä 2020-08-24 15:54:45 UTC
Created attachment 109020 [details] [review]
Bug 23166: Remove baskets_orders code

This is never used.
Comment 31 Joonas Kylmälä 2020-08-24 15:54:48 UTC
Created attachment 109021 [details] [review]
Bug 23166: Remove baskets_deletedorders code

This is never used.
Comment 32 Joonas Kylmälä 2020-08-24 15:54:52 UTC
Created attachment 109022 [details] [review]
Bug 23166: Replace the count of orders with Koha::Acq::Orders
Comment 33 Joonas Kylmälä 2020-08-24 15:54:55 UTC
Created attachment 109023 [details] [review]
Bug 23166: Remove filtering from controllers
Comment 34 Joonas Kylmälä 2020-08-24 15:54:59 UTC
Created attachment 109024 [details] [review]
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods
Comment 35 Joonas Kylmälä 2020-08-24 15:55:02 UTC
Created attachment 109025 [details] [review]
Bug 23166: Call the methods from the .inc

We do not longer need the order variables to be passed from the
controllers, we can call the methods on the biblio object instead.

There is something wrong with our ->search method and TT behaviours, it
is hard to retrieve object list in a scalar context.
If [% objects.method.count %] is called, objects.method will get the
first object of the list and count will explode (Koha::Object->count
does not exist)

We need to force the call in a scalar context to retrieve an iterator
and prevent to fetch all the objects (we could have called all then
size, but it's not efficient)

If adopted I will move the plugin on a separate bug report to ease
backport
Comment 36 Joonas Kylmälä 2020-08-24 15:55:06 UTC
Created attachment 109026 [details] [review]
Bug 23166: (QA follow-up) Add POD text
Comment 37 Joonas Kylmälä 2020-08-24 15:58:26 UTC
I attached a follow-up for the POD and removed the sign-offs given the other issue Nick pointed out still remains:

>  FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc
>  FAIL	  valid_template
>		Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 43.
Comment 38 Joonas Kylmälä 2020-08-24 16:28:47 UTC
There seems to be nothing wrong with the code, it is just that the QA tool doesn't add the "orders" object because this is not being executed from the real .pl script, so therefore the $set variable is undef. I think we could say this is false positive then, right? Or if we want to prevent the QA tool failing maybe such a check can be added:

  sub Scalar {
      my ( $self, $set, $method ) = @_;
+     return undef unless $set;
      $set = $set->$method;
      return $set;
  }

What do you think? I think it is safe to ignore this here and we could go ahead with the current patches.
Comment 39 Katrin Fischer 2020-08-24 20:14:43 UTC
(In reply to Joonas Kylmälä from comment #37)
> I attached a follow-up for the POD and removed the sign-offs given the other
> issue Nick pointed out still remains:
> 
> >  FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/catalog-strings.inc
> >  FAIL	  valid_template
> >		Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 43.

Hi Joonas,
we usually don't remove the sign-off lines - that's one reason we prefer follow-ups - because it allows to see the changes, but also because that means you don't change what has been signed off by someone.
Comment 40 Nick Clemens 2020-08-24 20:32:04 UTC
Created attachment 109037 [details] [review]
Bug 23166: Remove baskets_orders code

This is never used.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 41 Nick Clemens 2020-08-24 20:32:08 UTC
Created attachment 109038 [details] [review]
Bug 23166: Remove baskets_deletedorders code

This is never used.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 42 Nick Clemens 2020-08-24 20:32:11 UTC
Created attachment 109039 [details] [review]
Bug 23166: Replace the count of orders with Koha::Acq::Orders

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Nick Clemens 2020-08-24 20:32:14 UTC
Created attachment 109040 [details] [review]
Bug 23166: Remove filtering from controllers

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Nick Clemens 2020-08-24 20:32:17 UTC
Created attachment 109041 [details] [review]
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 45 Nick Clemens 2020-08-24 20:32:20 UTC
Created attachment 109042 [details] [review]
Bug 23166: Call the methods from the .inc

We do not longer need the order variables to be passed from the
controllers, we can call the methods on the biblio object instead.

There is something wrong with our ->search method and TT behaviours, it
is hard to retrieve object list in a scalar context.
If [% objects.method.count %] is called, objects.method will get the
first object of the list and count will explode (Koha::Object->count
does not exist)

We need to force the call in a scalar context to retrieve an iterator
and prevent to fetch all the objects (we could have called all then
size, but it's not efficient)

If adopted I will move the plugin on a separate bug report to ease
backport

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 46 Nick Clemens 2020-08-24 20:32:24 UTC
Created attachment 109043 [details] [review]
Bug 23166: (QA follow-up) Add POD text

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 47 Joonas Kylmälä 2020-08-24 20:55:12 UTC
Created attachment 109044 [details] [review]
Bug 23166: Remove baskets_orders code

This is never used.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 48 Joonas Kylmälä 2020-08-24 20:55:16 UTC
Created attachment 109045 [details] [review]
Bug 23166: Remove baskets_deletedorders code

This is never used.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 49 Joonas Kylmälä 2020-08-24 20:55:19 UTC
Created attachment 109046 [details] [review]
Bug 23166: Replace the count of orders with Koha::Acq::Orders

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 50 Joonas Kylmälä 2020-08-24 20:55:23 UTC
Created attachment 109047 [details] [review]
Bug 23166: Remove filtering from controllers

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 51 Joonas Kylmälä 2020-08-24 20:55:26 UTC
Created attachment 109048 [details] [review]
Bug 23166: Add filter_by_current and _cancelled Koha::Acq::Orders methods

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 52 Joonas Kylmälä 2020-08-24 20:55:30 UTC
Created attachment 109049 [details] [review]
Bug 23166: Call the methods from the .inc

We do not longer need the order variables to be passed from the
controllers, we can call the methods on the biblio object instead.

There is something wrong with our ->search method and TT behaviours, it
is hard to retrieve object list in a scalar context.
If [% objects.method.count %] is called, objects.method will get the
first object of the list and count will explode (Koha::Object->count
does not exist)

We need to force the call in a scalar context to retrieve an iterator
and prevent to fetch all the objects (we could have called all then
size, but it's not efficient)

If adopted I will move the plugin on a separate bug report to ease
backport

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 53 Joonas Kylmälä 2020-08-24 20:55:33 UTC
Created attachment 109050 [details] [review]
Bug 23166: (QA follow-up) Add POD text

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 54 Joonas Kylmälä 2020-08-24 21:08:54 UTC
(In reply to Katrin Fischer from comment #39)
> we usually don't remove the sign-off lines - that's one reason we prefer
> follow-ups - because it allows to see the changes, but also because that
> means you don't change what has been signed off by someone.

sorry for the mess! I was working on signing-off the patches the same time as Nick and hadn't noticed he had submitted his signed-off-by lines just some minutes earlier so when I ran git-bz attach with my patches it overrode his signed-off-by lines with mine. And then Nick had noticed the QA tool failure after his sign-off and put the bug to FQA state so I thought it would be best to wait for him to sign-off again, which also prompted for me to remove my own SO lines.

Anyhow, Nick signed-off this now and I added my QA sign-off so ready for RM to merge :)
Comment 55 Katrin Fischer 2020-08-24 22:39:20 UTC
Sounds good and I can stop to stare at this because I am getting nowhere tonight :)
Comment 56 Jonathan Druart 2020-08-25 09:44:05 UTC
Created attachment 109058 [details] [review]
Bug 23166: Early return if no set

It prevents valid_template check to raise
Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 49.
Comment 57 Jonathan Druart 2020-08-25 09:45:06 UTC
(In reply to Joonas Kylmälä from comment #38)
> There seems to be nothing wrong with the code, it is just that the QA tool
> doesn't add the "orders" object because this is not being executed from the
> real .pl script, so therefore the $set variable is undef. I think we could
> say this is false positive then, right? Or if we want to prevent the QA tool
> failing maybe such a check can be added:
> 
>   sub Scalar {
>       my ( $self, $set, $method ) = @_;
> +     return undef unless $set;
>       $set = $set->$method;
>       return $set;
>   }
> 
> What do you think? I think it is safe to ignore this here and we could go
> ahead with the current patches.

I've decided to fix it here, it should not hurt.
Comment 58 Joonas Kylmälä 2020-08-25 09:47:09 UTC
Created attachment 109059 [details] [review]
Bug 23166: Early return if no set

It prevents valid_template check to raise
Can't call method "filter_by_current" without a package or object reference at /kohadevbox/koha/Koha/Template/Plugin/Context.pm line 49.

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 59 Joonas Kylmälä 2020-08-25 09:47:57 UTC
(In reply to Jonathan Druart from comment #57)
> I've decided to fix it here, it should not hurt.

Ok, I added my SO for it in case you didn't push yet.
Comment 60 Jonathan Druart 2020-08-25 13:16:11 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 61 Lucas Gass 2020-09-04 19:49:58 UTC
enhancement will not be backported to 20.05.x