Added filing validators class, rearranged code #530
Conversation
jcadam14
requested review from
hkeeler,
guffee23,
lchen-2101 and
nargis-sultani
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class CheckPeriodFilingExists(ActionValidator): | ||
| def __init__(self): | ||
| super().__init__("check_period_filing_exists") |
There was a problem hiding this comment.
maybe this one should be something like period filing not started; at the moment it has the same naming convention as period exists above, but it's the opposite logic as what's above.
There was a problem hiding this comment.
The point is to check if a filing already exists for that period and if it does fail. So the two checks when starting a filing are:
- does the period exist, if not fail
- does a filing already exist for that period, if so fail
I had to name it something different from the sign "check_filing_exists" which has the reverse logic, it fails to sign if a the filing for that period + LEI doesn't exist yet. But since all the validators get put into one registry, I needed a different name (key) in the registry map. Naming is definitely something an issue we could run up against and is something I thought that maybe sticking all the validator subs into one file might help, but then that could potentially get very large. But I also though of moving your signing "check_filing_exists" into this filing_validators.py and naming it like "check_filing_not_exists" (since that's what it is doing) and have this one be "check_filing_exists".
There was a problem hiding this comment.
yeap, I like the check_filing_exits, and check_filing_not_exists into filing_validator; just wanting to have the name to be what the function actually does.
| class CheckFilingExists(ActionValidator): | ||
| def __init__(self): | ||
| super().__init__("check_filing_exists") | ||
|
|
||
| def __call__(self, filing: FilingDAO, period_code: str, lei: str, **kwargs): | ||
| if filing: | ||
| return f"Filing already exists for Filing Period {period_code} and LEI {lei}" | ||
|
|
||
|
|
||
| class CheckFilingNotExists(ActionValidator): | ||
| def __init__(self): | ||
| super().__init__("check_filing_not_exists") | ||
|
|
||
| def __call__(self, filing: FilingDAO, lei: str, period_code: str, **kwargs): | ||
| if not filing: | ||
| return f"There is no Filing for LEI {lei} in period {period_code}, unable to sign a non-existent Filing." |
There was a problem hiding this comment.
These 2's name and logic are flipped compared to the voluntary filer, and contact info checks. My initial thought of these function names are passing conditions instead of failing conditions. We probably should rename all the checks to be more explicit then, like FailIfFilingExists and FailIfFilingNotExists; or maybe InvalidIfFilingExists, etc. if we want to make them pass conditions: ValidIfFilingExists, etc.; and we can make the other validators names explicit as well, i.e. ValidVoluntaryFilerStatus, or InvalidVoluntaryFilerStatus, etc.
| class ValidFilingNotExists(ActionValidator): | ||
| def __init__(self): | ||
| super().__init__("valid_filing_exists") |
There was a problem hiding this comment.
name attribute and class name not matching
Closes #529