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

Provider implements a include filter to define relevant notifications #91

Merged
merged 14 commits into from
Oct 1, 2021

Conversation

chadell
Copy link
Collaborator

@chadell chadell commented Sep 30, 2021

Addressing #82

The goal is to make the Provider capable of filtering in (and implicitly out), which are the relevant notifications that must be parsed and if not successful a ProviderError must be raised.

With the _include_filter dictionary, the Provider can define a dictionary of different data_type to match some string in, and if the filter exists, and the notification doens't match, instead of raising error, returns an empty list of Maintenances

TODO, once approach is approved:

  • Add tests
  • Update readme

@chadell
Copy link
Collaborator Author

chadell commented Sep 30, 2021

@glennmatthews , take this as a proposal, it's just a draft, I will add tests and improve it (I believe it's time to make the keywords data type standard), but just raising to get early feedback on the overall idea

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

I like the design here - relatively simple to implement or add to individual providers' filters, but flexible and powerful as well.

circuit_maintenance_parser/provider.py Outdated Show resolved Hide resolved
@@ -49,6 +50,10 @@ class GenericProvider(BaseModel):
that will be used. Default: `[SimpleProcessor(data_parsers=[ICal])]`.
_default_organizer (optional): Defines a default `organizer`, an email address, to be used to create a
`Maintenance` in absence of the information in the original notification.
_include_filter (optional): Dictionary that defines matching string per data type to take a notification into
account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to flesh this out with an example or two - it's not obvious from just reading this how a valid _include_filter would be structured.

circuit_maintenance_parser/provider.py Show resolved Hide resolved
circuit_maintenance_parser/provider.py Outdated Show resolved Hide resolved
@chadell chadell marked this pull request as ready for review September 30, 2021 15:54
@@ -172,6 +214,8 @@ class HGC(GenericProvider):
class Lumen(GenericProvider):
"""Lumen provider custom class."""

_include_filter = {EMAIL_HEADER_SUBJECT: ["Scheduled Maintenance Window"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at more examples that I have locally, I think just "Scheduled Maintenance" would be better here. I see some subject lines like Lumen Scheduled Maintenance #: 22194642, Scheduled that appear to me to be valid maintenance notifications.

@chadell chadell requested a review from glennmatthews October 1, 2021 05:11
glennmatthews
glennmatthews previously approved these changes Oct 1, 2021
circuit_maintenance_parser/provider.py Outdated Show resolved Hide resolved
@chadell chadell merged commit ce4173c into develop Oct 1, 2021
@chadell chadell deleted the enable-filtering-relevant-notifications-per-provider branch October 1, 2021 12:57
@glennmatthews glennmatthews mentioned this pull request Oct 1, 2021
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.

2 participants