search

cclrobotics / ARTBot

Project to allow submissions of agar art and conversion of those submissions to bot-readable
MIT License
8 stars 5 forks source link

Emails #34

Closed j3py closed 4 years ago

j3py commented 5 years ago

Forgot to add some import statements in the asm emailer script.

tsdobbs commented 4 years ago

If app is not required then we should remove it entirely

You mean the scheduled mailer app?

brcb20 commented 4 years ago

If app is not required then we should remove it entirely

You mean the scheduled mailer app?

yes

tsdobbs commented 4 years ago

Do we have a way to upload the final images to the db? Might be worth exploring saving these images in a folder instead but should be ok for now.

No, no upload method yet. We need one. We came to the conclusion that, ultimately, at our scale, it doesn't matter if we upload bytes to db or save in a folder. Both would work. The downside of switching to saving files in a folder is that we'd need to implement a media bucket on our deployment server - which is doable, but doesn't seem worth the work unless there's a good reason.

tsdobbs commented 4 years ago

If app is not required then we should remove it entirely

You mean the scheduled mailer app? yes

It's handy to have an email automatically be sent on upload instead of manually sending (although manually sending isn't the end of the world). The other way I can imagine to do this is to implement it into the workflow, e.g. when a photo of art gets uploaded into the db, the app sends an email. The drawback of that is that the upload workflow would have to wait for email to finish unless you split it off a separate process. I do agree, however, that we'd rather not have an extra independent app in the repo if we can avoid it.

tsdobbs commented 4 years ago

With latest commits email sending works (I can send config vars offline). What we need for this to be merged is to be able to run email_scheduler.py in a separate thread from the main server activity. Currently expects to be run as a Linux service, which could work, but is not ideal for deployment and complicates the repo structure. I think a multi-thread handler as part of the python app would be better?

brcb20 commented 4 years ago

With the latest commits email sending works (I can send config vars offline). What we need for this to be merged is to be able to run email_scheduler.py in a separate thread from the main server activity.

Maybe for now we can split this branch into two, one for sending the confirmation email when we receive the art request and then another branch for the email scheduler. Then we can merge the changes for the confirmation email now and figure the scheduler out this week. @tsdobbs what do you think?

Currently expects to be run as a Linux service, which could work, but is not ideal for deployment and complicates the repo structure. I think a multi-thread handler as part of the python app would be better?

From what I understand, Flask does not play nice with multi-threading. I think we would save ourselves a lot of time and potential issues, if we worked on a user friendly way of uploading the final images using a new route in the main application, and in that route we email the corresponding user.

tsdobbs commented 4 years ago

Sounds like a sensible path forward. @brcb20 can you make that split and then merge the confirmation email side?

On Tue, Nov 12, 2019, 8:35 AM Benjamin B notifications@github.com wrote:

With latest commits email sending works (I can send config vars offline). What we need for this to be merged is to be able to run email_scheduler.py in a separate thread from the main server activity. Currently expects to be run as a Linux service, which could work, but is not ideal for deployment and complicates the repo structure. I think a multi-thread handler as part of the python app would be better?

Maybe for now we can split this branch into two, one for sending the confirmation email when we receive the art request and then another branch for the email scheduler. Then we can merge the changes for the confirmation email now and figure the scheduler out this week. @tsdobbs https://github.com/tsdobbs what do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cclrobotics/ARTBot/pull/34?email_source=notifications&email_token=AEB3IRRBBXKG6B5LLKMTZWTQTLLNDA5CNFSM4JD3HYP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOED23Q7A#issuecomment-552974460, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEB3IRUE4MQDTKSS5CRZGBDQTLLNDANCNFSM4JD3HYPQ .