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

Fixed #35528 -- Added helper method EmailMultiAlternatives.body_contains(). #18278

Conversation

GitRon
Copy link
Contributor

@GitRon GitRon commented Jun 17, 2024

Trac ticket number

ticket-35528

Branch description

Currently, it's very hard and tedious to assert the content of an email object. Therefore, we want to add a method "body_contains()" to "EmailMultiAlternatives" to check a search string in all available text-based alternatives (content parts, like HTML).

This method can then be easily asserted in any given unit-test.

There's a forum discussion going on about this topic: ​https://forum.djangoproject.com/t/improve-email-unit-testing/32044/1

I've already created a PR for a suggetion: ​https://github.com/django/django/pull/18278/files

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@GitRon
Copy link
Contributor Author

GitRon commented Jun 17, 2024

@sarahboyce Here's my first draft. Is this going in the desired direction?

@GitRon GitRon marked this pull request as ready for review June 17, 2024 09:38
@sarahboyce
Copy link
Contributor

@sarahboyce Here's my first draft. Is this going in the desired direction?

Yes, let's also raise a ticket on Trac 👍
We will need docs and a release note

I wonder if we want it to have a raise_exception optional kwarg to have it error saying something like f"{text} not found in EmailMultiAlternatives.body"/f"{text} not found in EmailMultiAlternatives.alternatives[{index}]" 🤔

@GitRon
Copy link
Contributor Author

GitRon commented Jun 17, 2024

Thx @sarahboyce !

Yes, let's also raise a ticket on Trac 👍

Am I supposed to do this? (Which I can surely do!)

We will need docs

I was wondering about where to put the docs since there is no detailed page for this class.

[...] and a release note

Is this documented somewhere? Every project does things differently...

I wonder if we want it to have a raise_exception optional kwarg to have it error saying something like f"{text} not found in EmailMultiAlternatives.body"/f"{text} not found in EmailMultiAlternatives.alternatives[{index}]" 🤔

Sounds good to me. Should I start implementing or do we want to gather some more opinions on this?

@sarahboyce
Copy link
Contributor

Yes, let's also raise a ticket on Trac 👍

Am I supposed to do this? (Which I can surely do!)

Yes please

We will need docs

I was wondering about where to put the docs since there is no detailed page for this class.

Maybe in here: https://docs.djangoproject.com/en/5.0/topics/email/#sending-alternative-content-types

[...] and a release note

Is this documented somewhere? Every project does things differently...

https://docs.djangoproject.com/en/5.0/internals/contributing/writing-code/submitting-patches/#new-features

Release note needs to be added for 5.2

Sounds good to me. Should I start implementing or do we want to gather some more opinions on this?

As it's something that could be added later, I don't think it needs to be discussed now 🤔 can see when other people review also

@GitRon
Copy link
Contributor Author

GitRon commented Jun 17, 2024

@sarahboyce

  1. Ticket has been created. Does it look ok to you?
  2. Docs added. Are you happy with content and location?
  3. Added release notes
  4. Good idea 👍

@sarahboyce sarahboyce changed the title Added "body_contains" method to "EmailMultiAlternatives" Fixed #35528 -- Added helper method EmailMultiAlternatives.body_contains(). Jun 17, 2024
@GitRon
Copy link
Contributor Author

GitRon commented Jun 18, 2024

Hi @sarahboyce! Another noobie question 😅

One check is failing but I can't see how I can fix the issue.

pull-requests-focal/database=sqlite3,label=focal-pr,python=python3.12

complains about

stderr: fatal: Unable to create '/home/jenkins/workspace/pull-requests-focal/database/sqlite3/label/focal-pr/python/python3.12/.git/shallow.lock': File exists.

Any ideas on that?

Thx!
Ronny

@sarahboyce
Copy link
Contributor

Ticket has been created. Does it look ok to you?

Yes thank you ✅

Docs added. Are you happy with content and location?

I think you've done a good job with the docs that are available.
I think it might be beneficial for us to restructure it a little to make this extension easier, I have raised a PR with what I have in mind here: #18279 Feel free to review the PR and let me know what you think (has a benefit that it's easier to link the release notes to the updated docs etc)

On the failing check, a couple of the test runs have been failing recently on many PRs and I'm not sure why. It's unrelated to your PR 👍

docs/topics/email.txt Outdated Show resolved Hide resolved
docs/topics/email.txt Outdated Show resolved Hide resolved
tests/mail/tests.py Outdated Show resolved Hide resolved
tests/mail/tests.py Outdated Show resolved Hide resolved
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Lovely, looking good 👍 some more thoughts on the docs

docs/topics/email.txt Outdated Show resolved Hide resolved
docs/topics/email.txt Show resolved Hide resolved
docs/releases/5.2.txt Outdated Show resolved Hide resolved
@nessita
Copy link
Contributor

nessita commented Jun 18, 2024

On the failing check, a couple of the test runs have been failing recently on many PRs and I'm not sure why. It's unrelated to your PR 👍

The Jenkins workspace for this job was corrupted, I have wiped it so future runs should pass OK.

docs/topics/email.txt Outdated Show resolved Hide resolved
@sarahboyce
Copy link
Contributor

Note: merged in #18261 which caused a couple of docs conflicts but shouldn't require further updates

@GitRon
Copy link
Contributor Author

GitRon commented Jun 20, 2024

@sarahboyce Solved the merge conflicts 👍

@sarahboyce sarahboyce force-pushed the feature/email-multi-alternative-body-contains-method branch from 40a6efe to 13e056b Compare June 20, 2024 09:06
@sarahboyce sarahboyce force-pushed the feature/email-multi-alternative-body-contains-method branch from 13e056b to f7b4421 Compare June 21, 2024 07:46
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you 👍 welcome onboard ⛵

@sarahboyce sarahboyce merged commit 5fef6d2 into django:main Jun 21, 2024
35 checks passed
@GitRon GitRon deleted the feature/email-multi-alternative-body-contains-method branch October 5, 2024 13:48
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