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

Implemented recipients per basket #442

Merged
merged 13 commits into from
Jan 14, 2024
Merged

Conversation

lars-devs
Copy link
Contributor

"RecipientsPerItem" may be used in config to define by-basket recipients. Invalid JSON, item to be notified not found in JSON or not defining RecipientsPerItem falls back to regular Recipient as email address that gets notified.

Pull Request Checklist

  • Have you checked to ensure there aren't other open
    Pull Requests for the same update/change?
  • Did you make your Pull Request on the dev branch?
  • Does your submission pass tests? make test
  • Have you lint your code locally prior to submission? make lint
  • Could you build and run the docker image successfully? make images
  • Could you create a running executable? make executable
  • Have you added an explanation of what your changes do
    and why you'd like to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran manual tests with your changes locally?

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (e3b685a) 64.80% compared to head (bc79e7c) 64.67%.
Report is 5 commits behind head on dev.

Files Patch % Lines
tgtg_scanner/notifiers/smtp.py 47.05% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #442      +/-   ##
==========================================
- Coverage   64.80%   64.67%   -0.13%     
==========================================
  Files          26       26              
  Lines        2216     2231      +15     
==========================================
+ Hits         1436     1443       +7     
- Misses        780      788       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@Der-Henning Der-Henning 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 for your contribution!
This is a good add-on to the current smtp notifier and could be a thing for the other notifiers too.
Please try to stick to the single responsibility principle, which says that every function or method should only have a single responsibility.
The responsibility of the _send_mail method is to send a mail. It shouldn't have to check if everything is set up correctly. This is the responsibility of the init method.

config.sample.ini Outdated Show resolved Hide resolved
tgtg_scanner/notifiers/smtp.py Outdated Show resolved Hide resolved
tgtg_scanner/models/config.py Show resolved Hide resolved
Single recipients are allowed to be string instead of a list with only one item.
@lars-devs
Copy link
Contributor Author

Thank You for Your feedback and suggestions! You're right in regards of separation of concerns, that makes it a lot easier to maintain and more secure during runtime. I moved the validation part to the init() method and removed unnecessary parts of the recipient determination. I also updated the config sample to show, that (single) recipients are not limited to [list representation] and validated this as well. Still, I prefer the Recipients to be mandatory, to make sure, a recipient can always be determined if a favorite gets added during runtime which is not present in the Json.

Covered syntax cases

Fallback to Recipients:

  • RecipientsPerItem not in configuration
  • RecipientsPerItem = {}
  • RecipientsPerItem = {"other-id": "a"} (Item ID not in Json)

Catched by exception:

  • RecipientsPerItem = {{} (Invalid syntax)
  • RecipientsPerItem = {"id": 000} (Recipients not list or str format)

Valid formats:

  • RecipientsPerItem = {"id": ["a", "b"]} (Multiple recipients as list)
  • RecipientsPerItem = {"id": [a]} (Single recipient as list)
  • RecipientsPerItem = {"id": "a"} (Single recipient as str)

Copy link
Owner

@Der-Henning Der-Henning left a comment

Choose a reason for hiding this comment

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

In the first review I forgot to mention, that the configuration documentation in wiki/configuration.md should also be modified. The markdown files in wiki/ are uploaded to the github wiki.

tgtg_scanner/models/config.py Show resolved Hide resolved
tgtg_scanner/notifiers/smtp.py Outdated Show resolved Hide resolved
tgtg_scanner/notifiers/smtp.py Show resolved Hide resolved
tgtg_scanner/notifiers/smtp.py Outdated Show resolved Hide resolved
tgtg_scanner/notifiers/smtp.py Outdated Show resolved Hide resolved
tgtg_scanner/notifiers/smtp.py Outdated Show resolved Hide resolved
tgtg_scanner/notifiers/smtp.py Outdated Show resolved Hide resolved
lars-devs and others added 6 commits January 14, 2024 11:49
Co-authored-by: Henning Merklinger <henning.merklinger@gmail.com>
Co-authored-by: Henning Merklinger <henning.merklinger@gmail.com>
Co-authored-by: Henning Merklinger <henning.merklinger@gmail.com>
@Der-Henning
Copy link
Owner

Let me know when you are done. Then I will merge.

Co-authored-by: Henning Merklinger <henning.merklinger@gmail.com>
@lars-devs
Copy link
Contributor Author

I finally did some testing again and am ready to merge.

@Der-Henning Der-Henning merged commit 9208a19 into Der-Henning:dev Jan 14, 2024
21 of 22 checks passed
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.

None yet

2 participants