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

feat(attachments): add support for creating new attachments #6676

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pbirrer
Copy link

@pbirrer pbirrer commented Nov 21, 2024

These changes enable users to create and insert new attachment files directly from within the editor.

📝 Summary

  • Introduced a new endpoint (Attachment#createAttachment) to create attachment files via POST requests.
  • Added createAttachment method in AttachmentService to handle file creation logic with permission checks.
  • Updated MediaHandler.vue and MediaHandler.provider.js to integrate the new attachment creation functionality in the editor.
  • Enhanced ActionAttachmentUpload.vue to support dynamic attachment creation from file app templates.
  • Extended SessionApi.js with a createAttachment method to interface with the new API endpoint.

🖼️ Screenshots

text-create-attachment

☑️ Todo

  • The attachment cleanup does not wipe the files because it thinks the attachments were removed from the text content.
  • Tests ensure we don't accidentally break creating new files as attachments.
  • Check with design if it makes sense to visually separate the attachments section and the embedding new content section.

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

- Introduced a new endpoint (`Attachment#createAttachment`) to create attachment files via POST requests.
- Added `createAttachment` method in `AttachmentService` to handle file creation logic with permission checks.
- Updated `MediaHandler.vue` and `MediaHandler.provider.js` to integrate the new attachment creation functionality in the editor.
- Enhanced `ActionAttachmentUpload.vue` to support dynamic attachment creation from predefined templates.
- Included a new "Plus" icon for attachment creation actions in `icons.js`.
- Extended `SessionApi.js` with a `createAttachment` method to interface with the new API endpoint.
- Minor refactor and UI enhancements to support seamless attachment creation in the text editor.

These changes enable users to create and insert new attachment files directly from the editor, improving workflow efficiency.

Signed-off-by: Peter Birrer <[email protected]>
@max-nextcloud
Copy link
Collaborator

Dear @pbirrer

At first sight this looks like a good addition to me. I wonder if it becomes very verbose if one has a lot of possible file types that can be created.

@marcoambrosini What do you think from a design perspective?

@max-nextcloud max-nextcloud self-assigned this Dec 3, 2024
@marcoambrosini
Copy link
Member

marcoambrosini commented Dec 4, 2024

I agree that this is a great addition too, should we have too many "New" menu items, we can add a "Create new" submenu in the future.
How many filetypes can be created currently here?

@pbirrer
Copy link
Author

pbirrer commented Dec 4, 2024

Yeah, a 'Create New' submenu could also work pretty well here. The number of file types you can create depends on what’s registered in the files app—it adjusts based on the apps being used in your Nextcloud instance.

Copy link
Contributor

github-actions bot commented Dec 6, 2024

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@max-nextcloud
Copy link
Collaborator

How many filetypes can be created currently here?

This is what the files menu looks like on an instance with collabora and the whiteboard app enabled:
grafik

We're basically talking about the 6 elements from 'new diagram' to 'new whiteboard'. That seems acceptable I'd say.

Either way we can move this into a submenu in a follow up if needed.
So looks like we're all fine with the ui side of things. I'll take a look at the code next.

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Code looks good. Have not tested it though and there's no tests included. So I'll take another look and give it a try then.

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

I tested it locally and noticed that creating the new files is inconsistent with how attachments are inserted thus far.

I'll attach a screencast to illustrate what i mean by that.

If I upload a .md file from my computer it will show as a tiny preview with the filename and size. Same if I insert it from the files already on the Nextcloud.

However if i use the new Create new text file functionality the file will be embedded.

The markdown syntax matches this difference (first file was uploaded, second was created):

![New text file.md](.attachments.2371/New%20text%20file.md)

[http://nextcloud.local/index.php/f/2393](http://nextcloud.local/index.php/f/2393 (preview))

I understand you actually want the embedding, not just an attachment. From my point of view these are two different steps:

  1. Enable the creation of new files as attachments (this PR)
  2. Make attachments behave (more) like previews.

Please change the behavior to match the one of other attachments for now so we can get this in. I'll add a (failing) cypress test case in the attachments test suite that should pass once the created text files behave the same as the uploaded ones.

@max-nextcloud
Copy link
Collaborator

Here's a screencast illustrating what I am referring to.

@rvjr
Copy link

rvjr commented Dec 17, 2024

Hi @max-nextcloud,

you mean you want the link to use the syntax with the global file id, instead of the relative path? Or do you mean that the file shouldn't be previewed initially (which would not make much sense, when inserting new files...)?

@pbirrer
Copy link
Author

pbirrer commented Dec 18, 2024

I understand that the inconsistency between the attachment options is unfortunate. However, as it stands, there’s no way to interact with the default view created by “Upload from computer” or “Insert from file” beyond directly downloading the file or deleting the entry.

attachment

But I’m assuming this isn’t the intended behavior, correct?

@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Dec 18, 2024

Thanks @rvjr and @pbirrer for getting back so swiftly!

Let me try and answer you both at once...

you mean you want the link to use the syntax with the global file id, instead of the relative path? Or do you mean that the file shouldn't be previewed initially (which would not make much sense, when inserting new files...)?

Yes... both was my thinking - but I see how an empty attachment that cannot even be accessed makes little sense.

But I’m assuming this isn’t the intended behavior, correct?

Well - it's not what we want in the long run - but it's the current behavior of attachments.

I'd like to see a way to convert between the three:

  • plain link
  • small preview that opens in new tab / downloads
  • embedded content that can be edited.

But I think this is out of scope for this PR.

I'd be fine with what @pbirrer currently developed if we make sure...

  • the attachment cleanup does not wipe the files because it thinks the attachments were removed from the text content.
  • tests ensure we don't accidentally break it.
  • maybe the attachments section and the embedding new content section are visually separated in the menu.

I'll add these as todo items to the PR description.

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 26.78571% with 41 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@94765c9). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/components/Editor/MediaHandler.vue 0.00% 24 Missing ⚠️
src/services/SessionApi.js 0.00% 8 Missing ⚠️
src/components/Menu/ActionAttachmentUpload.vue 57.14% 6 Missing ⚠️
src/services/SyncService.js 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6676   +/-   ##
=======================================
  Coverage        ?   46.86%           
=======================================
  Files           ?      730           
  Lines           ?    34112           
  Branches        ?     1223           
=======================================
  Hits            ?    15986           
  Misses          ?    17522           
  Partials        ?      604           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@max-nextcloud max-nextcloud removed their assignment Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

5 participants