-
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
task/WG-182: adding additional logging for project uuid upon deletion #163
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.
LGTM! Tested locally and see the geoapi log as expected. Thanks!
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.
Looks good 👍 Left two comments to consider
geoapi/routes/projects.py
Outdated
else: | ||
abort(404, "Project not found") |
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.
good thought on adding a check related to the project not being found. but it can be dropped in this instance as the project_admin_or_creator_permissions
decorator has that logic in a check_access_and_get_project
method that it calls.
geoapi/routes/projects.py
Outdated
project = ProjectsService.get(db_session, project_id=projectId, user=u) | ||
# Check if the project exists and log the information including the UUID | ||
if project: | ||
logger.info("Delete project:{} with UUID:{} for user:{}".format( |
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.
instead of UUID
what about project_uuid
?
There are lots of uuids in the code for things like task, streetview, asset path etc. so maybe being more specific would improve things.
I got the somewhat related PR #161 where I did something different but I am going to alter that to match what we decide here and the rest of the code.
Seems like the rest of the log statements use project:
when referring to the int id 👍 so just what should we call the project uuid.
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 can change it to "Project UUID:" or "project_uuid:"
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.
LGTM 👍
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.
LGTM!
Overview:
Improve logging of delete to include UUID with project ID. See notes of the related WG-70 issue of why this is needed.
In https://github.com/TACC-cloud/geoapi
in geoapi/routes/projects.py
Related Jira tickets:
Summary of Changes:
Testing Steps:
'http://localhost:8000/projects/1/' \
-H X-JWT-Assertion-designsafe:$JWT
Delete project:1 with UUID:2b097db7-d370-40c5-a632-6ed441ca47a5 for user:tgrafft
UI:
Notes: