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

Abstract Notification processing and RawNotification model #23

Merged
merged 7 commits into from Jun 11, 2021

Conversation

chadell
Copy link
Contributor

@chadell chadell commented Jun 10, 2021

The goal of the PR was to start abstracting how do we handle Notifications from external sources, with these changes:

  • Simplify the get_notifications function extracting the Providers validation to each Source class as this will depend on each type of integration, and fix a logic issue.
  • RawNotification.souce is now a foreingkey to a NotificationSource and the sender is optional

Comment on lines 238 to 243
for custom_field, value in provider.get_custom_fields().items():
if custom_field.name == "emails_circuit_maintenances" and value:
self._emails_to_fetch.extend(value.split(","))
providers_with_email.append(provider.name)
else:
providers_without_email.append(provider.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic seems incorrect as it will add a provider that has multiple custom fields to both providers_with_email and providers_without_email.

Suggested change
for custom_field, value in provider.get_custom_fields().items():
if custom_field.name == "emails_circuit_maintenances" and value:
self._emails_to_fetch.extend(value.split(","))
providers_with_email.append(provider.name)
else:
providers_without_email.append(provider.name)
custom_fields = provider.get_custom_fields()
emails = custom_fields.get("emails_circuit_maintenance")
if emails:
self._emails_to_fetch.extend(emails.split(","))
providers_with_email.append(provider.name)
else:
providers_without_email.append(provider.name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I agree with you

@chadell chadell changed the title [WIP] Abstract Notification processing Abstract Notification processing and RawNotification model Jun 10, 2021
@chadell chadell marked this pull request as ready for review June 10, 2021 15:09
glennmatthews
glennmatthews previously approved these changes Jun 10, 2021
Comment on lines 239 to 242
for custom_field, value in provider.get_custom_fields().items():
if custom_field.name == "emails_circuit_maintenances" and value:
provider_emails = value
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Still not seeing why this needs to be a for loop instead of just provider_emails = provider.get_custom_fields().get("emails_circuit_maintenance", ""). What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

provider.get_custom_fields() returns and OrderedDict that doesn't work fine with .get("emails_circuit_maintenance", "")... but maybe it's me who is missing something 🤒

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? OrderedDict is a subclass of dict, it ought to support the same syntax.

Copy link
Contributor Author

@chadell chadell Jun 10, 2021

Choose a reason for hiding this comment

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

it actually works as you say and I expected, but the code was not behaving well because the key is a Django object, will refine it...

>>> from collections import OrderedDict
>>> numbers = OrderedDict()
>>> numbers["one"] = 1
>>> numbers.get("one")
1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay, I hadn't remembered that get_custom_fields() returns a dict of CustomField objects. That makes sense then. The way you've coded it is fine; the alternative would be to use provider.cf (which is an ordinary key-value dictionary) instead of provider.get_custom_fields(), but either way is fine.

glennmatthews
glennmatthews previously approved these changes Jun 10, 2021
@chadell chadell merged commit e7858c2 into develop Jun 11, 2021
@chadell chadell deleted the issue22-raw-notification-adjustements branch June 11, 2021 05:59
@chadell chadell added this to the v0.1.4 milestone Jun 11, 2021
itdependsnetworks pushed a commit to itdependsnetworks/nautobot-app-circuit-maintenance that referenced this pull request Apr 3, 2024
…nexpected-exception

Handle unexpected exceptions for HTML parser
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