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

Feature: Send CSV Report as attachment #86

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

Conversation

chalhoub-university
Copy link

@chalhoub-university chalhoub-university commented Mar 22, 2021

In Reference to Issue: #55

Hello,
To add the functionality for CSV attachment to be emailed to the user, we have explored using the File API and Messaging API to create a CSV File and attach it to the email body.

We were successfully able to add an option for the user, create the CSV File using the File API but the step involving fetching the file to attach always returns 'false'.

The moodle documentation for "Read File" states the below which might be the reason the below method does not work.

This is a way to read a file, equivalent to file_get_contents. Please note your are allowed to do this ONLY from mod/mymodule/* code, it is not acceptable to do this anywhere else. Other code has to use file_browser interface instead.

The Messaging API on the otherhand is able to accept file attachments, this has been added in the code in the PR too.

Would really appreciate your review. Additionally, for the "Read/Get File", we would love to get any tips or redirection to any alternate moodle functionality that we can use for plugins of type report to fix the missing functionality.

@timhunt
Copy link
Member

timhunt commented Mar 22, 2021

Why is it necessary to use the File API? - Oh, because the message API can only sent stored_files, not a temporary file. Well that is annoying.

However, swtiching all files used by this plugin to be stored_files is not a good design change. And, it is not correct (e.g. report_customsql_delete_old_temp_files does not work with your changes.)

Also, if possible, please ensure that the automated tests pass before opening a pull request.

If it was me, I would first get a chagen into Moodle core, so that the message API could handle any file, and then use that to implement this feature here.

@timhunt
Copy link
Member

timhunt commented Mar 22, 2021

Also, before submitting, please clean up the git history.

@timhunt timhunt force-pushed the main branch 4 times, most recently from a6e426a to 29660f6 Compare July 7, 2021 08:13
@timhunt timhunt force-pushed the main branch 6 times, most recently from 003c70f to ce26f60 Compare December 13, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants