UIREQ-1346: Migrate config values from mod-configuration and mod-settings

[Dmitriy-Litvinenko]

Purpose

Migrate config values from mod-configuration

Refs

https://issues.folio.org/browse/UIREQ-1346

[github-actions]

Jest Unit Test Results

    1 files  ±0     66 suites  ±0   1m 38s ⏱️ -1s
  849 tests +4    849 ✅ +4  0 💤 ±0  0 ❌ ±0 
1 211 runs  +1  1 211 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit efd4007. ± Comparison against base commit 678f141.

This pull request removes 3 and adds 7 tests. Note that renamed tests count towards both.

RequestsRoute getPrintHoldRequestsEnabled should return false when printHoldRequestsEnabled is false ‑ RequestsRoute getPrintHoldRequestsEnabled should return false when printHoldRequestsEnabled is false
RequestsRoute getPrintHoldRequestsEnabled should return false when value absent ‑ RequestsRoute getPrintHoldRequestsEnabled should return false when value absent
RequestsRoute getPrintHoldRequestsEnabled should return true when printHoldRequestsEnabled is true ‑ RequestsRoute getPrintHoldRequestsEnabled should return true when printHoldRequestsEnabled is true

utils getPrintHoldRequestsEnabled should return false if input is undefined ‑ utils getPrintHoldRequestsEnabled should return false if input is undefined
utils getPrintHoldRequestsEnabled should return false if printHoldRequestsEnabled is false ‑ utils getPrintHoldRequestsEnabled should return false if printHoldRequestsEnabled is false
utils getPrintHoldRequestsEnabled should return false if printHoldRequestsEnabled is missing in value ‑ utils getPrintHoldRequestsEnabled should return false if printHoldRequestsEnabled is missing in value
utils getPrintHoldRequestsEnabled should return false if records array is empty ‑ utils getPrintHoldRequestsEnabled should return false if records array is empty
utils getPrintHoldRequestsEnabled should return false if records is missing ‑ utils getPrintHoldRequestsEnabled should return false if records is missing
utils getPrintHoldRequestsEnabled should return false if value is missing in records[0] ‑ utils getPrintHoldRequestsEnabled should return false if value is missing in records[0]
utils getPrintHoldRequestsEnabled should return true if printHoldRequestsEnabled is true ‑ utils getPrintHoldRequestsEnabled should return true if printHoldRequestsEnabled is true

♻️ This comment has been updated with latest results.

[sonarqubecloud]

Quality Gate failed

Failed conditions
34.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[zburke]

It is now more than a year after PR #1224 implemented the "temporary" feature flag that moved non-ECS functionality into a directory misleadingly named deprecated and there has been no effort to address this abomination despite repeated complaints. This requires extra effort from developers who must implement things twice, it loads the codebase with tech debt, and it requires devs to ignore Sonar's advice about duplicated code.

It was a bad idea then and remains one now. When will this be fixed?

[zburke]

What of these permissions:

ui-requests/package.json

Lines 126 to 128 in 678f141

	"mod-settings.global.read.circulation",
	"mod-settings.entries.collection.get",
	"mod-settings.entries.item.get",

Can these lines be removed too?

[zburke]

Maybe "Migrate from mod-configuration and mod-settings to mod-circulation" or "Consolidate config values within mod-circulation"? What you are migrating to (i.e. the status the code will be in once this PR merges) is more important than what you are migrating from (i.e. the status of the code that will be gone once this PR merges).

[Dmytro-Melnyshyn]

The TLR setting should be stored in mod-settings, not mod-circulation. TLR is not a local setting, it is used in many modules. When migrating from mod-configuration to mod-settings, global settings should be stored in mod-settings, and local settings should be stored in the module-specific settings.

[zburke]

This is an interesting point, @Dmytro-Melnyshyn. It increasingly feels like "global" permissions -- tenant-locale is another example -- fell into a gap in the analysis when deciding to abandon mod-configuration.

Do you think even mod-settings provides a good way to handle this? If we moved these values into mod-settings, we'd end up with a permission like, say, mod-settings.global.read.ui-requests.tlr.manage. A consequence is that other entities then become dependent on ui-requests (which instantiates this permission), which we may not want if "other entities" could mean "other backend modules", because it seems upside-down for BE things to depend on FE things.

CC: @okolawole-ebsco

[Dmytro-Melnyshyn]

@zburke If this setting is also used on the BE, it should be created by the BE, not the UI, to avoid tenant-locale issues like we had when UI-created settings were used by the BE. mod-tlr can be used instead of ui-requests, but I don't think it's needed at all if the setting is implemented on the BE side.

[Dmitriy-Litvinenko]

@alexanderkurash Could you please take a look on comment above?

[alexanderkurash]

@Dmytro-Melnyshyn @zburke
We consider this setting to be specific to circulation domain only, which makes it quite local in scope of FOLIO. Yes, it is used by multiple modules, but only those that are responsible for circulation (more precisely - requests). For example, it's very unlikely that this setting is ever going to be needed by something like inventory, data import, acquisition etc. That was our justification for moving it to mod-circulation-storage's settings.
To the question of which module should create this setting - mod-circulation-storage is responsible for the migration of the current value from mod-configuration to it's own database. Here's the migration SQL script: https://github.com/folio-org/mod-circulation-storage/pull/539/files#diff-074397d7eda53d5ae81dec338c0ec750555d85316e5bd2be266399e8dce05362. We're not merging this PR yet because BE and FE should be merged together.
From our perspective, the purpose of the FE work is to stop using mod-configuration for CRUD operations with this setting and switch to mod-circulation-storage's API.
cc @Dmitriy-Litvinenko

[Dmytro-Melnyshyn]

@alexanderkurash The TLR setting is primarily relevant to the circulation/requests domain and keeping it in mod-circulation-storage makes sense. However, ui-inventory already depends on this setting today. Moving it from mod-configuration to a module-specific settings store will be a breaking change for at least one consumer. Please either update dependent modules too, or announce the breaking change on Slack and coordinate a short migration plan.

[Dmitriy-Litvinenko]

Hello For ui-inventory we already create pull folio-org/ui-inventory#2949 which is associated with these changes