There is a lot of failures: # got: 'ILLLIBS' # expected: 'IL' I think we should adjust the atomic update from bug 32911 and set the syspref to "IL" if the config was "ILLLIBS". What do you think Pedro? To recreate: root@kohadevbox:koha$ perl /kohadevbox/misc4dev/run_tests.pl --run-db-upgrade-only kohadev-koha@kohadevbox:koha$ prove t/db_dependent/Illrequest/Config.t
Hum, I don't understand. Is the config still exists or not? Is the syspref only a fallback?? * is($config->partner_code, "IL", "partner_code: Undefined partner_code is undefined."); Then it's not undefined! * Koha/Illrequest/Config.pm 258 partner_code => 'IL', then... 322 $configuration->{partner_code} = C4::Context->preference('ILLPartnerCode') || 'IL'; Please clarify!
Created attachment 156089 [details] [review] Bug 34842: Fix Illrequest/Config.t if DB upgraded twice Not sure this is correct, but I think it is!
Pedro is back on Monday.. I'll poke him to take a look as I'm not entirely close enough myself to comment I'm afraid.
(In reply to Jonathan Druart from comment #1) > Hum, I don't understand. Is the config still exists or not? > Is the syspref only a fallback?? > > * is($config->partner_code, "IL", "partner_code: Undefined partner_code is > undefined."); > > Then it's not undefined! > > * Koha/Illrequest/Config.pm > 258 partner_code => 'IL', > then... > 322 $configuration->{partner_code} = > C4::Context->preference('ILLPartnerCode') || 'IL'; > > Please clarify! Hey! k-t-d has <partner_code>ILLLIBS</partner_code> in /etc/koha/sites/kohadev/koha-conf.xml. To preserve functionality, the upgrade takes whatever already exists in koha-conf.xml and sets that to the new system preference ILLPartnerCode. We don't need to worry about multiple upgrade because of INSERT IGNORE, i.e. it'll only read from the koha-conf.xml and put it in the database the first time. For clean-up, we could consider removing <partner_code> from koha-conf.xml after the upgrade has ran successfully, but it even if it's there - it's not being used anymore. I'm not sure about the change to Config.pm, see bug 21548. The fixes in the tests look good, I missed mocking the preference.
(In reply to Jonathan Druart from comment #0) > I think we should adjust the atomic update from bug 32911 and set the > syspref to "IL" if the config was "ILLLIBS". What do you think Pedro? I think we should make sure that k-t-d now has <partner_code>IL</partner_code> instead of <partner_code>ILLLIBS</partner_code>. The problem is that the updatedatabase will set the preference to ILLLIBS as that is what is coming from the config file, but IL is the category in the database, not ILLLIBS, so it gets botched. Existing installations using this feature will certainly have a existing <partner_code> in their conf file so will not experience this. (In reply to Pedro Amorim from comment #4) > > I'm not sure about the change to Config.pm, see bug 21548. This is fine, I tested and it works as expected. Ran update systempreferences set value = "" where variable ="ILLPartnerCode"; And tried to place a request with partners, it still defaults to 'IL' correctly.
Created attachment 156179 [details] [review] Bug 34842: Fix Illrequest/Config.t if DB upgraded twice Not sure this is correct, but I think it is! Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
(In reply to Pedro Amorim from comment #5) > (In reply to Jonathan Druart from comment #0) > > I think we should adjust the atomic update from bug 32911 and set the > > syspref to "IL" if the config was "ILLLIBS". What do you think Pedro? But the config entry has been removed. We should actually remove it instead of modifying the value. That's why I was suggesting the update the syspref as well.
(In reply to Jonathan Druart from comment #7) > (In reply to Pedro Amorim from comment #5) > > (In reply to Jonathan Druart from comment #0) > > > I think we should adjust the atomic update from bug 32911 and set the > > > syspref to "IL" if the config was "ILLLIBS". What do you think Pedro? > > But the config entry has been removed. We should actually remove it instead > of modifying the value. > > That's why I was suggesting the update the syspref as well. to update*
Reading the discussion, this feels like unfinished business.
(In reply to Marcel de Rooy from comment #9) > Reading the discussion, this feels like unfinished business. I think this can go as it. It's the last missing piece to have Jenkins running the upgrade DB process...
Created attachment 156362 [details] [review] Bug 34842: Fix Illrequest/Config.t if DB upgraded twice Not sure this is correct, but I think it is! Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #10) > (In reply to Marcel de Rooy from comment #9) > > Reading the discussion, this feels like unfinished business. > > I think this can go as it. It's the last missing piece to have Jenkins > running the upgrade DB process... Should we address the potentially wrong contents of the pref at a new report?
I don't know. Bug 32911 is moving the value from koha-conf.xml to the sys pref to preserve functionality, as intended. If the value coming from koha-conf.xml is wrong, it's not its responsibility to address that, at least in my opinion. The problem is that in k-t-d, the value coming from koha-conf.xml is wrong. I don't fully agree with Jonathan's suggestion of setting it to 'IL' if 'ILLLIBS' because it sounds like a hardcoded fix for a problem that shouldn't occur in the first place, i.e. the value in koha-conf.xml should be a valid one (a category that exists), and if not, it needs to be fixed in the sys pref manually, as it would need to be in the koha-conf.xml anyway. Except now that is possible through the sys pref UI whereas before it was required to edit the koha-conf.xml manually. I'm not fully against Jonathan's suggestion it, either, of course.
(In reply to Pedro Amorim from comment #13) > I don't fully agree with Jonathan's suggestion of setting it to 'IL' if > 'ILLLIBS' because it sounds like a hardcoded fix for a problem that > shouldn't occur in the first place Ok, but it's the default value in the module :D Koha/Illrequest/Config.pm: $configuration->{partner_code} = C4::Context->preference('ILLPartnerCode') || 'IL';
Pushed to master for 23.11. Nice work everyone, thanks!
Depends on Bug 32911 not in 23.05.x