Bug 12627 - SQLHelper replacement - C4::Suggestions
Summary: SQLHelper replacement - C4::Suggestions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 12445
Blocks: 11385 13005 13007 13329 13731 22907
  Show dependency treegraph
 
Reported: 2014-07-22 12:52 UTC by Yohann Dufour
Modified: 2019-05-14 17:26 UTC (History)
6 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:


Attachments
Bug 12627: SQLHelper replacement - C4::Suggestions (3.16 KB, patch)
2014-07-23 07:36 UTC, Yohann Dufour
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12627: SQLHelper replacement - C4::Suggestions (3.23 KB, patch)
2014-07-25 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12627: SQLHelper replacement - C4::Suggestions (3.34 KB, patch)
2014-07-28 21:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12627: SQLHelper replacement - C4::Suggestions (8.00 KB, patch)
2014-07-29 12:08 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 12627: SQLHelper replacement - C4::Suggestions (8.47 KB, patch)
2014-08-08 13:13 UTC, Yohann Dufour
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12627: SQLHelper replacement - C4::Suggestions (8.66 KB, patch)
2014-08-08 16:22 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 12627: SQLHelper replacement - C4::Suggestions (8.99 KB, patch)
2014-08-17 09:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12627: DBIx::Class is case sensitive for column names (1.81 KB, patch)
2014-09-29 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Remove warnings (1.07 KB, patch)
2014-09-29 13:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Fix date management (3.51 KB, patch)
2014-09-30 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Fix default values (2.69 KB, patch)
2014-09-30 10:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Fix default values (2.65 KB, patch)
2014-09-30 10:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Fix default values (2.88 KB, patch)
2014-09-30 10:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Bug 12627: Fix default values - TT changes (7.93 KB, patch)
2014-09-30 10:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Fix default values (2.99 KB, patch)
2014-09-30 11:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Bug 12627: Fix default values - TT changes (8.74 KB, patch)
2014-09-30 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Fix default values (3.09 KB, patch)
2014-09-30 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12627: Fix default values (6.32 KB, patch)
2014-11-05 12:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 12627: SQLHelper replacement - C4::Suggestions (9.05 KB, patch)
2014-11-05 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12627: DBIx::Class is case sensitive for column names (1.88 KB, patch)
2014-11-05 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12627: Remove warnings (1.14 KB, patch)
2014-11-05 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12627: Fix date management (3.58 KB, patch)
2014-11-05 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12627: Bug 12627: Fix default values - TT changes (8.81 KB, patch)
2014-11-05 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12627: Fix default values (6.48 KB, patch)
2014-11-05 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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 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 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 :)