fix(license): apply the review follow-ups on the license stack head
Follow-ups of the three /pr-review passes on #386, #387 and #388, applied on the stack head before the fast-forward merge: - #386: pin the relay of the revoked state by revocationCheckLauncher (a launcher passing false instead of wasRevoked passed the whole suite), and say in the rate budget comment that two checks in one minute bring the machine-limit path to 6 requests. - #387 S1: the fake `t` of the removal tests and of the License card tests marks what it translates, so a string that skipped `t` fails its test. - #387 S2: pin that storing another key stops on Storage while the previous token cannot be dropped, the previous key staying. - #387 S3: reload the license context after a failed removal too. Rust deletes the key first, so a token that resisted left the key gone while the card kept showing it under "the key was not removed". - #388: pin that an unreadable token stays (the directory case could not tell, its deletion fails anyway). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
le king fu
parent
fd90de5424
commit
2cbc886653
5 changed files with 114 additions and 35 deletions
|
|
@ -1979,6 +1979,22 @@ mod tests {
|
|||
assert!(dir.0.join(ACTIVATION_FILE).exists(), "the same key keeps its activation");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn store_refuses_another_key_while_the_previous_token_cannot_be_dropped() {
|
||||
// Kept next to the new key, that token would name the previous license to the price
|
||||
// server (#347 review, the folder of #367): the key change stops on `Storage` instead,
|
||||
// and the previous key stays (#387 review).
|
||||
let (enc, dec) = default_keys();
|
||||
let dir = TestDir::new("store_token_not_deletable");
|
||||
let old_key = base_license_with_features(&enc, vec!["balance".to_string()]);
|
||||
dir.write(LICENSE_FILE, &old_key);
|
||||
// A directory in place of the token: removing it as a file fails.
|
||||
fs::create_dir(dir.0.join(ACTIVATION_FILE)).unwrap();
|
||||
let new_key = base_license_with_features(&enc, vec![]);
|
||||
assert_eq!(store_license_in(&dir.0, &new_key, &dec).unwrap_err(), LicenseError::Storage);
|
||||
assert_eq!(fs::read_to_string(dir.0.join(LICENSE_FILE)).unwrap(), old_key);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn store_drops_a_token_left_without_a_key() {
|
||||
let (enc, dec) = default_keys();
|
||||
|
|
@ -3575,6 +3591,16 @@ mod tests {
|
|||
assert_eq!(remove_activation_token_if_unchanged_in(&dir.0, " token-a"), Ok(true));
|
||||
assert!(!dir.0.join(ACTIVATION_FILE).exists());
|
||||
|
||||
// A token that does not read as text: nothing says it is the one the answer is about,
|
||||
// so it stays (#388 review; the directory below cannot tell, its deletion fails anyway).
|
||||
dir.write(ACTIVATION_FILE, NOT_UTF8);
|
||||
assert_eq!(
|
||||
remove_activation_token_if_unchanged_in(&dir.0, "token-a"),
|
||||
Err(LicenseError::Storage)
|
||||
);
|
||||
assert!(dir.0.join(ACTIVATION_FILE).exists(), "an unreadable token stays");
|
||||
fs::remove_file(dir.0.join(ACTIVATION_FILE)).unwrap();
|
||||
|
||||
// A directory in place of the token: it cannot be read as a file.
|
||||
fs::create_dir(dir.0.join(ACTIVATION_FILE)).unwrap();
|
||||
assert_eq!(
|
||||
|
|
|
|||
|
|
@ -10,10 +10,13 @@ vi.mock("@tauri-apps/api/core", () => ({ invoke: vi.fn() }));
|
|||
vi.mock("@tauri-apps/plugin-opener", () => ({ openUrl: vi.fn(() => Promise.resolve()) }));
|
||||
|
||||
let language = "fr";
|
||||
// The fake `t` marks what it translates, `t(key[options])`: the assertions written in that form
|
||||
// fail on a string that skipped `t` (#387 review). Substring assertions on a bare key still read
|
||||
// the key inside the mark.
|
||||
vi.mock("react-i18next", () => ({
|
||||
useTranslation: () => ({
|
||||
t: (k: string, o?: Record<string, unknown>) =>
|
||||
o ? `${k}[${Object.entries(o).map(([a, b]) => `${a}=${String(b)}`).join(",")}]` : k,
|
||||
o ? `t(${k}[${Object.entries(o).map(([a, b]) => `${a}=${String(b)}`).join(",")}])` : `t(${k})`,
|
||||
i18n: { language },
|
||||
}),
|
||||
}));
|
||||
|
|
@ -300,8 +303,8 @@ describe("LicenseCard machines", () => {
|
|||
|
||||
it("locks key entry while an activation runs, and Activate while a key is stored", () => {
|
||||
// Either order would leave the previous key's token next to the new key (#364 review).
|
||||
const enterKeyDisabled = /<button[^>]*disabled=""[^>]*>license\.enterKey<\/button>/;
|
||||
const activateDisabled = /<button[^>]*disabled=""[^>]*>license\.activate<\/button>/;
|
||||
const enterKeyDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.enterKey\)<\/button>/;
|
||||
const activateDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.activate\)<\/button>/;
|
||||
|
||||
machines = hookView({ activation: notActivated, activating: true });
|
||||
expect(render()).toMatch(enterKeyDisabled);
|
||||
|
|
@ -328,11 +331,11 @@ describe("LicenseCard machines", () => {
|
|||
// "Remove the key" shows for any stored key, valid or revoked (#370): `hasLicense` (a paid
|
||||
// edition) would hide it for a revoked key, whose daily check is what the removal stops.
|
||||
describe("LicenseCard key removal (#370)", () => {
|
||||
const REMOVE = "license.removeKey.button";
|
||||
const removeDisabled = /<button[^>]*disabled=""[^>]*>license\.removeKey\.button<\/button>/;
|
||||
const enterKeyDisabled = /<button[^>]*disabled=""[^>]*>license\.enterKey<\/button>/;
|
||||
const activateDisabled = /<button[^>]*disabled=""[^>]*>license\.activate<\/button>/;
|
||||
const deactivateDisabled = /<button[^>]*disabled=""[^>]*>license\.machines\.deactivate<\/button>/;
|
||||
const REMOVE = "t(license.removeKey.button)";
|
||||
const removeDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.removeKey\.button\)<\/button>/;
|
||||
const enterKeyDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.enterKey\)<\/button>/;
|
||||
const activateDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.activate\)<\/button>/;
|
||||
const deactivateDisabled = /<button[^>]*disabled=""[^>]*>t\(license\.machines\.deactivate\)<\/button>/;
|
||||
|
||||
it("is offered for a valid key", () => {
|
||||
const html = render();
|
||||
|
|
@ -376,7 +379,7 @@ describe("LicenseCard key removal (#370)", () => {
|
|||
it("while it runs, says so, and key entry, Activate and Deactivate wait", () => {
|
||||
machines = hookView({ activation: notActivated, removing: true });
|
||||
let html = render();
|
||||
expect(html).toContain("license.removeKey.removing");
|
||||
expect(html).toContain("t(license.removeKey.removing)");
|
||||
expect(html).not.toContain(REMOVE);
|
||||
expect(html).toMatch(enterKeyDisabled);
|
||||
expect(html).toMatch(activateDisabled);
|
||||
|
|
@ -396,7 +399,7 @@ describe("LicenseCard key removal (#370)", () => {
|
|||
it("names a failed removal with its reason", () => {
|
||||
machines = hookView({ removeError: parseLicenseError('{"code":"storage"}') });
|
||||
const html = render();
|
||||
expect(html).toContain("license.removeKey.failed[reason=license.errors.storage]");
|
||||
expect(html).toContain("t(license.removeKey.failed[reason=t(license.errors.storage)])");
|
||||
expect(html.split("license.removeKey.failed")).toHaveLength(2);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -601,3 +601,33 @@ describe("performRevocationCheck", () => {
|
|||
expect(deps.refresh).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
// The launcher relays the revoked state the provider read: a reload follows a change only.
|
||||
// Every other test of the launcher starts from a key that is not revoked (#386 review).
|
||||
describe("revocationCheckLauncher", () => {
|
||||
const settle = () => new Promise<void>((resolve) => setTimeout(resolve, 0));
|
||||
function launcherHarness(result: RevocationCheck) {
|
||||
const deps: RevocationCheckDeps = {
|
||||
checkLicenseRevocation: vi.fn(async (): Promise<RevocationCheck> => result),
|
||||
refresh: vi.fn(async () => {}),
|
||||
logInfo: vi.fn(),
|
||||
logWarn: vi.fn(),
|
||||
};
|
||||
return deps;
|
||||
}
|
||||
|
||||
it("reloads when a key known as revoked is confirmed valid again", async () => {
|
||||
const deps = launcherHarness({ outcome: "valid", revoked: false });
|
||||
revocationCheckLauncher(deps)(true);
|
||||
await settle();
|
||||
expect(deps.refresh).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("does not reload a key known as revoked that is still revoked", async () => {
|
||||
const deps = launcherHarness({ outcome: "revoked", revoked: true });
|
||||
revocationCheckLauncher(deps)(true);
|
||||
await settle();
|
||||
expect(deps.checkLicenseRevocation).toHaveBeenCalledTimes(1);
|
||||
expect(deps.refresh).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -642,7 +642,7 @@ describe("useLicenseMachines wiring to the LicenseContext", () => {
|
|||
|
||||
await view.removeKey({ confirm, t: echo });
|
||||
|
||||
expect(confirm).toHaveBeenCalledWith("license.removeKey.confirmRevoked");
|
||||
expect(confirm).toHaveBeenCalledWith("t(license.removeKey.confirmRevoked)");
|
||||
expect(invoke).toHaveBeenCalledWith("remove_license", { releaseSlot: false });
|
||||
expect(context.refresh).toHaveBeenCalledTimes(1);
|
||||
expect(log.logInfo).toHaveBeenCalledWith("License: the stored key was removed");
|
||||
|
|
@ -653,17 +653,19 @@ describe("useLicenseMachines wiring to the LicenseContext", () => {
|
|||
const confirm = vi.fn(() => false);
|
||||
const view = renderHook();
|
||||
await view.removeKey({ confirm, t: echo });
|
||||
expect(confirm).toHaveBeenCalledWith("license.removeKey.confirmValid");
|
||||
expect(confirm).toHaveBeenCalledWith("t(license.removeKey.confirmValid)");
|
||||
expect(invoke).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
// The removal of the stored key (#370). The fake `t` echoes the key and its options, so each
|
||||
// assertion names the message the user would read.
|
||||
// The removal of the stored key (#370). The fake `t` marks what it translates, `t(key[options])`,
|
||||
// so each assertion names the message the user would read, and a string that skipped `t` (a raw
|
||||
// key) fails it (#387 review).
|
||||
function echo(key: string, options?: Record<string, unknown>): string {
|
||||
return options
|
||||
? `${key}[${Object.entries(options).map(([name, value]) => `${name}=${String(value)}`).join(",")}]`
|
||||
: key;
|
||||
const params = options
|
||||
? `[${Object.entries(options).map(([name, value]) => `${name}=${String(value)}`).join(",")}]`
|
||||
: "";
|
||||
return `t(${key}${params})`;
|
||||
}
|
||||
|
||||
describe("licenseMachinesReducer — removal of the key", () => {
|
||||
|
|
@ -776,7 +778,7 @@ describe("performRemoveLicense", () => {
|
|||
it("calls nothing when the confirmation is refused", async () => {
|
||||
const { deps, events, getState } = removalHarness([false], []);
|
||||
await performRemoveLicense(deps, activated);
|
||||
expect(events).toEqual(["confirm license.removeKey.confirmActivated"]);
|
||||
expect(events).toEqual(["confirm t(license.removeKey.confirmActivated)"]);
|
||||
expect(deps.removeLicense).not.toHaveBeenCalled();
|
||||
expect(deps.refresh).not.toHaveBeenCalled();
|
||||
expect(deps.logInfo).not.toHaveBeenCalled();
|
||||
|
|
@ -788,7 +790,7 @@ describe("performRemoveLicense", () => {
|
|||
const { deps, events, getState } = removalHarness([true], [removed]);
|
||||
await performRemoveLicense(deps, valid);
|
||||
expect(events).toEqual([
|
||||
"confirm license.removeKey.confirmValid",
|
||||
"confirm t(license.removeKey.confirmValid)",
|
||||
"REMOVE_START",
|
||||
"remove(false)",
|
||||
"refresh",
|
||||
|
|
@ -805,7 +807,7 @@ describe("performRemoveLicense", () => {
|
|||
const { deps, events } = removalHarness([true], [removed]);
|
||||
await performRemoveLicense(deps, activated);
|
||||
expect(events).toEqual([
|
||||
"confirm license.removeKey.confirmActivated",
|
||||
"confirm t(license.removeKey.confirmActivated)",
|
||||
"REMOVE_START",
|
||||
"remove(true)",
|
||||
"refresh",
|
||||
|
|
@ -820,7 +822,7 @@ describe("performRemoveLicense", () => {
|
|||
const { deps, events } = removalHarness([true], [removed]);
|
||||
await performRemoveLicense(deps, revoked);
|
||||
expect(events.slice(0, 3)).toEqual([
|
||||
"confirm license.removeKey.confirmRevoked",
|
||||
"confirm t(license.removeKey.confirmRevoked)",
|
||||
"REMOVE_START",
|
||||
"remove(false)",
|
||||
]);
|
||||
|
|
@ -829,10 +831,10 @@ describe("performRemoveLicense", () => {
|
|||
// Error table, second row: the server could not be reached or answered badly. Rust removed
|
||||
// nothing; the second confirmation names the reason and warns that the slot stays occupied.
|
||||
const retryable: [LicenseErrorCode, Record<string, unknown>, string][] = [
|
||||
["network", {}, "license.errors.network"],
|
||||
["server", { status: 502 }, "license.errors.server"],
|
||||
["rate_limit", { retry_after_s: 42 }, "license.errors.rateLimit[count=42]"],
|
||||
["invalid_response", {}, "license.errors.invalidResponse"],
|
||||
["network", {}, "t(license.errors.network)"],
|
||||
["server", { status: 502 }, "t(license.errors.server)"],
|
||||
["rate_limit", { retry_after_s: 42 }, "t(license.errors.rateLimit[count=42])"],
|
||||
["invalid_response", {}, "t(license.errors.invalidResponse)"],
|
||||
];
|
||||
|
||||
it.each(retryable)(
|
||||
|
|
@ -841,10 +843,10 @@ describe("performRemoveLicense", () => {
|
|||
const { deps, events, getState } = removalHarness([true, true], [failure(code, extra), removed]);
|
||||
await performRemoveLicense(deps, activated);
|
||||
expect(events).toEqual([
|
||||
"confirm license.removeKey.confirmActivated",
|
||||
"confirm t(license.removeKey.confirmActivated)",
|
||||
"REMOVE_START",
|
||||
"remove(true)",
|
||||
`confirm license.removeKey.confirmWithoutRelease[reason=${reason}]`,
|
||||
`confirm t(license.removeKey.confirmWithoutRelease[reason=${reason}])`,
|
||||
"remove(false)",
|
||||
"refresh",
|
||||
"REMOVE_END",
|
||||
|
|
@ -888,14 +890,15 @@ describe("performRemoveLicense", () => {
|
|||
const { deps, events, getState } = removalHarness([true], [failure(code)]);
|
||||
await performRemoveLicense(deps, activated);
|
||||
expect(events).toEqual([
|
||||
"confirm license.removeKey.confirmActivated",
|
||||
"confirm t(license.removeKey.confirmActivated)",
|
||||
"REMOVE_START",
|
||||
"remove(true)",
|
||||
"refresh",
|
||||
"REMOVE_ERROR",
|
||||
]);
|
||||
expect(getState().removeError?.code).toBe(code);
|
||||
expect(getState().removing).toBe(false);
|
||||
expect(deps.refresh).not.toHaveBeenCalled();
|
||||
expect(deps.refresh).toHaveBeenCalledTimes(1);
|
||||
expect(deps.logInfo).not.toHaveBeenCalled();
|
||||
expect(deps.logWarn).toHaveBeenCalledWith("License: the stored key could not be removed", code);
|
||||
});
|
||||
|
|
@ -905,9 +908,10 @@ describe("performRemoveLicense", () => {
|
|||
const { deps, events, getState } = removalHarness([true, true], [failure("network")]);
|
||||
await performRemoveLicense(deps, valid);
|
||||
expect(events).toEqual([
|
||||
"confirm license.removeKey.confirmValid",
|
||||
"confirm t(license.removeKey.confirmValid)",
|
||||
"REMOVE_START",
|
||||
"remove(false)",
|
||||
"refresh",
|
||||
"REMOVE_ERROR",
|
||||
]);
|
||||
expect(getState().removeError?.code).toBe("network");
|
||||
|
|
@ -916,9 +920,19 @@ describe("performRemoveLicense", () => {
|
|||
it("names a removal that fails after the slot was given up", async () => {
|
||||
const { deps, events, getState } = removalHarness([true, true], [failure("network"), failure("storage")]);
|
||||
await performRemoveLicense(deps, activated);
|
||||
expect(events.slice(-2)).toEqual(["remove(false)", "REMOVE_ERROR"]);
|
||||
expect(events.slice(-3)).toEqual(["remove(false)", "refresh", "REMOVE_ERROR"]);
|
||||
expect(getState().removeError?.code).toBe("storage");
|
||||
expect(deps.refresh).not.toHaveBeenCalled();
|
||||
expect(deps.refresh).toHaveBeenCalledTimes(1);
|
||||
expect(deps.logWarn).toHaveBeenLastCalledWith("License: the stored key could not be removed", "storage");
|
||||
});
|
||||
|
||||
it("reloads the license context after a failure, before naming it", async () => {
|
||||
// Rust deletes the key first: a token that resisted leaves the key gone, and the card must
|
||||
// follow the disk rather than keep the key under "not removed" (#387 review).
|
||||
const { deps, events, getState } = removalHarness([true], [failure("storage")]);
|
||||
await performRemoveLicense(deps, valid);
|
||||
expect(events.slice(-2)).toEqual(["refresh", "REMOVE_ERROR"]);
|
||||
expect(deps.refresh).toHaveBeenCalledTimes(1);
|
||||
expect(getState().removeError?.code).toBe("storage");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -29,9 +29,11 @@ import { useLicenseContext } from "../contexts/LicenseContext";
|
|||
// A `license_revoked` answer, from any of the three calls, is persisted by the Rust
|
||||
// command that received it (#365): this hook only reloads the LicenseContext, so the
|
||||
// edition (Free) and the card follow. The revocation check, made at app start and after
|
||||
// a key is entered (#373), shares that budget: with it, the machine-limit path reaches
|
||||
// the 5 requests of the first minute. Removing the key of an activated machine spends one
|
||||
// deactivate call (#370).
|
||||
// a key is entered (#373), shares that budget: one check brings the machine-limit path to
|
||||
// the 5 requests of the first minute, and two checks in that minute (one at start for a
|
||||
// previous key, one when a new key is entered) bring it to 6, so its last call can get a
|
||||
// 429, which the card names as a rate-limit error. Removing the key of an activated
|
||||
// machine spends one deactivate call (#370).
|
||||
//
|
||||
// Loading the list may remove this machine's activation token, when the list names no
|
||||
// activation of this machine (#367): the activation status is read again after every list
|
||||
|
|
@ -370,6 +372,9 @@ export interface RemoveLicenseDeps extends RemoveLicenseUi {
|
|||
* staying occupied. Any other failure is named on the card. Each removal, and each release or
|
||||
* removal that failed, is written to the in-app log. After a removal the license context is
|
||||
* reloaded: the card remounts without a key, and the revocation check has nothing to send.
|
||||
* It is reloaded after a failure too (local reads only): Rust deletes the key first, so a
|
||||
* token that could not be deleted leaves the key gone, and the card must not keep showing it
|
||||
* under "the key was not removed".
|
||||
*/
|
||||
export async function performRemoveLicense(
|
||||
deps: RemoveLicenseDeps,
|
||||
|
|
@ -392,6 +397,7 @@ export async function performRemoveLicense(
|
|||
}
|
||||
if (!result.ok) {
|
||||
deps.logWarn(REMOVE_LOG.failed, result.error.code);
|
||||
await deps.refresh();
|
||||
deps.dispatch({ type: "REMOVE_ERROR", error: result.error });
|
||||
return;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue