Skip to content
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

preset: fix: adding/removing global template not reflected in sidebar #4564

Merged
merged 1 commit into from
Mar 21, 2025

Conversation

Rash419
Copy link
Collaborator

@Rash419 Rash419 commented Mar 11, 2025

  • this patch appends the presentation template folder etag to shared setting etag so that COOL can detect changes
  • this patch also update the etag of global templates folder when template is added or removed by admin
  • Target version: main

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required

@@ -996,7 +996,7 @@ private function generateSettingToken(string $userId): string {
private function generateSettings(string $userId, string $type): array {
$nextcloudUrl = $this->appConfig->getNextcloudUrl() ?: trim($this->urlGenerator->getAbsoluteURL(''), '/');
$uri = $nextcloudUrl . '/index.php/apps/richdocuments/wopi/settings' . '?type=' . $type . '&access_token=' . $this->generateSettingToken($userId) . '&fileId=' . '-1';
$etag = $this->settingsService->getFolderEtag($type);
$etag = $this->settingsService->getFolderEtag($type) . $this->settingsService->getPresentationFolderEtag($type, $userId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a condition for guest users: if the user is a guest, then do not generate getPresentationFolderEtag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think we should change the method to also allow null for the userId, and not set it to a string in

https://github.com/Rash419/richdocuments/blame/template-preset/lib/Controller/WopiController.php#L142

Using user ids that are non existing for non-logged in users may lead to more problems on other parts.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliusknorr make sense

@juliusknorr juliusknorr added bug Something isn't working 3. to review Ready to be reviewed labels Mar 12, 2025
@juliusknorr
Copy link
Member

/backport to stable31

@juliusknorr
Copy link
Member

juliusknorr commented Mar 12, 2025

@Rash419 I've invited you as a contributor to the repo so CI will run automatically next time you push and you can push a branch directly which makes collaboration a bit easier :)

@Rash419 Rash419 force-pushed the template-preset branch 4 times, most recently from 86f2e25 to 257ffb3 Compare March 21, 2025 07:39
- this patch appends the presentation template folder etag to shared
setting etag so that COOL can detect changes
- this patch also update the etag of global templates folder when
template is added or removed by admin

Signed-off-by: Rashesh Padia <[email protected]>
@Rash419 Rash419 requested a review from codewithvk March 21, 2025 12:36
Copy link
Collaborator

@codewithvk codewithvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @Rash419 :)

I'll try to create a separate PR to resolve Julius's comment. https://github.com/nextcloud/richdocuments/pull/4564/files#r1988926909

@juliusknorr juliusknorr merged commit eed56f9 into nextcloud:main Mar 21, 2025
101 of 117 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to be reviewed bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants