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.
Created attachment 29963 [details] [review] Bug 12627: SQLHelper replacement - C4::Suggestions With this patch, the subroutines NewSuggestion and ModSuggestion uses DBIx::Class instead of C4::SQLHelper. Moreover, the tests has been adapted to the values returned by DBIx::Class. Test plan: 1) Apply the patch 12445 in order to get the last unit tests for Suggestions 2) Apply the patch 3) Execute the unit tests by launching : prove t/db_dependent/Suggestions.t 4) 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
Created attachment 30095 [details] [review] [SIGNED-OFF] Bug 12627: SQLHelper replacement - C4::Suggestions With this patch, the subroutines NewSuggestion and ModSuggestion uses DBIx::Class instead of C4::SQLHelper. Moreover, the tests has been adapted to the values returned by DBIx::Class. Test plan: 1) Apply the patch 12445 in order to get the last unit tests for Suggestions 2) Apply the patch 3) Execute the unit tests by launching : prove t/db_dependent/Suggestions.t 4) 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 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 30237 [details] [review] [PASSED QA] Bug 12627: SQLHelper replacement - C4::Suggestions With this patch, the subroutines NewSuggestion and ModSuggestion uses DBIx::Class instead of C4::SQLHelper. Moreover, the tests has been adapted to the values returned by DBIx::Class. Test plan: 1) Apply the patch 12445 in order to get the last unit tests for Suggestions 2) Apply the patch 3) Execute the unit tests by launching : prove t/db_dependent/Suggestions.t 4) 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 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Tests pass, no problems found. Patch applies on top of bug 12445.
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
Created attachment 30269 [details] [review] 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 12445 in order to get the last unit tests for Suggestions. 2) Apply the patch. 3) Execute the unit tests by launching : prove t/db_dependent/Suggestions.t 4) 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 5) Log in the intranet, create a suggestion and verify the created suggestion. 6) Edit a suggestion from the intranet and verify the suggestion is correctly modified. 7) Log in the OPAC and verify you can add a suggestion.
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
Created attachment 30624 [details] [review] 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.
Created attachment 30635 [details] [review] [SIGNED-OFF] 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.
Created attachment 30892 [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.
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;
Created attachment 31916 [details] [review] Bug 12627: DBIx::Class is case sensitive for column names STATUS should be "STATUS", not "status".
Created attachment 31917 [details] [review] Bug 12627: Remove warnings suggestion.pl: keys on reference is experimental
Created attachment 31937 [details] [review] 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
Created attachment 31939 [details] [review] 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.
Created attachment 31940 [details] [review] 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.
Created attachment 31941 [details] [review] 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.
Created attachment 31942 [details] [review] Bug 12627: Bug 12627: Fix default values - TT changes
Last 5 patches need to be QAed.
Created attachment 31944 [details] [review] 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.
Created attachment 31948 [details] [review] Bug 12627: Bug 12627: Fix default values - TT changes
Created attachment 31953 [details] [review] 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.
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.
Created attachment 33244 [details] [review] 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.
(In reply to Katrin Fischer from comment #22) > Sadly, I found a bug: Fortunately, you found it! :) It should be fixed now.
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>
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>
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>
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>
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>
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.
Patches pushed to master. Thanks Yohann and Jonathan!
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)
Possible selection for 3.16.x
(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'
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.
(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 :)