F4KRP-125 Get polyline from stops

[eddywang4340]

JIRA ticket link

Ticket Name

Implementation description

• Added ends_at_warehouse boolean field to the Routes model as required in the ticket
• Added routes_utils.py to handle fetching the polyline given a list of stops
• Added a test file for routes_utils.py (see below on how you can test)

Steps to test

What should reviewers focus on?

Checklist

• My PR name is descriptive and in imperative tense
• My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
• I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

[claude]

PR Review: F4KRP-125 Get polyline from stops

Summary

This PR adds functionality to fetch route polylines from Google Maps Routes API. The implementation includes a new utility function, database migration for ends_at_warehouse field, and test coverage.

---

🔴 Critical Issues

1. Migration Chain Conflict (backend/python/migrations/versions/eb010a6ed5ad_added_ends_at_warehouse_to_route_model.py:14)

The migration has down_revision = 'b1c2d3e4f5a6', but there's another migration add_polyline_fields_to_routes_table.py with down_revision = 'afbf8fe1a1a3'. This creates a divergent migration tree. You need to resolve this by:

• Determining the correct linear migration chain
• Updating the down_revision to point to the most recent migration
• Potentially squashing or reordering migrations

2. Unused Import in routes_utils.py (backend/python/app/utilities/routes_utils.py:3)

googlemaps is imported but never used. The code uses google.maps.routing_v2 instead. Remove this unused import:

import googlemaps  # <- Remove this line

3. Base Model Change Lacks Justification (backend/python/app/models/base.py:32-33)

The addition of an empty __init_subclass__ method appears unnecessary and has no docstring or comment explaining its purpose. This change seems unrelated to the PR's scope. If it's required, please:

• Add a comment explaining why it's needed
• Or remove it if it's not actually necessary

---

⚠️ Important Issues

4. Missing Newline at End of File (backend/python/app/utilities/routes_utils.py:106)

Python files should end with a newline character. Add one at line 106.

5. Test Quality Concerns (backend/python/tests/test_route_utils.py)

• No mocking: The test makes actual API calls to Google Maps, which:

  • Requires a valid API key in CI/CD
  • Incurs costs
  • Is slower and less reliable than unit tests
  • Can fail due to network issues

• Incomplete assertions: The test only checks that the polyline is a non-empty string and distance > 0. Consider asserting:

  • Expected polyline format/structure
  • Reasonable distance bounds for the given coordinates

• Limited coverage: Only tests the ends_at_warehouse=True case. Add tests for:

  • ends_at_warehouse=False
  • Single location
  • Multiple locations (2, 3, many)
  • Error cases (empty locations, invalid coordinates, API failures)

Recommendation: Mock the Google Maps API client using pytest-mock or unittest.mock to make tests faster, cheaper, and more reliable.

6. Error Handling Could Be More Specific (backend/python/app/utilities/routes_utils.py:100-105)

The generic Exception catch at line 105 is too broad. Consider handling specific exceptions separately:

• Authentication errors (invalid API key)
• Rate limit errors
• Invalid request errors
  Each should return appropriate HTTP status codes.

---

💡 Suggestions & Best Practices

7. Type Hints for Better Type Safety (backend/python/app/utilities/routes_utils.py:38)

Consider using more specific type hints:

from google.maps.routing_v2.types import ComputeRoutesResponse
# Then in function signature or assertions

8. Magic Numbers (backend/python/app/utilities/routes_utils.py:96)

The conversion factor 1000.0 should be a named constant:

METERS_PER_KILOMETER = 1000.0
distance_km = route.distance_meters / METERS_PER_KILOMETER

9. Docstring Could Be More Detailed

The function docstring is good but could benefit from:

• Example usage
• Note about API key requirement
• Link to Google Maps Routes API documentation

10. Consider Caching

For routes that don't change frequently, consider caching polyline results to reduce API calls and costs. This could be a future enhancement.

11. Migration Server Default (backend/python/migrations/versions/eb010a6ed5ad_added_ends_at_warehouse_to_route_model.py:21)

Good use of server_default='false' to handle existing rows.

---

✅ Positive Observations

• Good error messages: HTTPException details are clear and helpful
• Proper async/await usage: Correctly uses async client
• Field mask optimization: Only requesting needed fields (polyline.encodedPolyline,routes.distanceMeters)
• Well-structured function: Clear separation of concerns and logical flow
• Good routing configuration: Using TRAFFIC_AWARE preference is appropriate for real-world routing

---

📋 Checklist Review

From your PR description:

• PR name should be in imperative tense - "Get polyline from stops" ✅
• Commits should be atomic - Review needed: Some commit messages like "holy shit it worked" should be more professional
• Implementation description is empty - Please fill this in
• "What should reviewers focus on?" is empty - Please fill this in

---

🎯 Action Items

Must Fix Before Merge:

1. Resolve migration chain conflict
2. Remove unused googlemaps import
3. Explain or remove __init_subclass__ change in base.py
4. Add newline at end of routes_utils.py
5. Add test mocking to avoid real API calls
6. Improve test coverage (error cases, both ends_at_warehouse values)
7. Fill in PR description sections

Nice to Have:
8. Extract magic number to constant
9. Improve error handling specificity
10. Consider commit message improvements for better history

---

Great work on implementing the core functionality! The routing logic looks solid. Once the critical issues are addressed, this will be ready to merge. 🚀

Co-Authored-By: Claude Sonnet 4.5 [email protected]

[eddywang4340]

For context, these lines were added to fix this MyPy error:
Unexpected keyword argument "table" for "__init_subclass__" of "object"Mypy

[eddywang4340]

PR Review: F4KRP-125 Get polyline from stops

Summary

This PR adds functionality to fetch route polylines from Google Maps Routes API. The implementation includes a new utility function, database migration for ends_at_warehouse field, and test coverage.

🔴 Critical Issues

1. Migration Chain Conflict (backend/python/migrations/versions/eb010a6ed5ad_added_ends_at_warehouse_to_route_model.py:14)

The migration has down_revision = 'b1c2d3e4f5a6', but there's another migration add_polyline_fields_to_routes_table.py with down_revision = 'afbf8fe1a1a3'. This creates a divergent migration tree. You need to resolve this by:

• Determining the correct linear migration chain
• Updating the down_revision to point to the most recent migration
• Potentially squashing or reordering migrations

2. Unused Import in routes_utils.py (backend/python/app/utilities/routes_utils.py:3)

googlemaps is imported but never used. The code uses google.maps.routing_v2 instead. Remove this unused import:

import googlemaps  # <- Remove this line

3. Base Model Change Lacks Justification (backend/python/app/models/base.py:32-33)

The addition of an empty __init_subclass__ method appears unnecessary and has no docstring or comment explaining its purpose. This change seems unrelated to the PR's scope. If it's required, please:

• Add a comment explaining why it's needed
• Or remove it if it's not actually necessary

⚠️ Important Issues

4. Missing Newline at End of File (backend/python/app/utilities/routes_utils.py:106)

Python files should end with a newline character. Add one at line 106.

5. Test Quality Concerns (backend/python/tests/test_route_utils.py)

• No mocking: The test makes actual API calls to Google Maps, which:

  • Requires a valid API key in CI/CD
  • Incurs costs
  • Is slower and less reliable than unit tests
  • Can fail due to network issues

• Incomplete assertions: The test only checks that the polyline is a non-empty string and distance > 0. Consider asserting:

  • Expected polyline format/structure
  • Reasonable distance bounds for the given coordinates

• Limited coverage: Only tests the ends_at_warehouse=True case. Add tests for:

  • ends_at_warehouse=False
  • Single location
  • Multiple locations (2, 3, many)
  • Error cases (empty locations, invalid coordinates, API failures)

Recommendation: Mock the Google Maps API client using pytest-mock or unittest.mock to make tests faster, cheaper, and more reliable.

6. Error Handling Could Be More Specific (backend/python/app/utilities/routes_utils.py:100-105)

The generic Exception catch at line 105 is too broad. Consider handling specific exceptions separately:

• Authentication errors (invalid API key)
• Rate limit errors
• Invalid request errors
  Each should return appropriate HTTP status codes.

💡 Suggestions & Best Practices

7. Type Hints for Better Type Safety (backend/python/app/utilities/routes_utils.py:38)

Consider using more specific type hints:

from google.maps.routing_v2.types import ComputeRoutesResponse
# Then in function signature or assertions

8. Magic Numbers (backend/python/app/utilities/routes_utils.py:96)

The conversion factor 1000.0 should be a named constant:

METERS_PER_KILOMETER = 1000.0
distance_km = route.distance_meters / METERS_PER_KILOMETER

9. Docstring Could Be More Detailed

The function docstring is good but could benefit from:

• Example usage
• Note about API key requirement
• Link to Google Maps Routes API documentation

10. Consider Caching

For routes that don't change frequently, consider caching polyline results to reduce API calls and costs. This could be a future enhancement.

11. Migration Server Default (backend/python/migrations/versions/eb010a6ed5ad_added_ends_at_warehouse_to_route_model.py:21)

Good use of server_default='false' to handle existing rows.

✅ Positive Observations

• Good error messages: HTTPException details are clear and helpful
• Proper async/await usage: Correctly uses async client
• Field mask optimization: Only requesting needed fields (polyline.encodedPolyline,routes.distanceMeters)
• Well-structured function: Clear separation of concerns and logical flow
• Good routing configuration: Using TRAFFIC_AWARE preference is appropriate for real-world routing

📋 Checklist Review

From your PR description:

• PR name should be in imperative tense - "Get polyline from stops" ✅
• Commits should be atomic - Review needed: Some commit messages like "holy shit it worked" should be more professional
• Implementation description is empty - Please fill this in
• "What should reviewers focus on?" is empty - Please fill this in

🎯 Action Items

Must Fix Before Merge:

1. Resolve migration chain conflict
2. Remove unused googlemaps import
3. Explain or remove __init_subclass__ change in base.py
4. Add newline at end of routes_utils.py
5. Add test mocking to avoid real API calls
6. Improve test coverage (error cases, both ends_at_warehouse values)
7. Fill in PR description sections

Nice to Have: 8. Extract magic number to constant 9. Improve error handling specificity 10. Consider commit message improvements for better history

Great work on implementing the core functionality! The routing logic looks solid. Once the critical issues are addressed, this will be ready to merge. 🚀

Co-Authored-By: Claude Sonnet 4.5 [email protected]

Addressed Claude PR comments:

1. Migration Conflicts

• These conflicts should be ok now. I checked that the migration files follow a single chain (matching current revision and down revision)

2. Unused import

• Already deleted all unused imports with format

3. Base model changes lack justification

• Put a comment in the PR (TLDR fixes MyPy issue)

4. Missing newline at the end of file

• Not super important imo

5. Test quality concerns

• Removed the test file completely

6. Error handling could be more specific

• Error messages should be clear (I think it should be sufficient but let me know if we should cover anymore cases)

7. Type hints for better safety

• Don't think we should do this since then we will have multiple imports when we could have just imported the entire module

8. Magic number

• Meters to kilometers is a globally known constant (outside of code) so also don't think we need to do this (AKA we would never change this constant unless we wanted to change the units completely)

• Other comments are just suggestions so it should be fine for this PR

[ludavidca]

Great Job Eddy! This is a great PR. I looked into the mypy issue you were facing earlier and was able to get it resolved (not sure what was causing it in the first place). Great handling of edge cases and feel free to merge it in!