Implement board creation API and enhance currency handling across frontend components
This commit is contained in:
parent
78abf8acae
commit
39e22b03d0
13 changed files with 522 additions and 27 deletions
|
|
@ -1,3 +1,4 @@
|
|||
from django.db.models import Sum
|
||||
from drf_spectacular.utils import extend_schema_field
|
||||
from rest_framework import serializers
|
||||
|
||||
|
|
@ -263,6 +264,28 @@ class OpportunityCreateSerializer(serializers.ModelSerializer):
|
|||
if stage in AMOUNT_REQUIRED_STAGES and not self._resolved(data, "amount"):
|
||||
errors["amount"] = "A won deal has to record what it was worth."
|
||||
|
||||
# `amount` stops being the client's field once the deal has line items.
|
||||
# `OpportunityLineItem.save()` calls `recalculate_amount()`, which sets
|
||||
# `amount` from the lines and stamps `amount_source = "CALCULATED"`.
|
||||
# Nothing recomputed it on the deal's own update path, so a request
|
||||
# carrying a different figure overwrote the total while leaving the
|
||||
# source still claiming the number came from the lines. The record then
|
||||
# asserted two incompatible things about the same money.
|
||||
#
|
||||
# An unchanged value is accepted rather than refused, because clients
|
||||
# that send the whole record back on every edit (the mobile deal form
|
||||
# does) must still be able to move the stage or the close date. Only an
|
||||
# actual attempt to change the figure is an error.
|
||||
if self.instance is not None and "amount" in data:
|
||||
line_items = self.instance.line_items
|
||||
if line_items.exists():
|
||||
line_total = line_items.aggregate(total=Sum("total"))["total"] or 0
|
||||
if data["amount"] != line_total:
|
||||
errors["amount"] = (
|
||||
"This deal's amount is the sum of its line items. "
|
||||
"Edit the line items to change it."
|
||||
)
|
||||
|
||||
if errors:
|
||||
raise serializers.ValidationError(errors)
|
||||
return data
|
||||
|
|
|
|||
|
|
@ -1867,3 +1867,135 @@ class TestOpportunityNonAdminAssigned:
|
|||
format="json",
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
|
||||
|
||||
class TestAmountIsOwnedByLineItems:
|
||||
"""A deal with line items owns its own `amount`.
|
||||
|
||||
`OpportunityLineItem.save()` calls `recalculate_amount()`, which writes
|
||||
`amount` from the lines and stamps `amount_source = "CALCULATED"`. Nothing
|
||||
recomputed it on the deal's own update path, so an ordinary PUT or PATCH
|
||||
could set a different figure and leave the source still claiming the number
|
||||
came from the lines.
|
||||
|
||||
The web edit form disables the amount input when line items exist, but a UI
|
||||
guard is not a guard: the mobile form has no equivalent and neither does
|
||||
curl.
|
||||
"""
|
||||
|
||||
@pytest.fixture()
|
||||
def opportunity(self, admin_user, org_a):
|
||||
_set_rls(org_a)
|
||||
return Opportunity.objects.create(
|
||||
name="Line Item Owned Deal",
|
||||
stage="QUALIFICATION",
|
||||
amount=Decimal("100.00"),
|
||||
org=org_a,
|
||||
created_by=admin_user,
|
||||
)
|
||||
|
||||
@pytest.fixture()
|
||||
def priced(self, opportunity, org_a):
|
||||
"""One line item of 2 x 250, so the deal is worth 500 and knows it."""
|
||||
_set_rls(org_a)
|
||||
OpportunityLineItem.objects.create(
|
||||
opportunity=opportunity,
|
||||
name="Widget",
|
||||
quantity=2,
|
||||
unit_price=Decimal("250.00"),
|
||||
org=org_a,
|
||||
)
|
||||
opportunity.refresh_from_db()
|
||||
return opportunity
|
||||
|
||||
def test_line_items_take_ownership_of_the_amount(self, priced):
|
||||
"""The premise of every test below."""
|
||||
assert priced.amount == Decimal("500.00")
|
||||
assert priced.amount_source == "CALCULATED"
|
||||
|
||||
def test_patching_a_different_amount_is_refused(self, admin_client, priced):
|
||||
response = admin_client.patch(
|
||||
_detail_url(priced.pk), {"amount": "9.99"}, format="json"
|
||||
)
|
||||
assert response.status_code == status.HTTP_400_BAD_REQUEST
|
||||
assert "amount" in response.data["errors"]
|
||||
priced.refresh_from_db()
|
||||
assert priced.amount == Decimal("500.00")
|
||||
assert priced.amount_source == "CALCULATED"
|
||||
|
||||
def test_putting_a_different_amount_is_refused(self, admin_client, priced):
|
||||
"""PUT is the verb the mobile edit form uses."""
|
||||
response = admin_client.put(
|
||||
_detail_url(priced.pk),
|
||||
{
|
||||
"name": priced.name,
|
||||
"stage": "QUALIFICATION",
|
||||
"amount": "9.99",
|
||||
},
|
||||
format="json",
|
||||
)
|
||||
assert response.status_code == status.HTTP_400_BAD_REQUEST
|
||||
priced.refresh_from_db()
|
||||
assert priced.amount == Decimal("500.00")
|
||||
|
||||
def test_clearing_the_amount_is_refused(self, admin_client, priced):
|
||||
response = admin_client.patch(
|
||||
_detail_url(priced.pk), {"amount": None}, format="json"
|
||||
)
|
||||
assert response.status_code == status.HTTP_400_BAD_REQUEST
|
||||
priced.refresh_from_db()
|
||||
assert priced.amount == Decimal("500.00")
|
||||
|
||||
def test_echoing_the_computed_amount_is_allowed(self, admin_client, priced):
|
||||
"""A client that sends the whole record back must still be able to edit.
|
||||
|
||||
This is the case that stops the rule from breaking the mobile form,
|
||||
which posts every field on every save.
|
||||
"""
|
||||
response = admin_client.put(
|
||||
_detail_url(priced.pk),
|
||||
{
|
||||
"name": priced.name,
|
||||
"stage": "PROPOSAL",
|
||||
"amount": "500.00",
|
||||
},
|
||||
format="json",
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
priced.refresh_from_db()
|
||||
assert priced.stage == "PROPOSAL"
|
||||
assert priced.amount == Decimal("500.00")
|
||||
|
||||
def test_an_edit_that_omits_the_amount_is_allowed(self, admin_client, priced):
|
||||
response = admin_client.patch(
|
||||
_detail_url(priced.pk), {"description": "Still on track"}, format="json"
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
priced.refresh_from_db()
|
||||
assert priced.amount == Decimal("500.00")
|
||||
|
||||
def test_a_deal_without_line_items_still_owns_its_amount(
|
||||
self, admin_client, opportunity
|
||||
):
|
||||
"""The rule must not leak onto ordinary deals, which are typed by hand."""
|
||||
response = admin_client.patch(
|
||||
_detail_url(opportunity.pk), {"amount": "1234.00"}, format="json"
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
opportunity.refresh_from_db()
|
||||
assert opportunity.amount == Decimal("1234.00")
|
||||
|
||||
def test_deleting_the_last_line_item_hands_the_amount_back(
|
||||
self, admin_client, priced, org_a
|
||||
):
|
||||
_set_rls(org_a)
|
||||
priced.line_items.first().delete()
|
||||
priced.refresh_from_db()
|
||||
assert priced.amount_source == "MANUAL"
|
||||
|
||||
response = admin_client.patch(
|
||||
_detail_url(priced.pk), {"amount": "42.00"}, format="json"
|
||||
)
|
||||
assert response.status_code == status.HTTP_200_OK
|
||||
priced.refresh_from_db()
|
||||
assert priced.amount == Decimal("42.00")
|
||||
|
|
|
|||
|
|
@ -109,6 +109,28 @@ export async function createColumn({ cookies }, boardId, { name, color, order })
|
|||
return apiRequest(`/boards/${boardId}/columns/`, { method: 'POST', body }, { cookies });
|
||||
}
|
||||
|
||||
/**
|
||||
* Create a board.
|
||||
*
|
||||
* `BoardListCreateView.post` derives `org`, `owner` and `created_by` from the
|
||||
* JWT, enrols the caller as the owner member, and, unless told otherwise,
|
||||
* seeds the three default columns. So the whole write surface is a name and an
|
||||
* optional description, and a fresh board is usable the moment it exists.
|
||||
*
|
||||
* Until this existed no client could create one. The page said to use the
|
||||
* mobile app, which has no board feature at all, so an org that had never been
|
||||
* given a board through the API could not get one.
|
||||
*
|
||||
* @param {{ cookies: import('@sveltejs/kit').Cookies }} event
|
||||
* @param {{ name: string, description?: string }} board
|
||||
*/
|
||||
export async function createBoard({ cookies }, { name, description }) {
|
||||
/** @type {Record<string, any>} */
|
||||
const body = { name };
|
||||
if (description) body.description = description;
|
||||
return apiRequest('/boards/', { method: 'POST', body }, { cookies });
|
||||
}
|
||||
|
||||
/**
|
||||
* Add a card to a column. Any board member may; the backend derives the card's
|
||||
* `column`, `org` and `created_by` from the URL and the JWT. The write surface
|
||||
|
|
|
|||
|
|
@ -67,7 +67,18 @@ export async function load(event) {
|
|||
// from this `.server.js` module (SvelteKit forbids that, the same
|
||||
// protection as `$lib/server/`); the toggle just needs to know which of
|
||||
// the CURRENT params survive the switch.
|
||||
boardFields: BOARD_FIELDS
|
||||
boardFields: BOARD_FIELDS,
|
||||
// The currency for figures that are sums rather than one deal: the header
|
||||
// totals and the board lane subtotals. A per-deal currency cannot label a
|
||||
// sum, and `money()` would otherwise fall back to a hardcoded USD and
|
||||
// print a dollar sign over a euro figure.
|
||||
//
|
||||
// This does not make a mixed-currency sum correct. `get_totals` in
|
||||
// `opportunity_views.py` is `SUM(amount)` over every row regardless of
|
||||
// currency, so on an org running more than one the total is an addition
|
||||
// that should not have happened, and no symbol repairs it. Read from the
|
||||
// JWT's org settings, which is also where a new deal's currency comes from.
|
||||
orgCurrency: /** @type {any} */ (locals).org_settings?.default_currency || 'USD'
|
||||
};
|
||||
|
||||
if (boardMode) {
|
||||
|
|
|
|||
|
|
@ -74,8 +74,8 @@
|
|||
that was only ever true under the old hardcoded ?open=true would lie
|
||||
here as soon as somebody switched presets. -->
|
||||
<span class="v2-num">{count(totals.count)}</span> deals ·
|
||||
<span class="v2-num">{money(totals.amount_sum)}</span> ·
|
||||
<span class="v2-num">{money(totals.weighted_sum)}</span> weighted ·
|
||||
<span class="v2-num">{money(totals.amount_sum, data.orgCurrency)}</span> ·
|
||||
<span class="v2-num">{money(totals.weighted_sum, data.orgCurrency)}</span> weighted ·
|
||||
<span class="v2-num" style="color:var(--v2-rust)">{totals.stalled_count}</span> stalled
|
||||
{/snippet}
|
||||
{#snippet actions()}
|
||||
|
|
@ -119,7 +119,7 @@
|
|||
<section class="v2-lane">
|
||||
<div class="v2-lane-head">
|
||||
<span class="v2-label">{STAGE_LABEL[lane.stage]}</span>
|
||||
<span class="v2-num">{count(lane.count)} · {money(lane.sum)}</span>
|
||||
<span class="v2-num">{count(lane.count)} · {money(lane.sum, data.orgCurrency)}</span>
|
||||
</div>
|
||||
<div class="v2-lane-body">
|
||||
{#if lane.truncated}
|
||||
|
|
@ -143,7 +143,7 @@
|
|||
</div>
|
||||
<div class="v2-deal-card-foot">
|
||||
<Avatar name={d.assigned_to} size={21} />
|
||||
<span class="v2-num" style="font-weight:600">{money(d.amount)}</span>
|
||||
<span class="v2-num" style="font-weight:600">{money(d.amount, d.currency)}</span>
|
||||
<span class="v2-sub" style="margin-left:auto;font-size:11.5px"
|
||||
>{shortDate(d.closed_on)}</span
|
||||
>
|
||||
|
|
@ -197,7 +197,7 @@
|
|||
<td data-m="tag">
|
||||
<Pill tone={AGING_TONE[d.aging_status]} dot>{AGING_LABEL[d.aging_status]}</Pill>
|
||||
</td>
|
||||
<td class="v2-r v2-num" style="font-weight:600">{money(d.amount)}</td>
|
||||
<td class="v2-r v2-num" style="font-weight:600">{money(d.amount, d.currency)}</td>
|
||||
<td>{shortDate(d.closed_on)}</td>
|
||||
<!-- Hidden on a phone: the Health pill beside the title is computed
|
||||
from this same number, so showing both spends a line to say
|
||||
|
|
|
|||
|
|
@ -100,13 +100,13 @@
|
|||
<tr>
|
||||
<td>{li.name}</td>
|
||||
<td class="v2-r v2-num">{li.quantity}</td>
|
||||
<td class="v2-r v2-num">{money(li.unit_price)}</td>
|
||||
<td class="v2-r v2-num">{money(li.unit_price, deal.currency)}</td>
|
||||
<!-- Blank, not "0", where there is no discount. A column of
|
||||
zeroes reads as a discount that happened to be nothing. -->
|
||||
<td class="v2-r v2-num v2-muted">
|
||||
{li.discount_amount > 0 ? `−${money(li.discount_amount)}` : ''}
|
||||
{li.discount_amount > 0 ? `−${money(li.discount_amount, deal.currency)}` : ''}
|
||||
</td>
|
||||
<td class="v2-r v2-num">{money(li.total)}</td>
|
||||
<td class="v2-r v2-num">{money(li.total, deal.currency)}</td>
|
||||
</tr>
|
||||
{/each}
|
||||
</tbody>
|
||||
|
|
@ -116,12 +116,13 @@
|
|||
>
|
||||
{#if discount > 0}
|
||||
<span class="v2-muted">Subtotal</span>
|
||||
<span class="v2-num v2-muted">{money(subtotal)}</span>
|
||||
<span class="v2-num v2-muted">{money(subtotal, deal.currency)}</span>
|
||||
<span class="v2-muted">Discounts</span>
|
||||
<span class="v2-num v2-muted">−{money(discount)}</span>
|
||||
<span class="v2-num v2-muted">−{money(discount, deal.currency)}</span>
|
||||
{/if}
|
||||
<span style="font-weight:650">Total</span>
|
||||
<span class="v2-num" style="font-weight:650">{money(deal.amount)}</span>
|
||||
<span class="v2-num" style="font-weight:650">{money(deal.amount, deal.currency)}</span
|
||||
>
|
||||
</div>
|
||||
</div>
|
||||
{/if}
|
||||
|
|
|
|||
|
|
@ -302,8 +302,9 @@
|
|||
{:else if amountIsCalculated}
|
||||
<p class="v2-hint" id="h-amount">
|
||||
<a href="/pipeline/{deal.id}">{server.line_item_count} line items</a> add up to
|
||||
<span class="v2-num">{money(server.line_item_total)}</span>. The server recalculates
|
||||
this on every save, so typing over it would not last. Edit the line items instead.
|
||||
<span class="v2-num">{money(server.line_item_total, deal.currency)}</span>. The server
|
||||
refuses a different figure on a deal with line items, so this cannot be typed over. Edit
|
||||
the line items instead.
|
||||
</p>
|
||||
{:else}
|
||||
<p class="v2-hint">
|
||||
|
|
|
|||
|
|
@ -3,7 +3,14 @@ import { EDITABLE_FIELDS, createDeal, getDealFormOptions } from '$lib/server/v2/
|
|||
|
||||
/** @type {import('./$types').PageServerLoad} */
|
||||
export async function load(event) {
|
||||
return await getDealFormOptions(event);
|
||||
return {
|
||||
...(await getDealFormOptions(event)),
|
||||
// The currency this deal will be created in. The form has no currency
|
||||
// field, so the serializer stamps the org default
|
||||
// (`OpportunityCreateSerializer.create`), and the hint under the amount
|
||||
// used to say "USD" whatever that default actually was.
|
||||
orgCurrency: /** @type {any} */ (event.locals).org_settings?.default_currency || 'USD'
|
||||
};
|
||||
}
|
||||
|
||||
/** @type {import('./$types').Actions} */
|
||||
|
|
|
|||
|
|
@ -210,7 +210,9 @@
|
|||
<p class="v2-error" id="e-amount">{errors.amount}</p>
|
||||
{:else}
|
||||
<p class="v2-hint" id="h-amount">
|
||||
{Number(form.amount) > 0 ? money(Number(form.amount)) : 'USD'}
|
||||
{Number(form.amount) > 0
|
||||
? money(Number(form.amount), data.orgCurrency)
|
||||
: data.orgCurrency}
|
||||
</p>
|
||||
{/if}
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -1,5 +1,11 @@
|
|||
import { fail } from '@sveltejs/kit';
|
||||
import { createCard, createColumn, getBoard, moveBoardTask } from '$lib/server/v2/board.js';
|
||||
import {
|
||||
createBoard,
|
||||
createCard,
|
||||
createColumn,
|
||||
getBoard,
|
||||
moveBoardTask
|
||||
} from '$lib/server/v2/board.js';
|
||||
import { readableError } from '$lib/server/v2/form-errors.js';
|
||||
|
||||
/** Map an api-helpers error to a form `fail`, preserving a 403 as a 403. */
|
||||
|
|
@ -33,6 +39,25 @@ export const actions = {
|
|||
}
|
||||
},
|
||||
|
||||
// Create the org's first board, or another one.
|
||||
//
|
||||
// The empty state used to send people to the mobile app for this. The mobile
|
||||
// app has no board feature, and no client called `POST /boards/` at all, so
|
||||
// an org that had never been handed a board through the API had no way to
|
||||
// get one. The backend seeds the three default columns, so the board is
|
||||
// usable as soon as it lands.
|
||||
create: async ({ request, cookies }) => {
|
||||
const form = await request.formData();
|
||||
const name = String(form.get('name') || '').trim();
|
||||
if (!name) return fail(400, { error: 'A board needs a name.' });
|
||||
try {
|
||||
await createBoard({ cookies }, { name });
|
||||
return { added: 'board' };
|
||||
} catch (err) {
|
||||
return toFail(err, 'Could not create the board.');
|
||||
}
|
||||
},
|
||||
|
||||
// Add a card to a column. Any board member may; the backend is the boundary.
|
||||
addCard: async ({ request, cookies }) => {
|
||||
const form = await request.formData();
|
||||
|
|
|
|||
|
|
@ -72,6 +72,10 @@
|
|||
let addColumnBusy = $state(false);
|
||||
let addColumnError = $state('');
|
||||
|
||||
// Only ever seen on the empty state, where there is no board to add to yet.
|
||||
let createBoardBusy = $state(false);
|
||||
let createBoardError = $state('');
|
||||
|
||||
function openCardForm(/** @type {string} */ columnId) {
|
||||
addingCardTo = columnId;
|
||||
addCardError = '';
|
||||
|
|
@ -181,14 +185,54 @@
|
|||
<SectionTabs set="tasks" />
|
||||
|
||||
{#if !data.board}
|
||||
<!-- This used to say "Create one from the mobile app to see it here." The
|
||||
mobile app has no boards, and no client called POST /boards/, so an org
|
||||
that had never been given one through the API was stuck here. -->
|
||||
<div class="v2-pad" style="padding-top:32px">
|
||||
<div class="v2-empty">
|
||||
<Plus size={22} style="opacity:0.4" />
|
||||
<p class="v2-empty-title">No boards yet</p>
|
||||
<p class="v2-sub" style="max-width:34ch;text-align:center">
|
||||
A board organises work into columns you drag cards between. Create one from the mobile app
|
||||
to see it here.
|
||||
A board organises work into columns you drag cards between. It starts with To Do, In
|
||||
Progress and Done, and you can rename them later.
|
||||
</p>
|
||||
<form
|
||||
class="v2-lane-add-form"
|
||||
style="max-width:20rem;margin-top:14px"
|
||||
method="POST"
|
||||
action="?/create"
|
||||
use:enhance={() => {
|
||||
createBoardBusy = true;
|
||||
return async ({ result }) => {
|
||||
createBoardBusy = false;
|
||||
if (result.type === 'success') {
|
||||
createBoardError = '';
|
||||
await invalidateAll();
|
||||
} else if (result.type === 'failure') {
|
||||
createBoardError =
|
||||
/** @type {any} */ (result.data)?.error || 'Could not create the board.';
|
||||
} else if (result.type === 'error') {
|
||||
createBoardError = 'Could not create the board.';
|
||||
}
|
||||
};
|
||||
}}
|
||||
>
|
||||
<!-- svelte-ignore a11y_autofocus -->
|
||||
<input
|
||||
class="v2-input"
|
||||
name="name"
|
||||
placeholder="Board name"
|
||||
maxlength="255"
|
||||
required
|
||||
autofocus
|
||||
/>
|
||||
<button class="v2-btn v2-btn-primary" type="submit" disabled={createBoardBusy}>
|
||||
{createBoardBusy ? 'Creating…' : 'Create board'}
|
||||
</button>
|
||||
</form>
|
||||
{#if createBoardError}
|
||||
<p class="v2-error" role="status">{createBoardError}</p>
|
||||
{/if}
|
||||
</div>
|
||||
</div>
|
||||
{:else}
|
||||
|
|
|
|||
|
|
@ -1,11 +1,13 @@
|
|||
import 'dart:convert';
|
||||
import 'package:flutter/foundation.dart';
|
||||
import 'package:http/http.dart' as http;
|
||||
import 'package:jwt_decoder/jwt_decoder.dart';
|
||||
import '../config/api_config.dart';
|
||||
|
||||
/// Callback invoked when an authenticated request gets a 401, should hit the
|
||||
/// refresh endpoint and update the ApiService's access token, returning true
|
||||
/// if the new token is ready and the original request can be retried.
|
||||
/// Callback invoked when the access token has aged out (or, as a fallback, when
|
||||
/// a request comes back 401). Should hit the refresh endpoint and update the
|
||||
/// ApiService's access token, returning true if the new token is ready and the
|
||||
/// original request can be sent.
|
||||
typedef RefreshTokenCallback = Future<bool> Function();
|
||||
|
||||
/// API response wrapper
|
||||
|
|
@ -36,7 +38,14 @@ class ApiService {
|
|||
factory ApiService() => _instance;
|
||||
ApiService._internal();
|
||||
|
||||
final http.Client _client = http.Client();
|
||||
http.Client _client = http.Client();
|
||||
|
||||
/// Swap the HTTP client. Tests only: the app shares one client for the
|
||||
/// process lifetime, and `ApiService` is a singleton.
|
||||
@visibleForTesting
|
||||
void setClientForTesting(http.Client client) {
|
||||
_client = client;
|
||||
}
|
||||
|
||||
// Token and org getters - will be set by AuthService
|
||||
String? _accessToken;
|
||||
|
|
@ -88,16 +97,55 @@ class ApiService {
|
|||
}
|
||||
}
|
||||
|
||||
/// Send an HTTP request and transparently retry once on 401 after refreshing
|
||||
/// the access token. The `send` closure must be safe to invoke twice, every
|
||||
/// caller below builds a fresh request inside it.
|
||||
/// How close to expiry counts as expired.
|
||||
///
|
||||
/// Covers a token that would age out while the request is in flight, and
|
||||
/// small clock differences between the phone and the server.
|
||||
static const Duration _expiryGrace = Duration(seconds: 30);
|
||||
|
||||
/// True when the access token is at or near its `exp`.
|
||||
///
|
||||
/// A token that will not decode returns false: refreshing cannot repair a
|
||||
/// malformed credential, so the request carries it and the server answers.
|
||||
bool get _accessTokenNeedsRefresh {
|
||||
final token = _accessToken;
|
||||
if (token == null) return false;
|
||||
try {
|
||||
return JwtDecoder.getExpirationDate(
|
||||
token,
|
||||
).isBefore(DateTime.now().add(_expiryGrace));
|
||||
} catch (_) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/// Send an HTTP request, refreshing the access token first when it has aged
|
||||
/// out. The `send` closure must be safe to invoke twice, every caller below
|
||||
/// builds a fresh request inside it.
|
||||
Future<http.Response> _sendWithRetry({
|
||||
required bool requiresAuth,
|
||||
required Future<http.Response> Function(Map<String, String> headers) send,
|
||||
}) async {
|
||||
// Refresh BEFORE sending, not after a rejection.
|
||||
//
|
||||
// This API does not answer 401 for an expired credential. `HasOrgContext`
|
||||
// (backend `common/permissions.py`) denies with 403 before DRF's auth layer
|
||||
// would emit a 401, and it answers the same way for a missing, malformed or
|
||||
// expired token. So the 401 branch below never fires on expiry, and a
|
||||
// session that aged out while the app was open stayed broken until the next
|
||||
// cold start, where `AuthService.initialize()` does this same check once.
|
||||
//
|
||||
// The web client refreshes the same way, off the `exp` claim on every
|
||||
// request (`frontend/src/hooks.server.js`), rather than off a status code.
|
||||
if (requiresAuth && _accessTokenNeedsRefresh && _refreshCallback != null) {
|
||||
await _refreshAccessToken();
|
||||
}
|
||||
|
||||
var response = await send(_buildHeaders(requiresAuth: requiresAuth))
|
||||
.timeout(ApiConfig.connectTimeout);
|
||||
|
||||
// Kept as a fallback for a token revoked or rotated out from under us
|
||||
// before its `exp`, which the check above cannot see.
|
||||
if (requiresAuth &&
|
||||
response.statusCode == 401 &&
|
||||
_accessToken != null &&
|
||||
|
|
@ -267,8 +315,10 @@ class ApiService {
|
|||
bool requiresAuth = true,
|
||||
}) async {
|
||||
try {
|
||||
// The URL only. A request body is never safe to log here: the refresh
|
||||
// call carries the refresh token, sign-in carries the Google id token,
|
||||
// and an ordinary create carries a customer's name, email and phone.
|
||||
debugPrint('POST $url');
|
||||
debugPrint('Body: ${jsonEncode(body)}');
|
||||
|
||||
final uri = Uri.parse(url);
|
||||
final encoded = jsonEncode(body);
|
||||
|
|
|
|||
177
mobile/test/unit/api_service_refresh_test.dart
Normal file
177
mobile/test/unit/api_service_refresh_test.dart
Normal file
|
|
@ -0,0 +1,177 @@
|
|||
import 'dart:convert';
|
||||
|
||||
import 'package:bottle_crm/config/api_config.dart';
|
||||
import 'package:bottle_crm/services/api_service.dart';
|
||||
import 'package:flutter_test/flutter_test.dart';
|
||||
import 'package:http/http.dart' as http;
|
||||
import 'package:http/testing.dart';
|
||||
|
||||
/// A JWT that decodes but is not signed. `JwtDecoder` reads the claims without
|
||||
/// verifying, which is all the expiry check does.
|
||||
String jwtExpiring(Duration fromNow) {
|
||||
String seg(Map<String, dynamic> m) =>
|
||||
base64Url.encode(utf8.encode(jsonEncode(m))).replaceAll('=', '');
|
||||
final exp = DateTime.now().add(fromNow).millisecondsSinceEpoch ~/ 1000;
|
||||
return '${seg({'alg': 'HS256', 'typ': 'JWT'})}.${seg({'exp': exp})}.sig';
|
||||
}
|
||||
|
||||
void main() {
|
||||
// The singleton carries state between tests, so every test sets what it
|
||||
// needs and clears afterwards.
|
||||
tearDown(() {
|
||||
ApiService().clearAuth();
|
||||
ApiService().setRefreshCallback(null);
|
||||
ApiService().setClientForTesting(http.Client());
|
||||
});
|
||||
|
||||
group('access token refresh', () {
|
||||
test('refreshes an expired token before sending, even though the API '
|
||||
'answers 403 rather than 401', () async {
|
||||
final api = ApiService();
|
||||
final sentTokens = <String?>[];
|
||||
var refreshCalls = 0;
|
||||
|
||||
api.setAccessToken(jwtExpiring(const Duration(minutes: -5)));
|
||||
api.setRefreshCallback(() async {
|
||||
refreshCalls++;
|
||||
api.setAccessToken(jwtExpiring(const Duration(hours: 1)));
|
||||
return true;
|
||||
});
|
||||
api.setClientForTesting(
|
||||
MockClient((request) async {
|
||||
sentTokens.add(request.headers['Authorization']);
|
||||
// What the real API returns for a credential it will not accept:
|
||||
// HasOrgContext denies before DRF's auth layer emits a 401.
|
||||
return http.Response(
|
||||
jsonEncode({'detail': 'Organization context is required.'}),
|
||||
403,
|
||||
);
|
||||
}),
|
||||
);
|
||||
|
||||
await api.get(ApiConfig.leads);
|
||||
|
||||
expect(refreshCalls, 1, reason: 'expiry must trigger exactly one refresh');
|
||||
// One send, not two: the token was renewed on the way out, so there is
|
||||
// nothing to retry. Before this fix the count was one send and zero
|
||||
// refreshes, because the 401 branch never saw a 401.
|
||||
expect(sentTokens, hasLength(1));
|
||||
});
|
||||
|
||||
test('the request carries the refreshed token, not the expired one',
|
||||
() async {
|
||||
final api = ApiService();
|
||||
final expired = jwtExpiring(const Duration(minutes: -5));
|
||||
final fresh = jwtExpiring(const Duration(hours: 1));
|
||||
String? sent;
|
||||
|
||||
api.setAccessToken(expired);
|
||||
api.setRefreshCallback(() async {
|
||||
api.setAccessToken(fresh);
|
||||
return true;
|
||||
});
|
||||
api.setClientForTesting(
|
||||
MockClient((request) async {
|
||||
sent = request.headers['Authorization'];
|
||||
return http.Response('{}', 200);
|
||||
}),
|
||||
);
|
||||
|
||||
await api.get(ApiConfig.leads);
|
||||
|
||||
expect(sent, 'Bearer $fresh');
|
||||
expect(sent, isNot('Bearer $expired'));
|
||||
});
|
||||
|
||||
test('a live token is sent as-is, with no refresh round-trip', () async {
|
||||
final api = ApiService();
|
||||
final live = jwtExpiring(const Duration(hours: 1));
|
||||
var refreshCalls = 0;
|
||||
String? sent;
|
||||
|
||||
api.setAccessToken(live);
|
||||
api.setRefreshCallback(() async {
|
||||
refreshCalls++;
|
||||
return true;
|
||||
});
|
||||
api.setClientForTesting(
|
||||
MockClient((request) async {
|
||||
sent = request.headers['Authorization'];
|
||||
return http.Response('{}', 200);
|
||||
}),
|
||||
);
|
||||
|
||||
await api.get(ApiConfig.leads);
|
||||
|
||||
expect(refreshCalls, 0);
|
||||
expect(sent, 'Bearer $live');
|
||||
});
|
||||
|
||||
test('a token inside the grace window counts as expired', () async {
|
||||
final api = ApiService();
|
||||
var refreshCalls = 0;
|
||||
|
||||
// Valid for another 5 seconds, which is less than the 30-second grace:
|
||||
// it would age out mid-flight.
|
||||
api.setAccessToken(jwtExpiring(const Duration(seconds: 5)));
|
||||
api.setRefreshCallback(() async {
|
||||
refreshCalls++;
|
||||
api.setAccessToken(jwtExpiring(const Duration(hours: 1)));
|
||||
return true;
|
||||
});
|
||||
api.setClientForTesting(
|
||||
MockClient((_) async => http.Response('{}', 200)),
|
||||
);
|
||||
|
||||
await api.get(ApiConfig.leads);
|
||||
|
||||
expect(refreshCalls, 1);
|
||||
});
|
||||
|
||||
test('an unauthenticated call never refreshes', () async {
|
||||
final api = ApiService();
|
||||
var refreshCalls = 0;
|
||||
|
||||
api.setAccessToken(jwtExpiring(const Duration(minutes: -5)));
|
||||
api.setRefreshCallback(() async {
|
||||
refreshCalls++;
|
||||
return true;
|
||||
});
|
||||
api.setClientForTesting(
|
||||
MockClient((_) async => http.Response('{}', 200)),
|
||||
);
|
||||
|
||||
// The refresh endpoint itself posts with requiresAuth: false. Without
|
||||
// this exemption the expired token would trigger a refresh, whose own
|
||||
// request would trigger another, and `_refreshInFlight` would return the
|
||||
// future that is waiting on it.
|
||||
await api.post(ApiConfig.refreshToken, {'refresh': 'x'},
|
||||
requiresAuth: false);
|
||||
|
||||
expect(refreshCalls, 0);
|
||||
});
|
||||
|
||||
test('a token that will not decode is sent as-is', () async {
|
||||
final api = ApiService();
|
||||
var refreshCalls = 0;
|
||||
String? sent;
|
||||
|
||||
api.setAccessToken('not-a-jwt');
|
||||
api.setRefreshCallback(() async {
|
||||
refreshCalls++;
|
||||
return true;
|
||||
});
|
||||
api.setClientForTesting(
|
||||
MockClient((request) async {
|
||||
sent = request.headers['Authorization'];
|
||||
return http.Response('{}', 403);
|
||||
}),
|
||||
);
|
||||
|
||||
await api.get(ApiConfig.leads);
|
||||
|
||||
expect(refreshCalls, 0, reason: 'refreshing cannot repair a bad token');
|
||||
expect(sent, 'Bearer not-a-jwt');
|
||||
});
|
||||
});
|
||||
}
|
||||
Loading…
Reference in a new issue