Skip to content

Conversation

@mrsaicharan1
Copy link
Member

@mrsaicharan1 mrsaicharan1 commented May 17, 2019

Fixes #5888

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

Short description of what this resolves:

Order Invoices and Tickets are attached to the emails being sent out when the orders are placed. Currently, we are manually sending an API request to SendGrid which makes it hard to add any new headers, attachments or any new data. The SendGrid Client makes it easier to do this.

Changes proposed in this pull request:

  • Order Invoices and Tickets are attached as PDFs
  • Made the email message non ambiguous.
  • Switched to the SendGrid Python Client to support multiple attachments.

Screenshot 2019-05-18 at 7 20 05 PM

@auto-label auto-label bot added the feature label May 17, 2019
@mariobehling
Copy link
Member

The problem with this approach seems to be that we make ourselves entirely dependent on Sendgrid, which we don't want. We actually want to move away from Sendgrid. We want to be able to send out information no matter what gateway we use. Sendgrid is just one. A better option for now (as we are small) is to use SMTP.

@codecov
Copy link

codecov bot commented May 17, 2019

Codecov Report

Merging #5917 into development will decrease coverage by 0.08%.
The diff coverage is 18.42%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5917      +/-   ##
===============================================
- Coverage        66.48%   66.39%   -0.09%     
===============================================
  Files              286      286              
  Lines            13933    13958      +25     
===============================================
+ Hits              9263     9268       +5     
- Misses            4670     4690      +20
Impacted Files Coverage Δ
app/api/helpers/system_mails.py 100% <ø> (ø) ⬆️
app/api/helpers/mail.py 28.73% <100%> (ø) ⬆️
app/api/helpers/tasks.py 17.59% <19.04%> (+0.44%) ⬆️
app/api/orders.py 34.21% <6.66%> (-1.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 215b471...9990556. Read the comment docs.

@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented May 18, 2019

I’ve chosen this approach because this is being used in production right now. I will implement this for SMTP too. I just did this so that we could have a working system for now.
Also, as soon as this is good to go, will integrate a feature into the system which falls back to SMTP if sendgrid API calls exceed our quota

@mrsaicharan1 mrsaicharan1 force-pushed the attach-email branch 2 times, most recently from 83ccd4c to 8625b9a Compare May 18, 2019 05:33
@abhinavk96 abhinavk96 requested a review from iamareebjamal May 18, 2019 11:05
Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

Faulty commit history, can't merge

@mrsaicharan1
Copy link
Member Author

Faulty commit history, can't merge

Yeah , was fixing it. Will update.

@fossasia fossasia deleted a comment May 18, 2019
@fossasia fossasia deleted a comment May 18, 2019
@fossasia fossasia deleted a comment May 18, 2019
@mrsaicharan1
Copy link
Member Author

@CosmicCoder96 @iamareebjamal Fixed conflicts. Please check.

@mrsaicharan1 mrsaicharan1 force-pushed the attach-email branch 2 times, most recently from 5f8faf7 to 43240c0 Compare May 18, 2019 14:09
Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

@mrsaicharan1 a codacy issue is left.

iamareebjamal
iamareebjamal previously approved these changes May 18, 2019
@fossasia fossasia deleted a comment May 18, 2019
@fossasia fossasia deleted a comment May 18, 2019
@mrsaicharan1
Copy link
Member Author

@CosmicCoder96 @iamareebjamal Fixed Codacy & Hound Issues.

@iamareebjamal iamareebjamal merged commit 8afec83 into fossasia:development May 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tickets not attached when tickets confirmation mail is sent

5 participants