-
Notifications
You must be signed in to change notification settings - Fork 0
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
Ran linting, updated to have a single /cleanup endpoint #39
Ran linting, updated to have a single /cleanup endpoint #39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great. Just a couple nitpicks in there.
except Exception as e: | ||
raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to catch it if you you're just going to re-raise it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to instead raise the following like we do with our other exceptions, gracefully handling this condition:
raise RegTechHttpException(
status_code=HTTPStatus.INTERNAL_SERVER_ERROR,
name="Delete Filing Server Error",
detail=f"Server error while trying to delete filing for LEI {lei}.",
) from e
if not delete_domains: | ||
logger.error(f"Domain(s) for LEI {lei} not deleted.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing there are some test scenarios where there won't be email domains...like if you're testing that that user cannot get past that step in the process without those domains. :) It doesn't seem like an error
in that case. Maybe a warn
?
delete_sbl_type = repo.delete_sbl_type_by_lei(session, lei) | ||
if not delete_sbl_type: | ||
logger.error(f"sbl type(s) for LEI {lei} not deleted.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly looking at the repo code for deleting domains and types, I don't know how this code will even be hit. If there isn't a domain or type, the filter will just not do anything, but the return object for delete_domains and delete_sbl_type will always exist. So that logger code wouldn't be hit. If an exception is hit while trying to delete either, it would be thrown up in the response and the logger code wouldn't be hit.
I'm going to write a story for Nargis to rework the code for the repo or this section to properly check if the domains and types exist and log a warning because yeah I think these loggers are unreachable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #40 if you want to check if that properly captures the issue here.
Closes #35
Added the following paths:
Ran linting, so some of the changes are black/ruff adjustments