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] Add in TinyMCE Areas possibility for the user to upload a local file #277

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

babastienne
Copy link
Member

Demonstration of the feature :

mapentity.webm

The local media will be imported into MEDIA_ROOT folder and automatically added to the text area.

Files are store in the media folder and then into the following path : /tinymce/<generated-uuid>/file.ext. The generated uuid allow to avoid conflicts if multiple files with the same name are uploaded.

Everything seems to be working fine.

By default TinyMCE stored a relative path for the uploaded files. I ensure the URL stored for local files are absolute urls therefore GTR3 and others services can exploit data without having to rebuild URLs and API V2 from Geotrek needs no modification either.

One important element not done in this PR: The uploaded files are stored into media but there is no method to delete them if there not used anymore. I'd like some help to find the best strategy to find and remove unused uploaded file. A command that users will need to run ? An asynchrone task ? How can we determine which files are not used ?

…cal file

The local media will be imported into MEDIA_ROOT folder and automatically added to the text area.
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2023

Codecov Report

Patch coverage: 46.66% and project coverage change: -0.28% ⚠️

Comparison is base (2f04b5b) 89.08% compared to head (ef724aa) 88.80%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
- Coverage   89.08%   88.80%   -0.28%     
==========================================
  Files          31       31              
  Lines        2373     2386      +13     
==========================================
+ Hits         2114     2119       +5     
- Misses        259      267       +8     
Files Changed Coverage Δ
mapentity/settings.py 91.66% <ø> (ø)
mapentity/views/__init__.py 100.00% <ø> (ø)
mapentity/views/base.py 74.63% <42.85%> (-3.77%) ⬇️
mapentity/urls.py 100.00% <100.00%> (ø)

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

filename = f"tinymce/{uuid4()}/{str(file)}"
path = f"{settings.MEDIA_URL}{filename}"
location = f"{settings.MEDIA_ROOT}/{filename}"
os.makedirs(os.path.dirname(location), exist_ok=True)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
path = f"{settings.MEDIA_URL}{filename}"
location = f"{settings.MEDIA_ROOT}/{filename}"
os.makedirs(os.path.dirname(location), exist_ok=True)
with open(location, "wb+") as destination:

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
@camillemonchicourt
Copy link
Contributor

camillemonchicourt commented Sep 9, 2023

Nice.
I had in mind since a long time to activate upload features in Tinymce to make it a lot easier for user to include their images into static pages. 👍
Maybe we should not put "tinymce" in the file path, to have something more generic like "media/uploads/uuid/.."

@submarcos
Copy link
Member

Better way to achieve this is to create an attachment to keep naming / categorization logic and possibility to delete it if required

@camillemonchicourt
Copy link
Contributor

To me there will be just a few images uploaded in the WYSIWYG, so it is not crucial to have an orphan deletion mechanism.

In classical CMS, there are thousands of images uploaded in articles and no automatic system to delete unused ones.

So in our case it is secondary as most images are uploaded as objects attachments, and it shouldn't block this PR in my opinion.

@marcantoinedupre
Copy link
Contributor

The use of Attachment model to support the TinyMCE image upload should be considered for the following reasons:

  • security checks for free,
  • thumbnailing for free,
  • tracking of the links between uploaded images and mapentity objects (so we get a chance to detect unused uploaded images).

But using Attachment is not straightforward as an instance ID is needed to create an Attachment so we cannot create it when an image is uploaded on the form to create a new entity.

We could create temporary files/attachments in the image_upload handler and create the real Attachement on saving after form validation on the Django side.

Another concern is the absolute URLs which are inserted in the tinymce HTML content. When GTA server configuration changes (for instance with a new domain) all those URLs will be broken.

@submarcos submarcos marked this pull request as draft October 2, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants