Tech test - #5
Closed
alexmatthewowen wants to merge 26 commits into
Closed
alexmatthewowen wants to merge 26 commits into
alexmatthewowen wants to merge 26 commits into
Conversation
The rule for "does this listing match these criteria" lived inline in ListingController::index. Saved searches and the nightly alert run both need the same rule, so pull it out before adding either. ListingCriteria holds it once and exposes it three ways: applyTo() for querying listings, matches() for a listing already in hand, and rules() so the filter form and the saved-search form validate identically. applyTo() and matches() are separate implementations of one rule, so ListingCriteriaTest asserts directly that they select the same listings across a grid of boundary values rather than trusting them to agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
Criteria are stored as discrete nullable columns rather than a JSON blob so the nightly alert run can match listings against them in SQL, and so the values stay typed. SavedSearch::matching() is ListingCriteria::applyTo() inverted: given a listing, which searches want it. Region needs no join because the listing is already in hand. Its created_at clause is the backfill policy — you hear about listings that go live after you save a search, not the whole market — and SavedSearchTest asserts the inversion against the criteria it mirrors rather than trusting it by eye. The unique index on alerts (user_id, listing_id) is the anti-spam guarantee: one listing, one alert, however many searches matched it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
Alerts are produced by a scheduled command rather than an event fired when a listing goes live, because delivery has to be batched — twenty listings published across a Tuesday must not be twenty interruptions. The alert records stay granular so the alerts page can list properties individually; only delivery collapses, into one digest per buyer per run. Batching also filters out churn: a listing that goes live and sells before the run is never in the result set, so we don't advertise a property that has already gone. The run selects on the absence of an alert rather than on a time window, so it is idempotent — a missed day is caught up by the next run instead of being lost, and running it by hand to demo the feature is safe. The seven-day window only bounds the scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
The criteria validation rules are shared with the browse filters, so a search can only be saved for something the site can actually search for. An empty search is refused: it would match every new listing, which is the one thing Support asked us not to build. Searches are always reached through their owner rather than looked up by id and then checked, so an unscoped query cannot be written by accident and another buyer's search is a 404 by construction. That is why there is no policy. Saving does not backfill alerts. The saved-searches page links each search to the browse results for its criteria instead, so "what matches right now" is one click away while alerts stay meaningful as "new since I asked". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
Alerts are listed newest first, each naming the search that matched and reusing the browse page's listing card. A property can sell or be withdrawn after we alerted on it, and the detail page only serves live listings — so a stale alert renders as "no longer on the market" rather than offering a link that would 404. Marking read is an explicit action rather than a side effect of loading the page, so the nav badge behaves predictably when paging back and forth. Alerts whose search has since been deleted still appear: we already told the buyer about the property. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
The shared unread count was nested under an `alerts` prop, which the alerts page's own `alerts` prop silently overrode — so the nav badge disappeared on the one page it matters most. It is now a top-level scalar, with the regression test covering all three pages rather than just the browse page. Also corrects the command's summary: NewListingMatches is queued, so digests are queued rather than sent. NOTES.md covers the decisions and where this wouldn't hold up; the README explains how to run alerts:dispatch and read the digest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
The first version issued one saved-search query per new listing and a firstOrCreate per alert. Benchmarked, that was 1,544 queries and 12.2s for 200 saved searches against 20 listings, and 1,000 searches against 50 listings did not finish inside two minutes. Matching listings to saved searches is a join, so do it as one: INSERT ... SELECT over listings x saved_searches. The same dataset is now 1 query and 13.5ms; 50,000 searches against 500 listings produces 4.8M alerts in 10.8s, still one statement. Deduplication moved into GROUP BY ... min(s.id), so one alert per buyer per listing with the oldest search credited is now the database's job rather than something PHP does after fetching every matching search. Delivery was the other half. It chunks buyers, marks notified_at once per chunk rather than once per digest, and loads only the five alerts a digest prints plus a count — without that cap one broad search after an outage pulls thousands of models into memory to send five lines. For 5,000 searches against 100 listings: 3,992 queries and 61s -> 198 and 1.9s, with memory flat in alert volume (93.2MB at 95,805 alerts, 96.6MB at 574,830). notified_at replaces "hold every created alert in memory and group it there", and makes delivery restartable: a run that dies part way leaves the rest un-notified for the next run rather than dropping them. Marking before sending makes delivery at-most-once, which is the right way round when the alerts are on the buyer's alerts page regardless. SavedSearch::matching() is gone — the join replaced it, and keeping it would have been a fourth expression of one rule. Its grid test moves to GenerateAlertsTest, where it now asserts the join that actually runs agrees with ListingCriteria::matches() for every combination of set and "any". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
"Buyer" came from the brief's prose, but the codebase only ever says User, so the feature ended up with two words for one thing. Settle on the one the code already uses. Renames Controller::buyer() to currentUser(), and the page tests' buyer() helper to demoUser() — it creates the first user, which is what the auth stub resolves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
ListingCriteria::matches() had no production caller — the application expresses the rule twice, both in SQL: applyTo() for the browse page and the join in GenerateAlerts for the nightly run. matches() existed purely so the tests had something obvious to check those two against. That is a test fixture, so it belongs in tests/. It is now Tests\Support\CriteriaOracle, which also lets it say what it is for instead of carrying a docblock hinting at callers that don't exist. The guard is unchanged: mutating a single <= to < in applyTo(), or >= to > in the alert join, still fails the grid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
DispatchAlerts reads $user->alerts_count, which withCount('alerts')
adds at runtime. PHPStan accepts it because Larastan knows the
{relation}_count convention, so nothing flagged that a reader of the
User model has no way to know the attribute exists.
The other models carry @Property blocks for exactly this reason.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
NOTES claimed the min(s.id) tie-break is why attribution is stable between runs. It isn't. Once a (user, listing) pair has an alert the NOT EXISTS excludes it, so no later run reconsiders it and the credit cannot change — verified by deleting the credited search and re-running, which leaves the alert orphaned rather than handing it to another search that still matches. min() is a within-run tie-break only, and GROUP BY needs an aggregate on that column anyway; the choice it makes is "oldest matching search" rather than "whichever row the database reached first". Corrects the same claim in the GenerateAlerts comment and the test, and pins the orphaning behaviour with a test rather than leaving it as prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
Deleting a saved search leaves its alerts reading "From a search you've since deleted" even when another of the user's searches still matches the listing. The behaviour is tested; this records why it happens and what the fix would be, so it stays a decision rather than a surprise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
NOTES argued that backfill would be "exactly the complaint Support raised", which contradicts the separation the rest of the document rests on: alert records are not notifications, so a backfilled search would still produce one digest. Anti-spam doesn't decide this. What does: backfilled alerts are a frozen copy of a search the user can run live, so the alerts page stops meaning "what's new since I asked"; and backfill makes the table grow with searches x listings already live, paid per search at save time, rather than with new listings x matching searches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
It doesn't. GenerateAlerts references ListingCriteria only in a comment; its join is hand-written SQL restating the same comparisons inverted, because matching every new listing against every search in one statement isn't something an object built for one query at a time can express. NOTES also counted rules() as a third expression of "what matches means". It isn't — it governs which values are valid input, and what the form shares is the field list, not the matching semantics. And the mutation example named the wrong operator for the join, which uses >=, not <=. Adds the cross-reference to GenerateAlerts so anyone changing one copy finds out about the other, and records in NOTES that a test is holding an invariant a type could hold instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
"The knob if you disagree" doesn't say what the knob does or what disagreeing would mean. It is the number of digests a mid-run failure can lose, weighed against how many update statements the run costs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
Four things a review turned up: The listings index issued one branch query per row — 23 queries for a page of 15. Pre-existing (the original controller had no eager load), but it is in a file this branch already touches, so fix it rather than work around it: / now costs 9 queries. Model::preventLazyLoading() is on outside production so the next one fails a test instead of quietly slowing a page; removing the eager load again fails six tests. saved_searches had no index at all. "FKs are indexed" is a MySQL habit and this app runs on SQLite, which does not, so every read of a user's saved searches was a full scan. The alerts index was (user_id, created_at), but the page orders by id — deliberately, because a run inserts every alert in one statement and they share a created_at, so ordering by it would put the same row on two pages. The index could never serve the query it was added for; it is now (user_id, id). alerts:dispatch had no overlap guard. Reading un-notified alerts and marking them notified are separate statements, so two runs would send the same digest twice - the one failure this feature exists to prevent, and the README tells people to run it by hand. The lock lives in the command rather than on the schedule, because withoutOverlapping() would not cover a manual run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
min_bedrooms=0 got past the "at least one criterion" check and saved a search matching bedrooms >= 0 — every new listing, which is precisely what that check exists to prevent. The Min beds input has min="0", so it was reachable from the UI without crafting a request. The guard tested each field with filled(), which is presence, not selectivity: filled(0) is true. It now asks ListingCriteria::isEmpty() instead, and fromArray() drops a zero minimum, since "0 or more bedrooms" is not a filter. One definition of empty rather than two. Also from the review: constrain the delete route to a numeric id, so "404 by construction" survives on Postgres where a non-numeric id would reach findOrFail() as an invalid-integer 500; reattach a docblock that had drifted onto the wrong test; and redirect after saving to the browse results for the saved criteria, so the saved criteria, the URL and the results on screen cannot disagree when saving from an edited form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
preventLazyLoading() does less than my comment said. Builder::hydrate only arms the flag on models from a query that returned more than one row, which is where an N+1 can happen — a lazy load on a single model goes through silently. So Listings/Show really was fetching its branch in a second round trip, and the guard was never going to say so. Also pins alerts pagination: a run inserts every alert in one statement so they share a created_at, and ordering by that would put the same row on two pages. The page orders by id and the index is (user_id, id); 30 alerts across two pages now assert no overlap. Drops a reset handler on the saved-searches form that became dead when store() started redirecting to the browse results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
The NOTES paragraph described the within-a-run failure window but not concurrent runs, which is the other half of the guarantee — and was genuinely broken until the command started taking a lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
Only plain "0" reaches ListingCriteria::fromArray today, because the integer rule rejects "00" and "0.0" first — so the string comparison was correct by coincidence of a rule in another class. Comparing numerically makes it hold on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
The saved_searches.user_id index was added for the saved-searches page, but SQLite chooses join order, so it re-planned the alert run too: it used to drive from the new listings and scan the searches, and now drives from the searches and seeks the listings. The query counts still stand — an index cannot change how many statements run — but the timings were taken against a plan that no longer executes, and repeated seeks are not obviously cheaper than the scan they replaced. Says so rather than presenting stale numbers as current, and records that nothing in the suite would have caught the re-plan. Also notes that swapping the drive order cannot help by itself: the IS NULL OR predicates are on the join condition, not on either table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
The run lock expired after 10 minutes but the docblock and NOTES both said it was held for the length of a run. On a long catch-up — the outage case NOTES itself describes — it would lapse mid-run and the next invocation would re-send the chunks this one had not marked yet. Laravel's locks cannot be extended, so it is sized past the worst case instead, and the comment now says what that costs. The unread badge counts on every page load and had no index covering read_at, so it scanned every alert a user had ever received. Now a covering index on (user_id, read_at). The code comment pointed at NOTES for this; NOTES did not mention it, because I dropped that bullet when rewriting the section. Both fixed. max_price had no upper bound but lands in an unsignedInteger column — an out-of-range insert on MySQL and Postgres, an out-of-domain row here. Capped at a domain bound, the way min_bedrooms is capped at 20 rather than at 255. The Save buttons treated "0" as a filter, so with only Min beds at 0 they offered a request the server would always reject. The rule now lives in one client module mirroring ListingCriteria, used by both pages. Also cuts the "On working with AI" section from NOTES. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
The demo told you to publish "a listing that matches" your saved search but the snippet took an arbitrary draft, which almost never matches a realistic one. Following it verbatim — save "Leeds, under £250k, 3+ beds", run the snippet — publishes a 2-bed in Bristol and prints "No new matches", which reads as the feature being broken. Reordered so it works every time: pick a draft, see what it is, save a search matching it, publish it last. The order is the point, since a saved search only alerts on listings that go live after it was saved, and every seeded listing already predates it. Verified verbatim on a fresh clone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
The review flagged "held for the length of a run" in the DispatchAlerts docblock. The same false claim was in NOTES and I fixed only the code. It has a fixed hour-long expiry, because Laravel's locks cannot be extended once taken — which is a different guarantee, with a different failure mode, and NOTES now says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
criteria.js, tests/Support/CriteriaOracle and the notified_at migration were all added during the work and never made it into the tree. The oracle mattered most: NOTES makes a point of it living outside app/, and the map a reader navigates by did not show it at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtydwJw5gdxRWQHbJcBbro
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.
Completed tech test for "Street Listings" website.
Added a scheduled job which runs every 24 hours, matches listed properties to saved searches and creates alerts for users with new matches (multiple alerts bundled into a single notification).
Users can create and delete searches in the UI, and view the properties they've been alerted on. More details given in NOTES.md.