Skip to content

Adds ability to transfer data from temporary crashes when they are deleted via modal #1968

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.

Sign up for GitHub

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

Open
mateoclarke wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Adds ability to transfer data from temporary crashes when they are deleted via modal #1968

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0b25ad5
adds modal to transfer and delete temp crash record
mateoclarke Feb 19, 2026
c90ef63
transfers victim photo data
mateoclarke Feb 24, 2026
f95c19e
Merge branch 'main' into mateo/26663_transfer_temp_data
mateoclarke Feb 24, 2026
2f0a64b
fix merge conflict resolution
mateoclarke Feb 24, 2026
a235d9b
refactor CORS headers into reusable array
mateoclarke Feb 24, 2026
7666e47
no updated crash diagram data to transfer
mateoclarke Feb 25, 2026
6e00869
Also transfer updated lat/lons
mateoclarke Feb 25, 2026
76d0b1e
refactor `DeleteTemporaryCrashModal`
mateoclarke Feb 25, 2026
4b5ebe8
rename `CrashSearchResult`
mateoclarke Feb 25, 2026
5322cf1
continue rename `CrashSearchResult`
mateoclarke Feb 25, 2026
5c46b5f
continues continue rename `CrashSearchResult`
mateoclarke Feb 25, 2026
cb8afe2
simplify edited field labels dedupe
mateoclarke Feb 25, 2026
9b9a6cd
comments update
mateoclarke Feb 25, 2026
0ae17fc
make sure only non-null edits transfer
mateoclarke Feb 25, 2026
6ff5883
refactor not null check
mateoclarke Feb 25, 2026
68f1950
reduce the list of fields we transfer
mateoclarke Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 24 additions & 83 deletions api/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
_upsert_person_image,
_delete_person_image,
_get_person_image_url,
_transfer_person_image,
validate_file_size,
_get_crash_diagram_image_url,
_upsert_crash_diagram_image,
Expand All @@ -55,6 +56,7 @@
EDITOR_ROLE_NAME = "editor"
MAX_IMAGE_SIZE_MEGABYTES = 5
CORS_URL = "*"
CORS_HEADERS = ["Content-Type", "Authorization", "Access-Control-Allow-Origin", CORS_URL]
Copy link
Contributor Author

@mateoclarke mateoclarke Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't in scope, but when I was in this file, I just noticed how much this array was being repeated for every route and thought I could DRY it up a bit. Hope that's ok. I think there was one route that differed, so I assumed that's intentional and left it as it.

Copy link
Member

@frankhereford frankhereford Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you did this -- you cleaned up a lot of repetition, I think this is good.


app = Flask(__name__)

Expand Down Expand Up @@ -393,14 +395,7 @@ def healthcheck():


@app.route("/cr3/download/<record_locator>")
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
def download_crash_report(record_locator):
"""A valid access token is required to access this route"""
Expand All @@ -417,14 +412,7 @@ def download_crash_report(record_locator):


@app.route("/images/person/<int:person_id>", methods=["GET"])
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
# No role requirement - all authenticated users can GET
def get_person_image(person_id):
Expand All @@ -433,14 +421,7 @@ def get_person_image(person_id):


@app.route("/images/person/<int:person_id>", methods=["DELETE", "PUT"])
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
@requires_roles(ADMIN_ROLE_NAME, EDITOR_ROLE_NAME)
@validate_file_size(MAX_IMAGE_SIZE_MEGABYTES)
Expand All @@ -453,31 +434,27 @@ def modify_person_image(person_id):
return jsonify(message="Bad Request"), 400


@app.route("/images/crash_diagram/<record_locator>", methods=["GET"])
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
@app.route(
"/images/person/<int:source_person_id>/transfer/<int:target_person_id>",
methods=["POST"],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
def transfer_person_image(source_person_id, target_person_id):
Copy link
Member

@johnclary johnclary Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love to see a new API route come into being! What do you think about renaming this to copy_person_image? Transfer suggests to me that the original image is being moved, but we are leaving the original image in place.

Copy link
Member

@johnclary johnclary Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that's missing for this new route is a test or two. Would you mind adding test coverage?

"""Transfers a person image from one person record to another"""
return _transfer_person_image(source_person_id, target_person_id, s3)


@app.route("/images/crash_diagram/<record_locator>", methods=["GET"])
Copy link
Contributor Author

@mateoclarke mateoclarke Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the important part of the changes in this file.

@cross_origin(headers=CORS_HEADERS)
@requires_auth
# No role requirement - all authenticated users can GET
def get_crash_diagram_image(record_locator):
"""Retrieves a crash diagram image URL"""
return _get_crash_diagram_image_url(record_locator, s3)


@app.route("/images/crash_diagram/<record_locator>", methods=["DELETE", "PUT"])
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
@requires_roles(ADMIN_ROLE_NAME, EDITOR_ROLE_NAME)
@validate_file_size(MAX_IMAGE_SIZE_MEGABYTES)
Expand All @@ -491,28 +468,14 @@ def modify_crash_diagram_image(record_locator):


@app.route("/user/test")
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
def user_test():
return jsonify(message=dict(current_user))


@app.route("/user/list_users")
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
def user_list_users():
page = request.args.get("page")
Expand All @@ -530,14 +493,7 @@ def user_list_users():


@app.route("/user/get_user/<id>")
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
def user_get_user(id):
endpoint = f"https://{AUTH0_DOMAIN}/api/v2/users/" + id
Expand All @@ -547,8 +503,7 @@ def user_get_user(id):


@app.route("/user/create_user", methods=["POST"])
@cross_origin(headers=["Content-Type", "Authorization"])
@cross_origin(headers=["Access-Control-Allow-Origin", CORS_URL])
@cross_origin(headers=CORS_HEADERS)
@requires_auth
@requires_roles(ADMIN_ROLE_NAME)
def user_create_user():
Expand Down Expand Up @@ -583,14 +538,7 @@ def user_create_user():


@app.route("/user/update_user/<id>", methods=["PUT"])
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
@requires_roles(ADMIN_ROLE_NAME)
def user_update_user(id):
Expand All @@ -602,14 +550,7 @@ def user_update_user(id):


@app.route("/user/delete_user/<id>", methods=["DELETE"])
@cross_origin(
headers=[
"Content-Type",
"Authorization",
"Access-Control-Allow-Origin",
CORS_URL,
],
)
@cross_origin(headers=CORS_HEADERS)
@requires_auth
@requires_roles(ADMIN_ROLE_NAME)
def user_delete_user(id):
Expand Down
10 changes: 10 additions & 0 deletions api/utils/graphql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@
}
"""

GET_PERSON_IMAGE_METADATA_FULL = """
query GetPersonImageFull($person_id: Int!) {
people_by_pk(id: $person_id) {
image_s3_object_key
image_source
image_original_filename
}
}
"""


UPDATE_PERSON_IMAGE_METADATA = """
mutation UpdatePersonImage($person_id: Int!, $object: people_set_input!) {
Expand Down
51 changes: 51 additions & 0 deletions api/utils/images.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from utils.graphql import (
make_hasura_request,
GET_PERSON_IMAGE_METADATA,
GET_PERSON_IMAGE_METADATA_FULL,
UPDATE_PERSON_IMAGE_METADATA,
GET_CRASH_DIAGRAM_METADATA,
UPDATE_CRASH_DIAGRAM_METADATA,
Expand Down Expand Up @@ -284,6 +285,56 @@ def _handle_image_upload(person_id, file, s3):
return new_image_obj_key


def _transfer_person_image(source_person_id, target_person_id, s3):
Copy link
Contributor Author

@mateoclarke mateoclarke Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a little confused about the use of the underscore prefixes in this file, but I went with the flow and named this method similarly. My understanding is that the leading _ signals (but doesn't enforce) a private method. Meaning, this handler method should be internal to the module and not exported for use in other files. See this article for a longer explanation.

But we do import this method, along with a bunch of similar handler methods in server.py#32. I missed this detail in the initial code review, which probably would've been the appropriate time to discuss our use of this convention. But it's an interesting pythonic detail so worth noting now too.

Copy link
Member

@frankhereford frankhereford Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I have heard of this convention and recognize it when I see it. Thanks for pointing it out -- maybe a spin off issue for the backlog to polish it up?

Copy link
Member

@johnclary johnclary Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah thanks for going with the flow. I agree these are not private methods in conventional sense and would support removing the leading underscore.

"""Transfer an image from one person record to another.

Copies the S3 object to a new key for the target person and updates the
target person's image metadata in the database.
"""
source_obj_key, source_original_filename = _get_person_image_metadata(
source_person_id
)

if not source_obj_key:
abort(
404,
description=f"No image found for source person ID: {source_person_id}",
)

source_image_source = make_hasura_request(
query=GET_PERSON_IMAGE_METADATA_FULL,
Copy link
Member

@johnclary johnclary Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can safely update _get_person_image_metadata() to return image_source as well, and then you can remove this graphql API call entirely?

it's annoying that python doesn't have fancy js destructing so you'll have to update all the places that use _get_person_image_metadata()—seems worth it though.

variables={"person_id": source_person_id},
)["people_by_pk"]["image_source"]
Comment on lines +304 to +307
Copy link
Member

@frankhereford frankhereford Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a try/except guardrail in use around the S3 call, but not around these other API calling functions. Is this OK, should there be more exception handling?

I see we only have a pass in the HasuraAPIError so if there is a problem, our flask app is going to return a 200 if there is an upstream problem with the graphql endpoint. I think based on that, it seems reasonable to want to trap not getting back the data expected so a 500 with a description can be sent, instead of a potentially app-breaking empty message.

In particular, I'm talking about the API failing here, not the potential follow-on NoneType error.

As a distant, secondary thought, if you were to split the API call and the deep accessing of a value out of the returned dictionary -- splitting those two operations into two "lines" of code seems like a marginally better way to represent the two steps being taken.

Copy link
Member

@johnclary johnclary Mar 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's odd we don't have any handlers for the hasura api calls. I believe, Frank, the pass you're seeing in HasuraAPIError is required for the way you must construct custom classes that inherit from Exception. It will definitely throw an error if raised.

But yes you're totally right this will just short-circuit the app and flask will return a generic 500 AFAIK.

Feels like tech-debt work scoping outside of this work?

Copy link
Member

@chiaberry chiaberry Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to cosign Frank's comments here, I don't know enough to be able to assess how likely it could be for this to fail, but just wondering if we should protect it anyway.


ext = source_obj_key.rsplit(".", 1)[-1] if "." in source_obj_key else "jpg"
Copy link
Member

@johnclary johnclary Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it feels kinda odd to fallback to jpg here. we might want to just throw an error since we should never encounter source_obj_key without a file extension?

target_obj_key = f"{AWS_S3_PERSON_IMAGE_LOCATION}/{target_person_id}.{ext}"

try:
s3.copy_object(
Bucket=AWS_S3_BUCKET,
CopySource={"Bucket": AWS_S3_BUCKET, "Key": source_obj_key},
Key=target_obj_key,
)
except Exception as e:
current_app.logger.exception(str(e))
abort(500, description="Failed to copy image in S3")

make_hasura_request(
query=UPDATE_PERSON_IMAGE_METADATA,
variables={
"person_id": target_person_id,
"object": {
"image_s3_object_key": target_obj_key,
"image_source": source_image_source,
"image_original_filename": source_original_filename,
"updated_by": get_user_email(),
},
},
)
Comment on lines +322 to +333
Copy link
Member

@chiaberry chiaberry Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

along the lines of what Frank said above, but do we need to check the response at all from this request?

Copy link
Member

@johnclary johnclary Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make_hasura_request will throw if there's an HTTP error, so I think this falls into the same bucket of adding better error message handing to these calls which to me feels out of scope.


return jsonify(success=True), 200


def _delete_person_image(person_id, s3):
"""Delete an image from S3 and nullify its metadata in the db"""
image_obj_key, image_original_filename = _get_person_image_metadata(person_id)
Expand Down
2 changes: 1 addition & 1 deletion editor/app/crashes/[record_locator]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export default function CrashDetailsPage({
}
{
// show alert if crash is a temp record
crash.is_temp_record && <CrashIsTemporaryBanner crashId={crash.id} />
crash.is_temp_record && <CrashIsTemporaryBanner crash={crash} />
}
<Row>
<Col sm={12} md={6} lg={4} className="mb-3">
Expand Down
Loading