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

load profiles when attaching a file, so smtp drafts are correctly saved #508

Merged

Conversation

kroky
Copy link
Member

@kroky kroky commented Aug 30, 2021

... into a linked imap profile drafts folder; additionally send local attachments saved to first draft as part of imap draft sending procedure - otherwise, attachments stay on the server and are never sent out

@henrique-borba I think saving imap drafts when smtp/imap profile is setup is your work, can you check out these fixes? The main problem I was hunting is missing files in the sent email after attaching them to a message. However, I found another bug as well. In short:

  1. Profiles were not loaded upon auto-saving an imap draft when attaching a file. This made the actual draft miss the attachments.
  2. Even with the above fix, attachments still were not sent out when compose form was submitted. This was because the attachments are saved locally with Cypht draft ids (0, 1, 2, etc.) while imap drafts have IDs equal to the message UID from the remote imap folder. So, when sending out the email, attachments were not related properly due to mismatch of IDs. I added a temporary fix here - whenever a draft with ID > 0 is sent we also add the default attachments saved to draft ID 0. This fixed the immediate issue we have but is not perfect as it might interfere with Cypht drafts when no profiles are used - in that case, if you save draft 0 (upload 1 file), save draft 1 with another file and send draft 1, file from draft 0 will be sent as well. I think a proper fix here implies properly parsing the saved imap draft (when this feature is used) and reattaching the files to the sent email. This means:
    a) delete local attachments when imap draft is saved as those are saved as part of the imap message
    b) show attachments to the imap draft in the compose form when editing an imap draft
    c) send those attachments even if they are not locally uploaded when submitting the compose form

If you have a different/easier to implement idea, I am also fine but I think this imap draft feature needs some updates in order to work fine with attachments.

…ed into a linked imap profile drafts folder; additionally send local attachments saved to first draft as part of imap draft sending procedure - otherwise, attachments stay on the server and are never sent out
@henrique-borba
Copy link
Contributor

Hi @kroky, thanks for the analysis and the hotfix, I'm already working with your proposal and I'll come back here with more information. I'm prioritizing this fix before continuing on to other tasks. Thanks again.

@jasonmunro
Copy link
Member

Thanks to both of you for looking into this. Attachments repeatedly fail for me I think so I might be able to consistently reproduce + test a fix. Not loading profiles on calls is likely a big part of the problem so nice catch there @kroky. If it helps recall that we should be ONLY supporting IMAP drafts at this point, I want to remove/obsolete the original draft functionality. Also when saving a draft with an attachment, the attachment should be part of the saved draft itself in IMAP, and when the draft is loaded fetched from there, not stored locally like when you originally attach one for a message you are composing (hope that makes sense!)

@kroky
Copy link
Member Author

kroky commented Sep 2, 2021

Ah, that makes sense! I think this latest fix now saves the attachments as part of the imap draft but more code is needed to load them fine in the compose form (parsing of email parts needs to be done and figuring out what is an attachment and what not). I wasn't aware that obsolete draft functionality will be removed.

@jasonmunro
Copy link
Member

@kroky and @henrique-borba is this ready to merge?

@kroky
Copy link
Member Author

kroky commented Sep 15, 2021

@jasonmunro I have no new input here, maybe @henrique-borba will push his changes when they are ready. If you merge this PR now, we will have a partial fix for the attachment issue which is still better than no fix but still not 100% ready.

@henrique-borba
Copy link
Contributor

@kroky @jasonmunro The implementation I made (#518) works the way you guys discussed. Basically, after removing the old draft method I updated the save_uploaded_file method calls to use the UID of the IMAP drafts and update the uploaded files keys with the new UID every time a draft is edited.

The moment a previously saved draft is opened, the attachments are downloaded and added again to the "uploaded_files" array, this way the compose page can correctly manage the attachments.

As this PR (#508) assumes that the legacy drafts are still in use, I believe it is no longer necessary.

@kroky
Copy link
Member Author

kroky commented Sep 27, 2021

Thanks @henrique-borba ! Yes, PR is partially outdated now but I think you still need the bits about profiles when auto-saving draft with an attachment via ajax. save_imap_draft method uses compose_profiles but I found out that these are not initalized in ajax_smtp_attach_file. Can you double check that you don't really need load_smtp_servers_from_config and compose_profile_data handlers in that ajax page?

@henrique-borba
Copy link
Contributor

henrique-borba commented Sep 28, 2021

@kroky you are correct, how should we proceed? I think @jasonmunro can accept yours first and I merge mine after, any conflicts if they happen I can resolve. Thanks again =D

@kroky
Copy link
Member Author

kroky commented Sep 28, 2021

@henrique-borba I don't mind you copy those couple of lines to your PR and have @jasonmunro merge only one PR. He can close this one in favor of yours. Thanks!

@jasonmunro jasonmunro merged commit 893c550 into cypht-org:master Sep 29, 2021
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.

None yet

3 participants