Dansday

The Month a Plus Sign Broke the Checkout

Published on Apr 30, 2024

Nine pull requests in April, the quietest month since I joined. Quiet months are where you find the strange bugs, because there is time to actually look at them — and where you start the thing that eats the next one.

The plus sign

The strange one arrived on 5 April: customers could not check out a used iPad. Not all products — iPads specifically. Everything else went through the payment gateway fine.

The cause was a plus sign. The product was named something like iPad Air 4th Gen 64GB + Cellular, and that name was being fed into two places at once: the signature hash, and the query string sent to the gateway. In a URL query string, a plus sign means a space. So the gateway received the name with a space where the plus had been, hashed what it received, and got a different answer than the hash we had sent. Signature mismatch, payment refused, no useful error — just a product nobody could buy.

The original line had been carrying a superstition since the January spike:

$hashed_string = md5(
    $secretkey
    . urldecode($detail)
    . urldecode($amount)
    . urldecode($order_id)
);

Those urldecode calls were applied to values that had never been encoded. On a plain product name they do nothing at all, which is why nobody noticed. On a name containing a plus sign, urldecode helpfully converts it to a space — so the hash was built from a mangled name while the query string carried the real one.

My first attempt was to strip the decoding entirely. That fixed iPads and broke everything else, because the amount and order id genuinely did need normalising. My second attempt was to encode the detail first and then decode it, which is a round trip that arrives back where it started while looking deliberate. The third attempt was the right one: leave the product name exactly as it is, and decode only the fields that need it.

$hashed_string = md5(
    $secretkey
    . $detail
    . urldecode($amount)
    . urldecode($order_id)
);

Three commits to delete one function call from one place. The whole fix was a single-character diff on one line in one file, and the pull request is one addition and one deletion. The lesson was not about encoding — it was that I had inherited a line I did not understand and left it alone because it seemed to work. It did seem to work. It worked for every product that did not have a plus sign in its name, which was every product until it wasn't.

Arithmetic that hid good news

The second bug was arithmetic that hid good news. The product page shows customers what they save, and the calculation looked reasonable:

return $original > $cartPrice
    ? ($original - $cartPrice) + $warranty + $shipping
    : 0;

If the original price is higher than what you pay, add up the discount plus the free warranty and shipping. Otherwise, zero. But a product sold at its normal price still comes with a one-year warranty and free delivery — real value, worth real money, and the early return was throwing it away. Every product without a markdown displayed no savings at all, despite having some.

return (($original > $cartPrice ? $original : $cartPrice) - $cartPrice)
    + $warranty
    + $shipping;

Now the price difference floors at zero instead of the whole calculation doing so. The warranty and shipping always count, because they are always given. It is the same class of mistake as December's hardcoded discount badge, inverted: that one claimed a saving that did not exist, this one hid one that did.

A default is a decision

Then a marketing opt-out for the checkout, which is mostly a checkbox and entirely a question about defaults. Adding a consent column to a table of existing orders means deciding what those old orders consented to, and there is no correct answer — only a choice you have to make explicitly:

$table->boolean('offer_approved')->default(true)->nullable();

The default landed on true, matching the pre-existing behaviour rather than retroactively opting everyone out of something they had not been asked about. I mention it because a nullable column with no default would have left those rows as null, and null is not a decision — it is the absence of one, waiting to be misread by whatever code reads it next.

The TODO I finally answered

The most satisfying commit of the month deleted a comment I had been walking past for weeks:

{{-- TODO - check if they want the whole card clickable --}}

They did. Making a card fully clickable is not just moving the anchor tag outward, though, because the card already contained a link — the call-to-action button. An anchor inside an anchor is invalid HTML, and browsers resolve it by improvising. So the outer element became the link, the inner button became a plain div, and the hover state that used to belong to the button was reassigned to the card:

.product-card:hover .cta,
.category-card:hover .cta {

The button still lights up on hover. It just responds to the whole card now, and there is only one link where there used to be two pretending to be one.

262 lines out of a template

The biggest pull request of April was filed as a chat widget animation and was mostly not that. 335 additions, 271 deletions, six files.

The animation part is nine lines of stylesheet. The WhatsApp button had been sitting still, and a button that never moves is a button nobody sees:

.whatsapp-bounce {
    display: inline-block;
    animation: whatsapp-bounce 0.5s infinite linear;
}

@keyframes whatsapp-bounce {
    0%, 100% { transform: translateY(0); }
    50%      { transform: translateY(-20px); }
}

Half a second, infinite, twenty pixels. In hindsight that is a button having a panic attack, and May opens with a pull request titled More obvious Chat widget animation on reveal that replaced it with something that fires once when you scroll to it. Attention and irritation are close neighbours and I got the address wrong the first time.

The real content of that pull request was deleting 262 lines from a Blade template. The product page carried its entire variant-selection engine as an inline <script defer> block at the bottom of the markup — the thing that runs when you pick 128GB in Midnight:

var attributeCountToEnablePriceSycing = 2;
var variantsValue = JSON.parse(
    document.getElementById('pdp').dataset.pdpVariantsValue
);

async function selectVariant(event) {
    deactiveVariants(variantIndex);
    activeVariant(event.currentTarget);

    if (parseInt(variantIndex) === 0
        && parseInt(this.attributeCountValue) === this.attributeCountToEnablePriceSycing) {
        syncGroupPrice(variantKey, text);
    }

    syncInvalidVariant(variantIndex, variantKey, text);

    if (variantKey === 'color') {
        changeImageForColor(variantKey, text);
    }
}

Price syncing when the storage changes, swapping the gallery when the colour changes, greying out combinations that do not exist. That is real logic, and living in a template it had no build step, no linting, no module boundary, and no way to be reused — while being re-parsed by the browser on every single product page view. It also shipped a typo in a variable name, attributeCountToEnablePriceSycing, which is the sort of thing a linter mentions and a Blade file never will.

Moving it into a real JavaScript file changed nothing a customer could see. It is the January component decision again in a different layer: the value is not in the feature, it is in where the code now lives.

Reference for Irfan

One pull request that month was titled IP-29170: For irfan reference, and I like that it exists. It is 21 additions across five files, opened not to finish a ticket but to show a colleague the shape of one.

Checkout was growing a choice between delivery and self-pickup, which means two panels that must show and hide in step, and a set of totals that change with them:

this.reserveAmountTarget.classList.remove(this.hiddenClass);
this.pickupMethodTarget.classList.add(this.hiddenClass);

The other half was pulling hardcoded English out of the template and into a structured translation block:

'delivery_options' => [
    'self_pickup' => [
        'title'       => 'Self pick-up',
        'description' => 'Your device will be reserved for self pick-up until :date',
        'label'       => 'Inspect in Store',
    ],
    'delivery' => [
        'title'       => 'Delivery',
        'description' => 'Estimated delivery time 2-4 business days.',
    ],
],

A flat key called self_pickup_reserve_until became a nested structure that groups everything belonging to one option. Note the word reserved in that description. March's throwaway experiment with a reserved order status had no checkout flow to attach to; six weeks later, here is the checkout flow, and somebody else was going to build it with my working example open in the next tab.

The one that ran into May

On 23 April I opened the pull request that would define the following month. Products on promotion needed to look different on their cards — a badge, a border, custom title text.

It ended up carrying thirty-two commits, the last of them dated 24 May, and it was never merged. The first four commits are April's: IP-29242: Promotion product indicator for product cards, then Get promotion, then Backend, then Indicator finalize on 1 May. That last commit name is the funniest thing in my 2024 git history, because the work ran another three weeks past it.

What April was

Nine pull requests. An iPad you could finally buy, savings that stopped hiding, 262 lines of JavaScript that found a proper home, one TODO fewer than I started with, and one branch quietly getting longer.

The plus sign is the one I would tell in an interview. Not because the fix was clever — it was deleting seven characters — but because the bug had been sitting in production since January inside a line I had copied without understanding, and it only ever hurt the customers who wanted the one product with a plus in its name.