4.12.0 dev - #292
Merged
Merged
4.12.0 dev#292
Conversation
Central notification centre (provider interface + registry) replacing the reservation overview buttons, and release notes bundled as markdown in docs/release-notes/ with an in-app viewer and a bell announcement.
notifications are purged in the purge command
…ion, relesae notes will be translated automatically
…t translated twice
Optional "Sign in with SSO" button beside the existing password and passkey login. Authorization code flow with PKCE; ID tokens are verified against the provider's JWKS via web-token/jwt-library, and claims the ID token does not carry are read from the UserInfo endpoint. No users are created automatically: the account must already exist, is matched once by e-mail (or username) and is then bound to the provider's iss + sub. Roles stay in FewohBee; admins can release a binding under /settings/users. Configured entirely through .env. HTTPS for the issuer is required in prod and relaxed in dev/test. OIDC_ENFORCE switches password login off server side, not just visually: credentials posted straight to /login are rejected, remember-me cookies stop working and /reset-password returns 404. There is no bypass URL by design — recovery is setting OIDC_ENFORCE=false. Adds users.oidc_issuer / oidc_subject / oidc_linked_at with a unique index. web-token/jwt-library requires brick/math ^0.17, so the lock file moves it from 0.18.0 to 0.17.2; the existing WebAuthn dependents accept that range.
Time-based workflows ran at the first cron pass after midnight, which put payment reminders in guests' inboxes on Sunday night. Each one now carries a weekday preset and an earliest hour; ScheduleWindow holds that logic for all four triggers. Excluded days do not drop records: because the triggers match an exact date, a plain filter would lose them for good, so the next allowed run widens its date range to cover the days it skipped. Also fixes monthly workflows firing on every 15-minute cron pass of their target day - they carry no entity, so the existing log dedup never applied to them. The invoice PDF layout is now selectable instead of always being the default template, both for the invoice mail action and for invoices attached to a template mail.
Every guesthouse has hours at which someone can be reached, and until now FewohBee had nowhere to put them: they lived on the website, on the answering machine and on a notice by the door, and a change rarely reached all three. Opening hours are stored per branch as a JSON column, keyed by ISO weekday, each day holding a list of [from, to] ranges. A JSON column rather than a table because this is display data — no query asks whether a branch is open at a given moment, so there is nothing to index or join against. Should the hours ever gain behaviour, that deserves its own table and its own migration. The form is a full-width table with two time ranges per weekday, so a lunch break fits. An empty weekday means closed, which the help text says. It sits inside the branch form rather than on a settings page of its own, next to the data it belongs to. getOpeningHoursFormatted() folds consecutive weekdays with equal hours into a range and follows the current locale, which makes the hours usable as a placeholder in letter and email templates. A test pins that the branch stays reachable from a reservation within the template schema's depth limit, because the form promises it.
A website or home automation that prints opening hours should read them from the one place they are maintained, rather than keeping a second copy that drifts. Deliberately not an /openinghours endpoint: the hours are one property of a branch among several, and whoever prints them usually wants the branch name next to them. The response carries stored data only — whether a branch is open at this very moment is not computed, because that depends on the caller's timezone, and the rest of this API returns locale- and timezone-independent data. openingHours is null when nothing is configured. An empty PHP array serialises to [], which would contradict the object the schema promises, so "not configured" gets one shape rather than two. The invoice number pattern stays out of the payload: internal billing configuration, of no use to a consumer. The new subsidiaries:read scope requires ROLE_RESERVATIONS_RO, matching prices rather than the stricter admin role — branch names and opening hours are the least sensitive of the settings data already exposed here.
Locale. The formatting moved out of the entity into OpeningHoursExtension, which takes the locale from the translator instead of \Locale::getDefault(). That default is set from the request, so a workflow mail rendered by the cron printed English weekday names into a German letter. Offered as a Twig function and registered as an editor snippet for reservation and invoice templates, following PaymentQrExtension. The entity is plain state again, and `openingHours` no longer sits in the autocomplete looking printable. Half-filled ranges. A range with only one end filled in was dropped silently, leaving the operator believing a lone "07:00" had been saved. Both object-form actions now refuse the save with a warning, the way the invoice number range already does. A note per branch. Free text alongside the hours, for what a weekly grid cannot express — "outside these hours by arrangement". In the form, the API payload and the snippet. The column joins the existing migration, which has not been merged yet. The subsidiaries:read scope is in the central scope table, and the endpoint has functional tests along the lines of ApiStatisticsControllerTest: unauthenticated, wrong scope, scope without the underlying role, the listing itself, and that a branch without hours reports null rather than an empty list.
setUp() pinned \Locale::setDefault('en_US') so a passing test could only be
reading the translator, never the ambient default — but it never put the old
value back. The default is process-wide, so every test running after this class
inherited en_US, which is exactly the kind of hidden coupling the extension was
built to avoid in the first place.
tearDown() restores what was there before, following ReleaseNotesServiceTest,
which pairs its setUp with a tearDown for the same reason.
…-hours Let branches carry their opening hours
- validate and limit reservation periods before availability and pricing - return translated 422 responses for invalid periods - preserve reservation modals when displaying validation errors - add regression coverage for excessive date ranges
The deduction took the first reservation that carried an origin and applied its rate to the whole invoice. On an invoice combining two portals, or a portal booking with a direct one, that charged a rate to revenue it never applied to - and said nothing about it. The rates the invoice's reservations carry are now compared, and the action skips with a log line naming them when they disagree. A reservation without an origin counts as a rate of zero, which is what makes the portal-plus-direct case show up as the disagreement it is; two bookings from one portal taken under rates that changed in between are caught by the same rule. Rates are grouped by their formatted form, so "12", "12.00" and 12.0 are one rate rather than three. Splitting the deduction along the reservations would be the fuller answer, but an invoice records no attribution of its lines to reservations - InvoiceAppartment carries a room number and a period, not a reservation - so it could only be guessed at. A skipped entry that names the rates asks for the manual booking that these invoices need anyway. A typed-in percentage is left alone: it says what to book whatever the bookings behind the invoice were.
The form renders a field's default wherever the stored config says nothing, and writes every field back on save. A field added to an action later, whose default had to differ from the behaviour of configs saved without it, was therefore switched over by opening the workflow and pressing save - with a plausible value on screen the whole time and nothing to suggest the deduction would book a different amount from then on. The calculation base is the first such field: new actions are offered the base excluding tourist tax, older ones keep the full gross. A schema field can now carry defaultForExisting alongside default. The controller fills it in for keys the stored config lacks before handing the config to the form, so editing an older workflow shows what it books rather than the suggestion meant for new ones, and saving it changes nothing by itself. Moving such a workflow onto the new base stays possible - by picking it, visibly. The test compares the two paths rather than the constants: the action is run once with no key in the config and once with the value the form shows for that case, and both have to reach the same amount. Drift between the two would otherwise surface as quietly shifted figures.
Undoes "Show an existing workflow the settings it is running on". That commit kept workflows saved before the calculation base existed on the full gross, and taught the edit form to show them that base so saving would not move it silently. Both only matter if such workflows can exist - and they cannot: the field ships together with the action that reads it, so the only configs lacking it are the ones set up while this branch was being built. Those are few and known, and are better looked over by hand than served by a second code path that has to be kept in step with the fallback forever. A missing key now means the same as a new action: the gross without the tourist-tax positions. The generic defaultForExisting hook in the controller goes with it, since no field declares it anymore.
The tourist tax is one lodging levy among several - a tourism fee or a bed tax behave the same way and are just as little commissionable. The option said "without tourist tax" and so read as if it singled that one out, while the code excludes every position tagged as such a levy. The wording now names the group and keeps the tourist tax as the example, in both languages. The stored value stays gross_without_tourist_tax so existing configurations keep working.
createEntryFromStatement() exists for the bank import and marks what it returns as manual, leaving the source to the caller - which this action never did. A deduction nobody typed in was therefore indistinguishable from a hand-written entry in the data. Nothing reads the field yet, so nothing was visibly wrong; it starts to matter as soon as the entries are reconciled against the portal's own statement, which is where telling automatic from manual is the point. The test now works with a real BookingEntry: the stub swallowed every property the action set on it.
The surcharge an invoice shows the guest and the deduction the journal books were calculated separately, and could not be brought together: the base was a workflow setting, which the render path has no business reading and which several workflows may answer differently. So the guest was shown a percentage of the full gross while the journal booked a percentage of something else. Whether a portal charges commission on a position follows from the booking, not from a workflow, so OriginFeeCalculator derives both fees and both bases from the invoice, and both paths ask it. The rate resolution - pinned on the reservation, falling back to the origin - moves along with it, so it too exists once. The bases now differ on purpose. Booking.com exempts tourist tax from commission as long as it is billed separately, which is how a tax that shows up as a position of its own is set up, so commission is taken on the gross without it. The payment fee is charged on what the portal processed and keeps it. That second base is the rough one: whether the portal collected the payment at all is not recorded yet, so the full gross stands in for it, which overstates the fee for a stay whose tourist tax the house collects on arrival. It is in the workflow log as a figure and can be corrected there. amountBase survives for a percentage somebody types in, where nothing but the config says what it is of, and is hidden for the two origin sources rather than offered and ignored. The sums had to come out of InvoiceService for this - it needs the fees, the calculator needs the sums, and constructor injection has no room for a circle. InvoiceSumCalculator holds the arithmetic unchanged; InvoiceService::calculateSums() stays as the way every caller reaches it.
The base was worked out from the position group: everything counted, tourist tax did not. That covers one case of one portal. What it cannot say is that a breakfast ordered at the counter on a portal booking was never brokered, or that a tourist tax the portal collected carries its payment fee while carrying no commission. Both are properties of the position, so they are recorded there. brokered says the portal brokered it, commissionable says its commission is charged on it; they differ for exactly one thing, a separately billed tourist tax, which Booking.com exempts from commission while still processing the money when it collects the payment. A price answers the first question once for the service it sells, and hands it to every position made from it - recorded there, so a price changed next season leaves invoices already written alone. A position added by hand in the invoice form has no price behind it, and what the house sells on site is what tends to be added that way, so the form asks as well, starting from the answer of the price picked there. Who takes the money becomes an enum on the origin, asked twice because a portal can collect the stay while the tourist tax is paid on arrival, and pinned onto the reservation the way the rates already are - including their exception: an origin that charges no fee pins nothing. It is never asked who collects and only carries its default, so a booking taken before the fees are set up falls back to the origin for this answer as it does for the rates. An enum rather than a boolean because a portal collecting only part of the amount is a case that exists and needs a figure; that becomes another case rather than a migration. Defaults are what the code did before, one exception aside: existing tourist-tax positions are marked as collected by the property, since an invoice already written cannot say otherwise and that is the common case. Existing prices and all other positions count towards a portal's fees as they did. The calculator still derives its bases from the position group; moving it onto these fields is the next step, and the only place that changes.
The bases were a rule the calculator applied - everything counts, tourist tax does not - which is as far as a position group can carry it. Now the positions say it themselves: commission is taken on the stay plus what is marked commissionable, the payment fee on the brokered positions plus the stay where the portal collected the payment for it. Two things follow that the old rule could not express. A breakfast the guest orders at the counter on a portal booking drops out of both bases, which is what Alex's second point was about. And a booking the house was paid for directly carries no payment fee at all, however much the portal brokered - there was no payment for it to process. positionGroup is out of the calculation entirely and back to being what it says it is, a grouping for the invoice layout. Existing origins that charge a payment fee are marked as collecting the payment, since the column added for it defaults to the property and would otherwise have them quietly book nothing. An origin charging a percentage for processing payments does process them. Reservations keep their NULL and fall back to the origin, which is the point of pinning: stamping today's answer onto old bookings is what it exists to prevent. The manual base a workflow can still pick now goes by the same flag, and is named after it: the commissionable part of the invoice gross, rather than a list of what it leaves out. For invoices written before this it comes to what it always did, since that is what the flags were migrated from.
A stay with a tourist tax billed beside it, booked through a portal and run the way a workflow runs it. It is the only shape of invoice where commission and payment fee come to different figures, and the difference hangs on one setting on the origin - worth pinning down as a whole rather than in the pieces the unit tests check.
A house with no tourist tax configured has nothing for a portal to collect, so the question had one possible answer and still took up a row in the form. It is now asked only where a tax exists; with the field gone the form sends no value and the service falls back to the property, which is that one answer. "The house" and "the portal" read as the two sides of a sentence rather than as the things they name, so they are now "Unterkunft" and "Portal".
The form's min, max and step are a courtesy to whoever types; what arrives at the server is whatever was posted. A commission of "zwölf", of -5 or of 12.345 went straight into a decimal(5,2) column, to be rounded away or refused far from where anybody could connect it to what they entered. A value the column cannot hold is now dropped before it reaches the entity, and the save is refused with a message saying what a percentage may look like. The check for the flag set without any value at all joins it: both are reasons the same form cannot be saved, so they are one question with two answers now.
The rates and the payment collection are recorded on the reservation when its origin is assigned, so that a portal changing its terms cannot rewrite what older bookings were settled under. Who collects the tourist tax was the one answer still read live off the origin while the invoice was written - and an invoice is often written weeks after the stay. It is now pinned with the rest. Bookings taken before this keep nothing recorded and fall back to the origin, which answers for them; stamping today's setting onto them would assert something nobody ever recorded.
A night is the thing a portal brokers - the calculator counts it towards both fees whatever a price says, and an apartment position never carried the flag. The switch was offered there all the same, so the form asked a question whose answer it then ignored. It is now shown for miscellaneous prices only, like the other switches that mean nothing for a night. Since a hidden field posts nothing, apartment prices are kept on "brokered" rather than having the silence read as a no, which would have recorded the opposite of what the calculator does. The explanation moves into a tooltip on the label, as in the online booking settings. The price dialog is long enough without a paragraph under a switch.
An invoice whose stays were settled differently - one paid through the portal, one paid to the house - gives the payment fee no single base. The calculator answered by leaving the whole stay out, which books a fee on the extras alone and reads like a correct deduction; nothing said that a figure had been dropped. Whether the portal took the money now has three answers, the third being that the invoice does not say, and a fee carries whether it has one base at all. The journal refuses such an invoice the way it already refuses one whose reservations carry different rates, with its own reason given in the log. The commission is untouched by this: it is charged on what the portal brokered, not on what it processed, so a mixed settlement leaves it stateable and it goes on being booked.
The placeholders took the first reservation that carried an origin and applied its rate to the whole invoice. The journal refuses exactly that invoice, so the guest could be shown a commission the accounts never recorded - and the one thing this calculator exists for is that the two cannot drift apart. They now print nothing where the invoice does not yield one figure: rates that disagree, or stays settled partly through the portal and partly with the house. The portal is still named, so a template can say whose fees are missing rather than falling silent altogether. The ready-made snippets guard on the amount rather than printing the currency sign on its own, which is what an empty figure next to a hard-coded "€" would look like. That also covers the invoice with no portal behind it at all.
The test clears all tax rates and accounts before loading a preset. Entries booked by other tests reference both, and whether any exist by the time this one runs depends on the order PHPUnit happens to walk the directory in - which changes when a test file is added. It fell over on a foreign key today for exactly that reason. The entries now go first, which is what the test meant by starting from nothing.
The getter was annotated as returning a ReservationOrigin, while a booking that came in directly has none - which the fee calculation leans on rather than treats as an edge case. Static analysis believed the annotation, so every null check around the getter read as dead code to it, and a real one added here would have been reported as a mistake. Correcting the annotation removes seven findings in the files that call it and adds none.
…ith itself Three docblocks still described the arrangement from before the invoice stopped printing a figure it cannot stand behind: the journal refuses, the guest is shown the first reservation's rate anyway. Both callers stop now, so the comments said the opposite of what the code does - the kind of contradiction that costs a reader more than no comment at all. Whether the invoice agrees on who took the money is asked outright instead of through array_unique() over booleans, which worked by casting them to strings.
The pull request goes to 4.12.0-dev rather than to master, since that branch carries InvoiceReservationOriginCondition (#288) - without it the action has no condition to pick an origin by - and merging there is what would otherwise have to be done afterwards by hand. Three conflicts, all from both sides adding to the same spot: the workflow form's translations gained the notification keys on one side and the percentage entry's on the other, and WorkflowController gained a tax rate repository next to imports that had been reordered.
PriceService gained a ReservationPeriodService in its constructor there, and this test still built it with the entity manager alone. The neighbouring price tests hand it the real service, which is final and does no I/O, so this one does the same.
The test walked every .dropdown-item carrying a data-url and required it to open the shared modal. The button that marks everything as read matches that too, so the test only passed while the panel held no notifications at all - which is what the suite happens to arrange until this branch adds tests that leave some behind. It now looks at the links, which are the entries it is about.
The branch was built against master, where docs/release-notes does not exist. On 4.12.0-dev it does, and every user-visible change belongs in it.
Book a portal's commission and payment fee, and show them on the invoice
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
2 participants
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.
No description provided.