Bug 37628 - Remove get_opac_news_by_id
Summary: Remove get_opac_news_by_id
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 31383
Blocks: 37629
  Show dependency treegraph
 
Reported: 2024-08-13 14:24 UTC by Baptiste Wojtkowski (bwoj)
Modified: 2024-08-20 10:08 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 37628: (follow-up) Remove get_opac_news_by_id (7.63 KB, patch)
2024-08-13 14:30 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37628: (follow-up) Remove get_opac_news_by_id (7.71 KB, patch)
2024-08-14 12:29 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 37628: (follow-up) Remove get_opac_news_by_id (7.80 KB, patch)
2024-08-16 06:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Baptiste Wojtkowski (bwoj) 2024-08-13 14:24:01 UTC
The last patch from 31383 was not applied.
Comment 1 Baptiste Wojtkowski (bwoj) 2024-08-13 14:24:30 UTC
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157742
Comment 2 Baptiste Wojtkowski (bwoj) 2024-08-13 14:30:39 UTC
Created attachment 170265 [details] [review]
Bug 37628: (follow-up) Remove get_opac_news_by_id

Keep the same behaviour, but wondering why we don't return 'content'
when there is no match.

ie.
+    is( $additional_contents, undef );
should certainly be
+    is( $additional_contents->{content}->count, 0 );

WNC amended patch - tidied

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 3 Baptiste Wojtkowski (bwoj) 2024-08-13 14:32:37 UTC
May I switch the state to need signoff since it was signed off by Nick ?
Comment 4 Katrin Fischer 2024-08-13 14:40:39 UTC
I think better to have this tested again as the code base has moved quite a bit. Could you add a quick test plan?
Comment 5 Baptiste Wojtkowski (bwoj) 2024-08-13 15:08:48 UTC
Hum... Unfortunately I am not quite sure of the exact purpose of this patch. This was a followup for https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31383 

The thing is, I need this patch for https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37629 because it restores news search by ID, but the exact purpose is unclear to me and I found out that it was not applied while investigating on BZ37629.

Let's wait for their author's response :)
Comment 6 Jonathan Druart 2024-08-14 11:43:43 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #5)
> Hum... Unfortunately I am not quite sure of the exact purpose of this patch.
> This was a followup for
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31383 
> 
> The thing is, I need this patch for
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37629 because it
> restores news search by ID, but the exact purpose is unclear to me and I
> found out that it was not applied while investigating on BZ37629.

It is code cleaning. It removes a sub and makes the code more robust. If no regression found it means that it works.

It is a shame it has been missed.
Comment 7 Owen Leonard 2024-08-14 12:29:03 UTC
Created attachment 170317 [details] [review]
Bug 37628: (follow-up) Remove get_opac_news_by_id

Keep the same behaviour, but wondering why we don't return 'content'
when there is no match.

ie.
+    is( $additional_contents, undef );
should certainly be
+    is( $additional_contents->{content}->count, 0 );

WNC amended patch - tidied

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Marcel de Rooy 2024-08-16 06:40:13 UTC
Created attachment 170425 [details] [review]
Bug 37628: (follow-up) Remove get_opac_news_by_id

Keep the same behaviour, but wondering why we don't return 'content'
when there is no match.

ie.
+    is( $additional_contents, undef );
should certainly be
+    is( $additional_contents->{content}->count, 0 );

WNC amended patch - tidied

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Katrin Fischer 2024-08-16 15:37:51 UTC
Pushed for 24.11!

Well done everyone, thank you!