Skip to content

Updated send email to handle html body#2

Merged
mehuljhaver4 merged 3 commits into
masterfrom
feature/update-send-email
Oct 12, 2023
Merged

Updated send email to handle html body#2
mehuljhaver4 merged 3 commits into
masterfrom
feature/update-send-email

Conversation

@sinarest1608
Copy link
Copy Markdown
Contributor

What does this change do? Please be clear and concise.

Changes send_email() function to handle html content in the body. Strings in body have no impact and render as usual.

Why was this change made? Including an issue number is sufficient. Otherwise, briefly explain the benefit of the change.

send_email() parsed the html code in the body as string and sent the complete code instead of rendering the html.

Verification

NA

Affirmations

All of these should have a check by them. Any exception requires an explanation.

  • I have added docstrings with details on keyword arguments to new functions following the convention detailed in this gist
  • I have added type hinting to new function parameters.
  • I have added tests to new functions, following the rules of F.I.R.S.T..
  • I matched the style of the existing code.
  • I added and updated any relevant documentation (inline, README, CHANGELOG, and such).
  • I used Python's type hinting.
  • I ran the automated tests and ensured they ALL passed.
  • I ran the linter and ensured my changes have not introduced ANY warnings or errors.
  • I have made an effort to minimize code changes and have not included any cruft (preference files, *.pyc files, old comments, print-debugging, unused variables).
  • I have made an effort maintain a clear commit history (haven't merged other branches or rebased improperly).
  • I have written the code myself or have given credit where credit is due.

@sinarest1608 sinarest1608 requested review from mehuljhaver4 and michael-bentz and removed request for michael-bentz October 4, 2023 17:32
Copy link
Copy Markdown
Contributor

@mehuljhaver4 mehuljhaver4 left a comment

Choose a reason for hiding this comment

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

  • You can remove the print statements on line 98 and 101
  • The code in general works as expected.
  • Not sure if we will have this edge case but it fails when there's "\n" and "<some html tag>" in the body of the message.
  • Example: "This is \n a Python Script. <p> Testing the script"

@sinarest1608
Copy link
Copy Markdown
Contributor Author

@mehuljhaver4
I have removed the print statements.
Yes the edge case is a valid one but i'm also not sure if we would ever face this case.
@mbentz-uf what do you think?

@sinarest1608
Copy link
Copy Markdown
Contributor Author

@mbentz-uf
Mehul reviewed the code but is not able to merge it. Can you help with the merge?

@michael-bentz
Copy link
Copy Markdown
Contributor

@mehuljhaver4 Can you try now?

@michael-bentz
Copy link
Copy Markdown
Contributor

#3 would be a nice-to-have in the future.

@mehuljhaver4
Copy link
Copy Markdown
Contributor

mehuljhaver4 commented Oct 12, 2023

@mehuljhaver4 Can you try now?

Merging is still blocked @mbentz-uf

@mehuljhaver4 mehuljhaver4 merged commit f1f68d4 into master Oct 12, 2023
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