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

Use FileCreatedFromTemplateEvent to inject the already existing empty templates #1377

Merged
merged 6 commits into from
Jan 14, 2022

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Feb 24, 2021

  • Use the FileCreatedFromTemplateEvent to properly handle creation from the new template handling in Nextcloud 22
    • If a regular file (odt/docx) is used we just take the existing copy
    • If a actual template file (ott/ots/otp/otg) is used, store the fileid and templateid in a table and use it as TemplateSource when opening the file through WOPI for the first time
      • The mapping is cleaned up after first use and regularly in a background job as it is not needed for any other scenarios than the first opening

Scenarios to test:

  • Empty template files provided by richdocuments
    • opendocument
    • ooxml
  • Nextcloud template folder
    • opendocument
    • ooxml
  • Collabora user template folder
    • opendocument
    • ooxml
  • Collabora system template folder
    • opendocument
    • ooxml
  • File creation

@juliusknorr juliusknorr added enhancement New feature or request 2. developing Work in progress labels Mar 4, 2021
@juliusknorr juliusknorr force-pushed the bugfix/noid/use-proper-emptytemplate branch from 260a04c to 4a10f4b Compare October 15, 2021 10:31
@juliusknorr
Copy link
Member Author

/backport to stable4

@juliusknorr juliusknorr added this to the 6.0.0 milestone Nov 11, 2021
@juliusknorr
Copy link
Member Author

/backport to stable23

@juliusknorr juliusknorr force-pushed the bugfix/noid/use-proper-emptytemplate branch from 4a10f4b to 4dc9d04 Compare December 20, 2021 22:14
@juliusknorr juliusknorr added bug Something isn't working high and removed enhancement New feature or request labels Dec 21, 2021
@juliusknorr juliusknorr force-pushed the bugfix/noid/use-proper-emptytemplate branch from 2744c1b to 84b6a96 Compare December 27, 2021 15:23
@juliusknorr juliusknorr added 3. to review Ready to be reviewed and removed 2. developing Work in progress labels Dec 29, 2021
// Expire template mappings for file creation
$query = $this->db->getQueryBuilder();
$query->delete('richdocuments_template')
->where($query->expr()->lte('timestamp', $query->createNamedParameter(time() - 60, IQueryBuilder::PARAM_INT)));
Copy link
Member Author

Choose a reason for hiding this comment

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

Lets maybe reconsider the cleanup here in case a file is for some reason not opened properly and then retried after 1 hour.

src/view/NewFileMenu.js Outdated Show resolved Hide resolved
src/document.js Outdated Show resolved Hide resolved
… template files for Collabora

Signed-off-by: Julius Härtl <[email protected]>

Cleanup template loading

Signed-off-by: Julius Härtl <[email protected]>

Fix template handling

Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr juliusknorr force-pushed the bugfix/noid/use-proper-emptytemplate branch from 368f4f6 to b15c6be Compare January 14, 2022 07:57
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

All good, tested all creation scenarios with/without using templates (except direct editing ones).
A few minor inline remarks.
Also, it could be useful to have a short high level dev doc, like a brief abstract description of what happens, who calls what, what comes from where for all the use cases as this app is rather complex but not so complicated in the end 😁.

lib/Controller/DocumentAPIController.php Outdated Show resolved Hide resolved
lib/TemplateManager.php Outdated Show resolved Hide resolved
@@ -393,7 +409,7 @@ public function getUserTemplateDir() {
}

// has the user manually set a directory as the default template dir ?
$templateDirPath = $this->config->getUserValue($this->userId, $this->appName, 'templateFolder', false);
$templateDirPath = $this->config->getUserValue($this->userId, Application::APPNAME, 'templateFolder', false);
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking and out of topic 😁 : Might be confusing to call this APPNAME instead of APPID or APP_ID. Don't you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, lets do that after the merge in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yep

@@ -80,17 +83,21 @@ const NewFileMenu = {
OCA.Files.Files.isFileNameValid(filename)
filename = FileList.getUniqueName(filename)

$.post(
Copy link
Member

Choose a reason for hiding this comment

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

Note to self, TODO: get rid of JQuery in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to #541

@juliusknorr
Copy link
Member Author

Also, it could be useful to have a short high level dev doc, like a brief abstract description of what happens, who calls what, what comes from where for all the use cases as this app is rather complex but not so complicated in the end grin.

Yes, fully agree on that. Maybe also some quick diagram drawings might help to explain that a bit better. Something for a documentation ticket then I'd say.

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

🚀

@juliusknorr juliusknorr merged commit 42cfd25 into master Jan 14, 2022
@juliusknorr juliusknorr deleted the bugfix/noid/use-proper-emptytemplate branch January 14, 2022 15:04
@juliusknorr
Copy link
Member Author

/backport to stable23

@backportbot-nextcloud
Copy link

The backport to stable4 failed. Please do this backport manually.

@backportbot-nextcloud

This comment has been minimized.

@juliusknorr
Copy link
Member Author

Backporting (at least partially) to stable4 might be needed but could be tricky (ref #2008)

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 high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants