Fix critical error if storage path is missing by adding a new config option

[Zharktas]

Fixes #8966

Proposed fixes:

Adds a new config option ckan.uploads_enabled which only affects uploads_enabled helper. If uploader instances call get_storage_path they still get the error.

Does not affect translations or webassets directories as they use the config directly as seen on below:

https://github.com/ckan/ckan/blob/f2cc28e8b71608d70361cf2488ea536447e102a5/ckan/lib/i18n.py#L79

https://github.com/ckan/ckan/blob/f2cc28e8b71608d70361cf2488ea536447e102a5/ckan/lib/webassets_tools.py#L209-L212

Features:

• [ ] includes tests covering changes
• [x] includes updated documentation
• [ ] includes user-visible changes
• [ ] includes API changes
• [x] includes bugfix for possible backport

Please [X] all the boxes above that apply

[wardi]

If we add an ckan.uploads_enabled configuration option I'd like it to be optional for backwards compatibility.

h.uploads_enabled() could then:

• return value of ckan.uploads_enabled if it's set
• otherwise return True if any IUploader plugin is loaded or if the storage path is set (without logging an error)

When @smotornyuk 's replacement interface for IUploader is introduced we can probably get rid of h.uploads_enabled() and switch to h.check_access() calls on the new upload action.

[smotornyuk]

Yes, I experimented with different combinations of default values for configuration and see that @Zharktas 's solution is the best we can get in the current state.

Even after implementing the upload API for different upload types, this new option may be maintained as a global switch for uploads. For example, if you'd like to disable uploads for a day due to maintenance, but keep all existing files available, you can simply turn this option off. By that time, we'll change the declaration of the option to bool and set the default value to true, eliminating this none value that confuses me in boolean flags.

[ckanbot]

Backport failed for dev-v2.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dev-v2.11
git worktree add -d .worktree/backport-8977-to-dev-v2.11 origin/dev-v2.11
cd .worktree/backport-8977-to-dev-v2.11
git switch --create backport-8977-to-dev-v2.11
git cherry-pick -x 4e14587a7048ef4a08641742741418fc0eb93d19 66c98fa5d9b1c713a98ed7c6596929f9b604b70a f042f65a071f139c5d8e7c19367bdbe3ab6dbe3d c3ab34c5624b9dfbe8ec85a09023c1566574a046 1bc29d0599e73dca8847f172e59e1ea026a307a6 3180f8762550b6538e2d135a1c853ceb6c3b27a5 156843edf3081b2446751f1576030cc7d0deea50 ad3d4d257f34fa53b5015ec99eac00b08c7abe1b

[ckanbot]

Backport failed for dev-v2.10, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dev-v2.10
git worktree add -d .worktree/backport-8977-to-dev-v2.10 origin/dev-v2.10
cd .worktree/backport-8977-to-dev-v2.10
git switch --create backport-8977-to-dev-v2.10
git cherry-pick -x 4e14587a7048ef4a08641742741418fc0eb93d19 66c98fa5d9b1c713a98ed7c6596929f9b604b70a f042f65a071f139c5d8e7c19367bdbe3ab6dbe3d c3ab34c5624b9dfbe8ec85a09023c1566574a046 1bc29d0599e73dca8847f172e59e1ea026a307a6 3180f8762550b6538e2d135a1c853ceb6c3b27a5 156843edf3081b2446751f1576030cc7d0deea50 ad3d4d257f34fa53b5015ec99eac00b08c7abe1b

[amercader]

@Zharktas @smotornyuk I'm reviewing this while updating the CHANGELOG for CKAN 2.11 and 2.10, two questions:

Added a new config option :ref:ckan.uploads_enabled to fix critical error logs about missing storage_path.

If not enabled, you either must have IUploader plugin configured or have defined storage_path to have uploads in the UI.

Shouldn't this be "if enabled" on the second sentence?

Second question is, do existing sites that define ckan.storage_path need to additionally add ckan.uploads_enabled to their configs to have functioning uploads? I assume the answer is no otherwise that would be a major breaking change, so in that case this config documentation is misleading as ckan.uploads_enabled is not required right?

If set to some directory, also set :ref:ckan.uploads_enabled to true to enable uploads in the UI.

If I understood the change correctly what do you think about this changelog entry in the Minor Changes section (not the bugfixes):

Added a new config option :ref:ckan.uploads_enabled, which you can set to avoid a critical error logs about missing storage_path during startup.

If set to True, you either must have set :ref:ckan.storage_path or have an IUploader plugin configured to have uploads enabled in the UI.

[wardi]

Do we have a problem with the JS translations if a storage path isn't configured? The current code will use a temporary directory if there's no storage path but it will also no longer auto-regenerate the translations on every cli invocation so if the temp directory was removed we'll be serving ckan with no translation files, right?

We could document that if you don't have a storage path configured you'll need to generate the js translations each time before starting the server.

There's a similar problem with webassets, but webassets requires another configuration option ckan.webassets.path if ckan.storage_path is not defined.