Tech test

NOTES.md

@@ -0,0 +1,178 @@
+# Notes
+
+Users can save a search, view and delete their saved searches, and see alerts
+for new listings that match. Alerts are generated by a nightly command
+(`alerts:dispatch`) and delivered as one digest per user per run.
+
+## The decision everything else follows from
+
+Support's *"please don't spam people"* is the design driver, and it separates two
+things that look like one:
+
+- **The alert record** — a durable "this property matched your search", one per
+  listing, so the alerts page can list properties individually.
+- **The notification** — the thing that actually interrupts someone. Batched.
+
+Deduplicating per listing only stops the *same* listing alerting twice; twenty
+listings going live across a Tuesday would still be twenty interruptions. So
+alerts come from a **scheduled run**, not an event fired when a listing goes
+live, and one run sends one email per user covering everything it found.
+
+That removed machinery rather than adding it: no `ListingWentLive` event, no
+listener, no publish action, no observer — the run derives "went live" from
+`status` and `listed_at`. It also buys a behaviour an event can't have: a listing
+that goes live and sells before the evening run is never in the result set, so we
+don't advertise a property that's already gone.
+
+**The cost is latency** — up to a day. It is assumed that a latency of up to 24 hours
+until a user is notified of a property listing matching one of their searches is 
+acceptable for the vast majority of users.
+
+## Other decisions
+
+**Idempotent by state, not by time.** The run selects listings with *no alert row
+yet* (`NOT EXISTS`), not "added in the last 24 hours". A time window is fragile —
+one failed run and those listings are lost silently, which nobody notices until a
+user asks why they never heard about a house. Selecting on absence means a
+missed day is caught up by the next run, and re-running by hand is always safe.
+The seven-day window only bounds the scan; it's the recovery budget, not the
+correctness mechanism.
+
+**Duplicate alerts: one, not two.** A listing matching two of your searches is
+still one property. That's a unique index on `alerts (user_id, listing_id)`, so
+it's the database's guarantee, not a rule the writing code has to remember.
+Which search gets credited is a within-run tie-break: `GROUP BY` needs an
+aggregate on that column anyway, and `min(s.id)` makes it the oldest matching
+search rather than whichever row the database reached first. It never changes
+afterwards, but that's the `NOT EXISTS` — once the alert exists the pair is
+never reconsidered, so a later run can't re-credit it either.
+
+**Backfill: no** — and not for anti-spam reasons. Backfilling a broad search
+would create hundreds of alert records, but the digest would still be one email,
+so Support's complaint doesn't decide this either way. Two other things do.
+
+The first is what the alerts list is *for*. Backfilled alerts are a frozen copy
+of a search the user can run live, so the page stops meaning "what's new since I
+asked" and becomes a stale snapshot that decays as those properties sell. The
+browse page answers "what matches right now" properly, and the saved-searches
+page links each search straight to it.
+
+The second is volume. Backfill makes the alerts table grow with
+`searches × listings already live` — paid at save time, per search, unbounded by
+how broad the criteria are — rather than with `new listings × matching searches`.
+That is a much worse shape for a table that only ever grows.
+
+The upside of deciding it this way is that the policy is one clause of the
+matching join (`s.created_at <= l.listed_at`) rather than a special case
+anywhere.
+
+**One set of criteria, but two implementations of the match.** This is the part
+of the design I'd want a second opinion on.
+
+`App\Search\ListingCriteria` is the shared definition: the four fields, their
+validation (`rules()`, used by both the browse filters and the saved-search form,
+which also derives its "at least one criterion" check from the same field list),
+and `applyTo()`, which is how the browse page queries.
+
+The nightly run does **not** use it. Its join is hand-written SQL that
+reimplements the same comparisons inverted — given a listing, which searches want
+it — because the whole point of that rewrite was to match every new listing
+against every search in one statement, which an object built for one query at a
+time can't express. So the rule is written twice in production, and the second
+copy references the first only in a comment.
+
+Nothing structural keeps them honest. What does is
+`Tests\Support\CriteriaOracle`: the same rule stated naively in PHP, slow and
+obvious where the two real ones are fast and clever, with both asserted against
+it across every combination of set and "any". Changing one comparison — `<=` to
+`<` in `applyTo()`, or `>=` to `>` in the join — fails the grid in both cases.
+
+That's a test holding an invariant that a type could hold instead, and it's the
+first thing I'd fix given more time: have the join built from `ListingCriteria`
+rather than restating it, so adding a fifth criterion is one change rather than
+two and a hope.
+
+**Criteria are columns, not JSON** — the run matches in SQL, which needs real
+columns, and it keeps the values typed. Cost: a migration per new criterion.
+
+**Which criteria.** The same four the browse page has. A price *range* is
+probably better than a max, but a saved search should be expressible as a browse
+search, so I'd add `min_price` to both at once or neither. What I'd add first is
+a location filter finer than region — "Leeds" is a big place to be alerted about.
+
+**Delivery is at-most-once.** Alerts are marked `notified_at` before the digest
+is queued, one statement per chunk of users. If a run dies mid-chunk those
+users miss that digest rather than getting two — the alerts are on their alerts
+page either way, so a lost email costs less than a duplicate one, which is the
+whole point of the feature. How many users a crash could affect is exactly the
+chunk size — `USERS_PER_CHUNK`, currently 100 — so that number is the trade
+between how many digests a failure can lose and how many update statements the
+run costs.
+
+That only holds within one run. Selecting the un-notified alerts and marking
+them are separate statements, so two overlapping runs would both pick up the
+same alerts and both send — and the README encourages running the command by
+hand, which is exactly when it could collide with the schedule. The command
+takes a lock, which lives in the command rather than on the schedule entry
+because `withoutOverlapping()` guards scheduled invocations against each other
+and would do nothing about a manual run.
+
+Laravel's locks can't be extended once taken, so the lock has a fixed hour-long
+expiry rather than lasting exactly as long as the run. A run that outlives it
+would let the next one in and duplicate the digests it hasn't marked yet — so
+the hour is sized past the worst case, at the cost of a hard-killed process
+blocking the next run for that long. A run approaching an hour means the
+catch-up window is too big to clear in one pass, not that the hour is too short.
+
+**Ownership is structural.** These records are only reached via
+`$user->savedSearches()` / `$user->alerts()`, and `destroy` resolves through the
+relationship rather than route-model binding, so another user's record is a 404
+by construction. Hence no policy: there's no unscoped query to protect.
+
+## Where it wouldn't hold up
+
+- **The join is a nested loop.** Every criterion is `IS NULL OR …`, which no
+  index can serve, so cost is O(new listings × saved searches) whichever side
+  drives — the predicates sit on the join condition, not on either table, so
+  swapping the drive order doesn't remove them. Fine for a hundred listings a
+  day against a hundred thousand searches; not fine an order of magnitude past
+  that. The fix is to store saturating sentinels instead of NULL (`''` for any
+  region, the column max for any price, `0` for any bedrooms) so all four become
+  plain comparisons and a composite index can drive the join; `EXPLAIN QUERY
+  PLAN` confirms that turns the `SCAN` of `saved_searches` into a `SEARCH`. I
+  didn't do it because it trades the clarity of "null means any" for
+  index-ability, and the measured numbers don't demand it yet.
+- **Nothing pins the query plan.** SQLite chooses the join order, so an index
+  added for one page can silently re-plan the alert run — which is exactly what
+  happened, see the caveat above. There is no test or benchmark in the suite
+  that would catch that, and adding one is the thing I'd want before tuning any
+  further.
+- **The unread badge counts on every page load.** It is a shared Inertia prop,
+  so it runs even on pages that never show it. It is now indexed on
+  `(user_id, read_at)`; before that it scanned every alert the user had ever
+  received to count the unread ones. That makes four indexes on a table the
+  nightly run bulk-inserts into, which is the cost side of the trade.
+- **One digest per user is still one email per user.** Nothing here changes
+  that; it's the floor.
+- **The numbers above are SQLite.** On Postgres the `notified_at` round trip
+  could go away entirely via `INSERT … RETURNING`.
+- **The alerts table only grows.** It needs archiving long before any of the
+  above bites.
+- **`(user_id, listing_id)` unique forever** means a property withdrawn and
+  re-listed a year later never alerts again. Probably right; arguably wrong.
+- **Deleting a search orphans the alerts it produced.** They are kept — we did
+  tell the user about those properties — but they render as "From a search
+  you've since deleted" even when another of the user's searches still matches
+  the same listing, because deduplication is on `(user_id, listing_id)` and the
+  run never revisits a pair that already has an alert. Defensible, since an
+  alert records *that* we told them rather than *why*, but it reads oddly. The
+  fix is to re-point the alert at another matching search on delete rather than
+  nulling it; I left the behaviour alone and pinned it with a test
+  (`test_a_deleted_search_is_not_replaced_by_another_that_matches`) so it's a
+  decision rather than a surprise.
+
+## Left out deliberately
+
+Real email (`MAIL_MAILER=log` renders the digest so you can read it); front-end
+tests, so the Vue is covered only through the props its pages receive; editing a
+saved search; pausing one instead of deleting it.

README.md

@@ -4,6 +4,8 @@ A small property-listings application: a Laravel 13 API with a Vue 3 front end.
 
 This repository is the starting point for a technical exercise. **Your task is described in [`TASK.md`](TASK.md).** This README covers what's here and how to run it.
 
+Saved-search alerts have been built on top — the decisions and trade-offs are in [`NOTES.md`](NOTES.md).
+
 ## Requirements
 
 - PHP 8.4
@@ -44,30 +46,47 @@ All three run in CI (`.github/workflows/ci.yml`) on push and pull request, and a
 
 ```
 app/
+  Alerts/                GenerateAlerts (the nightly matching run)
+  Console/Commands/      DispatchAlerts (alerts:dispatch)
   Enums/                 PropertyType, ListingStatus
   Http/
-    Controllers/         ListingController
-    Requests/            ListingIndexRequest (filter validation)
-    Resources/           ListingResource, BranchResource
+    Controllers/         ListingController, SavedSearchController, AlertController
+    Requests/            ListingIndexRequest, StoreSavedSearchRequest
+    Resources/           ListingResource, BranchResource, SavedSearchResource, AlertResource
     Middleware/          ActAsDemoUser (auth stub — see below)
                          HandleInertiaRequests (shared props)
-  Models/                Branch, Listing
+  Models/                Branch, Listing, SavedSearch, Alert
+  Notifications/         NewListingMatches (the digest)
+  Search/                ListingCriteria (one definition of "matches")
 database/
-  factories/             BranchFactory, ListingFactory (with states)
-  migrations/            branches, listings
+  factories/             BranchFactory, ListingFactory (with states),
+                         SavedSearchFactory, AlertFactory
+  migrations/            branches, listings, saved_searches, alerts,
+                         alerts.notified_at
   seeders/               DatabaseSeeder
 resources/js/
   pages/Listings/        Index.vue, Show.vue
+  pages/SavedSearches/   Index.vue
+  pages/Alerts/          Index.vue
   components/            AppLayout, ListingCard, ListingFilters, Pagination
+  criteria.js            what counts as a filter, mirroring ListingCriteria
+  format.js              price and date formatting
   app.js                 Inertia entry point
-routes/                  web.php
-tests/Feature/           ListingPageTest, ListingTest
+routes/                  web.php, console.php (the daily schedule)
+tests/Feature/           ListingPageTest, ListingTest, SavedSearchTest,
+                         SavedSearchPageTest, AlertPageTest,
+                         GenerateAlertsTest, DispatchAlertsCommandTest
+tests/Unit/              ListingCriteriaTest
+tests/Support/           CriteriaOracle (the rule stated naively, to test the
+                         two SQL versions against — see NOTES.md)
 ```
 
 ### The domain
 
 - **Branch** — a name and a region.
 - **Listing** — address, price, bedrooms, bathrooms, `property_type`, `status` (`draft` / `live` / `under_offer` / `sold`), a branch, and a `listed_at` date.
+- **Saved search** — a user's standing criteria (`property_type`, `max_price`, `min_bedrooms`, `region`), any of which may be null meaning "any".
+- **Alert** — a record that a listing matched one of a user's saved searches. One per user per listing, enforced by a unique index.
 
 ### The stack
 
@@ -77,14 +96,57 @@ Laravel + **Inertia** + Vue 3 + Tailwind — the same shape as our internal apps
 | ------ | -------------------- | ----------------- | ---------------------------------------------------------- |
 | GET    | `/`                  | `Listings/Index`  | Live listings, paginated. Filters: `property_type`, `max_price`, `min_bedrooms`, `region`, `per_page`. |
 | GET    | `/listings/{listing}`| `Listings/Show`   | A single live listing. Non-live listings 404.              |
+| GET    | `/saved-searches`    | `SavedSearches/Index` | The current user's saved searches.                     |
+| POST   | `/saved-searches`    | —                 | Saves a search, then redirects to the browse results for it. Refuses one with no criteria. |
+| DELETE | `/saved-searches/{savedSearch}` | —      | Deletes one of the current user's searches.                |
+| GET    | `/alerts`            | `Alerts/Index`    | The current user's alerts, newest first, paginated.        |
+| POST   | `/alerts/read`       | —                 | Marks all of the current user's alerts read.               |
 
 Filters live in the query string, so a search is shareable, bookmarkable and survives the back button. `pages/Listings/Index.vue` seeds its form from the `filters` prop and re-issues a `router.get` on submit.
 
 ### Authentication
 
 Auth is **stubbed**. A real deployment would authenticate requests properly; to keep this exercise focused on the feature rather than on auth plumbing, every request is resolved as the seeded demo user (`demo@street.example`) via `App\Http\Middleware\ActAsDemoUser`, and shared to the front end as the `auth.user` prop. Build any user-scoped work against `$request->user()` / `auth()->user()` as you normally would — it will return the demo user.
 
-Note the resolver returns `null` until the database is seeded, so run `php artisan migrate --seed` before you start.
+Note the resolver returns `null` until the database is seeded, so run `php artisan migrate --seed` before you start. User-scoped pages (`/saved-searches`, `/alerts`) return 403 until then, which is the same shape as an unauthenticated request.
+
+### Saved-search alerts
+
+A user saves a search from the browse page ("Save this search") or from `/saved-searches`. Alerts are **not** generated the moment a listing goes live — they come from a command that runs once a day, so a user gets one digest rather than a message per property. See [`NOTES.md`](NOTES.md) for why.
+
+```bash
+php artisan alerts:dispatch
+```
+
+The command is idempotent: it selects listings that have no alert row yet rather than "listings added today", so a missed day is caught up by the next run and running it by hand is always safe. It is scheduled for 18:00 daily in `routes/console.php`.
+
+Matching is a single `INSERT ... SELECT` — one statement for the whole run, whatever the volume — and delivery chunks users, marking `alerts.notified_at` as it goes so a run that dies part way through resumes rather than re-sending. See [`NOTES.md`](NOTES.md) for the numbers.
+
+To see the whole thing end to end on a fresh database:
+
+```bash
+php artisan migrate:fresh --seed
+
+# The order matters: a saved search only alerts on listings that go live after
+# it was saved, and every seeded listing already predates it. So pick a draft
+# first, save a search that matches it, and publish it last.
+
+# 1. Pick a draft and see what it is:
+php artisan tinker
+>>> $listing = App\Models\Listing::with('branch')->where('status', 'draft')->first();
+>>> [$listing->branch->region, $listing->price, $listing->bedrooms, $listing->property_type->value];
+
+# 2. Save a search matching those at http://127.0.0.1:8000/saved-searches.
+
+# 3. Publish it, back in the same tinker session:
+>>> $listing->update(['status' => 'live', 'listed_at' => now()]);
+
+php artisan alerts:dispatch     # creates the alerts, queues one digest per user
+php artisan queue:work --once   # delivers it (or run `composer dev`, which
+                                # keeps a worker running)
+```
+
+The alert appears at `/alerts`. `NewListingMatches` is a queued notification, so the command reports digests as *queued* — with the default `MAIL_MAILER=log` the worker renders the digest into `storage/logs/laravel.log`, where you can read what the user would have received. No real email is sent and no queue worker is required for the alert records themselves.
 
 ---