[7.x] fix(simbrief): write OFP aircraft back to the user's bid#2232
[7.x] fix(simbrief): write OFP aircraft back to the user's bid#2232jordanhavard wants to merge 1 commit into
Conversation
The aircraft selected when generating a SimBrief OFP was only saved to the simbrief table; the user's bid for the flight kept a null or stale aircraft_id. Sync it in SimBriefService::downloadOfp() so the bid always reflects the briefing, covering both initial generation (check_ofp) and OFP updates (update_ofp). When bids.block_aircraft is enabled and another bid already holds the aircraft, the sync is skipped and logged, mirroring BidService::addBid(). No bid is ever created by the sync.
WalkthroughThis PR adds aircraft synchronization from SimBrief OFP generation to user bids. When an OFP is downloaded, the aircraft selected during briefing is written to the corresponding bid record, with a safeguard to prevent overwriting bids that hold aircraft blocked by configuration. ChangesOFP Aircraft Synchronization
Sequence DiagramsequenceDiagram
participant DownloadOFP as downloadOfp()
participant SimBrief as SimBrief::updateOrCreate()
participant SyncBid as syncBidAircraft()
participant Bid as Bid query
participant Setting as bids.block_aircraft
DownloadOFP->>SimBrief: upsert OFP record
SimBrief-->>DownloadOFP: SimBrief instance
DownloadOFP->>SyncBid: sync with aircraft_id
SyncBid->>Bid: find by user_id & flight_id
Bid-->>SyncBid: bid record
SyncBid->>Setting: check if block_aircraft enabled
Setting-->>SyncBid: setting value
SyncBid->>Bid: update aircraft_id (if allowed)
SyncBid-->>DownloadOFP: sync complete
DownloadOFP-->>DownloadOFP: return SimBrief
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.2.1)PHPStan was skipped because the user-provided config is missing the required Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (1)
tests/SimBriefTest.php (1)
332-360: ⚡ Quick winAlso assert the blocked sync stays non-fatal.
This test only proves the bid stays untouched. If
downloadOfp()regressed to returningnullafter the skip, the test would still pass and the OFP-creation guarantee would go untested. Capture the returned briefing and assert it was persisted.Suggested test tightening
- $this->downloadOfp($this->user, $flight, $aircraft, []); + $briefing = $this->downloadOfp($this->user, $flight, $aircraft, []); $bid->refresh(); + $this->assertNotNull($briefing); $this->assertNull($bid->aircraft_id);Based on PR objectives, the blocked path is supposed to skip bid sync without interrupting briefing creation.
🤖 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/SimBriefTest.php` around lines 332 - 360, In test_ofp_download_skips_bid_sync_when_aircraft_blocked, capture the return value from downloadOfp($this->user, $flight, $aircraft, []) into a variable (e.g. $briefing) and add an assertion that the briefing was persisted (e.g. assertNotNull($briefing) or assertDatabaseHas for the briefing record) to ensure downloadOfp() still creates/stores the briefing even when bid sync is skipped; keep the existing bid->refresh(); assertNull($bid->aircraft_id) assertion intact.
🤖 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/Services/SimBriefService.php`:
- Around line 127-137: The aircraft-blocking check in SimBriefService is racy
because Bid::where(...)->exists() and $bid->save() are separate; wrap the
check+assign in a DB transaction and acquire a row lock so the existence check
is done with a SELECT ... FOR UPDATE (or equivalent Eloquent lockForUpdate)
before assigning the aircraft_id and calling $bid->save(), matching the same
transactional/locked semantics used by the bid creation flow in BidService (use
the same lock/constraint logic and/or add a DB-level unique constraint on active
aircraft assignments if present in BidService); ensure the code paths reference
the same predicate for "active" bids and abort/return if the locked query finds
a conflicting bid.
In `@tests/SimBriefTest.php`:
- Around line 287-290: Add a Bid factory and update tests to use it: create
app/Database/factories/BidFactory.php and ensure the Bid model uses HasFactory
so tests can call Bid::factory()->create(...); replace the hand-rolled
Bid::create(...) usages in SimBriefTest with Bid::factory()->create([...])
(references: Bid model, BidFactory, SimBriefTest). Also in the
test_ofp_download_skips_bid_sync_when_aircraft_blocked test, add an assertion
that the return value of downloadOfp(...) is not null (downloadOfp) to ensure
the OFP was actually created/updated even when syncBidAircraft is skipped.
---
Nitpick comments:
In `@tests/SimBriefTest.php`:
- Around line 332-360: In
test_ofp_download_skips_bid_sync_when_aircraft_blocked, capture the return value
from downloadOfp($this->user, $flight, $aircraft, []) into a variable (e.g.
$briefing) and add an assertion that the briefing was persisted (e.g.
assertNotNull($briefing) or assertDatabaseHas for the briefing record) to ensure
downloadOfp() still creates/stores the briefing even when bid sync is skipped;
keep the existing bid->refresh(); assertNull($bid->aircraft_id) assertion
intact.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ce43357-aec8-47d9-b3f6-d451f98ed04c
📒 Files selected for processing (2)
app/Services/SimBriefService.phptests/SimBriefTest.php
| if (setting('bids.block_aircraft')) { | ||
| $blocked = Bid::where('aircraft_id', $ac_id)->where('id', '!=', $bid->id)->exists(); | ||
| if ($blocked) { | ||
| Log::info('SimBrief | Bid aircraft sync skipped, aircraft='.$ac_id.' is held by another bid'); | ||
|
|
||
| return; | ||
| } | ||
| } | ||
|
|
||
| $bid->aircraft_id = $ac_id; | ||
| $bid->save(); |
There was a problem hiding this comment.
Make the aircraft-blocking check atomic.
Line 128's exists() check and Line 137's save() are separate statements, so two concurrent OFP downloads can both see the aircraft as free and then assign it to different bids. That breaks the bids.block_aircraft guarantee under load. Please route this through a transactional/locked path shared with bid creation, or enforce the invariant with a database constraint on active aircraft assignments.
Based on PR objectives and app/Services/BidService.php:126-194, this path is expected to honor the same aircraft-blocking contract as bid creation.
🤖 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/Services/SimBriefService.php` around lines 127 - 137, The
aircraft-blocking check in SimBriefService is racy because
Bid::where(...)->exists() and $bid->save() are separate; wrap the check+assign
in a DB transaction and acquire a row lock so the existence check is done with a
SELECT ... FOR UPDATE (or equivalent Eloquent lockForUpdate) before assigning
the aircraft_id and calling $bid->save(), matching the same transactional/locked
semantics used by the bid creation flow in BidService (use the same
lock/constraint logic and/or add a DB-level unique constraint on active aircraft
assignments if present in BidService); ensure the code paths reference the same
predicate for "active" bids and abort/return if the locked query finds a
conflicting bid.
| $bid = Bid::create([ | ||
| 'user_id' => $this->user->id, | ||
| 'flight_id' => $flight->id, | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
FACTORIES="$(fd -i 'BidFactory.php' || true)"
if [ -z "$FACTORIES" ]; then
echo "BidFactory.php not found"
exit 0
fi
printf 'Found Bid factory file(s):\n%s\n' "$FACTORIES"
rg -n -C2 'class\s+BidFactory|protected \$model\s*=.*Bid|function\s+\w+\(' $FACTORIESRepository: phpvms/phpvms
Length of output: 82
🏁 Script executed:
#!/bin/bash
set -e
echo "== Locate potential Bid factory files =="
fd -a -i '*bid*factory*.php' . || true
fd -a -i 'Factory.php' database .github tests . || true
fd -a -i '*Factory*.php' database . || true
echo
echo "== Search for factory definitions/usage =="
rg -n "BidFactory|class\s+\w*Bid\w*Factory" -S .
rg -n "Bid::factory\(" -S .
rg -n "factory\(\s*Bid::" -S .
rg -n "database/factories|ModelFactory" -S .
echo
echo "== Inspect tests/SimBriefTest.php around mentioned lines =="
if [ -f tests/SimBriefTest.php ]; then
sed -n '250,390p' tests/SimBriefTest.php | nl -ba | sed -n '1,200p'
else
echo "tests/SimBriefTest.php not found"
fiRepository: phpvms/phpvms
Length of output: 1996
🏁 Script executed:
#!/bin/bash
set -e
echo "== List factory files under app/Database/factories/"
ls -1 app/Database/factories 2>/dev/null | nl -ba || true
echo
echo "== Find any factory files that mention Bid in their filename/path =="
fd -a -i --glob '*bid*factory*.php' . || true
fd -a -i --glob '*bid*.php' app/Database/factories || true
fd -a -i --glob '*bid*.php' app/Database || true
echo
echo "== Search for Bid factory class/definition and Bid::factory usage =="
rg -n "class\s+\w*Bid\w*Factory|BidFactory" -S app . || true
rg -n "Bid::factory\(" -S . || true
rg -n "factory\(\s*Bid::" -S . || true
echo
echo "== Inspect tests/SimBriefTest.php around 270-380 =="
if [ -f tests/SimBriefTest.php ]; then
sed -n '250,390p' tests/SimBriefTest.php | nl -ba
else
echo "tests/SimBriefTest.php not found"
fiRepository: phpvms/phpvms
Length of output: 675
🏁 Script executed:
#!/bin/bash
set -e
echo "== Check repo root =="
pwd
ls -la
echo
echo "== List app/Database/factories =="
ls -la app/Database/factories | head -n 50
echo
echo "== List any factory files that mention Bid in filename/path (glob) =="
fd -a -i --glob '*bid*factory*.php' . || true
fd -a -i --glob '*bid*.php' app/Database/factories app . || true
echo
echo "== Search for Bid factory class/definition and Bid::factory usage =="
rg -n "class\s+\w*Bid\w*Factory|BidFactory" app/Database/factories app database tests . || true
rg -n "Bid::factory\(" . || true
rg -n "->factory\(\)" app/Database/factories app tests . || true
echo
echo "== Inspect tests/SimBriefTest.php around 260-380 with line numbers =="
if [ -f tests/SimBriefTest.php ]; then
sed -n '250,390p' tests/SimBriefTest.php | cat -n
else
echo "tests/SimBriefTest.php not found"
fiRepository: phpvms/phpvms
Length of output: 12156
🏁 Script executed:
#!/bin/bash
set -e
echo "== Inspect app/Models/Bid.php for HasFactory / factory mapping =="
if [ -f app/Models/Bid.php ]; then
sed -n '1,200p' app/Models/Bid.php | cat
else
echo "Missing app/Models/Bid.php"
fi
echo
echo "== Search for any Bid factory class/file =="
rg -n "class\s+\w*Bid\w*Factory|BidFactory" -S . || true
echo
echo "== Search for Bid::factory usage in repo =="
rg -n "Bid::factory\(" -S . || true
echo
echo "== Search for Bid::create usage in tests =="
rg -n "Bid::create\(" -S tests/ || trueRepository: phpvms/phpvms
Length of output: 1530
🏁 Script executed:
#!/bin/bash
set -e
echo "== Show exact sections around each Bid::create usage (with real line numbers) =="
for start in 270 300 330 340 345 350; do
end=$((start+25))
echo
echo "--- tests/SimBriefTest.php:${start}-${end} ---"
awk -v s=$start -v e=$end 'NR>=s && NR<=e {printf "%d\t%s\n", NR, $0}' tests/SimBriefTest.php
doneRepository: phpvms/phpvms
Length of output: 5594
🏁 Script executed:
#!/bin/bash
set -e
rg -n "function\s+downloadOfp\s*\(" tests/SimBriefTest.php
rg -n "downloadOfp\(" tests/SimBriefTest.phpRepository: phpvms/phpvms
Length of output: 896
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,130p' tests/SimBriefTest.php | cat -nRepository: phpvms/phpvms
Length of output: 4767
🏁 Script executed:
#!/bin/bash
set -e
echo "== Locate SimBriefService::downloadOfp implementation =="
rg -n "function\s+downloadOfp\s*\(" app/Services/SimBriefService.php app -S
echo
echo "== Print downloadOfp() body in app/Services/SimBriefService.php =="
php_file="$(rg -l "function\s+downloadOfp\s*\(" app/Services/SimBriefService.php app -S | head -n 1 || true)"
if [ -n "$php_file" ]; then
echo "Using: $php_file"
# show a larger window around the first match
match_line="$(rg -n "function\s+downloadOfp\s*\(" "$php_file" -S | head -n 1 | cut -d: -f1)"
start=$((match_line-60))
end=$((match_line+160))
sed -n "${start},${end}p" "$php_file" | cat -n
else
echo "No downloadOfp() found"
fi
echo
echo "== Search for bids.block_aircraft usage =="
rg -n "bids\.block_aircraft" app -S || true
echo
echo "== Search for aircraft_id/bid sync logic in service/listeners =="
rg -n "aircraft_id|sync|downloadOfp|block_aircraft" app/Services app/Listeners app/Support app/Models -S || trueRepository: phpvms/phpvms
Length of output: 9105
🏁 Script executed:
#!/bin/bash
set -e
echo "== Line numbers for downloadOfp() in app/Services/SimBriefService.php =="
rg -n "public function downloadOfp|private function syncBidAircraft" app/Services/SimBriefService.php
echo
echo "== Print app/Services/SimBriefService.php around downloadOfp() (1-220) =="
sed -n '1,220p' app/Services/SimBriefService.php | cat -n
echo
echo "== Print app/Services/BidService.php around block_aircraft logic (90-220) =="
sed -n '90,220p' app/Services/BidService.php | cat -nRepository: phpvms/phpvms
Length of output: 14723
Add a Bid factory (or use one) in SimBriefTest
tests/SimBriefTest.php currently hand-rolls bids with Bid::create(...) (e.g., at ~287-290, and also ~314-318, ~343-347, ~352-355). The repo has no Bid factory (app/Database/factories/ contains no BidFactory.php, and app/Models/Bid.php has no HasFactory), so the tests can’t follow the “factory-backed model fixtures” guideline—add a BidFactory (with any needed factory state) and switch these fixtures to it.
For test_ofp_download_skips_bid_sync_when_aircraft_blocked, also assert the return value of downloadOfp(...) is not null (blocking only skips syncBidAircraft; downloadOfp() still returns the created/updated SimBrief), so a regression that fails the OFP download doesn’t get missed.
🤖 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/SimBriefTest.php` around lines 287 - 290, Add a Bid factory and update
tests to use it: create app/Database/factories/BidFactory.php and ensure the Bid
model uses HasFactory so tests can call Bid::factory()->create(...); replace the
hand-rolled Bid::create(...) usages in SimBriefTest with
Bid::factory()->create([...]) (references: Bid model, BidFactory, SimBriefTest).
Also in the test_ofp_download_skips_bid_sync_when_aircraft_blocked test, add an
assertion that the return value of downloadOfp(...) is not null (downloadOfp) to
ensure the OFP was actually created/updated even when syncBidAircraft is
skipped.
Source: Coding guidelines
|
came across this one while working through another issue. Unsure if this is intentional or not, if it is, feel free to close :D |
|
This syncs the user selected aircraft in sb to the right one in phpvms? |
|
This PR syncs the SimBrief aircraft_id with the flight bid. Both reference phpVMS aircraft (nothing is related to the SB API) A while back, we used to select the aircraft when generating a SimBrief OFP (which is why we have an aircraft_id field in the SimBrief table). However, this created a race condition where two people could select the same aircraft on the generation page before finalizing it, causing conflicts. To fix this, I added the ability to bid on an aircraft at the same time you bid on a flight. This PR ensures that if an aircraft wasn't assigned during the initial flight bid, but was chosen while generating the OFP, it will now be added to the bid retroactively. Omitting this from the first PR was intentional, as the aircraft is blocked whether it is recorded in the bid or the SimBrief table. I'm not against adding it now however you have to make sure you are using the right types: |
Yeah, from testing, during the SB creation flow, you select an AC, this sends it to SB and then once that generation comes back, it then sets aircraft_id on the SB table - this PR then also sets that on the bids table too.
Ahh makes sense. And I think this would reintroduce this issue. A couple options
My gut says bids should be the authoritave table for this but also know that there is existing history here so happy with either direction here, let me know your thoughts and happy to amend the PR for another pass 😁 Thanks for the input 🙏 |
|
One feature that would make a lot of sense for v8 is requiring users to have a active bid before generating a SimBrief OFP. By doing this, we would only need to store the aircraft at the bid level, leveraging DB transactions and locks to entirely prevent race conditions. While this would significantly streamline our codebase, it will slightly alter the current user workflow for those who aren't used to bidding before a flight. However, we can easily mitigate this in the UI: when a user clicks "Add Bid," a modal will open for aircraft selection, immediately followed by a redirection to the SimBrief generation page. Needs to be discussed with/decided by @nabeelio |
3 participants
Summary
When a pilot generates a SimBrief OFP, the selected aircraft is saved only to the
simbrieftable — the user's bid for the same flight keeps anull(or stale)aircraft_id. The bid is read to seed the aircraft selection inSimBriefController::generate(), but the choice never flows back.This adds a sync in
SimBriefService::downloadOfp()(single choke point covering both initial generation viacheck_ofp()and OFP updates viaupdate_ofp()):aircraft_idis synced to the aircraft the OFP was generated with (fills null, overwrites stale).bids.block_aircraftenabled, the sync is skipped (and logged) if another bid already holds the aircraft — mirroringBidService::addBid(), but non-fatal since the OFP already exists.Test Plan
test_ofp_download_writes_aircraft_to_bid— bid with null aircraft gets populatedtest_ofp_download_overwrites_stale_bid_aircraft— bid holding a different aircraft is synced to the OFP aircrafttest_ofp_download_skips_bid_sync_when_aircraft_blocked— withbids.block_aircraft, a bid is left untouched when another bid holds the aircrafttest_ofp_download_without_bid_creates_no_bid— no bid is created when none existsSimBriefTest+BidTestsuites passSummary by CodeRabbit
New Features
Tests