Sparkle Provider #73
Sparkle Provider #73
Conversation
carbonarok
requested review from
chadell,
glennmatthews and
pke11y
as code owners
tests/unit/test_processors.py
Outdated
| @@ -14,8 +14,35 @@ | |||
| from circuit_maintenance_parser.parser import Parser | |||
|
|
|||
|
|
|||
| PARSED_DATA = [{"a": "b"}, {"c": "d"}] | |||
| EXTENDED_DATA = {"z": "x"} | |||
| PARSED_DATA = [ | |||
There was a problem hiding this comment.
I would keep the fake data style, to make this completely independent from th eactual values from the parsers
There was a problem hiding this comment.
@carbonarok remember to address this too
| if len(maintenances_extracted_data) == 1: | ||
| self.combined_maintenance_data.update(maintenances_extracted_data[0]) | ||
| else: | ||
| raise ProcessorError(f"Unexpected data retrieved from parser: {maintenances_extracted_data}") | ||
| maintenances_data.extend(maintenances_extracted_data) |
There was a problem hiding this comment.
I believe it would be useful to document the new logic in the docstring.
To make clear that when the parser return one object we just accumulate it in the combined_maintenance_data but when it's multiple, we understand that there are multiple maintenances so we save them in the maintenances_data to be all extended at the end.
Maybe we could add a safety check if we try this maintenances_data.extend twice... this should only happen one time in this processor
| raise ProcessorError( | ||
| "Not enough information available to create a Maintenance notification." | ||
| ) from exc | ||
| else: |
There was a problem hiding this comment.
what if you do something like:
if not maintenances_data:
maintenance_data = [{}]
maintenances = maintenances_data.copy()
maintenances_data.clear()
...
so, you don't need the last try/except 😄
There was a problem hiding this comment.
Because your re-initalizing maintenance_data the value cannot be passed back out of the function.
So ive implemented the code before
if not maintenances_data:
maintenances = [{}]
else:
maintenances = maintenances_data.copy()
maintenances_data.clear()
| """After processing all the parsers, we try to combine all the data together.""" | ||
| """After processing all the parsers, we try to combine all the data together. | ||
|
|
||
| For some notifications there can be multiple maintenances in a single file. To handle this, maintenances are store in a |
There was a problem hiding this comment.
maybe move this explanation to process_hook where this logic actually take place?
| raise ParserError from exc | ||
|
|
||
| def clean_string(self, string): | ||
| """Remove hex characters and new lines.""" |
There was a problem hiding this comment.
For my own education, can you point out in the test files some examples of hex characters that are needing to be removed? I'm not sure I understand what is meant by that.
There was a problem hiding this comment.
Inside the sparkle test you can see strange charecters being used. Maybe they arent HEX, i can change the name if needed.
1111111=C2=A0/ 22222=C2=A0/ 33333 is in the sparkle test where its really meant to be 1111111 / 22222 / 33333
There was a problem hiding this comment.
Hm. I thought those were supposed to be removed by our use of quopri in the base HTML parser class. Are you seeing cases where these characters are still present in the processed HTML being passed through to the Sparkle parser?
There was a problem hiding this comment.
Yeah, I think I saw this in another parser but unsure which as it was a while ago.
| ) | ||
| elif "description of work" in td_elements[0].text.lower(): | ||
| self.set_all_tickets(data, "summary", self.clean_string(td_elements[1].text)) | ||
| self.set_all_tickets(data, "status", Status.COMPLETED) |
There was a problem hiding this comment.
maybe CONFIRMED? COMPLETED means that the maintenance is over...
New provider Sparkle has been added.
Because of the structure of the Sparkle notifications, a new feature has been impelemented to process multiple notifications from a single email. This meant modifications to the
CpmbinedProcessorand the processor testing.