Require EPP validation for complex OT&E stats#3107
Conversation
CydeWeys
left a comment
CydeWeys
left a comment
There was a problem hiding this comment.
I don't understand what problem this is solving or how this situation could come to be.
@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
gbrodman
left a comment
gbrodman
left a comment
There was a problem hiding this comment.
When registering OT&E stats we inspect history entries for a registrar. Many of the OT&E checks require inspecting the EPP xml bytes to figure out if the check was hit, e.g. DOMAIN_CREATES_START_DATE_SUNRISE.
There should always be XML bytes but if they were missing for some reason, previously this would return true for every single type of check that inspects XML bytes if the XML bytes were absent.
@gbrodman made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
CydeWeys
left a comment
CydeWeys
left a comment
There was a problem hiding this comment.
But why would they be missing? Under what circumstances does that happen? Are you writing this PR because this is an issue we faced? (And if so, what caused it?)
@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
CydeWeys
left a comment
CydeWeys
left a comment
There was a problem hiding this comment.
I agree that the new code is safer, and I'm not opposed in principle, I just feel like I'm still missing information?
@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
gbrodman
left a comment
gbrodman
left a comment
There was a problem hiding this comment.
They shouldn't be. As far as I'm aware they haven't been. But this is incorrect behavior nonetheless. This was one of the issues flagged, in section B2.
@gbrodman made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
CydeWeys
left a comment
CydeWeys
left a comment
There was a problem hiding this comment.
Cool. Can you just update the commit description to make it clear that isn't responding to any known data/issue in our system, but that the new code is more correct nonetheless.
@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
CydeWeys
left a comment
CydeWeys
left a comment
There was a problem hiding this comment.
@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
Previously, if there were no XML bytes somehow, it'd increment a bunch of counts for requirements that shouldn't be incremented. We should only increment these values if we have the data to do so. Note: this doesn't happen and shouldn't happen, so there's no issues currently occurring, but this code is more correct anyway.
2 participants
Previously, if there were no XML bytes somehow, it'd increment a bunch of counts for requirements that shouldn't be incremented. We should only increment these values if we have the data to do so.
Note: this doesn't happen and shouldn't happen, so there's no issues
currently occurring, but this code is more correct anyway.
This change is