feat: allow the data folder name to be set server-wide - #632
Conversation
|
Warning Review limit reachedNext included review available in 46 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy the coding objectives in issue [ Full details: Out of Scope Changes checkExplanation The changes are within the linked issue scope. The settings UI additions and delayed-save correction directly support the new server-wide and per-user configuration options. No unrelated code changes are identified. Full details: Docstring CoverageExplanation Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (2 skipped: 2 unsupported.) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61a7cbc4-d691-4b68-9788-5791e77df722
📒 Files selected for processing (1)
lib/Service/AssistantService.php
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Looks good. This would need to be documented at some point or even better, allow to change those values in the admin settings (the default) and user settings (the user value).
| } | ||
| // it does not exist or is not a folder or does not have write permissions: we create one | ||
| $dataFolder = $this->createAssistantDataFolder($userId); | ||
| $dataFolder = $this->createAssistantDataFolder($userId, $defaultFolderName); |
There was a problem hiding this comment.
| $dataFolder = $this->createAssistantDataFolder($userId, $defaultFolderName); | |
| $dataFolder = $this->createAssistantDataFolder($userId, $dataFolderName); |
The default one is used only if the user didn't define a value for data_folder. With the current line, only the default value is used and the user value is never used.
|
Thanks both — pushed two commits. @coderabbitai was right, and it was the same class of bug the PR set out to fix, just one line further down. @julien-nc — added both settings rather than only documenting it, since that was the option you preferred:
Both use the existing config endpoints, so no new routes. I moved the app-config read to Two things I did not do, happy to add either if you want them:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/Service/AssistantService.php (1)
527-536: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate folder settings before using them as paths.
Folder::getFullPath()normalizes.to the current user folder, sodata_folder='.'can makegetAssistantDataFolder()return$userFolder. A value such asfoo/barcan select a nested node or fail when its parent does not exist...is rejected only duringnewFolder(), afternodeExists()has swallowed the path error. Validate both settings as single folder names before these calls, including values written throughocc, and add coverage for these inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 40c439ec-206b-4b19-8142-87a263613de4
📒 Files selected for processing (5)
lib/Service/AssistantService.phplib/Settings/Admin.phplib/Settings/Personal.phpsrc/components/AdminSettings.vuesrc/components/PersonalSettings.vue
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline lib/Service/AssistantService.php --items all --match 'AssistantService|createAssistantDataFolder' --view expanded
printf '\n--- relevant references ---\n'
rg -n -C 12 '\bcreateAssistantDataFolder\s*\(|\$dataFolderName\b|\$defaultFolderName\b' lib/Service/AssistantService.phpLength of output: 4915
This fixes the reported user-setting overwrite case. Thanks. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
Both fixed in 055bb42. The The delayed save — also right, and worth spelling out because it is not specific to this PR: That one is pre-existing behaviour in the admin settings rather than something this PR introduced — I fixed it here because the new field is subject to it and the fix is four lines. @julien-nc, if you would rather have it as a separate PR, say so and I will split it out. Checks are clean: |
The folder the assistant creates in a user's files is named from a class constant, so a server whose users do not work in English has no supported way to hand new accounts a localized name. The per-user setting that already exists does not fill the gap either: when the configured folder does not exist yet, createAssistantDataFolder() falls back to the constant and the caller then writes that name back over the preference, so setting it for a user who has not used the assistant silently reverts. Resolve the default from an app config value, keeping the constant as the last resort, and pass the resolved name down to the creation path so it is used for the folder that gets created. This mirrors how Talk resolves its attachment folder. Nothing changes for a server that sets no app config, and a user who already has a folder keeps it. Signed-off-by: Baki Burak Öğün <burak@burakogun.com>
The creation path was passed the server default rather than the name resolved for the user, so a user whose configured folder had been deleted got a folder named after the administrator's default, and their own setting was then overwritten with it. Reported by CodeRabbit on the pull request. Signed-off-by: Baki Burak Öğün <burak@burakogun.com>
Add the server-wide default to the admin settings and the per-user override to the personal settings, so neither has to be set with occ. The personal field shows the administrator's default as its placeholder and an empty value falls back to it, which keeps the existing precedence visible in the interface. Signed-off-by: Baki Burak Öğün <burak@burakogun.com>
Two issues CodeRabbit raised on the pull request. PHP reads "0" as false, so `?: Application::ASSISTANT_DATA_FOLDER_NAME` silently discarded a folder actually named "0" in the service and in both settings pages. Check for an empty string explicitly instead. `delay()` keeps a single module-level timer, so editing a second field within the delay cancelled the first callback before it had queued its value, losing that edit. Queue the value when it changes rather than when the timer fires, and hand the pending set to saveOptions as a snapshot. This is pre-existing behaviour in the admin settings; it is fixed here because the new field is subject to it. Signed-off-by: Baki Burak Öğün <burak@burakogun.com>
055bb42 to
d336728
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 452ac705-0939-4bfb-ad62-2722144bd9c9
📒 Files selected for processing (5)
lib/Service/AssistantService.phplib/Settings/Admin.phplib/Settings/Personal.phpsrc/components/AdminSettings.vuesrc/components/PersonalSettings.vue
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
A user who had used the assistant before an administrator set a default may have a folder under the built-in name without a stored data_folder preference. Applying the server default to them started a second folder and left their existing output behind it. Fall back to the built-in name when that folder already exists, so the server default reaches accounts that have nothing to strand. This is also what the admin settings text promises. Signed-off-by: Baki Burak Öğün <burak@burakogun.com>
|
Fixed in d3dcaa7 — and this one was worth catching, because the code disagreed with what the admin settings text promises. A user who had used the assistant before an administrator set a default can have a folder under the built-in name with no stored Now an empty per-user value falls back to the built-in name when that folder already exists, and to the server default only when there is nothing to strand: $dataFolderName = $this->config->getUserValue($userId, Application::APP_ID, 'data_folder', '');
if ($dataFolderName === '') {
$dataFolderName = $userFolder->nodeExists(Application::ASSISTANT_DATA_FOLDER_NAME)
? Application::ASSISTANT_DATA_FOLDER_NAME
: $defaultFolderName;
}I did not add a migration that renames existing folders. Moving a user's files because an administrator changed a setting seems like more than this PR should do on its own, and the existing behaviour of leaving them in place is at least predictable. Happy to add one if you would rather the rename be offered explicitly. DCO was also failing on my earlier commits — sorry, that was mine, the sign-off was missing on three of them. Rebased with
|
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Thanks a lot.
if you would rather have it as a separate PR, say so and I will split it out.
It's fine here, the change is kind of related.
Tested, works great.
2 participants
Fixes #631
Summary
Application::ASSISTANT_DATA_FOLDER_NAMEis a class constant, so a server whose users do not work in English has no supported way to give new accounts a localized name for the folder the assistant creates in their files.The per-user
data_foldersetting does not fill the gap. When the configured folder does not exist yet,getAssistantDataFolder()falls through tocreateAssistantDataFolder(), which reads the constant rather than the configured name, and the caller then writes the created name back over the preference:So
occ user:setting <uid> assistant data_folder Somethingon a user who has not used the assistant yet createsAssistantand silently reverts the setting.What this changes
Resolve the default from an app config value, keeping the constant as the last resort, and pass the resolved name down so the creation path uses it:
createAssistantDataFolder()now takes the base name instead of reading the constant, keeping its existing " 1", " 2" collision suffixes.An administrator can then run:
This is the same shape Talk uses for its attachment folder (
default_attachment_folderinspreed/lib/Config.php): an app-level default behind the per-user value.Behaviour
Testing
Running on Nextcloud 34 with the equivalent change against the bundled version, on a server with several thousand accounts. New accounts get the configured folder name, existing accounts keep theirs, and unsetting the app config restores the previous behaviour.
php-cs-fixerandpsalmare clean for the changed file.