Bug 12627

Summary: SQLHelper replacement - C4::Suggestions
Product: Koha Reporter: Yohann Dufour <yohann.dufour>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: bgkriegel, dcook, jonathan.druart, mtj, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 12445    
Bug Blocks: 11385, 13005, 13007, 13329, 13731, 22907    
Attachments: Bug 12627: SQLHelper replacement - C4::Suggestions
[SIGNED-OFF] Bug 12627: SQLHelper replacement - C4::Suggestions
[PASSED QA] Bug 12627: SQLHelper replacement - C4::Suggestions
Bug 12627: SQLHelper replacement - C4::Suggestions
Bug 12627: SQLHelper replacement - C4::Suggestions
[SIGNED-OFF] Bug 12627: SQLHelper replacement - C4::Suggestions
[PASSED QA] Bug 12627: SQLHelper replacement - C4::Suggestions
Bug 12627: DBIx::Class is case sensitive for column names
Bug 12627: Remove warnings
Bug 12627: Fix date management
Bug 12627: Fix default values
Bug 12627: Fix default values
Bug 12627: Fix default values
Bug 12627: Bug 12627: Fix default values - TT changes
Bug 12627: Fix default values
Bug 12627: Bug 12627: Fix default values - TT changes
Bug 12627: Fix default values
Bug 12627: Fix default values
[PASSED QA] Bug 12627: SQLHelper replacement - C4::Suggestions
[PASSED QA] Bug 12627: DBIx::Class is case sensitive for column names
[PASSED QA] Bug 12627: Remove warnings
[PASSED QA] Bug 12627: Fix date management
[PASSED QA] Bug 12627: Bug 12627: Fix default values - TT changes
[PASSED QA] Bug 12627: Fix default values

Description Yohann Dufour 2014-07-22 12:52:59 UTC
The module C4::Suggestions depends on module C4::SQLHelper. However, the module SQLHelper is obsolete, so the calls in NewSuggestion and UpdateSuggestion have to be replaced by the use of the module DBIx::Class.
Comment 1 Yohann Dufour 2014-07-23 07:36:34 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall (khall) 2014-07-25 13:14:21 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2014-07-28 21:29:54 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2014-07-28 21:33:40 UTC
I am sorry, but running the tests proved to be not enough. This patch breaks adding a suggestion from the staff interface (didn't test OPAC).

DBIx::Class::Row::new(): No such column 'EUR' on Koha::Schema::Result::Suggestion at /home/katrin/kohaclone/C4/Suggestions.pm line 435
Comment 5 Yohann Dufour 2014-07-29 12:08:15 UTC Comment hidden (obsolete)
Comment 6 Bernardo Gonzalez Kriegel 2014-08-07 21:53:58 UTC
Yohann, now on staff I can create a suggestion, but on opac I got

DBIx::Class::ResultSet::create(): No such column negcap on Koha::Schema::Result::Suggestion
Comment 7 Yohann Dufour 2014-08-08 13:13:49 UTC Comment hidden (obsolete)
Comment 8 Bernardo Gonzalez Kriegel 2014-08-08 16:22:20 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2014-08-17 09:24:01 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi (tcohen) 2014-08-26 18:39:58 UTC
Tests don't pass, presumably related to bug 12781 patches. Once I changed status for STATUS in several places, more tests pass [1], but it is still failing.

[1]
diff --git a/C4/Suggestions.pm b/C4/Suggestions.pm                                                                                                                                           
index ddfa1b6..9461fb2 100644
--- a/C4/Suggestions.pm
+++ b/C4/Suggestions.pm
@@ -428,7 +428,7 @@ sub NewSuggestion {
 
     my $new_suggestion = { %$suggestion };
     $suggestion->{STATUS} = "ASKED" unless $suggestion->{STATUS};
-    $new_suggestion->{status} = $suggestion->{STATUS};
+    $new_suggestion->{STATUS} = $suggestion->{STATUS};
     delete $new_suggestion->{STATUS};
 
     my $rs = Koha::Database->new->schema->resultset('Suggestion');
@@ -455,7 +455,7 @@ sub ModSuggestion {
     my $mod_suggestion = { %$suggestion };
     my $status = $suggestion->{STATUS};
     delete $mod_suggestion->{STATUS};
-    $mod_suggestion->{status} = $status;
+    $mod_suggestion->{STATUS} = $status;
Comment 11 Jonathan Druart 2014-09-29 13:20:17 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-09-29 13:25:44 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-09-30 09:28:02 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-09-30 10:22:38 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-09-30 10:31:32 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-09-30 10:57:21 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-09-30 10:57:26 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-09-30 10:58:05 UTC
Last 5 patches need to be QAed.
Comment 19 Jonathan Druart 2014-09-30 11:51:17 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-09-30 14:34:24 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-09-30 15:18:18 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2014-10-22 06:30:13 UTC
Sadly, I found a bug:

1) create a new suggestion from OPAC (filled out Title and item type)
2) switch to staff and edit it (changed publication year)
3) save: A DateTime object can only be compared to another DateTime object (DateTime=HASH(0x793ac50), 0). at /home/katrin/kohaclone/C4/Suggestions.pm line 479.
Comment 23 Jonathan Druart 2014-11-05 12:06:34 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2014-11-05 12:07:33 UTC
(In reply to Katrin Fischer from comment #22)
> Sadly, I found a bug:

Fortunately, you found it! :)

It should be fixed now.
Comment 25 Katrin Fischer 2014-11-05 21:39:23 UTC
Created attachment 33276 [details] [review]
[PASSED QA] Bug 12627: SQLHelper replacement - C4::Suggestions

With this patch, the subroutines NewSuggestion and ModSuggestion use DBIx::Class instead of C4::SQLHelper.
Moreover, the tests and the .pl have been adapted.

Test plan:
1) Apply the patch.

2) Execute the unit tests by launching :
prove t/db_dependent/Suggestions.t

3) The result has to be a success without error or warning :
t/db_dependent/Suggestions.t .. ok
All tests successful.
Files=1, Tests=91,  2 wallclock secs ( 0.05 usr  0.01 sys +  1.65 cusr  0.09 csys =  1.80 CPU)
Result: PASS

4) Log in the intranet, create a suggestion and verify the created suggestion.

5) Edit a suggestion from the intranet and verify the suggestion is correctly modified.

6) Log in the OPAC and verify you can add a suggestion.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass, suggestion created on staff and opac,
suggestion edited without problems, no koha-qa errors.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes tests and QA script:
Also tested:
- adding suggestion from staff and OPAC
- edit suggestion from staff
- deleting suggestion from OPAC
- changing to a normal status (email got created)
- changing to a custom status (SUGGEST_STATUS)
- display of custom status in OPAC

No problems found.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2014-11-05 21:39:29 UTC
Created attachment 33277 [details] [review]
[PASSED QA] Bug 12627: DBIx::Class is case sensitive for column names

STATUS should be "STATUS", not "status".

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2014-11-05 21:39:32 UTC
Created attachment 33278 [details] [review]
[PASSED QA] Bug 12627: Remove warnings

suggestion.pl: keys on reference is experimental

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2014-11-05 21:39:36 UTC
Created attachment 33279 [details] [review]
[PASSED QA] Bug 12627: Fix date management

C4::Dates->today returns a string formatted depending on the dateformat
syspref.

Before the original patch, SQLHelper managed both format (string
formatted and DateTime.

Now DBIX::Class only manages DateTime, so the call to NewSuggestion and
ModSuggestion should pass a DateTime object

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2014-11-05 21:39:40 UTC
Created attachment 33280 [details] [review]
[PASSED QA] Bug 12627: Bug 12627: Fix default values - TT changes

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2014-11-05 21:39:43 UTC
Created attachment 33281 [details] [review]
[PASSED QA] Bug 12627: Fix default values

The default value for *by and *date fields is NULL.
But without this patch, the values are 0 or 0000-00-00.
It comes from the fact that the form set to an empty string the values
and DBIX::Class does not consider them as undefined.

This patch is very ugly, not sure how we can fix that.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
No regressions found, adding and editing suggestions from
OPAC and staff.
Passes tests and QA script.
Comment 31 Tomás Cohen Arazi (tcohen) 2014-11-07 18:24:20 UTC
Patches pushed to master.

Thanks Yohann and Jonathan!
Comment 32 David Cook 2014-11-24 02:54:35 UTC
Could someone double-check adding a new suggestion via the OPAC?

My git is pretty messy at the moment, and I'm balancing a few different things at once, so I might have to check later.

However, when I try to add a suggestion via the OPAC in master, I get this error:

DBIx::Class::Row::new(): No such column 'branch' on Koha::Schema::Result::Suggestion at /koha/dev/dcook/git/C4/Suggestions.pm line 449

When I look up on the script, I notice this:

my $suggestion      = $input->Vars;

Which is rather nasty... and I notice this:

delete $$suggestion{$_} foreach qw<op suggestedbyme>;

Which is also rather nasty...

--

In any case, the form from opac-suggestions.pl passes branchcode as "branch", but DBIC expects to receive "branchcode". 

So... I guess the easy fix is to probably change the template. 

(But we should probably be explicitly building the $suggestion hash rather than relying on $input->Vars, as all sorts of badness could be passed in there which will cause the script to croak)
Comment 33 Mason James 2015-03-26 02:29:15 UTC
Possible selection for 3.16.x
Comment 34 Mason James 2015-05-29 07:34:53 UTC
(In reply to David Cook from comment #32)
> Could someone double-check adding a new suggestion via the OPAC?
> 
> However, when I try to add a suggestion via the OPAC in master, I get this
> error:
> 
> DBIx::Class::Row::new(): No such column 'branch' on
> Koha::Schema::Result::Suggestion at /koha/dev/dcook/git/C4/Suggestions.pm
> line 449


hmm, i might switch this bug to 'discussion'
Comment 35 Katrin Fischer 2015-05-31 12:55:34 UTC
Hi Mason, 

I am switching this back - I believe "In Discussion" is the wrong status here, as this has already been pushed and resolved. The problem described by dcook in comment#32 has been fixed by bug 13329.

But anyway, I don't think tidy up patches like this are needed in the stable releases - ideally they don't change anything for the end user and will have more testing before reaching stable this way.
Comment 36 Mason James 2015-06-02 07:18:44 UTC
(In reply to Katrin Fischer from comment #35)
> Hi Mason, 
> 
> I am switching this back - I believe "In Discussion" is the wrong status
> here, as this has already been pushed and resolved. The problem described by
> dcook in comment#32 has been fixed by bug 13329.

aah, bug 13329... good spotting!

> 
> But anyway, I don't think tidy up patches like this are needed in the stable
> releases - ideally they don't change anything for the end user and will have
> more testing before reaching stable this way.

yes, i agree.. i'll pass on pulling this patch for 3.16.x, thanks :)