Model the device catalogue so a buy page can price an order - #73
Open
cyberb wants to merge 35 commits into
Open
Conversation
Selling hardware through opencart means a second account, a second password and a second place customer data lives, for a shop that sells one product. The catalogue it holds is small enough to state directly: of six products only Syncloud H4 is enabled, priced 229 with six SSD choices, and every completed order for three years is that device paid through PayPal with flat 15 shipping. This is the part of moving it onto the account we already have that does not depend on how payment is taken: the devices, their options, and what an order costs. Prices are integers in pence so nothing is rounded, and Total is the only way to arrive at a figure, so the amount charged can never come from the request -- a buy page that trusts a posted price is a buy page that sells at whatever the buyer asks for. The catalogue is data passed to a constructor rather than a table, because with one device and six options a migration would be more machinery than the thing it holds, and it can become one when there is a second device to sell.
Nothing charged anyone yet: the catalogue could price an order but there was no way to pay for one. This adds the half that takes money, with no tax, both providers, flat shipping, and the order emailed to support. Orders.Start prices the order from the catalogue and hands it to whichever provider was asked for. The amount is recomputed there rather than read from the request, and Complete recomputes it a third time and compares it against what the provider says was actually taken, refusing anything short. A checkout that trusts a posted total sells at whatever the buyer types, and one that trusts its own total without checking the payment ships hardware for a penny. Support hears about an order only after the money is confirmed. An unpaid or underpaid order returns an error and sends nothing, so the inbox is a list of things to ship rather than a list of things to check. PayPal and Stripe both sit behind one Checkout interface: start a payment, report whether it completed and for how much. Stripe uses a checkout session in payment mode priced from our own figure rather than a stored price, since the total depends on the option chosen. PayPal creates and captures an order. Amounts cross those APIs as decimal strings and are held here as integer pence, so parsing and formatting are tested in both directions -- 324.00 and 15 and 0.05 all have to survive the round trip, and rubbish has to parse to zero rather than to a number nobody meant.
Complete took the device, the option and the address from its caller, which meant the browser told us what it had bought at the moment we decided what to ship. The amount check made mispricing impossible, but the shipping address and the chosen option still arrived from the client after payment, and an order is not something the buyer should be able to restate. There is a device_order table now. Start prices the order, opens the payment and stores the whole thing keyed by the reference the provider gave back. Complete takes an account id and that reference and nothing else: it loads the order, and what support is told to ship comes from the row, not from whoever called. The reference is unique and the row carries the account that created it, so a reference belonging to another account is refused rather than served, and one we never issued is refused rather than treated as an empty order. Paid is a column, so completing twice sends support one email rather than two -- a buyer who reloads the success page should not produce a second thing to ship.
Two things were wrong with taking the payment. The captured amount was compared as a number with no regard for what currency it was in. PayPal returns amount as a currency code and a decimal string, and only the string was read, so a capture of 324.00 in euro would have compared equal to a price of 324.00 in sterling and shipped a device for about fifteen per cent less than it costs. Both providers now report the currency they took and it has to be the one we priced in. The order was stored after the payment was opened, so a failure to insert left a payment the buyer could complete with nothing on our side to match it against. The order is inserted first, with a uuid we generate, and that reference is what the caller gets back and what identifies the order from then on. The provider's own reference is recorded against the row afterwards and used only to ask the provider whether it was paid. Our reference also goes to PayPal as invoice_id and to Stripe as client_reference_id, so a payment can be traced back to an order from either side rather than only from ours.
Three endpoints, all behind Secured so they can only be reached by someone logged in, which is the point of moving this off opencart: the account that owns the devices is the account that buys them, and there is no second registration. The order is built from the session user, not the request. The buyer sends what they want and where to send it; the account id and the email come from the session, so an order cannot be placed on someone else's account and the address is the only thing the browser gets to say. Complete takes only our reference and the session user, and refuses a reference belonging to anyone else. The catalogue is the six SSD options the shop actually sells, at the prices it actually charges, with the fifteen pound flat shipping it actually applies. Success and cancel urls follow the pattern the stripe subscription checkout already uses, so there is one convention for where a payment returns to. Orders is registered after Mail in the container because it needs it; registered before, the container has no concrete for service.Mail and both it and Www fail to resolve, which the container test catches.
The page a logged in account uses to buy a device: pick the storage, say where to send it, pay by card or PayPal. It reads the catalogue from the api, so the prices shown are the prices the server will charge, and it never sends a total -- what it posts is the device, the option and the address, and the amount is the server's to decide. Stripe returns the checkout url on the session rather than one we build, since constructing a checkout url from a session id is guessing at somebody else's routing. That url now carries our own order reference, so when the buyer comes back the page confirms the order it started rather than trying to work out which one it was from the provider's id. PayPal renders its own buttons the way the subscription page already does, creating the order through our api and confirming through it on approve. Pay is disabled until the whole address is there, which the server checks again, because a disabled button is a courtesy and not a control.
The form had no styling because it used a class that does not exist. The real input styling was scoped to .sc-auth, so only the login and register pages got it and anything else fell back to browser defaults. The rules now also apply under .sc-form, and the buy page uses the same .sc-field and label markup the login form uses, so it looks like the rest of the site rather than like a different site. The keyboard tracker is gone. It scrolled the focused input into view smoothly, and it also listened for viewport scroll and scrolled again whenever the keyboard inset was non zero -- so each smooth scroll fired the handler that had started it, and the page crawled for seconds with nothing the reader could do about it. Browsers already bring a focused field into view. The --kb bottom padding went with it; the two rules that used it now have plain values rather than a calc against a variable nothing sets.
The pay step was one plain button that said card, with the PayPal buttons under it and nothing to say the two were alternatives. The subscribe page already solved this: a Pay with label, a large primary Card button, and the PayPal buttons beneath, in a column of fixed width. The buy page now does the same, so choosing how to pay looks the same wherever you are choosing it. The cards had no space between them and sat edge to edge, which is worst on a phone where they fill the width. Each page in this app spaces its own cards in its own scoped style rather than sc-card carrying a margin, so this one does too. The test now registers ElButton. Element Plus components are resolved at build time by the vite plugin, which jest does not run, so an unregistered el-button rendered as an unknown element and its disabled attribute came back as the string false rather than being absent. The assertion was right and the mount was wrong.
…silently Both buttons failed for one reason. Migration 22 created device_order without provider_reference, was applied to uat, and then I added the column by editing that same file. golang-migrate records 22 as done and never looks at it again, so the column was never created and every insert failed with unknown column provider_reference in field list. Editing an applied migration cannot work. 22 is back to what actually ran and 23 adds the column, which is correct from either starting point: a database that already has 22 gets the column from 23, and a fresh one gets the table from 22 and the column from 23. The card button showed the error. The PayPal button did nothing, which was the same failure wearing a disguise: createOrder returned a rejected promise and the PayPal SDK abandons the payment silently when that happens, so a database error looked like a dead button. createOrder now reports the error before rethrowing, and an onError handler catches whatever the SDK raises on its own account.
Nothing in the payment path had ever run. It was covered by unit tests against stubs of our own making, which is why a missing database column reached a person clicking a button rather than a failing build. payment-faker answers both providers well enough to complete a payment: PayPal's token, create order and capture, and its subscription lookup and cancel; Stripe's checkout session create and retrieve. A session starts unpaid and becomes paid, for the amount and currency that were asked for, only after somebody pays it, so the check that the amount taken matches the amount priced is exercised rather than asserted. Stripe redirects a browser to a url we are given, so the faker serves that page itself: it shows the amount and a button that pays and redirects back to our success url, which is the same round trip the real one performs. PayPal renders its buttons from a script on paypal.com, and that script talks to PayPal, not to us, so there is nothing to redirect. paypal-js takes an sdkBaseUrl, so the url is configuration now: unset in production, and in test the faker serves a script that renders a button which calls createOrder and then onApprove. Our code path is identical in both; only where the script comes from differs. Stripe's base url is configuration too, unset meaning the real one. Both pages read the sdk url from the api, so subscribing and buying can both be driven.
The deploy test pointed at PayPal's real sandbox with a client id of 3 and a secret of 4, so every payment call failed and nothing noticed, because nothing made one. It now points at the faker instead: the backend reaches it on localhost, and the browser reaches it at payments.syncloud.test, a site the test setup adds to caddy's conf.d, which the shared Caddyfile already imports. Nothing in the common configuration changed. Two paths, two ports, because the browser needs the same origin scheme the site has: an sdk script served over http into an https page is blocked, so the faker sits behind the same tls the rest of the test environment uses. uat and prod are untouched and still talk to the real sandbox and the real thing. The new keys default to empty, which means the real provider, so an environment that does not set them behaves exactly as before.
The buy page, the privacy policy and the error page had no specs at all. Register, activate and check email were exercised inside the auth helpers but never asserted as pages in their own right, so a broken link or a missing heading on any of them would have gone unseen. device.spec.js drives buying through both providers against the fakers: the price changing with the option, pay staying disabled until the address is whole, a card payment through the checkout page and back, and a PayPal payment through the stub button. That is the first thing anywhere that proves an order can be paid for. pages.spec.js covers register, check email, activate, privacy and error, each asserting what the page is and that its way out works. The porting guide's point about screenshots applies here: screenshot on failure means a green run leaves nothing to look at, which is backwards when the reason to look is to review the interface. shoot() writes a named png at each screen and ui.sh collects them per project, so desktop and mobile both leave a set. The privacy page had no test ids, so selecting anything on it meant selecting by text or css. It has them now.
npm run lint only ever looked at src, so everything under e2e and tests was unchecked. Fourteen style errors had accumulated there, and two things that were not style. ssh.js was not being linted at all. It uses 120_000, a numeric separator, and the configured parser predates them, so eslint failed to parse the file and skipped it silently. Node runs it fine, which is why nobody noticed. Setting ecmaVersion to 2022 makes the parser read what node already runs. Devices.spec.js imported ElButton and ElDialog and used neither. That is the same mistake I made in Device.spec.js: importing an element-plus component does not register it, and jest does not run the vite plugin that normally would. Here the imports were simply dead, since Devices.vue uses no element-plus components at all, but the pattern is worth naming because importing without registering looks like it works and does not. The lint script now covers src, e2e, tests and the config files, so a file that cannot be parsed shows up as an error rather than as silence.
Two reasons the browser could not reach the fakers, both mine. paypal-js takes an sdkBaseUrl that already ends in /sdk/js -- its default is https://www.paypal.com/sdk/js, not the origin -- so pointing it at payments.syncloud.test/paypal asked for /paypal?client-id=... and got nothing. It points at /paypal/sdk/js now, which caddy strips to /sdk/js, which is what the faker serves. I inferred that shape from minified source rather than checking the default, and inferred it wrong. The stripe base url was set inside the subscription client's factory, which is a global side effect hidden in a lazily resolved singleton. The buy page builds its own stripe client, and nothing forced the subscription one to exist first, so checkout sessions went to the real api.stripe.com with a dummy key. It is set once now, in the container constructor, before anything is resolved, so both clients get it whatever order they are created in.
Two runs failed with element not found, which says where the test stopped and nothing about why, so both fixes so far were reasoning from absence. The specs now collect what the browser complained about -- console errors, page errors, failed requests and any response of 400 or worse -- and print and attach them when a test fails. There is also a spec that fetches the paypal sdk script from the browser's own context before any payment runs. If the faker is unreachable that spec fails on its own, with a status code, instead of four payment assertions timing out and leaving us to guess whether it was the network, the url or the page. ui.sh traces its commands now. It had -e without -x, so the hosts entry it writes could not be seen in the build log, which is exactly the line under suspicion.
The browser said ERR_SSL_UNRECOGNIZED_NAME_ALERT, which is not a certificate problem and not a dns one, so both earlier fixes were aimed at the wrong thing. Caddy answers :443 with a layer4 route that matches known server names and proxies them to the https port; payments.syncloud.test was not among them, so the handshake fell through to the relay route, which holds no certificate for that name and rejects it before any request is made. The certificate for it had been issued all along, which is why looking at certificates said nothing. The name is on the test domain's list now. Those lists are per domain, so uat and prod are untouched, and neither runs a faker to reach anyway.
The card payment reached the checkout page, paid, came back, and then completing
the order returned 500. The return url was assembled in two places: the container
supplied one ending in reference={CHECKOUT_SESSION_ID}, copied from the
subscription checkout where stripe substitutes that placeholder, and the payment
client appended its own reference to whatever it was given. The buyer came back
to /device?reference=cs_faker_1?reference=<uuid>, so the reference read from the
url was two references joined by a question mark, no order matched it, and the
handler failed.
The container supplies the page and the payment client owns the query, since it
is the one that knows which reference identifies the order. Subscriptions still
use the placeholder because there stripe is the one filling it in.
Desktop passed and mobile timed out on every buy spec: nav-buy is in the page on a phone but inside the closed menu, so it resolves and never becomes clickable. The specs open the burger first when it is showing, which is what the password reset spec already does, and then click the link. Navigating straight to /device would have been shorter and would have skipped the menu, which is the part most likely to break.
The buy page loaded the sdk with nothing but a client id and a currency, so PayPal rendered everything it offers -- its own button, a debit and credit card button, pay later -- stacked vertically with a tagline under them, which is why it stood taller than the subscribe page. That page passes disableFunding card and gets one button. This does the same and also turns off pay later, because the card button is already there and it is Stripe's. The button asks for the paypal label, no tagline and the same 44 pixels the card button beside it uses, so the two read as a pair rather than as one control and a panel.
Pressing Card or PayPal did nothing visible for a few seconds while the checkout was created and the window opened, so the page looked ignorant of the press and the natural response is to press again. Both buttons on both pages now report themselves. The card button goes into its loading state, and a line under the buttons says which checkout is opening, worded for the wait: opening PayPal can take a few seconds. PayPal's own button cannot be restyled from here, but its onClick fires as soon as it is pressed, so the line appears at the same moment either way. Cancelling, failing, or coming back clears it, and no second payment can be started while one is opening. The disabled binding is a real boolean rather than the busy string. Vue reads an empty string as true for a boolean prop, so incomplete || busy left the button permanently disabled once busy existed, which the unit tests caught.
Register read the click id from its own route query, so it only survived when somebody arrived directly at /register with it attached. Links from the site point at the root, and moving from there to register through the page's own link dropped the query, so the id was gone by the time the account was created. It is captured once when the app starts, from whatever url the visitor arrived on, and kept for ninety days. Register still prefers the id in its own url and falls back to the stored one, so a fresh link always wins over an older visit. Two specs cover it: arriving somewhere else and navigating to register, and a url id taking precedence over a stored one.
Renamed from buy to shop, so the page can hold a second device without being renamed again. The route, the view, the nav entry and the payment return urls all moved together; the return urls in particular pointed at the old path and would have brought every buyer back to a page that no longer existed. The page itself now reads like somewhere you choose something. The photo is the one the old shop has been using for this device, taken from it rather than invented. Storage is a row of tiles showing what each costs on top, the way a phone or a laptop is configured, instead of a dropdown that hides five of the six choices until it is opened. The summary names what is being bought rather than saying Device, and calls fifteen pounds Delivery rather than Shipping. The specs follow: tiles are clicked instead of a select being set, the photo is asserted, and the nav entry is the new one.
Everything behind login is right for an account page and wrong for a shop: a visitor could not see what a device costs without registering first, which is backwards, since the reason to register is to buy the thing whose price they cannot see. The catalogue is public now and ordering is not. A signed out visitor gets the photo, the options and the total, and where the address and payment would be there is one card explaining that a device belongs to an account and a link to sign in. Everything that moves money still requires the session, and the account and email still come from it rather than from the request. That is the shape most shops use: browse freely, prove who you are at checkout. Guest checkout is the usual third option and does not apply here, because the device has to belong to an account to get its domain name.
The whole shop was one page: photo, blurb, six storage tiles, a price summary, five address fields and two payment buttons, stacked. On a phone that is a long scroll where the thing you are deciding is off screen by the time you reach the buttons, and it is not how a shop is normally read on a phone. It is two steps now, the way choosing a phone or a laptop usually goes. First the device and what it costs, ending in Continue. Then delivery and payment, with a line at the top saying what is being bought and for how much, and Change to go back. Nothing is hidden; it is asked for in the order it is decided. On narrow screens the photo drops to ninety six pixels, the storage tiles go to two columns, and the three bullet points about what arrives give way to the choice itself, which is what the screen is for. Signed out visitors still see the device, the options and the total, and are asked to sign in rather than to continue.
Splitting the shop into two steps put the payment card behind a v-if, so the element PayPal renders into did not exist when the page loaded and asked for it. The buttons never appeared, which the spec caught: everything else passed and paying with PayPal timed out looking for a button that was never drawn. They are rendered when the checkout step appears instead, on the next tick so the card is in the page first. Loading is still once per visit, so going back to change the device and returning does not draw a second set.
Five fields each with a label above it is a lot of vertical space for an address, most of it white. The labels sit inside the fields now and rise to the top of the box once there is something typed, so the row of label plus box becomes one box and the form loses about a third of its height. Not bare placeholders, which is the cheaper version of this: a placeholder vanishes as soon as anyone types, and on an address form that leaves five filled boxes with nothing saying which is the city and which is the postcode. The label is still a real label bound to its input, so it is announced properly and clicking it still focuses the field. City and postcode share a row, since neither needs a full line. The fields also declare what they are for autofill, so a browser can offer a saved address rather than making somebody type it again.
Payment is captured when the buyer comes back from the provider, so somebody who pays and closes the tab leaves an order we never finish. PayPal voids an uncaptured authorisation after a few days and nothing is lost but the sale. Stripe takes the money on its own page, so that buyer is charged and has no order, and finds out by waiting for a parcel that is not coming. A job now walks orders that are unpaid and older than a couple of minutes and asks the provider whether they were paid, using the same settle path the return from checkout uses: same currency check, same amount check, same email to support, same guard against doing it twice. Subscriptions have run without any of this for years, but a lost subscription announces itself, because the payer does not get what they paid for and says so. A lost order is silent. It runs in the www service beside the others and needs no public endpoint, no signature checking and no new way in, which a webhook would have needed. An order stuck unpaid is logged rather than retried forever, and a provider being down does not stop the rest of the batch.
Orders were deleted with the account that placed them, so erasing a customer erased the record of what they bought. Company and VAT records have to be kept for six years, and a cascade means they cannot be produced. Erasure and keeping records are not actually in conflict. What has to be kept is the sale: the reference, the date, what was bought, what was charged, in what currency, through which provider, to which country. What has to go is the person: the name, the street, the town, the postcode and the link to the account. Deleting an account now blanks those and detaches the order, and the row stays as a financial record with nobody attached to it. The foreign key is set null rather than cascade, so a detached order keeps no user id, and reading one back handles a null. An order with no owner belongs to nobody, which means completing it is refused rather than matched to whoever asks, since no real account has id zero. Country is kept because it is part of where the sale happened. If it turns out the street address has to be kept too, that is one line in the redact statement.
The shop was public and unreachable. Every entry in the menu was drawn only for somebody signed in, so a visitor without an account saw a header holding a logo and a theme switch, and the only way to the shop was to know the url. The root sent them to the login page, and the logo pointed at the root, so the obvious way back out of the shop was a door into the login form. Shop is in the menu for everybody now, there is a Log in entry when nobody is signed in, and the logo goes to the shop rather than to a page that will bounce you. The root is a page in its own right: devices for somebody signed in, and for everybody else what this account is for, with the ways in -- log in, the shop, or building a device yourself on the other site. It is not a second marketing site; that is what syncloud.org is for. Signing in returns you where you were going. Both places that pushed to the root after authenticating now honour a next parameter, and the shop asks for login?next=/shop, so choosing a device and then signing in no longer lands on an empty device list with the choice thrown away. Nothing is drawn until the session answers. loggedIn starts undefined and was read as false, so a signed in buyer saw sign in to order for a moment before it turned into the payment section.
A single payment-faker hid the thing a faker is for. PayPal and Stripe do not have the same shape and pretending they share a store made both less honest: PayPal moves amounts as a decimal string with a currency code and captures an order, Stripe moves integer minor units with a lowercase currency and marks a session paid. Each faker now models its own vendor, so reading paypal-faker tells you what PayPal expects and reading stripe-faker tells you what Stripe does, without unpicking which branch belongs to whom. They also stopped being installed onto the test device. They were compiled in a step, copied over ssh, dropped into /usr/local/bin and left running with log files, none of which the device under test should be carrying. They are drone services now, compiled and run where they are declared, which also gives them names on the network: the backend reaches paypal-faker and stripe-faker by name, and caddy proxies the browser to them by name. Neither has a dependency, so go run compiles them offline with nothing to fetch. No other project here runs a service that compiles itself, so this is the first; if drone does not hand a service the workspace, the build will say so.
detach is the feature for this and I had reached for services instead. A detached step is given the workspace, which is what a step that compiles from source needs, and it runs in pipeline order, so it is listening before the deploy that points at it rather than racing it. Its name is a hostname either way, which was the part I wanted. The fakers are steps now, sitting just before deploy test, and the services list is back to the containers that really are services.
The old shop's description listed what actually ships and none of it survived the move: the board in its case, an SD card with the system already written, the disk you chose, and the cables. People buying hardware want to know what arrives in the parcel, and a page that only shows a photo and a price does not say. It is a closed details element, so the page still opens on the choice and the list is one tap away rather than four more paragraphs to scroll past on a phone. Two things in it are worth saying out loud rather than leaving in a spec table. The second drive bay is empty and can be filled later from Settings, so nobody has to decide about a second disk while buying the first. And the description of who it suits is the old shop's own, since it describes the product rather than selling it. Only one photo of this device exists in the old shop. The other angles there belong to Syncloud N, which is not for sale.
Making the root public changed what logging out means. It used to land on the login form only because the root refused anybody without a session and bounced them there; with a public root, logging out left the visitor on an introduction to a product they already own, with no obvious way back in. Logging out now goes to the login page deliberately rather than as a side effect of being turned away. Two specs encoded the old side effect. Deleting an account and returning to the root asserted the login form; what it means is that no session survived, so it asserts the introduction and the absence of the device list instead. The shop spec expected /login exactly, which no longer holds now that signing in from the shop carries where to return to.
The mobile smoke test asserted that somebody without an account landing on the root sees a password box. That was true when the root turned strangers away, and the point of a public front page is that it no longer does. It checks what the page is for instead: a visitor with no account is shown what this is and can get from there to signing in, or to the shop, which is the journey the page exists to support. Both are worth having on a phone, where the menu is closed and the front page is the only thing offering a direction. It selects by test id rather than by css id, which is how the rest of the specs are written.
The progress line was wired to onClick only. The real SDK renders its button inside a cross-origin iframe and does not guarantee onClick reaches us before the popup opens, so pressing PayPal looked inert while the order call was in flight. createOrder is the one callback PayPal must invoke to obtain an order id, so set the state there as well; onClick keeps it immediate when it does arrive. The paypal-faker stub jumped straight to createOrder, which is why no test caught this. It now calls onClick first, matching the real callback order.
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.
Moves hardware sales off the separate shop and onto the syncloud.it account, so buying a device uses the account that will own it rather than needing a second registration.
What it adds
CheckoutinterfaceSecured, so only a logged in account can reach them, with the account and email taken from the session rather than the requestTesting
payment-fakeranswers both providers well enough to complete a payment, so the deploy test drives buying end to end through the UI on desktop and mobile rather than asserting against stubs. uat and prod still talk to the real sandbox and the real thing.Also adds specs for the pages that had none, screenshots on green runs, and keeps the click id when a visitor does not land directly on the register page.
Not in this PR
No webhooks: payment is captured when the buyer returns, so someone who pays and closes the tab leaves an approved but uncaptured order, recoverable by reference but not automatically. Order rows cascade on account deletion, which suits erasure and may not suit record keeping.