Skip to content

add support of remote storage #303

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

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

Conversation

badziyoussef
Copy link
Contributor

Actually, the package is not compatible with django-storages, which is a valuable feature, as django can run in production in a scallable environment, actually with django-storages it raise (see screenshot bellow)

image

@badziyoussef badziyoussef force-pushed the feature/add-support-remote-storage branch from 4ace5da to a175bab Compare December 27, 2024 11:12
@badziyoussef badziyoussef force-pushed the feature/add-support-remote-storage branch from ef90206 to 0be2afd Compare December 27, 2024 11:39
@badziyoussef badziyoussef changed the title add support of remote storage add support of remote storage and fix issue when retrieving google auth token Dec 30, 2024
@badziyoussef badziyoussef force-pushed the feature/add-support-remote-storage branch from 97b5657 to c08461c Compare December 30, 2024 10:14
@badziyoussef
Copy link
Contributor Author

badziyoussef commented Jan 8, 2025

@pfouque can we move forward with this PR? the bug is a real blocker when using Google Oauth2

@pfouque
Copy link
Collaborator

pfouque commented Jan 8, 2025

Hi @badziyoussef,
Firstly, It would be easier to merge two separate PR, with distinct purposes.

Additionally, merging doesn't necessarily mean a new release will be published soon.
So I'd recommend you, especially if it's urgent, to run your project using your own fork in the meantime (Multiple solutions exist to do so, with or without publishing to pypi).

@badziyoussef badziyoussef force-pushed the feature/add-support-remote-storage branch from c08461c to 0be2afd Compare January 8, 2025 15:20
@badziyoussef badziyoussef changed the title add support of remote storage and fix issue when retrieving google auth token add support of remote storage Jan 8, 2025
@badziyoussef
Copy link
Contributor Author

Hi @pfouque, I splitted to two separate PR as requested, if you can check

@pfouque
Copy link
Collaborator

pfouque commented May 8, 2025

Hi @badziyoussef,
Thanks for splitting the PR but this implementation is what we had previously and it was generating some trouble.
According to PR #273 there was an issue when too many files were opened.
I assume it was solved by using "with" which garantee to have only one file opened at once.
To solve both issues at once, we need to find a way to close files after processing them, using a remote-storage compatible accessor...

Comment on lines +722 to +733
output = BytesIO()
encode_quopri(
BytesIO(
attachment.document.read()
),
output,
quotetabs=True,
header=False,
)
new.set_payload(
output.getvalue().decode().replace(' ', '=20')
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs to be tested, but it's probably enough to make it compatible with remote storage

Suggested change
output = BytesIO()
encode_quopri(
BytesIO(
attachment.document.read()
),
output,
quotetabs=True,
header=False,
)
new.set_payload(
output.getvalue().decode().replace(' ', '=20')
)
with attachment.document.open('rb') as f:
output = BytesIO()
encode_quopri(
BytesIO(
f.read()
),
output,
quotetabs=True,
header=False,
)
new.set_payload(
output.getvalue().decode().replace(' ', '=20')

del new['Content-Transfer-Encoding']
new['Content-Transfer-Encoding'] = 'quoted-printable'
else:
with open(attachment.document.path, 'rb') as f:
new.set_payload(f.read())
new.set_payload(attachment.document.read())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs to be tested, but it's probably enough to make it compatible with remote storage

Suggested change
new.set_payload(attachment.document.read())
with attachment.document.open('rb') as f:
new.set_payload(f.read())

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.

2 participants