Skip to content

District and Location Comments - #427

Closed
nofurtherinformation wants to merge 90 commits into
mainfrom
district-level-metadata
Closed

District and Location Comments#427
nofurtherinformation wants to merge 90 commits into
mainfrom
district-level-metadata

Conversation

@nofurtherinformation

@nofurtherinformation nofurtherinformation commented Aug 4, 2025

Copy link
Copy Markdown
Collaborator

Adds metadata fields for per-district comments and location based comments

Description

  • Comment on specific districts
  • Cleans up public view

Reviewers

Checklist

  • Update FE and BE schemas/types
  • UI to add comments
  • UI to view comments

@nofurtherinformation nofurtherinformation changed the title District level metadata District and Location Comments Aug 5, 2025

@raphaellaude raphaellaude left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

schema thoughts! PS. any reason you're merging into public ids rather than main?

Comment thread backend/app/models.py Outdated
description: str | None = None
event_id: str | None = None
draft_status: DocumentDraftStatus | None = DocumentDraftStatus.scratch
comments: list[dict] | None = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

H: let's please please please not expand the scope of this json blob

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a good opportunity to refactor. Things like tags and comments definitely make sense as their own tables IMO

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I know this is not a great pattern and pretty inelegant, but it would strictly work as is.

I am with you longer term, but this feature is a moderately strong nice to have for this week. I don't think total capacity is there to do the refactor this week.

Do you have any concerns to this addition to the metadata?

@nofurtherinformation

Copy link
Copy Markdown
Collaborator Author

any reason you're merging into public ids rather than main?

2 things -- I wanted to avoid alembic shenanigans swapping between branches when possible, and the way metadata loads to the FE changes slightly with Public IDs to be on the map document instead of in user maps so I wanted to follow that pattern

@nofurtherinformation

nofurtherinformation commented Aug 5, 2025

Copy link
Copy Markdown
Collaborator Author

2 things -- I wanted to avoid alembic shenanigans swapping between branches when possible...

P.s. I figured this would merge into main after public IDs so we can just swap the merge target!

Base automatically changed from save-share-public-ids to main August 6, 2025 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants