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

Form & Email attachments #12218

Closed

Conversation

MikeKry
Copy link
Contributor

@MikeKry MikeKry commented Aug 18, 2022

Fixes #5108, fixes #6027. Related to some other tasks.

I have created task to store attachments from OrchardCore Forms (or POST requests using multipart/form-data). It is based on code from #6027 and enhanced, still it is more like a POC, but completely working & tested. Feel free to merge or finish what is needed, I might have some time to do customizations also.

Can be used in workflows for sending mails, or just to store files without sending.
image

Inside task, you can configure if you want files to be stored in configured media storage or custom path in App_Data/Tenant/Folder

image

In existing Email task there are two new checkboxes to configure this functionality. You can enable/disable sending attachments and choose if you want to remove files after sending (you may want to keep them, if you log your emails somewhere).

image

What I think could be added:

  • Secure folder in default media file storage from public access (currently they are secured in app_data folder when not using media file storage)
  • Attachment size and type limitation
  • Error notifications / output
  • Maybe choose source fields, so we can send multiple mails with different attachments.
  • automatic tests?
  • strip workflowid prefix from file when sending?

var filePaths = new List<string>();
foreach (var file in _http.HttpContext.Request.Form.Files)
{
var filePath = PathExtensions.Combine(Folder, $"{workflowContext.WorkflowId}-{file.FileName}");
Copy link
Contributor

Choose a reason for hiding this comment

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

the funny thing is that I had same episode in my dev life, and wrote sth similiar to your code :P

maybe it would be better to use _mediaFileStore.Combine instead of PathExtensions.Combine

image

Copy link
Contributor Author

@MikeKry MikeKry Aug 18, 2022

Choose a reason for hiding this comment

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

@lampersky I guess there are not many other ways to implement this on OrchardCore framework, so it should be similliar :) Not sure what you mean by same episode in dev life though :D

If you have some code working, feel free to contribute and update this PR.

Anyway thanks for feedback, I have changed it to fileStore.Combine


public override LocalizedString DisplayText => S["Save Form Attachments Task"];

public override LocalizedString Category => S["UI"];
Copy link
Contributor

Choose a reason for hiding this comment

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

I will suggest "Media" category here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, changed to Media

@hishamco
Copy link
Member

@MikeKry is the intend to store the attachment or send it with the sent email? We already have Attachments property that we can add the attachments to it, I presume the workflow should utilize this. Let me review this PR

// to put files in different folder, we would need either to register some private MediaFileStore, or allow to specify folder outside base path (?)
var fileStore = UseMediaFileStore ? _fileStore : CreateDefaultFileStore();
var filePaths = new List<string>();
foreach (var file in _http.HttpContext.Request.Form.Files)
Copy link
Contributor

Choose a reason for hiding this comment

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

How does that work in the context that the form is not set to enctype="multipart/form-data"???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Skrypt

It does not work without that - does not break but does nothing. I put it in description of task so everyone sees that it needs multipart formdata

@MikeKry
Copy link
Contributor Author

MikeKry commented Aug 19, 2022

@hishamco

I think both use cases are valid.
1/ you can store attachments to filesystem without sending (using new task)
2/ you can send them via customized Email task, that can auto-include them (using customized email task)

Email sending customization utilizes Attachments property that you mention.

@sebastienros
Copy link
Member

Can we define the destination media filename using liquid? Or at least have a default pattern that includes a custom folder and the workflow id. This could even use the request headers if the sender provides them. Users would then use the same pattern in the Email task to reference the file that was uploaded. And then create a task to delete the file (no need for a custom checkbox).

In need to check what it means though to switch between media and file stores (I don't remember).

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@Piedone
Copy link
Member

Piedone commented Mar 13, 2024

Is this something you'd like to revisit any time soon @MikeKry or should we close? I think this doesn't need to cover everything you mention in the PR description, but restrictions on file uploads (size and extension) need to be added, otherwise it wouldn't be securely usable. I'd use a per-Task config for this, with defaults coming from the Media config (and the max file size should by <= than the Media config too).

@MikeKry
Copy link
Contributor Author

MikeKry commented Mar 16, 2024

@Piedone

That is good point about extension and size. I thought it is checked by MediaFileStore but now I see it is not.

@Piedone
Copy link
Member

Piedone commented Mar 19, 2024

Yep, that would be necessary. Would you like to revisit this in the foreseeable future?

Copy link
Contributor

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up and remove the "stale" label.

@github-actions github-actions bot added the stale label May 19, 2024
Copy link
Contributor

github-actions bot commented Jun 6, 2024

Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it.

@github-actions github-actions bot closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants