Skip to content

smtp_batch: add feature for grouping and templating #2610

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

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

Lukas-Heindl
Copy link
Contributor

@Lukas-Heindl Lukas-Heindl commented Apr 29, 2025

Like discussed in #2586, here is the thing. I already did some manual testing regarding templating the subject string. For this configured

  1. add extra.my_template_value to the events (I did this with the modify bot for now)
  2. add that key (extra.my_template_value) to the new additional_grouping_keys parameter
  3. use subject: "Prefix {{ extra_my_template_value }}"
  4. activate the templating capabilites for the subject line with templating: {subject: true}

But probably some tests in the test suite will be needed for this (but I'd like to defer this one after the first feedback round)

Any comments?

Maybe issues to discuss:

  • how values get hashed generically with hash_arbitrary
  • passing the template_data along with each mail as a tuple
  • really a warning if jinja2 couldn't be imported? (the user might not want jinja2) -- also whether to add this optional requirement to the requirements.txt?

Also I didn't get the testing environment setup correctly until now, so it would be nice if someone could trigger the pipelines yet already so I can check the outputs regarding code-style.

@Lukas-Heindl
Copy link
Contributor Author

Lukas-Heindl commented Apr 29, 2025

Feel free to have a look, but it will be some time (estimate: 1-2 weeks) until I can have a look at the failing pipelines.
My way to go would be

  1. looking into what makes them fail
    • code style should pass obviously
    • failing tests here might indicate a not fully backwards compatible change
  2. feedback (of course any time, also before/while 1.)
  3. Write additional tests covering the new features

@sebix sebix requested a review from e3rd April 29, 2025 18:55
@sebix sebix added bug Indicates an unexpected problem or unintended behavior feature Indicates new feature requests or new features labels Apr 29, 2025
Copy link
Member

@e3rd e3rd left a comment

Choose a reason for hiding this comment

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

Seems good, thanks! :)

@Lukas-Heindl
Copy link
Contributor Author

Finally the tests are working. (Sorry for the frequent pushes)

So now from my point of view only some tests for the new features are missing. I'd like to keep the old/current tests as "basic" config with the "basic" set of features and one new test (different config is needed for this anyhow) for the more advanced features (one run which includes the new grouping and the templating).

@Lukas-Heindl Lukas-Heindl force-pushed the smtpBatchGroup branch 4 times, most recently from 2b7e2da to cdd5d60 Compare May 6, 2025 16:32
@Lukas-Heindl
Copy link
Contributor Author

The test should be easily fixed now. But I noticed another issue (for which we do not have a test yet:

for mail in (field if isinstance(field, list) else [field]):
self.cache.redis.rpush(f"{self.key}{mail}", message.to_json())

When source.abuse_contact is multiple addesses it is important to use the address stored in the key and not the soruce.abuse_contact field.

The fix I thought about would be to simply add a number to the redis key serving as index which indicates which address of the source.abuse_contact field should be used in case there are multiple ones.

Any other ideas / comments on this approach?

@Lukas-Heindl
Copy link
Contributor Author

Ok I noted at least with the default harmonization.yml, source.abuse_contact shouldn't be a list. Still the old version also handled the list case. What do you think should we continue supporting that case? (and if yes what do you think about my current approach?)

Apart from that issue this is finished from my point of view (maybe cleanup the commit history even more). Any comments / changes required?

@e3rd
Copy link
Member

e3rd commented May 14, 2025

(sorry I'm late again, trying to get to you in the next days)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior feature Indicates new feature requests or new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants