Ziply address availability

[danry25]

Summary

The Internet page looked up FCC broadband data by census block, so every address in a block was shown the best provider in that block, including many addresses Ziply won't install at. This replaces it with an address-level lookup against Ziply's WFI and EIA building lists, prices fiber with the phone bundle discount, and records the contract term and the building the fiber was qualified at on the order.

It also removes FCCStateGeoIdLookup, which downloaded and scanned 100–400 MB FCC CSVs in /tmp on every request. Its route also ended in a zero-width space, so the page's fetch never matched it. /v2/PhoneNumbers/{PhoneNumber} has the same stray character and is not changed here.

Building lists

• ServiceAddresses table (~125k rows, 38 MB), created by NumberSearch.DataAccess/ServiceAddresses.sql, with indexes on (Postal, StreetKey), (Postal, HouseNumber) and (Latitude, Longitude), plus a partial unique index on (Product, BuildingKey) WHERE BuildingKey <> ''.
• Ziply/import_ziply_building_list.py streams the 40 MB+ .xlsx lists at about 45 MB peak memory and replaces one product's rows in a single transaction. It reads inline and shared-string workbooks, and cells with or without an r reference. It refuses a list before touching the database if the header is missing, no rows qualify, or two rows share a C2F Building Key. It warns about Sellable rows with no key, and a psql failure or missing psql leaves the existing rows in place.
• Status per row:
  • WFI Sellable: BFI/WFI serviceable = Y and PRODUCT = BFI/WFI (100,371 buildings).
  • WFI Confirm: only one of the two flags set (16,821).
  • EIA Quote: EIA On-Net = Y (7,591).

Availability

GET /Internet/Availability?houseNumber=&street=&postal=&latitude=&longitude= returns serviceable, matchedAddress, exactMatch, matchType, buildingKey and offers.

• Matching: the exact house number on the street and ZIP comes first, then the same building number without suffixes or fractions (512 1/2 → 512), then the nearest listed building within 30 m. matchType is Exact, HouseNumber, Nearby or None, and the page words each differently.
• Fiber is only offered at a price on an exact match, at a WFI Sellable building listed at or above the tier's speed (ParseMbps: 300 Mbps for $75, 1 Gbps for $115) that has a building key. Anything short of that shows "Contact us to confirm", and a Sellable building we can't sell logs a warning.
• InternetBundle.QualifyingAddress picks the building the page offers, and Cart/Add re-checks with the same rule. This is safe because CanSellAt only varies between rows by speed, so the row that qualifies the 300 tier qualifies every tier any row can.
• EIA on-net buildings show "Request a Quote".

Cart, checkout and invoices

• Cart/Add only accepts fiber with a buildingKey that qualifies the tier, and refuses fiber at a second building when the cart already has fiber at another one. The page shows the server's reason.
• Bundle discount: $15/mo off each fiber connection when the order has phone service (Standard Lines or Concurrent Seats, the seat services the 911 acknowledgment uses) or the Partner coupon, never stacked. InternetBundle is the single rule, used by checkout, the order sidebar and Ops invoice regeneration. A Partner line with no 5G in the order is no longer shown or billed at $0.
• Partner coupon: keeps its $10 off 5G and now also qualifies fiber for the bundle discount. Its description is updated to match.
• Checkout requires a 2/3/5 year term when fiber is in the cart, and rejects fiber without a qualified address. It saves Orders.InternetTermYears, InternetServiceAddress and InternetBuildingKey. The address comes from the session, never the form. The fiber invoice line reads "3 year term. Service address: …".
• Ops shows the service address, term, building key and a map link on the order page, and the address on the order list. It maps the three columns with EF save behavior Ignore, so editing an order there can't change them.

Production status

• ServiceAddresses.sql and InternetBundle.sql are already applied to production, including every index above. That happened on 2026-09-24, with a backup of Orders, Coupons and Services before each run. Both Ziply lists are imported.
• Production's schema is ahead of its code until this deploys. That's safe: the running code never names the new table or columns, and the integration tests pass against it.
• Both files are safe to re-run, and stop at the first error.
• Also included: "Fixed Integration test (Yealink T54W deprecated)", which changes t54w → t87w in TeleDynamicsProductCheckQuantityAsync. It's an unrelated CI fix: the T54W was delisted, so that test fails on every branch without it.

Deploy

1. Already done on production; needed only for a fresh database:

   psql -d numberSearch -v ON_ERROR_STOP=1 -f NumberSearch.DataAccess/ServiceAddresses.sql -f NumberSearch.DataAccess/InternetBundle.sql

2. Deploy the Mvc and Ops apps. Ops reads the new Orders columns, so step 1 must come first on any other database.

3. When Ziply sends new lists, run:

   python3 NumberSearch.DataAccess/Ziply/import_ziply_building_list.py WFI "<WFI list>.xlsx" -d numberSearch

   and the same with EIA for the EIA list.

Follow-ups

• totalCost in the invoice builders is computed but never read, so it is left alone here. Invoices are built from the quantity-correct line items. Deleting the accumulator belongs in its own PR.
• Ops' regenerated invoices still don't re-apply the Partner coupon's $10 5G discount. That predates this PR; the fiber bundle discount is re-applied.
• Ask Ziply what a blank BFI/WFI serviceable or PRODUCT means, so the 16,821 Confirm buildings can be resolved.
• One fiber service address per order. Multi-site fiber would need the address stored per product order.

Testing

• Clean --no-incremental builds add no warnings: Mvc 1 and Ops 7, all pre-existing. The Unit tests pass 28/28. They cover the bundle rules, speed parsing including overflow, house keys, CanSellAt, QualifyingAddress with duplicate and blank keys in either order, invoice notes and street keys.
• 59 of 60 random list addresses matched exactly through the page's ArcGIS lookup. The miss (512 1/2 S Main St, Moscow ID) is now caught by the building-number match.
• In scratch databases, I checked:
  • the importer against real and synthetic workbooks, including shared strings, missing references, missing header, duplicate and blank keys, and a missing psql
  • both migrations run twice
  • a failed unique index stopping psql before the DROP
  • the partial unique index being used by PostgreSQL 11 generic plans
• Not yet clicked through checkout end to end in a running site.

[LawOfSynergy]

Reviewed against the branch at 4344f7c, built and tested locally on .NET SDK 10.0.401 (macOS arm64). Clean rebuilds of Mvc and Ops on this branch and on the merge-base, plus the xunit Unit suite, plus the importer run against synthetic xlsx files.

Claims in the description

Claim	Result
Unit tests pass 9/9	Verified. 9/9, 40 ms.
dotnet build of Mvc and Ops succeeds with 0 warnings	Not reproducible. Mvc emits 2, Ops emits 8. One warning in each is on a line this PR adds.

Details in the first item below. Everything else in the description that I could check locally held up.

Should fix before merge

1. The build is not warning-free, and two of the warnings are new here.

Clean rebuilds (--no-incremental), unique warning sites:

Project	merge-base	this branch
Mvc	2	2
Ops	7	8

Mvc holds at 2 only because this PR deletes the FCC code that carried the old CS8602 at Endpoints.cs:291 and introduces a new one of its own:

• NumberSearch.Mvc/Controllers/CartController.cs:1371 col 58, CS8602 on InternetBundle.Discount(cart.ProductOrders)
• NumberSearch.Ops/Controllers/OrdersController.cs:1621 col 43, CS8602 on cart.ProductOrders.Select(...)

Both are the same thing: a bare cart. dereference in a method where every neighbouring access is cart?.. The earlier null-conditional accesses leave the flow state maybe-null, so the new lines warn. Neither is a live NRE today, since each method has one call site and it passes a non-null cart, so this is not urgent. cart?.ProductOrders ?? [] matches the surrounding code and clears both.

Worth checking how the 0-warning result was obtained. An incremental rebuild will not re-emit warnings for unchanged projects, which is exactly what caught me out on my first comparison run.

2. The 1 Gbps tier is gated on MaxSpeed.StartsWith("1.0G").

Endpoints.cs, in the WFI Sellable branch. A building listed at 2.0G or 10.0G fails that prefix and is offered only the 300 Mbps tier. So does any other spelling: 1G, 1000M, 1 Gbps. There is no test on the string form and no log when the prefix misses, so the failure mode is silent and indistinguishable from "Ziply has no gig there."

Could you add the output of SELECT DISTINCT "MaxSpeed" FROM public."ServiceAddresses" WHERE "Status" = 'Sellable' to the description? If the values are a small closed set, a parsed numeric comparison is a two-line change and removes the whole class of problem.

3. Nothing checks MaxSpeed before offering the 300 Mbps tier.

The same gap pointed the other way, and this direction oversells: every Sellable WFI building gets the $75 300/300 add-to-cart button unconditionally. If any Sellable building maxes below 300M we accept an order we cannot install. The distinct-values query above answers this one too.

4. HouseNumber is compared raw while StreetKey is normalized.

ServiceAddress.cs:251. ToStreetKey and street_key exist precisely because free-text street names do not compare byte for byte, but the house number is exact string equality between ArcGIS AddNum and Ziply's Primary Number. Leading zeros, 1250A, 1250 1/2 and hyphenated numbers all miss silently and drop into the 30 m geo fallback.

The number that would settle how much this matters is not in the description: across a sample of real addresses, how often does the exact-match path hit versus the fallback? That ratio is the quality measure for this whole feature.

Non-blocking

5. matchedAddress is computed, returned, and then dropped by the UI.

Confirmed by serializing the record: the payload is

{"serviceable":true,"matchedAddress":"1250 1st Ave S, Seattle, WA 98134","offers":[{"product":"WFI","status":"Sellable","name":"Fiber Internet 300 Mbps","speed":"300/300 Mbps","price":75,"serviceId":"...","note":"..."}]}

and matchedAddress appears nowhere in Internet.cshtml. Given the 30 m fallback can answer for the building next door, this is the one field that would tell the customer we matched something other than what they typed. Either render it or drop it from the response.

Separately, matched uses FirstOrDefault() over an unordered result set, so it can name an EIA row while the offers displayed are WFI.

6. Nothing records which address the fiber was qualified at.

AddToCart('Service', ...) adds the service with no link to the searched address or the matched building. InternetTermYears got a column on Orders; the qualified service address did not. Ops receives an order for "Fiber Internet 300 Mbps" with only the billing address and no way to recover which Ziply building was matched. The premise of the PR is that census-block matching was too coarse, so losing the address at the cart boundary gives a good deal of that precision back.

7. The importer reads nothing from a shared-strings xlsx, and says the wrong thing about it.

rows() reads <t> and falls back to <v>, which is correct for t="inlineStr" but reads a shared-string index for t="s", the Excel default. I built two minimal xlsx files with identical content and ran this branch's importer against both:

=== inline.xlsx ===
  rows yielded after header: 1
  BFI/WFI serviceable = 'Y'   PRODUCT = 'BFI/WFI'   -> status(WFI) = 'Sellable'

=== shared.xlsx ===
  rows yielded after header: 0

Good news: it fails closed. The header probe values.get(0) == 'Building Name' does not match, kept stays 0, and the script exits without touching the database. End to end it prints:

No WFI rows found in shared.xlsx, leaving the existing rows in place.

That message sends the operator after the data when the cause is the file format. A check for xl/sharedStrings.xml with an explicit error is one line and saves a bad afternoon the next time Ziply changes export tooling. Same failure class, same misleading message: header detection is a single equality on column A, and column_index raises TypeError on a <c> with no r attribute.

8. The totalCost follow-up describes dead code.

totalCost is assigned from summary.TotalCost at CartController.cs:917 and never read again. The Ops copy at OrdersController.cs:1437 is likewise never read. Customer-visible totals come from the Line_Items lists, which are quantity-correct. Listing it as a follow-up invites someone to spend real time on a variable nothing consumes. Worth saying it is inert, or deleting the accumulator.

(This PR does make that variable internally inconsistent, since totalCost += service.Price ignores quantity while totalCost -= bundleDiscount respects it. Only worth mentioning because it is further evidence nobody reads it.)

9. Undocumented asymmetry in the bundle rule.

HasPhoneService requires Quantity > 0, but HasPartnerCoupon has no quantity gate, so a Partner coupon line with Quantity = 0 still grants the $15 per connection while a zero-quantity phone line correctly does not. Confirmed with a test. Probably intentional, since coupons are not quantity-bearing, but it is an unstated rule in pricing code and deserves a comment in InternetBundle.

10. Partner coupon plus fiber with no 5G renders a $0 coupon row.

The Type == "Service" branch discounts services whose name contains 5G. With fiber only that is $0, and it still emits a line carrying the newly reworded "5G service and fiber internet at partner pricing." next to a separate "Fiber + Phone Bundle" line. Pre-existing branch, newly reachable state because the coupon now has a fiber meaning.

Checked and clean

Recording these so they do not get re-reviewed:

• C# ToStreetKey and Python street_key agree on 29 of 30 adversarial inputs (combining marks, ss-ligature, non-breaking space, curly apostrophes, en-dashes, dotted-I, fractions). The only divergence is U+212A KELVIN SIGN, which will not appear in a street name. The "must match" comments are doing their job.
• The JSON contract holds. Every key Internet.cshtml reads is present with matching casing under JsonSerializerDefaults.Web.
• The Ops bundle discount sits outside the per-productOrder loop, so no double counting, and cart is non-null there.
• cart.Order = order in SubmitAsync means the term select keeps its posted value on the validation bounce. The select is inside #orderForm and the Cart.Order.InternetTermYears name matches the asp-for prefix.
• Order.GetByIdAsync selects the new column, so the post-insert reload round-trips it.
• INSERT ... VALUES (..., 75, ...) into the character varying Services."Price" column is legal: PostgreSQL permits assignment casts to string types via I/O conversion.
• No injection in the importer (product is whitelisted to WFI/EIA), and \copy is client-side, so it works against a remote database.
• .CacheOutput() varies by query string under the default policy, so no cross-address cache leakage. It also buys close to nothing here, since the lat/lon parameters make nearly every key unique.
• Degenerate latitude/longitude (NaN, +/-Infinity, out of range) produce empty BETWEEN ranges rather than a full scan of the 125k rows.
• The two new fiber services do not leak onto /Services between SQL step 1 and the app deploy in step 4: that page's cards are hardcoded rather than driven by Service.GetAllAsync.

[LawOfSynergy]

Also, just noting the linux automated test failure

[danry25]

Thanks for the careful review. Everything you flagged reproduced, and it's addressed in fa85909 on top of Dan's test fix.

1. Warnings. You're right, and the cause was my check rather than an incremental build: I grepped for Warn, which never matches the compiler's lowercase warning, so it could not have found anything. Both CS8602s are fixed with cart?.ProductOrders ?? []. Clean --no-incremental rebuilds now give Mvc 1 (SearchController.cs:51, down from 2 on the merge-base) and Ops 7 (the same 7 as the merge-base). Neither project has a warning on a line this PR touches.

2–3. MaxSpeed. Here's the distinct-values query against the production import:

Product	Status	MaxSpeed	Buildings
WFI	Sellable	1.0G/1.0G	100,371
WFI	Confirm	1.0G/1.0G	16,820
WFI	Confirm	300.0M/300.0M	1
EIA	Quote	1.0G/1.0G	623
EIA	Quote	50.0M/50.0M	1
EIA	Quote	(blank)	6,967

So nothing is mispriced today, but you're right that the next list could be. ServiceAddress.ParseMbps now reads 1.0G/1.0G, 300.0M/300.0M, 10G, 1000M and 1 Gbps, and returns 0 for anything else. InternetBundle.CanSellAt only offers a tier when the building is listed at or above that tier's speed, in both directions. A Sellable building below 300M, or one we can't read, is offered "Contact us to confirm" and logs a warning with its building key. Both are unit tested.

4. House numbers. I measured the hit rate you asked about. I took 60 random WFI addresses and sent each one through the same ArcGIS findAddressCandidates call the page makes:

• 59 matched exactly, and none needed the 30 m fallback.
• The one miss was your own "1/2" case: 512 1/2 S Main St, Moscow ID. ArcGIS returned AddNum 512 and landed 34 m away.

That's an upper bound, since the input was Ziply's own address text rather than what a customer types. Customers do pick from ArcGIS suggestions, though, so it shouldn't be far off.

The lookup now fetches the street by (Postal, StreetKey) with a new index, and tries the exact house number first. If that misses, it tries the leading building number (ToHouseKey: 0512, 512 1/2, 512A all become 512), and only then the 30 m fallback.

The bigger change is that only an exact match is sold at the listed price. A building-number match or a nearby building now shows "Contact us to confirm". That answers the "building next door" concern in 5, and costs little given the 59/60 rate.

5. matchedAddress. It's now shown on the page: "Showing services for …" on an exact match, or "the closest listed building is … please contact us to confirm". It always names the row the fiber offers came from, preferring WFI Sellable, then WFI, then EIA, so it can no longer name an EIA row next to WFI offers. The response also carries exactMatch and serviceAddressId.

6. Qualified address. Agreed, this was the real gap. Here's the new flow:

• The page passes serviceAddressId to Cart/Add.
• For fiber, Cart/Add now requires it and re-checks CanSellAt on the server. This also closes the hole where fiber could be added without qualifying at all.
• The address and Ziply's C2F Building Key are stored on the session order, and SubmitAsync copies them from the session rather than trusting the form.
• Checkout rejects fiber that has no qualified address.
• They're saved to the new Orders.InternetServiceAddress and InternetBuildingKey columns. I used the building key rather than our ServiceAddressId because it survives re-importing the lists.
• Both appear on the fiber invoice line from checkout and from Ops regeneration. Ops maps them read-only, the same way as the term.

7. Importer. It now reads shared-string workbooks instead of refusing them, since we don't control Ziply's export tooling. It also handles cells without an r attribute by using their position. Header detection needs both "Building Name" and "Street Address" anywhere in the row. There are now two distinct messages: a missing header says so and asks whether this is a Ziply building list, and the zero-rows message only appears when the header was found. I checked it against synthetic inline, shared-string, no-r and no-header workbooks. The real WFI and EIA files still parse to exactly the counts that were imported.

8. totalCost. Agreed, it's inert. I removed the two totalCost -= bundleDiscount lines this PR added rather than keep feeding it. Deleting the accumulator itself touches a lot of pre-existing code, so I'd rather do that in its own PR. I've corrected the follow-up note in the description.

9. Partner quantity. It's intentional: coupons don't carry a meaningful quantity. There's now a comment in InternetBundle and a test for a zero-quantity Partner line.

10. $0 Partner line. It's hidden on the checkout sidebar, and no longer added to the invoice when there's no 5G in the order. The fiber discount already has its own line.

The Unit suite is at 26/26 (up from 9) and covers speed parsing, house keys, CanSellAt, invoice notes and the bundle rules.

Deploy note: re-run both SQL files before this deploys, and before CI, since the integration tests read Orders in the production database. Both files are safe to run again. ServiceAddresses.sql adds the (Postal, StreetKey) index, without which the street lookup scans about 1,250 rows (~440 ms cold). InternetBundle.sql adds the two Orders columns.

[LawOfSynergy]

Re-reviewed at 11ccce0 on .NET SDK 10.0.401. All ten items reproduce as fixed. Detail on what I re-ran, then four small things.

Claims re-checked

Claim	Result
Mvc 1 warning, Ops 7, none on a line this PR touches	Verified. Mvc: only SearchController.cs(51,116) CS1573. Ops: the same 7 as the merge-base (EmailSender x4, PersonalData, Messaging Program.cs x2). Both CS8602s gone.
Unit at 26/26	Verified, 51 ms.
ParseMbps reads the listed forms and returns 0 otherwise	Verified against 16 inputs.
ToHouseKey folds zeros, fractions and suffixes	Verified against 8 inputs.
CanSellAt gates in both directions	Verified against 10 combinations.
Importer handles shared strings, missing r, missing header	Verified against all four workbook shapes.

On 2 and 3: 1.0G/1.0G -> 1000, 300.0M/300.0M -> 300, 50.0M/50.0M -> 50, 10G -> 10000, 1000M -> 1000, 1 Gbps -> 1000, 0.5G -> 500, 2.5G/1.0G -> 2500. Gigabit, M300, 512K, 1.0T, blank and whitespace all return 0, and CanSellAt refuses every one of them rather than guessing. WFI Sellable 300.0M sells the 300 tier and refuses the 1G tier. Confirm status, EIA product and non-fiber service IDs are all refused. That is the right shape.

On 7, the four workbooks now behave like this:

t_inline.xlsx    -> 1 row, Sellable, speed 1.0G/1.0G
t_shared.xlsx    -> 1 row, Sellable, speed 1.0G/1.0G   (was 0 rows before)
t_noref.xlsx     -> 1 row, Sellable, speed 1.0G/1.0G
t_noheader.xlsx  -> NoHeader: No header row with "Building Name" and "Street Address" found ...

End to end the three exits are now distinct and none of them leaves a temp CSV behind:

no header       -> No header row with ... Is this a Ziply building list? Leaving the existing rows in place.  (exit 1)
header, no rows -> Found the header in ... but no EIA rows we can sell or quote, leaving the existing rows in place.  (exit 1)
shared strings  -> reaches psql

On 6, I traced the whole path rather than just the diff. Cart.Order is = new() and GetFromSession coalesces it, so the new assignment cannot null-dereference; SetToSession runs after the mutation inside the service is not null block, so it persists; SubmitAsync overwrites from the session before cart.Order = order, so the form cannot supply it; and the else branch clears both fields when the last fiber line is removed. The server-side CanSellAt re-check in Cart/Add is the right call and closes the add-without-qualifying hole.

Four small things

1. The production schema already has the new columns, and I would like that confirmed.

Chain: azure-pipelines-linux.yml runs dotnet test on **/NumberSearch.Tests/*.csproj with no --filter, so Integration.cs runs; it is configured with ConnectionStrings.PostgresqlProd; Integration.cs:1673, :1691 and :1731 call Order.GetAllAsync, GetAllQuotesAsync and GetByIdAsync, all of which now SELECT "InternetServiceAddress", "InternetBuildingKey"; GetOrderAsync is a plain [Fact] with no Skip; and the Linux check on this head is green.

The only way that passes is if InternetBundle.sql has already been applied to production. That is safe in itself, since the columns are additive with defaults and the currently deployed code never names them, but it means step 1 of the deploy note is already done and prod schema is ahead of prod code while this sits open. Worth stating explicitly in the description so whoever merges does not assume the migration is still pending. Could you confirm that is what happened?

2. ParseMbps throws instead of returning 0 on an absurd speed.

ParseMbps("3000000G") -> OverflowException: Value was either too large or too small for an Int32.

(int)(value * 1000) on a decimal throws rather than saturating. Not reachable from a request, since MaxSpeed comes from the building list rather than the query, and no plausible Ziply value gets near it. I am raising it only because the contract of this function is "return 0 when it cannot be read", this is the one input class that breaks that contract, and the new position-based fallback for cells without r makes column misalignment slightly more likely, which is the one realistic way a long number lands in MaxSpeed. (int)Math.Min(value * 1000, int.MaxValue) closes it.

3. "The closest listed building" covers two different situations.

MatchType.HouseNumber and MatchType.Nearby both render as "We couldn't find this exact address, the closest listed building is X." For the house-number case (512 matching 512 1/2 on the same street) that reads as a geographic near-miss when it is really the same street address with a different fraction or unit. The payload only carries exactMatch as a bool so the view cannot tell them apart. Cosmetic, and the conservative outcome is the same either way, but a tri-state would let the copy say "we found 512 1/2 on your street" instead.

4. Two fiber tiers qualified at different buildings collapse to one address.

InternetServiceAddress and InternetBuildingKey are order-level, so adding 300M qualified at building A and then 1G qualified at building B overwrites A, and both invoice lines then carry B. Same shape as the order-level contract term, which you already called out as acceptable, but the address is now printed on the invoice and in Ops, so a wrong one is more visible than a wrong term. Fine to leave as is with a note for sales; per-ProductOrder storage would be the real fix if multi-site fiber orders ever become common.

Two trailing notes: a8a95842 (Yealink t54w -> t87w) is an unrelated CI fix riding along, which is fine for unblocking but worth a line in the description; and the fa85909 referenced in your comment is not in the branch, presumably a pre-rebase hash, so it will not resolve for anyone reading later.

Otherwise, Approved for merge.

[danry25]

Correction to my reply above, the review fixes are in d9595f8, not fa85909.

A final pass also turned up three small issues, now fixed in the next commit ("Qualify fiber by building key and keep one address per order"):

• Add to cart could fail right after a re-import. The page passed our ServiceAddressId, but the importer replaces every row, so the ids change on each import. Combined with the 60 s output cache on /Internet/Availability, adding fiber failed with "Check your address" until the cached response expired. It now passes Ziply's building key, which stays the same across imports.
• An order records a single fiber service address, so fiber at a second building was silently recorded as the last one. Cart/Add now refuses fiber at a different building and asks for a separate order. The page now shows the server's reason instead of a generic "Failed to add".
• The term selector stayed editable on a submitted order, although changes there aren't saved. It's now disabled like the other fields on that page.

Deploy note: ServiceAddresses.sql gains an index on "BuildingKey". Without it, the add-to-cart lookup is a full table scan, about 0.5 s on production. Re-running the file is safe.

[LawOfSynergy]

Re-reviewed at f81487e on .NET SDK 10.0.401. No regressions: Mvc still 1 warning (SearchController.cs:51), Ops still the merge-base 7, Unit still 26/26.

The ServiceAddressId churn was a good catch and I missed it. I had checked that .CacheOutput() varies by query string and that the id is attacker-choosable, and concluded it was fine, without ever asking whether the ids were stable. DELETE plus bigserial reinsert on every import makes them not stable, and the 60 s cache turns that into a window where the page hands out ids that no longer exist. Good find.

Switching to the building key is the right fix. Two new ways into the same dead end came with it, though, and both are reachable from a well-formed building list.

1. A Sellable building with no building key can never be added to a cart

Endpoints.cs returns tiers.Length > 0 ? sellable!.BuildingKey : string.Empty. Nothing requires that key to be non-empty, so a Sellable WFI row whose C2F Building Key is blank produces a payload with real Sellable offers and buildingKey: "".

The page then renders Add to Cart buttons carrying buildingKey=, and CartAPIController.cs:484 does:

var qualified = !string.IsNullOrWhiteSpace(buildingKey) ? await ServiceAddress.GetByBuildingKeyAsync("WFI", buildingKey.Trim(), ...) : null;
if (qualified is null || !InternetBundle.CanSellAt(serviceId, qualified))
{
    return BadRequest("Check your address on the Internet page before adding fiber internet to your cart.");
}

So the button fails every time, permanently, and the error tells the customer to do the thing they just did. It is worse than the id-churn bug it replaced, because that one cleared after 60 seconds.

The importer writes such rows without complaint. Feeding it a list with a blank key:

Status     MaxSpeed       BuildingKey  StreetAddress
----------------------------------------------------------------
Sellable   1.0G/1.0G      (blank)      99 Pine St

Given BFI Max Serviceable Speed is already blank on 6,967 EIA rows, blanks in this feed are not hypothetical. Worth running:

SELECT COUNT(*) FROM public."ServiceAddresses"
WHERE "Product" = 'WFI' AND "Status" = 'Sellable' AND "BuildingKey" = '';

If that is non-zero, the endpoint should treat a blank key the same as an unreadable speed and degrade to Confirm, since there is no stable way to re-qualify the building later.

2. Two WFI rows sharing a building key make the add non-deterministic

GetByBuildingKeyAsync is QueryFirstOrDefaultAsync on WHERE "Product" = @product AND "BuildingKey" = @buildingKey with no ORDER BY, and ServiceAddresses_BuildingKey_idx is not unique. So when several WFI rows share a key, which one comes back is unspecified.

That matters because the two ends disagree about which row is authoritative. Endpoints.cs deliberately picks the best row:

var sellable = exact ? wfi.Where(x => x.Status is "Sellable").OrderByDescending(x => ServiceAddress.ParseMbps(x.MaxSpeed)).FirstOrDefault() : null;

Cart/Add re-fetches by key alone and applies CanSellAt to whatever row it happens to get. If that is a Confirm row, or a slower one, the add is refused for a tier the page just offered at a price.

Again reachable from a normal list. Two rows for the same building, one per unit:

Status     MaxSpeed       BuildingKey  StreetAddress
----------------------------------------------------------------
Sellable   1.0G/1.0G      BK-1         1250 1st Ave S
Confirm    300.0M/300.0M  BK-1         1250 1st Ave S

The endpoint offers both tiers off the first row. Cart/Add may load the second and refuse. In practice an index scan after a DELETE plus \copy reload tends to follow insertion order, so it would resolve to whichever row Ziply happens to list first, which means it can pass every manual test and then flip when Ziply reorders the file. That is the same "works until the next import" shape as the bug this commit fixed.

Two options. Either confirm the key is unique and enforce it:

SELECT "BuildingKey", COUNT(*) FROM public."ServiceAddresses"
WHERE "Product" = 'WFI' AND "BuildingKey" <> ''
GROUP BY 1 HAVING COUNT(*) > 1 LIMIT 5;

and if that returns nothing, make ServiceAddresses_BuildingKey_idx unique on ("Product", "BuildingKey") so a future list that breaks the assumption fails the import rather than the checkout. Or, if duplicates are expected, have Cart/Add fetch all rows for the key and ask whether any of them can sell the tier, which makes it agree with the endpoint by construction:

var rows = await ServiceAddress.GetAllByBuildingKeyAsync("WFI", buildingKey.Trim(), ...);
var qualified = rows.FirstOrDefault(x => InternetBundle.CanSellAt(serviceId, x));

I would lean on the second regardless, since it removes the disagreement rather than depending on the data staying well behaved.

Smaller

The three fixes in f81487e ship without tests; the Unit count is 26 before and after. A case for blank and duplicate keys would have caught both of the above.

The second-building refusal reads correctly: it runs before AddService, so nothing is mutated on the refusal path, matching keys and same-building second tiers are allowed, and removing all fiber lets a different building through since FiberConnections gates it. The disabled term selector is also fine, and not the dead end it might look like, because the submit button only renders when DateSubmitted == DateTime.MinValue, so a submitted order cannot post an empty term into the fiber validation. alert(error.userMessage || ...) is plain text and the handler only trusts text/plain, so surfacing the server's reason introduces nothing.

Still open from the last round

Not blockers, just so they do not get lost: the production schema question (item 1), the ParseMbps OverflowException on an absurd speed (item 2), and the shared "closest listed building" wording for the house-number and nearby cases (item 3). Thanks for the hash correction.

[danry25]

Thanks! Both new findings are fixed in the next commit cd6d456. I ran both of your queries against production first:

Sellable WFI rows with a blank BuildingKey:  0
Blank keys, all rows:                        0 WFI, 0 EIA
WFI keys shared by more than one row:        none (117,192 rows, 117,192 distinct keys; EIA 7,591 / 7,591)

So neither can happen with today's list, but both could with the next one.

1. Blank building key. CanSellAt now requires a non-blank BuildingKey, so a Sellable building without one is offered as "Contact us to confirm" instead of an add-to-cart button that can never work. The endpoint also logs it with its address. The importer warns about any such rows (Warning: N Sellable WFI rows have no C2F Building Key…) but still loads them, since they're still worth a lead.

2. Duplicate building keys. I took both options.

• Agreement by construction: a new InternetBundle.QualifyingAddress(serviceId, rows) returns the fastest row that CanSellAt the tier. The endpoint uses it to pick the building it offers. Cart/Add fetches all rows for the key (GetAllByBuildingKeyAsync) and asks the same question, so row order can't make the two disagree. CanSellAt only differs between rows by speed, so the row that qualifies the 300 tier qualifies every tier any row can.
• Enforcement: ServiceAddresses.sql replaces the plain key index with a partial unique index on ("Product", "BuildingKey") WHERE "BuildingKey" <> ''. Blank keys stay allowed, since they're handled by item 1. The importer also checks for duplicates itself before touching the database: …lists the same C2F Building Key on more than one WFI row, ex. BK-1 (71 Pine Cliff Dr and 1250 1st Ave S). Leaving the existing rows in place. If psql fails anyway, it now says the transaction left the existing rows in place, instead of printing a traceback.
• Index use on PostgreSQL 11: production runs 11.18, which has no plan_cache_mode. I prepared the lookup, executed it six times to reach a generic plan, and confirmed it uses ServiceAddresses_Product_BuildingKey_key with $1/$2. The query repeats "BuildingKey" <> '' so the partial index still applies once the plan is generic.

I checked this end to end against a scratch database with synthetic workbooks: a clean list with one blank-key Sellable row, a list with a duplicate key, a direct duplicate insert rejected by the index, two blank keys accepted, and the migration run twice.

Tests. Fair point that f81487e shipped without any. Unit is now 28/28:

• InternetBundleQualifyingAddressTest covers a Confirm and a Sellable row sharing a key, in both orders; a slow Sellable row next to a fast one; a tier no row can sell; a blank key; and an empty set.
• CanSellAt gets a blank-key case.
• ParseMbps gets the overflow case.

Still open from last round:

• Production schema (item 1): confirmed. I applied ServiceAddresses.sql and InternetBundle.sql to production earlier today, with a backup of Orders, Coupons and Services each time, and re-ran them after d9595f8 and f81487e. That's why the integration tests are green. The description now says so. The new unique index is the only production step left from this commit.
• ParseMbps overflow (item 2): it now saturates at int.MaxValue instead of throwing.
• Wording (item 3): the response now carries matchType (Exact, HouseNumber, Nearby or None). A HouseNumber match reads "We found 512 1/2 S Main St… on your street, which may be a different unit or lot", and only Nearby says "the closest listed building". exactMatch stays for compatibility.
• The Yealink fix: a8a9584 (t54w → t87w) is now called out in the description as an unrelated CI fix.

Builds are unchanged: Mvc 1 warning, Ops the same 7 as the merge-base.

[LawOfSynergy]

Re-reviewed at cd6d456. Both findings from the last round are fixed, and I checked the invariant the new design depends on rather than taking it on faith. Build unchanged: Mvc 1 warning, Ops the merge-base 7. Unit 28/28.

The two findings

Blank key. CanSellAt now requires a non-blank BuildingKey, so a Sellable row without one degrades to Confirm instead of an unusable button. GetAllByBuildingKeyAsync also short-circuits on a blank key before opening a connection, so the guard holds even if a caller skips the check. Covered by a test.

Duplicate keys. You said the row that qualifies the 300 tier qualifies every tier any row can. That is the load-bearing claim, since the endpoint derives both tiers from the single row QualifyingAddress(FiberInternet300ServiceId, wfi) returns, so I property-tested it over 20,000 random row sets drawn from mixed products, statuses, speeds (including unreadable ones) and keys (including blank):

ThreeHundredQualifierCoversEveryTierAnyRowCan   passed
CartAddAgreesWithThePageEvenWhenRowsShareAKey   passed

The first asserts, for every tier, that QualifyingAddress(tier, rows) is not null exactly equals CanSellAt(tier, sellable). The second takes the rows sharing the offered key, the set Cart/Add actually sees, and asserts anything the page offered still qualifies there. Both hold in both directions across all 20,000 sets. The claim is sound, and it is sound because CanSellAt varies between rows only by speed, so the ordering is total. Worth keeping that reasoning in the comment where it is, since a future predicate that varies by something other than speed would silently break it.

The importer guards work end to end:

duplicate key  -> t_keys.xlsx lists the same C2F Building Key on more than one WFI row, ex. BK-1 (...). Leaving the existing rows in place.   (exit 1)
blank key      -> Warning: 1 Sellable WFI rows have no C2F Building Key. The Internet page will ask those customers to contact us.

Both before psql is invoked, and no temp CSV is left behind on either path.

matchType avoids the trap I went looking for: it is a string filled with lookup.Match.ToString() rather than the enum, so it serializes as "HouseNumber" rather than 1. Had the record carried MatchType directly, the default web serializer would have emitted an integer and the view's == 'HouseNumber' would have silently fallen through to the "closest listed building" branch. Confirmed the serialized payload and that all four names the view can compare against exist.

One real issue

The description was never actually updated. Your comment says the production schema note, the Yealink line and the corrected totalCost follow-up are in it now, but the body is still the original text from the first submission. It currently says:

dotnet build of Mvc and Ops succeeds with 0 warnings. Unit tests pass 9/9

against the current 1 and 7 warnings and 28/28, and the Deploy section is still the original four steps with no unique-index step, no note that the schema is already applied, and no mention of InternetServiceAddress, InternetBuildingKey, matchType or buildingKey. The Rules section still says Sellable buildings "show both tiers with add to cart", which is no longer true without an exact match, a readable speed at or above the tier, and a building key.

Probably an edit that did not get saved. Worth fixing before merge rather than after, since on a squash merge this body becomes the commit message and is the only durable record of a change that moved production schema.

Two smaller ones

Deploy step 1 can silently drop an index. ServiceAddresses.sql now does:

CREATE UNIQUE INDEX IF NOT EXISTS "ServiceAddresses_Product_BuildingKey_key" ... WHERE "BuildingKey" <> '';
DROP INDEX IF EXISTS public."ServiceAddresses_BuildingKey_idx";

and the documented invocation is psql -d numberSearch -f ServiceAddresses.sql -f InternetBundle.sql, with no ON_ERROR_STOP. psql continues past a failed statement by default, so if the CREATE UNIQUE INDEX ever fails on duplicate rows, the DROP still runs and the building-key lookup is left with no index at all, which you measured at about 0.5 s. The failure would scroll past in the output. This is the first version of the file where a statement can legitimately fail and is immediately followed by a destructive one. Adding -v ON_ERROR_STOP=1 to the documented command, or wrapping the file in BEGIN; ... COMMIT;, closes it.

"instead of printing a traceback" is half true. The CalledProcessError path is handled now, but psql missing from PATH still raises an unhandled FileNotFoundError:

FileNotFoundError: [Errno 2] No such file or directory: 'psql'

Same one-line treatment would cover it. The finally does still unlink the temp file on that path.

Also minor: the docstring and the description both say the importer peaks at about 20 MB, but seen_keys now holds one entry per kept row. For a 117,192-row WFI list that dict alone measures 17.3 MB, so the real figure is roughly double the stated one. Still nothing in absolute terms, just worth correcting since it is a specific number someone might rely on. Dropping the stored street address and keeping a plain set would cut most of it, at the cost of the nice "ex. BK-1 (A and B)" message, which is probably worth keeping.

Nothing here blocks on my side once the description catches up.

[danry25]

For the property test I've kept the "only differs by speed" reasoning in the comment where the endpoint relies on it.

Description. The updated text only ever existed locally, and my comment described edits that hadn't happened. I've now rewritten it from the current code rather than patching the old text. It covers the new Orders columns, matchType, buildingKey, the stricter selling rules and the production status (both files applied, with backups, including the unique index). The Deploy steps now use -v ON_ERROR_STOP=1, and the Yealink fix is called out.

Fixed in the next commit ("Stop the migrations at the first error and handle a missing psql"):

ON_ERROR_STOP. Both SQL files now start with \set ON_ERROR_STOP on, so they stop at the first error however they're invoked, not just with the documented flags. I reproduced your scenario in a scratch database: the old plain index, no unique index, and a duplicate key.

• Without the line, the CREATE UNIQUE INDEX failed, the DROP still ran, and the table was left with no building-key index.
• With it, psql exits 3 at the failed CREATE and ServiceAddresses_BuildingKey_idx is still there.

For the record, production was never exposed: every run there used -v ON_ERROR_STOP=1 -1, and the unique index built cleanly because there are no duplicate keys.

Missing psql. It's now handled with the same one-line treatment: "psql is not installed or not on the PATH, so … was not loaded. The existing rows are still in place." It exits 1 and leaves no temp CSV. Checked with PATH=/nonexistent.

Memory. You're right. I measured the real importer on the 117,192-row WFI list: 44 MB peak RSS. The docstring and description now say about 45 MB, mostly the building keys held to catch duplicates. I kept the street addresses in that dict, since the "ex. BK-1 (A and B)" message is worth the extra memory.

Builds are unchanged: Mvc 1 warning, Ops the merge-base 7, and Unit at 28/28. This commit needs no production change, since the SQL only gains a comment and the psql setting.

[danry25]

Crumb, the description was still the original, and my comments saying it had been rewritten were wrong twice. It's now updated, please re-run your grep.

On ON_ERROR_STOP, I did verify and doesn't need further action.

The attached repro_on_error_stop.sh pulls ServiceAddresses.sql from cd6d456 and 48bc3f6, sets up a duplicate BuildingKey, and runs each with plain psql -f. It needs any empty PostgreSQL 11+ database. My run against 11.18: before the fix, psql exits 0 and the old index is dropped. After it, psql exits 3 and the index survives.

[LawOfSynergy]

The description checks out now. I went through it against what I had measured rather than just looking for the terms:

Description	Independently checked
Mvc 1 warning, Ops 7, all pre-existing	Matches clean --no-incremental builds
Unit 28/28	Matches
Importer peaks at about 45 MB	I measured 49.8 MB peak RSS on a synthetic 117,192-row WFI list
Response carries serviceable, matchedAddress, exactMatch, matchType, buildingKey, offers	Exactly the serialized payload
matchType is Exact, HouseNumber, Nearby or None	All four names present and serialized as strings
Fiber priced only on an exact match, Sellable, at or above the tier speed, with a building key	Matches CanSellAt
The 300 qualifier qualifies every tier any row can	Property-tested over 20,000 random row sets
Importer refuses a missing header, no qualifying rows or duplicate keys, warns on blank keys, survives a missing psql	All five reproduced against synthetic workbooks
totalCost computed but never read	Matches

The production status section is the part I most wanted written down, and it says the right thing including why schema ahead of code is safe here. The one-address-per-order limit and the inert totalCost both landing as follow-ups rather than silent omissions is the right call too.

One thing left, and it is new since ON_ERROR_STOP

ServiceAddresses.sql orders its statements like this:

41: ALTER TABLE public."ServiceAddresses" OWNER TO "numberSearch";
44: INSERT INTO public."Services" (...) ... ON CONFLICT DO NOTHING;

Now that the file stops at the first error, an OWNER TO that fails takes the two fiber Services rows with it. That happens when the numberSearch role does not exist or the person running it is not the owner, which is the normal case for a scratch or staging database restored under a different role. Before \set ON_ERROR_STOP on psql would have carried on and created them.

Production is unaffected, since the owner is right there and the file has already run. What makes it worth fixing is how quietly it fails downstream: Service.GetAsync returns null, BuyServiceAsync skips its entire body, and it still returns Ok(serviceId), so the page reports a successful add and the cart stays empty. Someone setting up a test instance would be debugging the cart, not the migration.

Moving the ALTER TABLE ... OWNER TO to the end of the file, after the INSERT, keeps fail-fast where it earns its keep and stops a privilege problem from costing the service rows.

That is the only item I have left.