mirror of
https://github.com/FOSSBilling/FOSSBilling.git
synced 2026-11-09 21:43:22 +09:00
Adam Daley
69b10d3858
* Fix missing gap between card-footer action buttons The card-footer wrapper in partial_admin_form_card.html.twig used d-flex justify-content-end without a gap utility, unlike the form_header_actions wrapper in the same file which already has gap-2. This caused action buttons (e.g. Go Back / Delete / Update) to render flush against each other with no spacing. Adds gap-2 and flex-wrap, matching the header actions wrapper. Affects every admin page that uses this shared partial. * Fix cookie consent info banner text layout The .alert component in Tabler is display:flex with a row gap, meant for icon/content children. The info banner had a bare text node as a direct child alongside <strong> and <code>, so the text node became its own anonymous flex item and got squeezed into a narrow column instead of flowing normally. Wraps the descriptive text and the <code> path together in a single span so the alert only has two flex children (label + content), matching the pattern used elsewhere (e.g. Activity settings). * Fix crash on admin Service Management tab for unprovisioned orders admin.order_service() legitimately returns null when an order has no active service yet (e.g. status pending_setup), as documented by Order\Service::getOrderServiceData(). The admin order management template included the service partial unconditionally, so every service module's *_manage.html.twig (e.g. Servicedomain's, which dereferences service.domain) fatally crashed with a Twig RuntimeError instead of rendering. The client-side equivalent already guards this by only including the partial when order.status == 'active'; mirror that intent on the admin side with an explicit null check and a friendly placeholder message, since staff can view orders in any status. Fixes: Twig\Error\RuntimeError accessing "domain" on null in mod_servicedomain_manage.html.twig, reproducible via /admin/order/manage/2 (a pending_setup domain order). * Show which preview build is available on the update page The dashboard's 'Update Available' alert names the specific preview build (e.g. 'FOSSBillingb60fbcais available for download.'), but /admin/system/update never surfaced that version — it always showed the same generic 'Preview Build Updates' copy regardless of whether admin.system_update_available was true, because updateCandidateAvailable is forced true by isPreviewBranch alone. Surfaces update_info['version'] on the update page so it echoes the same version the dashboard alert names, and distinguishes the 'a new build is available' case from 'already on the latest build' instead of always presenting the same install prompt either way. * Ensure event listeners are connected before the first cron run Every application event (staff/client login history, invoice and order lifecycle hooks, antispam checks, etc.) depends entirely on listener rows in extension_meta that are normally (re)connected by the cron job's hook_batch_connect task. Nothing seeds these at install time, so on a fresh install extension_meta has zero listener rows until cron has run at least once - until then every fired event silently does nothing. In production this is surfaced via a dashboard danger alert (System\Service, gated on Environment::isProduction()), but the gap itself remains: any logins (or other events) that happen before the first cron tick are permanently unrecorded, and the condition is invisible in dev/staging environments where that alert doesn't show. Reproduced on this ddev instance: extension_meta had 0 listener rows and setting.last_cron_exec had never been set, so /admin/staff/logins stayed empty despite successful logins. Adds Hook\Service::hasConnectedListeners() and has the staff login flow connect listeners on demand (bypassing the event system, mirroring how cron's hook_batch_connect task itself calls batchConnect() directly) whenever none are connected yet, so logins are never silently unaudited while cron is catching up. The check is a single cheap query and short-circuits permanently once cron has run for the first time. * Fix Illegal invocation crash when jumping to a tab from a note Both the client Notes summary row and the transaction Note summary row rendered their note text as <a data-bs-toggle="tab" href="#tab-x">, intending a click to jump to the corresponding Edit/Notes tab. Bootstrap's Tab plugin assumes any data-bs-toggle="tab" trigger lives inside the real .nav tab list it belongs to, so it can walk to sibling nav items; these links live in an unrelated summary table instead, so Tab._getChildren() throws 'TypeError: Illegal invocation' when clicked, instead of switching tabs. Reproduced via /admin/client/manage/10, clicking the client's 'Suspended - unpaid invoices' note. Replaces data-bs-toggle="tab" on these two links with a plain data-tab-jump marker, and adds a small delegated click handler in fossbilling.ts that reuses the existing showTabById() helper (already used for hash deep-linking) to activate the real nav-link instead. Rebuilt the admin_default theme JS bundle. * Use the notes icon instead of headset for note indicators The client Notes row, the transaction Note row, and the order Notes row all used the #headset icon (a support-agent glyph) to mark a note field, which made no visual sense. #headset is otherwise only used on the dashboard's support ticket tile, where it is appropriate. Adds the 'notes' icon to the admin_default sprite manifest (not previously included) and swaps all three note indicators to it. Rebuilt the theme's icon sprite and JS/CSS bundles. * Fix PurgeCSS stripping popover/tooltip styles The shared PurgeCSS safelist covers every other Bootstrap JS-injected component (toast, modal, dropdown, collapse, offcanvas, accordion, carousel, ...) but not tooltip/popover. Since .popover-header, .popover-body, .tooltip-inner etc. never appear as literal text in any scanned .twig/.js/.ts file (Bootstrap injects them into the DOM at runtime), PurgeCSS stripped their rules from the built CSS, leaving popovers and tooltips with zero padding, no background, and no header/body divider. Reproduced via the Discord popover shown on first admin login (/admin dashboard) and the theme-toggle tooltip - both rendered as unstyled, borderless text. Adds /^tooltip/ and /^popover/ to the same standard safelist used for the other JS-injected components. Shared by both admin_default and huraga, so it fixes both themes. Rebuilt admin_default and huraga assets to verify. * Bold the version in the update-available message on the Update page The Update page (/admin/system/update) read 'FOSSBilling <version> is available for download.' with the version in plain text. Bolds it using the fragment pattern already used elsewhere in the codebase (e.g. mod_product_manage.html.twig): literal <strong> around an auto-escaped variable, no raw HTML needed. (The same change was attempted for the dashboard alert too, but that message is a single PHP string rendered generically for several unrelated alert types, and the split-based workaround needed to bold just part of it didn't render well - reverted, dashboard alert stays plain text.) * Trim jump-to-tab comment to match file's section-header convention Every other //===== Section =====// block in this file goes straight to code with no prose underneath; the jump-to-tab handler added in08c133bd1was the only one with a multi-line explanation. Removed it - the section header plus the code (checking data-tab-jump, delegating to the existing showTabById()) already says enough. * Address CodeRabbit review: serialize batchConnect(), guard click target Two findings from PR #4172's review: - Hook::batchConnect() rebuilds the listener set with a delete-then- incremental-insert-and-flush loop and no uniqueness constraint on extension_meta. hasConnectedListeners() (added for the staff-login fix) can observe a partially rebuilt set, and two callers racing (e.g. cron's hook_batch_connect overlapping an on-demand rebuild triggered by a login) can interleave connect()'s check-then-insert and create duplicate listener rows, which Box_EventManager would then dispatch more than once. A full fix (a DB-level unique constraint plus a migration for existing installs) is real but out of scope for this batch of fixes. Serializes batchConnect() itself with a MySQL/MariaDB advisory lock (GET_LOCK/RELEASE_LOCK) instead: cheap, needs no schema change, and closes the concrete correctness bug (duplicate rows, and hence duplicate dispatch) since connect()'s check-then- insert can no longer interleave across callers. A second caller arriving mid-rebuild still won't wait for it to finish, so a concurrent login in that narrow first-boot window can still fire once before the rebuild it triggered completes - deliberately not chasing that last sliver for the same reason, and it self-resolves for every login after the in-flight rebuild finishes. Updated the existing 'batch connects' unit test's connection mock to distinguish the new GET_LOCK query from the pre-existing listener-exists check, and added a test for the lock-contention path (batchConnect() returns true without touching the listener set when another process holds the lock). - fossbilling.ts's jump-to-tab click handler called event.target.closest() unguarded. The file uses @ts-nocheck, so nothing catches event.target being a non-Element at compile time; guard it explicitly. * Address CodeRabbit review: make batchConnect's rebuild atomic Follow-up to1515cb2: the lock-contention branch was reporting success (true) even when it never ran a rebuild, so hasConnectedListeners() could still be false right after Staff::login() called batchConnect(). More importantly, even the happy path could let a concurrent reader observe a partially rebuilt listener set, since connect() commits each row with its own flush() as the rebuild loop runs. Wraps the rebuild (_disconnectUnavailable() plus the connect() loop) in a single DB transaction via Connection::transactional(), so hasConnectedListeners() can only ever observe the previous complete set or the new complete set, never one row of a half-finished rebuild. The lock-timeout branch now returns false instead of true, so a caller that depends on the return value can tell initialization didn't happen. Staff::login() now retries once if the first batchConnect() call reports failure, rather than firing the login event against a set we know is incomplete. A single bounded retry, not a wait/poll loop - GET_LOCK already blocks for its own timeout before failing, and retrying indefinitely would risk turning this housekeeping step into a login-availability problem. Updated Hook's 'batch connects' test to stub Connection::transactional (now required for the rebuild to run) and renamed/adjusted the lock-contention test for the corrected false return. Added a Staff test asserting login() retries batchConnect() once and still succeeds when the second attempt connects the listeners. * Address CodeRabbit review: assert lock release, don't silently drop failure Two follow-ups to447e0c0f5: - Hook's 'batch connects' test only had a byDefault() expectation on executeStatement(), so it couldn't tell a leaked GET_LOCK (never released) from a correct run. Added a specific expectation that RELEASE_LOCK is called exactly once on the success path. - Staff::login() retried batchConnect() once but silently proceeded if both attempts failed, with nothing recording that the listener set was left unavailable. Considered making login itself wait or fail in that case, per the suggested fix, but that would turn a rare missed audit entry into every admin being unable to log in while a lock is stuck - a worse failure mode than the one this PR set out to fix. Logs a warning instead so the condition is visible to operators without making login availability depend on this housekeeping step. Restructured the retry into sequential statements rather than one chained boolean expression, since PHPStan's booleanNot.alwaysTrue rule doesn't know batchConnect() is stateful and flagged the second identical call as dead code. Added a test asserting login still succeeds (with the warning logged) when both attempts fail, documenting the trade-off explicitly.
Frontend and Theme Builds
FOSSBilling builds shared browser assets from frontend/ and theme assets from each theme workspace.
Build Commands
npm run buildchecks frontend source, builds core assets, and builds all themes.npm run build:productionruns the same build withNODE_ENV=production.npm run checkruns lightweight JavaScript syntax checks and validates theme icon manifests.npm run build-admin_defaultandnpm run build-huragabuild a single theme.
Generated assets are written to src/public/assets and src/themes/*/assets/build. These paths are ignored by Git and are copied into release artifacts by the Docker build.
Theme Dependencies
Themes own the UI packages they consume. Shared tooling in frontend/tools can help compile assets, generate sprites, and post-process CSS, but it should not make a theme depend on a specific UI package.
Examples:
admin_defaultowns Tabler, Coloris, Chart.js, Litepicker, Tom Select, Flag Icons, and its Tabler icon package.huragaowns Bootstrap, Tom Select, Flag Icons, and its Tabler icon package.svg-spriteis a root development dependency because the shared sprite helper imports it.
Icons
Theme icon sprites are generated from each theme's icon-manifest.json.
- Add static SVG icon usage to the relevant theme manifest.
- Mark icons as
"dynamic": truewhen the icon name is resolved at runtime, such as admin navigation icons from module metadata. - Put theme-specific SVG files in the theme's
custom-iconsdirectory. - Reference generated symbols with inline sprite IDs, for example
<use href="#home" />.
Run npm run check-icons before changing icon usage.
CSS
Theme vendor CSS belongs in src/themes/*/assets/css/vendor.css. Theme-specific styles belong in the theme Sass entrypoint.
PurgeCSS runs only for production builds. A PurgeCSS failure fails production builds so release CSS problems are caught early.