Feature/add first order notification

[SimonOmey]

No description provided.

[claude]

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, reopen this pull request to trigger a review.

[JennisDev]

@claude review

[claude]

Claude finished @JennisDev's task in 2m 22s —— View job

---

Code Review — Feature/add first order notification

I reviewed the diff against origin/master. Overall this is solid, well-structured work — the dedup design (single daily_notification_sent_date guard shared between the reminder cron and the first-order path) is clean, and the new tests cover the delete-store behaviour. A few things worth addressing below.

---

🐛 Correctness / data integrity

1. DeleteStoreController hard-deletes products that historical orders may still reference
app/Http/Controllers/Store/DeleteStoreController.php:18-23 deletes products via $product->delete(). But orders.product_id is an unsignedBigInteger with no foreign-key constraint (see 2022_07_04_125552_create_orders_table.php:19 — only store_id got nullOnDelete). So deleting a store's products leaves past orders pointing at now-missing product IDs, silently corrupting order history rather than erroring. Worth confirming with the product owner whether stores with historical orders should be deletable at all, or whether this should soft-delete / be blocked when orders exist. Fix this →

2. Delivery confirmation vs. queued send (NotifyCompany::execute)
NotifyCompany::execute() now returns $channels->count(), and both call sites use $channelsNotified > 0 to decide whether to stamp daily_notification_sent_date. But FirstOrderPlaced/PlaceOrderReminder now implement ShouldQueue, so the count reflects enabled channels, not successful delivery. Locally QUEUE_CONNECTION=sync so it's fine, but on a real queue you'll mark the day as "notified" even if the queued job later fails — suppressing the notification permanently for that day. Consider renaming the return to reflect intent (e.g. enabledChannelCount) and being explicit that this gates on "we had somewhere to send", not "it was delivered".

3. Non-atomic first-order check (minor / concurrency)
AddProductController:33 computes isFirstForDay() before the order is saved, and the daily_notification_sent_date read-then-write at :62-69 isn't atomic. Two near-simultaneous "first" orders could both pass the guard and both notify. With sync queue + typical traffic this is unlikely, but if you want a hard guarantee an atomic conditional update (update ... where daily_notification_sent_date != today) would close the window.

---

🎨 Style / conventions

4. Missing trailing newline — app/Notifications/FirstOrderPlaced.php ends with \ No newline at end of file. Minor, but most other files in the repo terminate with one.

5. Copy nit — Menu.vue:50: "Create here your first store" reads awkwardly; "Create your first store here" is more natural. The can('edit-store') / non-admin callout branch is a nice touch.

---

🤔 Design questions (not blockers)

6. First-order notification is gated on reminder_enabled (AddProductController:63). That couples the new feature to the reminder flag — a company with reminders off gets no first-order ping. If that's intentional (one master switch for all Google Chat nudges), great; just flagging it's not obvious from the column name.

7. PR scope — the title is "first order notification", but the diff also bundles the delete-store feature (DeleteStoreController, route, Products.vue modal, tests), the non-admin Menu branch, an AGENTS.md (+373), and assorted CSS. These are reasonable individually, but mixing them makes the PR harder to review/revert. Not a code issue — just noting for future splitting.

---

✅ Things done well

• Dedup logic in SendReminderNotifications (skip if an order already exists for the day and skip if already notified) is clear and well-commented.
• --test-for-company correctly bypasses the daily_notification_sent_date stamp so tests don't poison real state.
• DeleteStoreController scopes the lookup through company->stores() so cross-company deletes 404 — and there's a test for exactly that. 👍
• Migration has a proper down().

Nothing here is a hard blocker except confirming the intended behaviour of #1 (orphaned order history). Nice work overall.
• feature/add-first-order-notification

[claude]

🟡 The new html { font-size: 105%; } rule in resources/css/app.css carries a trailing comment /* 18px */, but 105% of the browser default 16px resolves to 16.8px, not 18px (18px would need 112.5%). Since the surrounding comment frames this as the single dial that scales the rem-based design system, the misleading annotation invites a future tweak to 'restore' a size that was never actually shipped. One-character fix: either correct the comment to /* 16.8px */ or change the value to 112.5%.

Extended reasoning...

What the bug is

resources/css/app.css line 51 introduces a new html { font-size: 105%; } rule with the inline annotation /* 18px */. The math doesn't line up: browsers use a default root font-size of 16px, so 105% resolves to 16.8px, not 18px. For 18px the value would need to be 112.5% (18 / 16 = 1.125).

Why this matters

The bug is purely a documentation/value mismatch — there is no functional regression — but the immediately preceding comment frames this as the knob future authors are meant to turn:

App-wide base type size. The whole design system is rem-based, so bumping this scales all text, prices and spacing proportionally. Dial this single value to make everything larger/smaller.

That framing actively tells the next person editing this rule to reason about it in absolute pixels (18px → 20px → 125%, etc.). With a misleading /* 18px */ they will either:

1. Trust the comment and 'fix' the value to 112.5% to actually produce 18px — making all text/prices/spacing across the app ~7% larger than what was actually shipped and tested, or
2. Trust the value and use 16.8px as a px reference when calibrating other sizes — propagating an off-by-1.2px error.

Step-by-step proof

1. Open resources/css/app.css:46-51. The rule reads font-size: 105%; /* 18px */.
2. Compute against the browser default: 16px × 1.05 = 16.8px. Not 18px.
3. To verify in a browser: load the app, open DevTools, inspect <html>, and read the computed style — font-size will show 16.8px on any default user-agent. The body and rem-based descendants scale from 16.8px, not 18px.
4. To actually hit 18px, the declaration would need to be font-size: 112.5% (or equivalently font-size: 18px).

Why existing code doesn't prevent it

Nothing else in the stylesheet pins the root unit; the rule is the sole base for the rem-driven design system. There's no test, computed-style assertion, or CSS lint rule that catches a numeric mismatch between a value and a sibling comment, so the discrepancy is invisible until a human reads it.

Impact

Low — purely a comment-vs-value mismatch with no behavioural difference at the moment. But the comment exists specifically to guide future readers tuning the design-system base, which is exactly when the misleading annotation will cause harm.

Fix

One-character change in resources/css/app.css:51 — pick whichever matches the author's intent:

-    font-size: 105%; /* 18px */
+    font-size: 105%; /* 16.8px */

or

-    font-size: 105%; /* 18px */
+    font-size: 112.5%; /* 18px */

If the latter is chosen, note that all text/prices/spacing will scale ~7% larger than what's currently on the branch — visually re-verify before shipping.