|
Description
Jonathan Druart
2015-12-30 17:05:43 UTC
Created attachment 46095 [details] [review] Bug 15446: Add tests Created attachment 46096 [details] [review] Bug 15446: Rename Koha::Object[s]->type with _type In a few case (at least systempreferences and export_format (csv profiles), the type method of Koha::Object and Koha::Objects can be in conflict with the column names. Indeed systempreferences.type exists and so the method will return 'Systempreference' (the name of the module) instead of the value of the row in DB. I have found at least 1 place where it can cause issue: In C4::Context->set_preference: 601 my $syspref = Koha::Config::SysPrefs->find( $var ); 602 my $type = $syspref ? $syspref->type() : undef; 603 604 $value = 0 if ( $type && $type eq 'YesNo' && $value eq '' ); type will always be 'Systempreference' and the YesNo pref will be set to an empty string '' instead of 0. I am not sure about the consequences of this, but it is preferable to fix it ASAP. To reproduce: 0/ Do not apply this patch 1/ Edit a YesNo prefs, AutoEmailOpacUser for instance 2/ Set it to "Don't sent" 3/ Check the value in DB, it should be set to an empty string, instead of 0 4/ Apply this patch and try again. Now the value should be 0 Created attachment 46097 [details] [review] Bug 15446: Update systempreferences rows where type=YesNo and value='' Created attachment 46104 [details] [review] Bug 15446: Add tests Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 46105 [details] [review] Bug 15446: Rename Koha::Object[s]->type with _type In a few case (at least systempreferences and export_format (csv profiles), the type method of Koha::Object and Koha::Objects can be in conflict with the column names. Indeed systempreferences.type exists and so the method will return 'Systempreference' (the name of the module) instead of the value of the row in DB. I have found at least 1 place where it can cause issue: In C4::Context->set_preference: 601 my $syspref = Koha::Config::SysPrefs->find( $var ); 602 my $type = $syspref ? $syspref->type() : undef; 603 604 $value = 0 if ( $type && $type eq 'YesNo' && $value eq '' ); type will always be 'Systempreference' and the YesNo pref will be set to an empty string '' instead of 0. I am not sure about the consequences of this, but it is preferable to fix it ASAP. To reproduce: 0/ Do not apply this patch 1/ Edit a YesNo prefs, AutoEmailOpacUser for instance 2/ Set it to "Don't sent" 3/ Check the value in DB, it should be set to an empty string, instead of 0 4/ Apply this patch and try again. Now the value should be 0 Followed test plan, value is now 0 as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 46106 [details] [review] Bug 15446: Update systempreferences rows where type=YesNo and value='' Signed-off-by: Marc Véron <veron@veron.ch> Oh, I was to fast with the 3rd patch, the update statement has a typo: UPDATE systempreferences SET value="0" where type="YesNo" and value=";" Created attachment 46107 [details] [review] Bug 15446: Update systempreferences rows where type=YesNo and value='' Signed-off-by: Marc Véron <veron@veron.ch> Amended to fix typo in sql statement. ...and tested, update works fine now. Oh, I get a software error with checkong out. Not sure if it is related to this bug, but setting back to NSO. Will go to a previous VM to test again. (In reply to Marc Véron from comment #10) > Oh, I get a software error with checkong out. Not sure if it is related to > this bug, but setting back to NSO. > > Will go to a previous VM to test again. See third patch on bug 15344 for a fix. Switching back to signed off again, the issue from comment #10 was not related to this bug. Created attachment 46268 [details] [review] Bug 15446: (follow-up) Rename Koha::Object[s]->type with _type Some occurrences have been pushed in the meantime. Created attachment 46424 [details] [review] Bug 15446: Add tests Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 46425 [details] [review] Bug 15446: Rename Koha::Object[s]->type with _type In a few case (at least systempreferences and export_format (csv profiles), the type method of Koha::Object and Koha::Objects can be in conflict with the column names. Indeed systempreferences.type exists and so the method will return 'Systempreference' (the name of the module) instead of the value of the row in DB. I have found at least 1 place where it can cause issue: In C4::Context->set_preference: 601 my $syspref = Koha::Config::SysPrefs->find( $var ); 602 my $type = $syspref ? $syspref->type() : undef; 603 604 $value = 0 if ( $type && $type eq 'YesNo' && $value eq '' ); type will always be 'Systempreference' and the YesNo pref will be set to an empty string '' instead of 0. I am not sure about the consequences of this, but it is preferable to fix it ASAP. To reproduce: 0/ Do not apply this patch 1/ Edit a YesNo prefs, AutoEmailOpacUser for instance 2/ Set it to "Don't sent" 3/ Check the value in DB, it should be set to an empty string, instead of 0 4/ Apply this patch and try again. Now the value should be 0 Followed test plan, value is now 0 as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 46426 [details] [review] Bug 15446: Update systempreferences rows where type=YesNo and value='' Signed-off-by: Marc Véron <veron@veron.ch> Amended to fix typo in sql statement. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 46427 [details] [review] Bug 15446: (follow-up) Rename Koha::Object[s]->type with _type Some occurrences have been pushed in the meantime. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I have added a new rule to the qa tools:
commit 75bf0eead3fab5b1918f4d2f2fc6c1bc66f2dc76
type subroutine should be _type
See bug 15446
Created attachment 48005 [details] [review] Bug 15446: Add tests Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 48006 [details] [review] Bug 15446: Rename Koha::Object[s]->type with _type In a few case (at least systempreferences and export_format (csv profiles), the type method of Koha::Object and Koha::Objects can be in conflict with the column names. Indeed systempreferences.type exists and so the method will return 'Systempreference' (the name of the module) instead of the value of the row in DB. I have found at least 1 place where it can cause issue: In C4::Context->set_preference: 601 my $syspref = Koha::Config::SysPrefs->find( $var ); 602 my $type = $syspref ? $syspref->type() : undef; 603 604 $value = 0 if ( $type && $type eq 'YesNo' && $value eq '' ); type will always be 'Systempreference' and the YesNo pref will be set to an empty string '' instead of 0. I am not sure about the consequences of this, but it is preferable to fix it ASAP. To reproduce: 0/ Do not apply this patch 1/ Edit a YesNo prefs, AutoEmailOpacUser for instance 2/ Set it to "Don't sent" 3/ Check the value in DB, it should be set to an empty string, instead of 0 4/ Apply this patch and try again. Now the value should be 0 Followed test plan, value is now 0 as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 48007 [details] [review] Bug 15446: Update systempreferences rows where type=YesNo and value='' Signed-off-by: Marc Véron <veron@veron.ch> Amended to fix typo in sql statement. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 48008 [details] [review] Bug 15446: (follow-up) Rename Koha::Object[s]->type with _type Some occurrences have been pushed in the meantime. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 48019 [details] [review] Bug 15446 [QA Followup] - Update new classes Pushed to master, thanks for your work. Patches pushed to 3.22.x, will be in 3.22.5 Note: 4th and 5th patches are not needed in 3.22.x, so they are not pushed |