-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(settings): add admin & user setting iframe #4373
base: main
Are you sure you want to change the base?
feat(settings): add admin & user setting iframe #4373
Conversation
62c98ce
to
104c780
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some early feedback, general approach seems good 👍
$wopi = $this->tokenManager->generateWopiToken($fileId, null, $adminUserId); | ||
|
||
$coolBaseUrl = $this->appConfig->getCollaboraUrlPublic(); | ||
$adminSettingsWopiSrc = $coolBaseUrl . '/browser/admin-settings.html?'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this would be an URL that can be obtained from the discovery endpoint of Collabora, could be a separate app element for settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to my TODO, will fix it before merging PR.
905e1e2
to
cc91ef8
Compare
Related serverside PR : nextcloud/server#50145 |
@codewithvk @juliusknorr thanks. Probably worth to create a separated PR for the vue conversion, right? |
c042023
to
5e11402
Compare
This comment was marked as spam.
This comment was marked as spam.
In order to resolve the failing tests, you should do the following:
|
5e11402
to
bf5a8bf
Compare
@elzody We could have one psalm error because we need to merge PR :nextcloud/server#50145 before.. I fixed other errors...Also could you please run CI - as I don't have access to run CI. |
src/components/PersonalSettings.vue
Outdated
@@ -34,6 +34,17 @@ | |||
</em> | |||
</p> | |||
|
|||
<!-- user settings iframe --> | |||
<div id="user-cool-frame-section" class="section"> | |||
<h2>{{ t('richdocuments', 'Collabora User Settings') }}</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should call this differently as we want to remove the duplicate word "settings" from all seections we have (ref #4351)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have discussed with Vivek that we should also remove Collabora
since it's part of Office settings section.
If we also remove settings (even thought I do see "Personal settings" in the #4351 you have referenced) then we either replace it with something such as "preferences" or - and I think better - we simply remove that h2 completely and let the headings in the iframe being h2 (so, autotext and workbook will become upper level which probably makes more sense)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedropintosilva Thanks for adding a screenshot :)
@codewithvk Can you also add some screenshots of how this looks in the settings page for review of @marcoambrosini as designer? |
@juliusknorr @elzody I'm not able to figure out why wopi integration for db test broken, any idea? |
Modified bit of UI of iframe |
Awesome, much better, thanks! I think We can still make it more clear in terms of the elements and their relation. What I mean is that right now the "Upload AutoText/Wordbook" buttons seem that they are referring to the list of files (which is not the case, those files were already uploaded). Best to clarify the main action by renaming it to "Add new AutoText" / "Add new Wordbook" but that seems a bit too long (specially when considering translations) -> so, what about "New AutoText" and "New Wordbook". I have noticed that in https://nextcloud-vue-components.netlify.app/#/Components/App%20containers/NcAppNavigation?id=ncappnavigationnewitem that sometimes they use the additional plus icon as a prefix (Maybe we could use it? @jancborchardt , @marcoambrosini ) |
lib/Service/SettingsService.php
Outdated
try { | ||
$categories = []; | ||
$folder = $this->appData->getFolder($type); | ||
$directories = $folder->getFullDirectoryListing(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed that the server change was also required, but we cannot get that in for 31 anymore due to API freeze.
However you can directly access it by injecting IRootFolder and then fetching the path directly from it
$instanceId = $this->config->getSystemValue('instanceid', null);
if ($instanceId === null) {
}
return $this->rootFolder->get('appdata_' . $instanceId . '/richdocuments/your-path');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliusknorr Is it safe to access the rootFolder from a security perspective?
May be for now we can do it as it seems we don't have other options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is fine, the server API that you have would do similar calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliusknorr I have added this commit - could you please checkout and restart CI ? 3e6964b
3e6964b
to
5302259
Compare
$uri = $nextcloudUrl . '/index.php/apps/richdocuments/wopi/settings' . '?type=' . $type . '&access_token=' . $this->generateSettingToken($userId) . '&fileId=' . '-1'; | ||
return [ | ||
'uri' => $uri, | ||
'stamp' => time() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliusknorr As of now temporary I'm sending stamp as time but we need to pass here Category folders etag/stamp - May be we can create method inside settingsService but still we need fetch stamp from Folder/SimpleFolder or even Node. I try to test with Node etag it seems not updating is something updated in child.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a simple approach you could try the using getMTime which is probably enough, but I'm not entirely sure this propagates as well
If that doesn't we may need to manually update the etag whenever a file is added/updated/removed through richdocuments
The following code should work for that then:
$folder->getStorage()->getCache()->update($folder->getId(), [
'etag' => uniqid(),
]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for future reference, the reason the etag is not propagating directly is because we skip it in https://github.com/nextcloud/server/blob/41c53648ad18e3a94b6ba18ad0e6c7c09a520bd9/lib/private/Files/Storage/Common.php#L346 as in other cases we do not make use of that for anything in appdata or we have a separate mountpoint like for collectives where we can apply our own storage wrapper nextcloud/collectives@b54e6be but that won't work for the case here.
9e7c050
to
5124cff
Compare
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
…directories Signed-off-by: codewithvk <[email protected]>
…y of user and system settings. Note: We will delete this commit/code after the entire prototype is ready. Reviewers can ignore this commit during the review process. Signed-off-by: codewithvk <[email protected]>
TODO: We should try to centralize the path everywhere so that a change in one place updates it everywhere! Signed-off-by: codewithvk <[email protected]>
… it to system-settings dir Signed-off-by: codewithvk <[email protected]>
…equest Signed-off-by: codewithvk <[email protected]>
- Implement dynamic routing for settings files, enabling URLs structured as /settings/{type}/{category}/{filename}. - Support various setting types (e.g. userconfigs, sharedconfigs) and categories (e.g. autotext, wordbook) so that multiple files can be stored for each category. - Ensure proper URL parsing and directory handling for uploading and retrieving files via the WOPI interface. Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
…om iframe Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
Signed-off-by: codewithvk <[email protected]>
e974c4a
to
e7e3f09
Compare
Summary
TODO
Checklist