Adds fuel level tracking to trips - #281
Open
chaptergy wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughChangesFuel-Level Tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TripForm
participant TripRoute
participant Trip
participant TripList
TripForm->>TripForm: Calculate fuel consumption
TripForm->>TripRoute: Submit fuel levels
TripRoute->>Trip: Store parsed fuel levels
TripList->>Trip: Read fuel data
Trip->>TripList: Return consumption and percentages
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
tests/test_models.py (1)
700-708: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new serialization contract.
The test sets both fuel levels but does not assert
d['start_fuel_level']ord['end_fuel_level']. Add both assertions so a regression inTrip.to_dict()fails the test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models.py` around lines 700 - 708, The test_trip_to_dict method constructs a Trip object with start_fuel_level and end_fuel_level values but does not verify these values appear in the dictionary returned by the to_dict() call. Add assertions for both d['start_fuel_level'] and d['end_fuel_level'] to validate the serialization contract includes these new fuel level fields, ensuring the test catches any regression in the Trip.to_dict() implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models.py`:
- Around line 452-454: Resolve the Ruff E701 violations by expanding each inline
conditional return in the shown logic into a properly indented multi-line if
block with its return on a separate line. Apply the same formatting change to
the additional violations around the corresponding logic at lines 1406–1411,
without changing behavior.
- Around line 1403-1412: In the fuel_consumption_human_readable method, the two
percentage consumption branches (when consumption is negative and when positive)
currently use str(consumption) which can produce floating-point artifacts.
Update both percentage return statements to format consumption with fixed
precision matching the litre branch format (using .1f or similar format
specifier) instead of relying on str conversion.
- Around line 443-445: Update the Tessie battery-level condition in the
surrounding battery-level method to check whether self.tessie_battery_level is
not None rather than relying on truthiness, so a valid 0% value returns through
the Tessie path and non-None values remain rounded.
In `@app/routes/api.py`:
- Around line 2746-2747: Update the alias lists used by auto_suggest_mappings(),
specifically the end_fuel_level and start_fuel_level mappings, to remove
ambiguous generic aliases such as fuel, fuel level, and battery that do not
identify the trip side. Retain only side-specific aliases, or explicitly
document and implement the intended default mapping if those generic aliases
must remain.
- Around line 2970-2971: Update the Trip construction flow around
start_fuel_level and end_fuel_level to validate each parsed non-null value is
within the inclusive range 0–100 before creating the Trip. Preserve blank inputs
as None, and reject out-of-range values before model creation.
- Around line 1570-1571: Address both export sites in app/routes/api.py at lines
1570-1571 and 1897-1898 by either adding matching import/restore handlers for
the export_json and export_full_backup formats, including both fuel fields, or
explicitly documenting these formats as export-only and non-reimportable; leave
the existing CSV import handling at line 2964 unchanged.
In `@app/routes/trips.py`:
- Around line 147-148: Add server-side validation for fuel level bounds before
assigning the parsed values in both the new() and edit() functions. After
calling parse_decimal() for start_fuel_level and end_fuel_level, validate that
each parsed value is within the 0–100 range, rejecting or raising an error for
out-of-bounds values before the assignment to trip.start_fuel_level and
trip.end_fuel_level occurs. This prevents bypass of the client-side min/max
template constraints through direct POST requests.
- Around line 147-148: The new() route's Trip() constructor call (around lines
81-92) is missing the fuel level assignments that exist in the edit route at
lines 147-148. Add identical parsing and assignment logic for start_fuel_level
and end_fuel_level in the new() route by applying the same parse_decimal
conditional pattern used in the shown diff, ensuring new trips capture fuel
levels from the form submission instead of storing NULL values.
In `@app/templates/trips/form.html`:
- Around line 93-94: Remove the required attribute from the start_fuel_level
input in the trip form so existing trips with nullable or missing fuel data can
be saved. Keep the current value binding and numeric constraints unchanged.
- Around line 206-215: Update calculateFuelConsumption to validate start and end
with a finite-number check rather than truthiness, so zero fuel levels are
processed as valid values while invalid or missing inputs retain the default
display behavior.
In `@app/templates/trips/index.html`:
- Around line 120-127: Update the fuel display conditions in the trip template
to use explicit None checks: render fuel_consumption and its human-readable
value when not None, and render start_fuel_level/end_fuel_level values without
treating zero as missing, using “?” only for None values.
In `@app/templates/vehicles/view.html`:
- Around line 66-69: Update Vehicle.get_last_fuel_level() to return None when no
trip or charging session contains a recorded fuel level, while preserving the
existing value for recorded levels. In the vehicle view template, handle a None
result before formatting and render an explicit empty-state label instead of
displaying 0%.
In `@migrations/versions/cc6e159f098a_adds_fuel_level_to_trip.py`:
- Around line 20-29: The upgrade conditionally adds columns based on schema
inspection using inspector.get_table_names() and inspector.get_columns() checks,
which makes the migration non-deterministic and creates asymmetry with the
downgrade operation. Remove all the conditional inspection guards and the nested
if-checks for existing_cols_trip so the migration always attempts to add
start_fuel_level and end_fuel_level columns to the trips table unconditionally.
Ensure the corresponding downgrade operation mirrors this behavior by always
dropping both columns, making the upgrade and downgrade symmetric regardless of
the pre-existing schema state.
In `@tests/test_trips.py`:
- Around line 70-71: Update the fuel-level assertion in the Trip persistence
test to reference the defined Trip attribute end_fuel_level instead of the
nonexistent send_fuel_level, while preserving the expected value of 80.0.
---
Nitpick comments:
In `@tests/test_models.py`:
- Around line 700-708: The test_trip_to_dict method constructs a Trip object
with start_fuel_level and end_fuel_level values but does not verify these values
appear in the dictionary returned by the to_dict() call. Add assertions for both
d['start_fuel_level'] and d['end_fuel_level'] to validate the serialization
contract includes these new fuel level fields, ensuring the test catches any
regression in the Trip.to_dict() implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8026fad-00ec-4fa2-ba0c-c27cb56c754d
📒 Files selected for processing (10)
app/models.pyapp/routes/api.pyapp/routes/trips.pyapp/templates/trips/form.htmlapp/templates/trips/index.htmlapp/templates/vehicles/view.htmlmigrations/versions/cc6e159f098a_adds_fuel_level_to_trip.pytests/conftest.pytests/test_models.pytests/test_trips.py
Comment on lines
+443
to
+445
| # If Tessie is enabled, use Tessie battery level exclusively | ||
| if self.uses_tessie_battery() and self.tessie_battery_level: | ||
| return round(self.tessie_battery_level) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return a valid 0% Tessie battery level.
0 is a valid battery level. The truthiness check bypasses Tessie at 0% and can return an older trip or charging value instead. Check for is not None.
Proposed fix
- if self.uses_tessie_battery() and self.tessie_battery_level:
+ if self.uses_tessie_battery() and self.tessie_battery_level is not None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # If Tessie is enabled, use Tessie battery level exclusively | |
| if self.uses_tessie_battery() and self.tessie_battery_level: | |
| return round(self.tessie_battery_level) | |
| # If Tessie is enabled, use Tessie battery level exclusively | |
| if self.uses_tessie_battery() and self.tessie_battery_level is not None: | |
| return round(self.tessie_battery_level) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models.py` around lines 443 - 445, Update the Tessie battery-level
condition in the surrounding battery-level method to check whether
self.tessie_battery_level is not None rather than relying on truthiness, so a
valid 0% value returns through the Tessie path and non-None values remain
rounded.
Comment on lines
+452
to
+454
| if not last_trip and not last_charge: return 0 | ||
| if not last_trip: return last_charge.end_soc | ||
| if not last_charge: return last_trip.end_fuel_level |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported Ruff E701 violations.
Ruff reports multiple statements on one line at these locations. Split each conditional and return into separate lines.
Also applies to: 1406-1411
🧰 Tools
🪛 Ruff (0.16.0)
[error] 452-452: Multiple statements on one line (colon)
(E701)
[error] 453-453: Multiple statements on one line (colon)
(E701)
[error] 454-454: Multiple statements on one line (colon)
(E701)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models.py` around lines 452 - 454, Resolve the Ruff E701 violations by
expanding each inline conditional return in the shown logic into a properly
indented multi-line if block with its return on a separate line. Apply the same
formatting change to the additional violations around the corresponding logic at
lines 1406–1411, without changing behavior.
Source: Linters/SAST tools
Comment on lines
+1403
to
+1412
| def fuel_consumption_human_readable(self) -> str | None: | ||
| """Calculate trip fuel consumption with a plus sign instead of negative for negative fuel consumption""" | ||
| consumption = self.fuel_consumption | ||
| if consumption is None: return None | ||
| if self.vehicle.tank_capacity: | ||
| abs_consumption = self.vehicle.tank_capacity * consumption / 100 | ||
| if abs_consumption < 0: return "~+{:.1f} L".format(abs(abs_consumption)) | ||
| return "~{:.1f} L".format(abs_consumption) | ||
| if consumption < 0: return '+' + str(abs(consumption)) + ' %' | ||
| return str(consumption) + ' %' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Format percentage consumption to a fixed precision.
When tank_capacity is absent, str(consumption) can display floating-point artifacts such as 7.200000000000003 %. Format both percentage branches to the same precision as the litre branch.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 1406-1406: Multiple statements on one line (colon)
(E701)
[error] 1409-1409: Multiple statements on one line (colon)
(E701)
[error] 1411-1411: Multiple statements on one line (colon)
(E701)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models.py` around lines 1403 - 1412, In the
fuel_consumption_human_readable method, the two percentage consumption branches
(when consumption is negative and when positive) currently use str(consumption)
which can produce floating-point artifacts. Update both percentage return
statements to format consumption with fixed precision matching the litre branch
format (using .1f or similar format specifier) instead of relying on str
conversion.
Comment on lines
+1570
to
+1571
| 'start_fuel_level': trip.start_fuel_level, | ||
| 'end_fuel_level': trip.end_fuel_level, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: every restore/import path assigns both fields when recreating Trip records.
rg -n -C 15 \
"start_fuel_level|end_fuel_level|data\\.json|restore|import" \
app testsRepository: dannymcc/may
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the specific lines where fuel fields are added to exports
echo "=== Lines 1560-1580 (export_json context) ==="
sed -n '1560,1580p' app/routes/api.py
echo ""
echo "=== Lines 1887-1910 (full-backup context) ==="
sed -n '1887,1910p' app/routes/api.py
# Find restore/import functions that handle Trip records
echo ""
echo "=== Searching for Trip restore/import in api.py ==="
rg -n -A 20 "def.*restore|def.*import|Trip\(" app/routes/api.py | head -200Repository: dannymcc/may
Length of output: 10990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the full-backup restore/import handler
echo "=== Searching for full-backup or full_backup restore function ==="
rg -n "def.*full.*backup|restore_full_backup" app/routes/api.py
echo ""
echo "=== Searching for JSON restore/import logic ==="
rg -n "def.*restore|import.*json|from_dict" app/routes/api.py | head -50
echo ""
echo "=== Looking for any function that reads trips from JSON and recreates them ==="
rg -n -B 5 -A 15 "vehicle_data\[.*trips" app/routes/api.py | head -100Repository: dannymcc/may
Length of output: 2613
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
# Find the restore/import handler for full backup
echo "=== Searching for import_full_backup or restore function around export_full_backup ==="
rg -n -A 100 "def import_full_backup|def.*restore.*backup" app/routes/api.py | head -200
echo ""
echo "=== Searching for where trips are imported from JSON (look for trip creation) ==="
rg -n "Trip(" app/routes/api.py | grep -v "Trip\." | head -30Repository: dannymcc/may
Length of output: 498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the restore/import handler for full backup
echo "=== Searching for import_full_backup or restore function ==="
rg -n "def import_full_backup|def.*restore" app/routes/api.py | head -20
echo ""
echo "=== Searching for where Trip objects are created from JSON ==="
rg -n "Trip\(" app/routes/api.py | grep -v "Trip\." | head -40Repository: dannymcc/may
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all function definitions in api.py to find restore/import handlers
echo "=== All functions in api.py containing 'import' or 'backup' or 'restore' ==="
rg -n "^def " app/routes/api.py | grep -i "import\|backup\|restore"
echo ""
echo "=== Search for where Trip objects are instantiated ==="
rg -n "= Trip\(" app/routes/api.py
echo ""
echo "=== Search for 'trips' in JSON restoration context ==="
rg -n -B 5 -A 10 "for.*trip.*in.*data" app/routes/api.py | head -80Repository: dannymcc/may
Length of output: 570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find how trips are created - broader search
echo "=== Search for Trip creation/instantiation ==="
rg -n "Trip\s*\(" app/routes/api.py | head -40
echo ""
echo "=== Look for where backup JSON is restored or imported ==="
rg -n "full.*backup.*json|backup.*restore|import.*backup" app/routes/api.py -i
echo ""
echo "=== Check if there's a download/restore endpoint for the backup ==="
rg -n "download_backup|restore_backup|@.*backup" app/routes/api.py -i | head -20Repository: dannymcc/may
Length of output: 283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the context around line 2964 where Trip is created
echo "=== Trip creation context at line 2964 ==="
sed -n '2955,2980p' app/routes/api.py
echo ""
echo "=== Search for all `@bp.route` decorators in api.py to find download/restore endpoints ==="
rg -n "`@bp.route`|`@app.route`" app/routes/api.py | grep -i "backup\|download\|export"
echo ""
echo "=== Check if export_full_backup has any corresponding import route ==="
sed -n '1663,1750p' app/routes/api.pyRepository: dannymcc/may
Length of output: 5228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any import route for the full backup
echo "=== All `@bp.route` in api.py ==="
rg -n "`@bp.route`" app/routes/api.py
echo ""
echo "=== Search for any route that imports/restores from ZIP or JSON backup ==="
rg -n "backup|restore|zip|\.json" app/routes/api.py | grep "route\|def " | head -30
echo ""
echo "=== Look at the end of api.py to see if there's an import_full_backup function ==="
tail -100 app/routes/api.py | head -50Repository: dannymcc/may
Length of output: 4867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's ANY function that reads/imports the backup ZIP or data.json
echo "=== Search for any function that reads backup JSON or ZIP ==="
rg -n "data\.json|\.zip|zipfile|ZipFile" app/routes/api.py
echo ""
echo "=== Check get_import_fields for trips to see if fuel fields are included ==="
sed -n '2676,2750p' app/routes/api.py
echo ""
echo "=== Confirm that CSV import for trips includes the fuel field mappings ==="
rg -n -A 20 "trips.*=.*\[" app/routes/api.py | grep -A 20 "2676" | head -40Repository: dannymcc/may
Length of output: 6334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# The export_full_backup creates a ZIP file, but is there an import endpoint for it?
echo "=== Check if there are ANY import routes for backup ZIP files ==="
rg -n "import.*backup|restore.*backup|upload.*backup" app/routes/api.py -i
echo ""
echo "=== Check templates for any backup import/restore UI ==="
find app/templates -name "*.html" -exec grep -l "backup\|restore\|import" {} \;
echo ""
echo "=== Check for any JavaScript that handles backup restore ==="
find app -name "*.js" -exec grep -l "backup" {} \;
echo ""
echo "=== Verify the Trip reconstruction at line 2964 includes both fuel fields ==="
sed -n '2960,2978p' app/routes/api.pyRepository: dannymcc/may
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify no import endpoint exists for export_json or export_full_backup
echo "=== All export routes ==="
rg -n "`@bp.route`.*export" app/routes/api.py
echo ""
echo "=== All import routes ==="
rg -n "`@bp.route`.*import" app/routes/api.py
echo ""
echo "=== Check if there are any templates or scripts that load JSON backup ==="
find app -type f \( -name "*.html" -o -name "*.js" \) -exec grep -l "export.*json\|import.*json\|data\.json" {} \;
echo ""
echo "=== Verify csv_import_execute properly handles trips with fuel fields ==="
sed -n '3101,3200p' app/routes/api.pyRepository: dannymcc/may
Length of output: 3723
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check templates to see if there's any UI for importing JSON/backup
echo "=== Check settings.html for backup restore UI ==="
grep -n "backup\|restore\|import" app/templates/auth/settings.html | head -30
echo ""
echo "=== Check if there's any JavaScript handling for backup files in static/ ==="
find app/static -name "*.js" -exec grep -n "backup\|restore" {} + | head -20
echo ""
echo "=== Conclusive check: look for any mention of unzipping or reading backup ==="
rg -n "unzip|ZipFile.*read|extract.*backup|import.*backup" app/ -i
echo ""
echo "=== Final confirmation of csv_import_execute route behavior for trips ==="
grep -A 50 "def create_record" app/routes/api.py | grep -A 35 "elif data_type == 'trips'"Repository: dannymcc/may
Length of output: 1913
Add restore endpoints for export_json and export_full_backup, or document these as export-only features.
Both export_json and export_full_backup serialize start_fuel_level and end_fuel_level, but no corresponding import routes exist to restore from these formats. Users can export their data but cannot reload it. Either implement import handlers for these export formats, or explicitly document that exports are for backup/portability only and cannot be re-imported into May.
The CSV import path correctly handles both fuel fields at line 2964 and requires no changes.
📍 Affects 1 file
app/routes/api.py#L1570-L1571(this comment)app/routes/api.py#L1897-L1898
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/api.py` around lines 1570 - 1571, Address both export sites in
app/routes/api.py at lines 1570-1571 and 1897-1898 by either adding matching
import/restore handlers for the export_json and export_full_backup formats,
including both fuel fields, or explicitly documenting these formats as
export-only and non-reimportable; leave the existing CSV import handling at line
2964 unchanged.
Comment on lines
+2746
to
+2747
| 'end_fuel_level': ['end fuel level', 'end fuel', 'end gas', 'fuel level', 'fuel', 'end battery', 'battery'], | ||
| 'start_fuel_level': ['start fuel level', 'start fuel', 'start gas', 'start battery'], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: alias candidates are filtered by data_type before matching.
rg -n -C 20 \
"_COLUMN_ALIASES|def .*map|auto.*map|get_import_fields|start_fuel_level|end_fuel_level|start_soc|end_soc" \
app/routes/api.pyRepository: dannymcc/may
Length of output: 28760
Remove ambiguous generic fuel aliases.
auto_suggest_mappings() scopes candidates to get_import_fields(data_type), so cross-type collisions do not occur. However, fuel, fuel level, and battery map to end_fuel_level without identifying the trip side. Remove these aliases or document the default mapping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/api.py` around lines 2746 - 2747, Update the alias lists used by
auto_suggest_mappings(), specifically the end_fuel_level and start_fuel_level
mappings, to remove ambiguous generic aliases such as fuel, fuel level, and
battery that do not identify the trip side. Retain only side-specific aliases,
or explicitly document and implement the intended default mapping if those
generic aliases must remain.
Comment on lines
+206
to
+215
| function calculateFuelConsumption() { | ||
| const start = parseFloat(document.getElementById('start_fuel_level').value); | ||
| const end = parseFloat(document.getElementById('end_fuel_level').value); | ||
| const consumption = start - end; | ||
| let textContent = '0'; | ||
| if (start && end) { | ||
| if (consumption > 0) textContent = consumption.toFixed(1); | ||
| else if (consumption < 0) textContent = '+' + Math.abs(consumption).toFixed(1); | ||
| } | ||
| document.getElementById('trip-fuel-consumption').textContent = textContent |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle valid zero fuel levels.
0 is valid because both inputs allow it. if (start && end) treats zero as missing, so a trip from 10% to 0% displays 0 consumption. Check that both parsed values are finite instead.
Proposed fix
- if (start && end) {
+ if (Number.isFinite(start) && Number.isFinite(end)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function calculateFuelConsumption() { | |
| const start = parseFloat(document.getElementById('start_fuel_level').value); | |
| const end = parseFloat(document.getElementById('end_fuel_level').value); | |
| const consumption = start - end; | |
| let textContent = '0'; | |
| if (start && end) { | |
| if (consumption > 0) textContent = consumption.toFixed(1); | |
| else if (consumption < 0) textContent = '+' + Math.abs(consumption).toFixed(1); | |
| } | |
| document.getElementById('trip-fuel-consumption').textContent = textContent | |
| function calculateFuelConsumption() { | |
| const start = parseFloat(document.getElementById('start_fuel_level').value); | |
| const end = parseFloat(document.getElementById('end_fuel_level').value); | |
| const consumption = start - end; | |
| let textContent = '0'; | |
| if (Number.isFinite(start) && Number.isFinite(end)) { | |
| if (consumption > 0) textContent = consumption.toFixed(1); | |
| else if (consumption < 0) textContent = '+' + Math.abs(consumption).toFixed(1); | |
| } | |
| document.getElementById('trip-fuel-consumption').textContent = textContent |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/templates/trips/form.html` around lines 206 - 215, Update
calculateFuelConsumption to validate start and end with a finite-number check
rather than truthiness, so zero fuel levels are processed as valid values while
invalid or missing inputs retain the default display behavior.
Comment on lines
+120
to
+127
| <p class="text-sm font-medium text-gray-900 dark:text-white"> | ||
| {{ "%.1f"|format(trip.distance) }} {{ current_user.distance_unit }} | ||
| {% if trip.fuel_consumption %}· {{ trip.fuel_consumption_human_readable }}{% endif %} | ||
| </p> | ||
| <p class="text-xs text-gray-500 dark:text-gray-400"> | ||
| {{ "%.0f"|format(trip.start_odometer) }} → {{ "%.0f"|format(trip.end_odometer or 0) }} | ||
| {% if trip.fuel_consumption %}· {{ trip.start_fuel_level or "?" }} % → {{ trip.end_fuel_level or "?" }} %{% endif %} | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use explicit None checks for optional fuel values.
On Line 122, a zero consumption value is valid, but the truthiness check hides it. On Line 126, or "?" renders a valid 0% value as ?. The human-readable property can also return None when vehicle.tank_capacity is unavailable, so guard that rendered value directly.
Proposed fix
- {% if trip.fuel_consumption %}· {{ trip.fuel_consumption_human_readable }}{% endif %}
+ {% if trip.fuel_consumption_human_readable is not none %}· {{ trip.fuel_consumption_human_readable }}{% endif %}
...
- {% if trip.fuel_consumption %}· {{ trip.start_fuel_level or "?" }} % → {{ trip.end_fuel_level or "?" }} %{% endif %}
+ {% if trip.start_fuel_level is not none and trip.end_fuel_level is not none %}· {{ trip.start_fuel_level }} % → {{ trip.end_fuel_level }} %{% endif %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <p class="text-sm font-medium text-gray-900 dark:text-white"> | |
| {{ "%.1f"|format(trip.distance) }} {{ current_user.distance_unit }} | |
| {% if trip.fuel_consumption %}· {{ trip.fuel_consumption_human_readable }}{% endif %} | |
| </p> | |
| <p class="text-xs text-gray-500 dark:text-gray-400"> | |
| {{ "%.0f"|format(trip.start_odometer) }} → {{ "%.0f"|format(trip.end_odometer or 0) }} | |
| {% if trip.fuel_consumption %}· {{ trip.start_fuel_level or "?" }} % → {{ trip.end_fuel_level or "?" }} %{% endif %} | |
| </p> | |
| <p class="text-sm font-medium text-gray-900 dark:text-white"> | |
| {{ "%.1f"|format(trip.distance) }} {{ current_user.distance_unit }} | |
| {% if trip.fuel_consumption_human_readable is not none %}· {{ trip.fuel_consumption_human_readable }}{% endif %} | |
| </p> | |
| <p class="text-xs text-gray-500 dark:text-gray-400"> | |
| {{ "%.0f"|format(trip.start_odometer) }} → {{ "%.0f"|format(trip.end_odometer or 0) }} | |
| {% if trip.start_fuel_level is not none and trip.end_fuel_level is not none %}· {{ trip.start_fuel_level }} % → {{ trip.end_fuel_level }} %{% endif %} | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/templates/trips/index.html` around lines 120 - 127, Update the fuel
display conditions in the trip template to use explicit None checks: render
fuel_consumption and its human-readable value when not None, and render
start_fuel_level/end_fuel_level values without treating zero as missing, using
“?” only for None values.
Comment on lines
+66
to
+69
| <div> | ||
| <dt class="text-sm font-medium text-gray-500 dark:text-gray-400">{{ _('Last Fuel Level') }}</dt> | ||
| <dd class="mt-1 text-sm text-gray-900 dark:text-white">{{ "%.0f"|format(vehicle.get_last_fuel_level())}} %</dd> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not display the no-data sentinel as 0%.
When no trip or charging session has a recorded fuel level, Vehicle.get_last_fuel_level() returns 0 (app/models.py, Lines [438-457]). This template then displays 0 %, which claims an empty tank instead of showing that no level is recorded. Return None for the no-data case and render an explicit empty state here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/templates/vehicles/view.html` around lines 66 - 69, Update
Vehicle.get_last_fuel_level() to return None when no trip or charging session
contains a recorded fuel level, while preserving the existing value for recorded
levels. In the vehicle view template, handle a None result before formatting and
render an explicit empty-state label instead of displaying 0%.
Comment on lines
+20
to
+29
| bind = op.get_bind() | ||
| inspector = sa.inspect(bind) | ||
| if 'trips' in inspector.get_table_names(): | ||
| existing_cols_trip = [col['name'] for col in inspector.get_columns('trips')] | ||
| if 'start_fuel_level' not in existing_cols_trip or 'end_fuel_level' not in existing_cols_trip: | ||
| with op.batch_alter_table('trips', schema=None) as batch_op: | ||
| if 'start_fuel_level' not in existing_cols_trip: | ||
| batch_op.add_column(sa.Column('start_fuel_level', sa.Float(), nullable=True)) | ||
| if 'end_fuel_level' not in existing_cols_trip: | ||
| batch_op.add_column(sa.Column('end_fuel_level', sa.Float(), nullable=True)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the migration revision deterministic.
If trips is absent, this upgrade performs no schema change but Alembic still records the revision. The unconditional downgrade then fails on the missing table. If one fuel column already exists, the downgrade can also remove a column that this revision did not add.
Remove the schema inspection guards and let the migration apply the exact revision delta, or make upgrade and downgrade symmetric for every supported pre-existing schema.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migrations/versions/cc6e159f098a_adds_fuel_level_to_trip.py` around lines 20
- 29, The upgrade conditionally adds columns based on schema inspection using
inspector.get_table_names() and inspector.get_columns() checks, which makes the
migration non-deterministic and creates asymmetry with the downgrade operation.
Remove all the conditional inspection guards and the nested if-checks for
existing_cols_trip so the migration always attempts to add start_fuel_level and
end_fuel_level columns to the trips table unconditionally. Ensure the
corresponding downgrade operation mirrors this behavior by always dropping both
columns, making the upgrade and downgrade symmetric regardless of the
pre-existing schema state.
Source: Coding guidelines
Comment on lines
+70
to
+71
| assert trip.start_fuel_level == 85.3 | ||
| assert trip.send_fuel_level == 80.0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the fuel-level attribute name.
Trip defines end_fuel_level, not send_fuel_level. This assertion raises AttributeError before it verifies persisted data.
Proposed fix
- assert trip.send_fuel_level == 80.0
+ assert trip.end_fuel_level == 80.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert trip.start_fuel_level == 85.3 | |
| assert trip.send_fuel_level == 80.0 | |
| assert trip.start_fuel_level == 85.3 | |
| assert trip.end_fuel_level == 80.0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_trips.py` around lines 70 - 71, Update the fuel-level assertion in
the Trip persistence test to reference the defined Trip attribute end_fuel_level
instead of the nonexistent send_fuel_level, while preserving the expected value
of 80.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds tracking of fuel levels to trips (#273)
Changelog
Testing
How were these changes tested?
Summary by CodeRabbit