Bug 39721 - Remove GetSuggestion from C4/Suggestions.pm
Summary: Remove GetSuggestion from C4/Suggestions.pm
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Baptiste Wojtkowski (bwoj)
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 39722
  Show dependency treegraph
 
Reported: 2025-04-24 08:49 UTC by Baptiste Wojtkowski (bwoj)
Modified: 2025-09-04 10:14 UTC (History)
2 users (show)

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


Attachments
Bug 39721: Remove GetSuggestion from C4/Suggestions.pm (13.26 KB, patch)
2025-04-24 08:51 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 39721: Remove GetSuggestion from C4/Suggestions.pm (12.96 KB, patch)
2025-04-24 09:31 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 39721: Remove GetSuggestion from C4/Suggestions.pm (12.96 KB, patch)
2025-05-28 07:57 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 39721: Remove GetSuggestion from C4/Suggestions.pm (12.99 KB, patch)
2025-08-08 14:38 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 39721: Changes suggestion_ref in order to remove it (57.34 KB, patch)
2025-08-08 14:38 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 39721: (follow-up) Remove unblessed (9.63 KB, patch)
2025-08-08 14:38 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 39721: tidy (4.59 KB, patch)
2025-08-08 14:38 UTC, Baptiste Wojtkowski (bwoj)
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) 2025-04-24 08:49:24 UTC
See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23990
Comment 1 Baptiste Wojtkowski (bwoj) 2025-04-24 08:51:34 UTC
Created attachment 181402 [details] [review]
Bug 39721: Remove GetSuggestion from C4/Suggestions.pm

This set of patches removes the major part of C4/Suggestions. They have
been split for sake of QA's readability but should be squashed.

Things to test:
Create suggestion
Update status of suggestion
Create, see and receive an order containing a suggestion
Create a suggestion on an already existing record (it will need
        validation)
test cronjobs purge_suggestion and notice_unprocessed_suggestions
Comment 2 Baptiste Wojtkowski (bwoj) 2025-04-24 09:31:36 UTC
Created attachment 181416 [details] [review]
Bug 39721: Remove GetSuggestion from C4/Suggestions.pm

1 - Create suggestion
2 - Display a suggestion
3 - Amend a suggestion
To do with and without patch
Comment 3 Baptiste Wojtkowski (bwoj) 2025-05-28 07:57:51 UTC
Created attachment 182810 [details] [review]
Bug 39721: Remove GetSuggestion from C4/Suggestions.pm

1 - Create suggestion
2 - Display a suggestion
3 - Amend a suggestion
To do with and without patch
Comment 4 Jonathan Druart 2025-06-04 07:44:55 UTC
I would not call unblessed but manipulate the object directly here.

It's a bit more work here but it will be easier for the other changes.
Comment 5 Jonathan Druart 2025-07-22 08:01:30 UTC
ping?
Comment 6 Baptiste Wojtkowski (bwoj) 2025-08-08 14:38:26 UTC
Created attachment 185280 [details] [review]
Bug 39721: Remove GetSuggestion from C4/Suggestions.pm

1 - Create suggestion
2 - Display a suggestion
3 - Amend a suggestion
To do with and without patch
Comment 7 Baptiste Wojtkowski (bwoj) 2025-08-08 14:38:28 UTC
Created attachment 185281 [details] [review]
Bug 39721: Changes suggestion_ref in order to remove it

suggestion_ref is quite messy, it needs to be reworked in order to use
objects in suggestion.pl.

As far as I studied, it is used as
- the whole input from forms,
- the default content to input in a form
- the content that will be amended in order to propose another hash to
  modSuggestion. However, ModSuggestion will be removed so we don't need
  such a hash
- every suggestion that need to be updated using the "change selected
  suggestion" tool
- create the object suggestion_only, whose purpose is still unclear to
  me.

As a result it is created by :
> 25 my $suggestion_ref = { $input->Vars };
And by removing a number of keys depending on the use of suggestion_ref
in the script.

In this patch, we remove every code made to use suggestion_ref as the
source object for form and use another variable, $stored_suggestion which is an object.

In next patch we aim at fully removing $suggestion_ref, which is still
used by ModSuggestion
Comment 8 Baptiste Wojtkowski (bwoj) 2025-08-08 14:38:31 UTC
Created attachment 185282 [details] [review]
Bug 39721: (follow-up) Remove unblessed
Comment 9 Baptiste Wojtkowski (bwoj) 2025-08-08 14:38:33 UTC
Created attachment 185283 [details] [review]
Bug 39721: tidy
Comment 10 Bernard 2025-08-21 11:23:28 UTC
Hi Baptiste,

When I install your patch, I am unable to create or modify a suggestion in the staff intranet and get a 500 error. The log shows:

DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'CAD' in 'where clause' at /kohadevbox/koha/Koha/Objects.pm line 401

Looks like that is a reference to Canadian Dollars currency (CAD)

It still works fine to create a request in the OPAC.

Hope this helps.
Comment 11 Jonathan Druart 2025-09-02 12:55:05 UTC
There is also:

qw( suggestedbyme op displayby tabcode notify filter_archived koha_login_context auth_forwarded_hash password userid );

[2025/09/02 12:49:03] [WARN] Useless use of a constant ("suggestedbyme") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                      
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("op") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                                 
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("displayby") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                          
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("tabcode") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                            
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("notify") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                             
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("filter_archived") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                    
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("koha_login_context") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                 
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("auth_forwarded_hash") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("password") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102.                                                                           
[2025/09/02 12:49:03] [WARN] Useless use of a constant ("userid") in void context at /kohadevbox/koha/suggestion/suggestion.pl line 102
Comment 12 Jonathan Druart 2025-09-02 12:56:38 UTC
I would restart from scratch here. You should not try to remove suggestion_ref.

suggestion.pl is used for add/edit but also for searching.

On this specific bug you should focus on GetSuggestion only.
Comment 13 Jonathan Druart 2025-09-02 13:02:52 UTC
(In reply to Jonathan Druart from comment #12)
> I would restart from scratch here. You should not try to remove
> suggestion_ref.
> 
> suggestion.pl is used for add/edit but also for searching.

Sorry, this is wrong since we now have bug 33430. Keep trying this way then! :)
Comment 14 Jonathan Druart 2025-09-02 13:18:06 UTC
comment 10 is because you removed this line:

-        map { delete $$suggestion_ref{$_} unless $_ eq 'branchcode' } keys %$suggestion_ref;

There are other lines that have been removed incorrectly IMO:

-foreach my $key ( keys %$suggestion_ref ) {
-    delete $suggestion_ref->{$key} if ( !$suggestion_ref->{$key} && ( $op eq 'else' ) );
-    delete $suggestion_ref->{$key} if $key =~ m{^DataTables_acqui_suggestions_suggestions};
-}