Skip to content

Conversation

@AndriusV4
Copy link

Currently Google parser statically sets status to CONFIRMED for all maintenances. But Google sends emails with multiple statuses (Scheduled, Completed, Canceled). To cover this:

  • Adding subject parser
  • Moving status parsing to subject parser (HTML status parsing is cumbersome because 'Scheduled' status maintenances have no status in the main div, only 'Completed' and 'Canceled' do (and the status is in a separate element, so current HTML parsing logic was failing on those)
  • Moving summary and maintenance_id parsing to subject parser for simplicity

Also Google sometimes send maintenances without any End Date specified at all, so to cover this:

  • Added a check if no End Time was found
  • Creating a dummy End Time of Star Time + 1hour, because Start/End times cannot be equal in this library.

Also added multiple tests to cover both parsers and e2e functionality as well.

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.

Thanks for the PR, looks solid at a quick readthrough. Would you mind adding a changelog fragment?

@AndriusV4
Copy link
Author

Thanks, added changelog fragment

Copy link
Contributor

@jvanderaa jvanderaa left a comment

Choose a reason for hiding this comment

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

Looking good with just one spelling in a comment suggestion.

match = re.search(r" - Reference (.*)$", summary)
data["summary"] = summary
data["maintenance_id"] = match[1]
# Google sometimes send notifications without End Time specificed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Google sometimes send notifications without End Time specificed
# Google sometimes send notifications without End Time specified

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