Backend: Add public_trips models and migration#7723
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| sa.Column("description", sa.String(), nullable=False), | ||
| sa.Column( | ||
| "status", | ||
| sa.Enum("active", "found_host", "cancelled", name="publictripstatus"), |
There was a problem hiding this comment.
Not sure I like the "found host" status but naming is hard.
| sa.Column( | ||
| "outcome", | ||
| sa.Enum( | ||
| "found_host_via_public_trip", | ||
| "found_host_other", | ||
| "trip_cancelled", | ||
| "no_responses", | ||
| name="publictripoutcome", | ||
| ), |
There was a problem hiding this comment.
The idea is after the date of the public_trip we will shop a dropdown or some select to mark the outcome. And maybe send a notification "let us know how it went?" or something for metrics.
| ), | ||
| nullable=True, | ||
| ), | ||
| sa.Column("profile_click_count", sa.BigInteger(), server_default=sa.text("0"), nullable=False), |
There was a problem hiding this comment.
People who clicked on the profile via the public_trip. This is the best measurable metric I could think of for metrics. But maybe we also want to show a view count on the UI? In any case good data for us?
| nullable=True, | ||
| ), | ||
| sa.Column("profile_click_count", sa.BigInteger(), server_default=sa.text("0"), nullable=False), | ||
| sa.Column("first_response_at", sa.DateTime(timezone=True), nullable=True), |
There was a problem hiding this comment.
Maybe overkill as we could do a join table but would be good to judge the success of the feature.
app/backend/src/couchers/migrations/versions/981fb62b20ce_add_public_trips.py
Outdated
Show resolved
Hide resolved
| ) | ||
|
|
||
| # Add public_trip_id to host_requests | ||
| op.add_column("host_requests", sa.Column("public_trip_id", sa.BigInteger(), nullable=True)) |
There was a problem hiding this comment.
We'll need a way to determine if a host_request came from a public_trip.
How exactly will this work?
What about places without city-level communities? |
|
Claude finished @nabramow's task in 2m 43s —— View job Migration Safety Review
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: MEDIUM RISK Files Reviewed
Findings
Previously Flagged Review Items (from other reviewers, still unaddressed)
SummaryThe migration itself is structurally safe for existing production data — it creates a new table and adds a nullable column, so no existing rows are affected. The blocking issue is the |
app/backend/src/couchers/migrations/versions/981fb62b20ce_add_public_trips.py
Outdated
Show resolved
Hide resolved
|
Claude finished @nabramow's task in 3m 28s —— View job
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: MEDIUM RISK Files Reviewed
Findings
Previously Flagged Review Items (from other reviewers, still unaddressed)
SummaryThe migration itself is structurally safe for existing production data — it creates a new table and adds a nullable column, so no existing rows are affected. The blocking issue is the |
app/backend/src/couchers/migrations/versions/981fb62b20ce_add_public_trips.py
Outdated
Show resolved
Hide resolved
|
Claude finished @nabramow's task in 2m 13s —— View job
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: MEDIUM RISK Files Reviewed
Findings
Previously Flagged Review Items (from other reviewers, still unaddressed)
SummaryThe migration itself is structurally safe for existing production data — it creates a new table and adds a nullable column, so no existing rows are affected. The blocking issue is the |
app/backend/src/couchers/migrations/versions/981fb62b20ce_add_public_trips.py
Outdated
Show resolved
Hide resolved
|
Claude finished @nabramow's task in 1m 32s —— View job
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: MEDIUM RISK Files Reviewed
Findings
SummaryThe migration itself is structurally safe for existing production data — it creates a new table (no existing rows affected) and adds a nullable column to |
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision = "981fb62b20ce" | ||
| down_revision = "e9190b051324" |
There was a problem hiding this comment.
Migration chain conflict (HIGH): This down_revision points to e9190b051324, but migration 0137_add_value_and_source_to_eventlog.py (revision 8eab3434914d) already exists on develop with the same down_revision, and 0138_add_admin_actions_and_tags.py (revision 0138) depends on it. This creates a branch in the migration chain.
The current develop head is 0138. This migration's down_revision must be updated to maintain a linear migration history.
| down_revision = "e9190b051324" | |
| down_revision = "0138" |
| <<<<<<< na/backend/public-trips-table | ||
| from couchers.models.public_trips import PublicTrip | ||
| ======= | ||
| from couchers.models.admin import UserAdminTag | ||
| >>>>>>> develop |
There was a problem hiding this comment.
Unresolved merge conflict: Both imports are needed. Resolve by keeping both:
| <<<<<<< na/backend/public-trips-table | |
| from couchers.models.public_trips import PublicTrip | |
| ======= | |
| from couchers.models.admin import UserAdminTag | |
| >>>>>>> develop | |
| from couchers.models.public_trips import PublicTrip | |
| from couchers.models.admin import UserAdminTag |
|
Claude finished @nabramow's task in 2m 16s —— View job
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: MEDIUM RISK Files Reviewed
Findings
SummaryThe migration itself is structurally safe for existing production data — it creates a new table (no existing rows affected) and adds a nullable column to |
app/backend/src/couchers/migrations/versions/981fb62b20ce_add_public_trips.py
Outdated
Show resolved
Hide resolved
…public_trips.py Co-authored-by: couchersbot[bot] <179870305+couchersbot[bot]@users.noreply.github.com>
|
Claude finished @nabramow's task in 2m 15s —— View job
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: LOW RISK Files Reviewed
Findings
SummaryThis migration is safe for production data. It creates a brand-new table ( |
| """Add public trips | ||
|
|
||
| Revision ID: 981fb62b20ce | ||
| Revises: e9190b051324 |
There was a problem hiding this comment.
Docstring mismatch (LOW): This says Revises: e9190b051324 but the actual down_revision on line 14 is "0138". Cosmetic only — Alembic uses the variable — but should be corrected for clarity.
| Revises: e9190b051324 | |
| Revises: 0138 |
app/backend/src/couchers/migrations/versions/981fb62b20ce_add_public_trips.py
Outdated
Show resolved
Hide resolved
…public_trips.py Co-authored-by: couchersbot[bot] <179870305+couchersbot[bot]@users.noreply.github.com>
|
Claude finished @nabramow's task in 2m 2s —— View job
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: LOW RISK Files Reviewed
Findings
SummaryThis migration is safe for production data. It creates a brand-new table ( |
app/backend/src/couchers/migrations/versions/981fb62b20ce_add_public_trips.py
Outdated
Show resolved
Hide resolved
| from alembic import op | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision = "0139" |
There was a problem hiding this comment.
Non-ordinal filename (LOW): The project convention (enforced by env.py:_next_ordinal()) is ordinal filenames like 0139_add_public_trips.py. The current filename 981fb62b20ce_add_public_trips.py won't be found by the ordinal scanner — the next auto-generated migration would also get 0139, creating a conflict. Consider renaming this file to 0139_add_public_trips.py.
There was a problem hiding this comment.
Claude finished @nabramow's task in 1m 30s —— View job
Renaming migration to ordinal convention
- Read current migration file
- Rename file from
981fb62b20ce_add_public_trips.pyto0139_add_public_trips.py - Fix docstring (
Revision ID: 981fb62b20ce→Revision ID: 0139) - Commit and push
Done. Renamed the migration file to 0139_add_public_trips.py and updated the docstring Revision ID to match. The revision variable and Revises line were already correct.
Commit: c1d34cb
…public_trips.py Co-authored-by: couchersbot[bot] <179870305+couchersbot[bot]@users.noreply.github.com>
|
Claude finished @nabramow's task in 1m 37s —— View job
Migration ReviewOne of our most common issues is migrations that don't work with existing data in production. This is very hard to catch with CI, so we have this bot to check for potential problems. Overall Risk: LOW RISK Files Reviewed
Findings
SummaryThis migration is safe for production data. It creates a brand-new table ( |
| from alembic import op | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision = "0139" |
There was a problem hiding this comment.
Non-ordinal filename (LOW): The project convention (enforced by env.py:_next_ordinal()) is ordinal filenames like 0139_add_public_trips.py. The current filename 981fb62b20ce_add_public_trips.py won't be matched by the ordinal scanner — the regex ^(\d+)_ sees 981 but the next char is f, not _, so it doesn't match. The next auto-generated migration would also get 0139, creating a revision ID conflict. Consider renaming this file to 0139_add_public_trips.py.
Also, the docstring on line 3 says Revision ID: 981fb62b20ce but the actual revision here is "0139" — should be updated for consistency.
Rename 981fb62b20ce_add_public_trips.py to 0139_add_public_trips.py and fix Revision ID in docstring to match the actual revision variable. Co-authored-by: Nicole <nabramow@users.noreply.github.com>
|
@aapeliv I am ignoring the migration review failure in this case as it seems to fail if Claude made the change via CouchersBot. When I check the migration itself it works fine. |
📝 Release NotesThis PR does not need to be included in release notes. Reason: This PR only adds backend database models and a migration for a 'public trips' feature. No frontend implementation, no API endpoints, and no user-facing functionality is included. The PR description explicitly states 'This is only the backend models and migration' and describes a detailed plan for how the feature will work in the future. According to the guidelines, backend infrastructure changes should only be included if they are 'major technical infrastructure' or 'particularly important technical work.' This is preparatory work for a future feature, not a complete implementation. Users cannot see or use anything from this PR. 🤖 Bot Debug InformationModel: |
Describe briefly what this PR is doing and why
Closes #7718
This is only the backend models and migration. But here is an overview of the plan so you can see the vision and discuss.
Public trips will be a way for travelers to publicly announce they're visiting a city and looking for a host, so potential hosts in that community can discover and reach out to them.
Overview of how it will work:
For Travelers:
For Potential Hosts:
Design Decisions:
Where It Appears
Please give clear steps for how the reviewer can best test this PR
Please include any necessary dev environment, .env, etc. adjustments.
Backend checklist
make formatinapp/backenddevelopif necessary for linear migration historyOther
Untick the following if you'd prefer that maintainers don't push commits/merge your branch.