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

issue #635: Add django-anymail #667

Closed

Conversation

vaishnavi-2901
Copy link

@vaishnavi-2901 vaishnavi-2901 commented Jun 28, 2024

Description

I added a depedencies and some base code to use django-anymail

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.
  • My change requires dependencies updates.
  • I have updated the dependencies accordingly.

#Note:
Please update the config file for

  1. SENDGRID_API_KEY (this needs to be get via sendgrid)
  2. DEFAULT_FROM_EMAIL

Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Please check comments. Also, please test on production to ensure it's working properly.

@vaishnavi-2901
Copy link
Author

vaishnavi-2901 commented Jun 28, 2024 via email

@vaishnavi-2901
Copy link
Author

vaishnavi-2901 commented Jun 28, 2024 via email

@fjsj
Copy link
Member

fjsj commented Jun 28, 2024

You'll follow the README to produce your own env to test.

@vaishnavi-2901
Copy link
Author

vaishnavi-2901 commented Jun 28, 2024 via email

@hugobessa
Copy link
Contributor

This change is probably breaking CI and render.com setup as it introduces new required env vars that aren't mentioned there.

@vaishnavi-2901
Copy link
Author

vaishnavi-2901 commented Jun 28, 2024 via email

@fjsj
Copy link
Member

fjsj commented Jun 28, 2024

@vishu1089 you'll have to update render.yaml to not use SENDGRID_USERNAME and SENDGRID_PASSWORD and start using SENDGRID_API_KEY.

CTRL+F everywhere in the project for sendgrid and update whatever is necessary too, please.

@vaishnavi-2901
Copy link
Author

vaishnavi-2901 commented Jun 28, 2024 via email

@vaishnavi-2901
Copy link
Author

done, please recheck

@vaishnavi-2901 vaishnavi-2901 requested a review from fjsj June 30, 2024 13:50
README.md Outdated Show resolved Hide resolved
Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

Check comments

@vaishnavi-2901 vaishnavi-2901 requested a review from fjsj July 2, 2024 08:08
@vaishnavi-2901
Copy link
Author

all the comments are resolved, please re-review

Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but I will test manually before merging. Thanks.

Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

After a closer review, there are still errors on this PR. You didn't update proj_main.yml removing SENDGRID_USERNAME and SENDGRID_PASSWORD and adding SENDGRID_API_KEY there. Also update .github/workflows/shared-build/action.yml.

While we appreciate the PR, please be extra-careful on changing settings. I had requested a full search over all files for "sendgrid" to perform all the changes. Please ensure you did that.

@fjsj
Copy link
Member

fjsj commented Jul 3, 2024

Additionally, remove the poetry.lock file. We don't include that in the boilerplate, and it's risky to keep it anyway due to supply-chain attacks.

@vaishnavi-2901
Copy link
Author

Okay

@fjsj
Copy link
Member

fjsj commented Jul 11, 2024

Closing due to inactivity. Please feel free to reopen with the issues addressed.

@fjsj fjsj closed this Jul 11, 2024
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