Bug 34842 - t/db_dependent/Illrequest/Config.t is failing if the DB has been upgraded
Summary: t/db_dependent/Illrequest/Config.t is failing if the DB has been upgraded
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 32911
Blocks: 34841
  Show dependency treegraph
 
Reported: 2023-09-20 12:19 UTC by Jonathan Druart
Modified: 2023-10-05 18:42 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 34842: Fix Illrequest/Config.t if DB upgraded twice (2.06 KB, patch)
2023-09-22 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34842: Fix Illrequest/Config.t if DB upgraded twice (2.12 KB, patch)
2023-09-25 15:59 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34842: Fix Illrequest/Config.t if DB upgraded twice (2.21 KB, patch)
2023-09-29 07:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-09-20 12:19:09 UTC
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
Comment 1 Jonathan Druart 2023-09-22 12:55:32 UTC
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!
Comment 2 Jonathan Druart 2023-09-22 13:00:24 UTC
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!
Comment 3 Martin Renvoize 2023-09-22 15:25:36 UTC
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.
Comment 4 Pedro Amorim 2023-09-25 15:44:31 UTC
(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.
Comment 5 Pedro Amorim 2023-09-25 15:59:03 UTC
(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.
Comment 6 Pedro Amorim 2023-09-25 15:59:40 UTC
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>
Comment 7 Jonathan Druart 2023-09-26 10:04:38 UTC
(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.
Comment 8 Jonathan Druart 2023-09-26 10:04:47 UTC
(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*
Comment 9 Marcel de Rooy 2023-09-29 06:17:36 UTC
Reading the discussion, this feels like unfinished business.
Comment 10 Jonathan Druart 2023-09-29 07:04:11 UTC
(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...
Comment 11 Marcel de Rooy 2023-09-29 07:24:47 UTC
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>
Comment 12 Marcel de Rooy 2023-09-29 07:25:16 UTC
(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?
Comment 13 Pedro Amorim 2023-09-29 10:54:11 UTC
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.
Comment 14 Jonathan Druart 2023-09-29 11:38:22 UTC
(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';
Comment 15 Tomás Cohen Arazi 2023-10-03 12:55:26 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 16 Fridolin Somers 2023-10-05 18:42:37 UTC
Depends on Bug 32911 not in 23.05.x