Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(web): drop issues context #1470

Merged
merged 3 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions web/src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { useProduct, useProductChanges } from "./queries/software";
import { CONFIG, INSTALL, STARTUP } from "~/client/phase";
import { BUSY } from "~/client/status";
import { useL10nConfigChanges } from "~/queries/l10n";
import { useIssues, useIssuesChanges } from "./queries/issues";

/**
* Main application component.
Expand All @@ -45,6 +46,7 @@ function App() {
const { language } = useInstallerL10n();
useL10nConfigChanges();
useProductChanges();
useIssuesChanges();

const Content = () => {
if (error) return <ServerError />;
Expand Down
6 changes: 6 additions & 0 deletions web/src/App.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { STARTUP, CONFIG, INSTALL } from "~/client/phase";
import { IDLE, BUSY } from "~/client/status";
import { useL10nConfigChanges } from "./queries/l10n";
import { useProductChanges } from "./queries/software";
import { useIssuesChanges } from "./queries/issues";

jest.mock("~/client");

Expand All @@ -52,6 +53,11 @@ jest.mock("~/queries/l10n", () => ({
useL10nConfigChanges: () => jest.fn(),
}));

jest.mock("~/queries/issues", () => ({
...jest.requireActual("~/queries/issues"),
useIssuesChanges: () => jest.fn(),
}));

const mockClientStatus = {
connected: true,
error: false,
Expand Down
61 changes: 1 addition & 60 deletions web/src/client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,6 @@ import { HTTPClient, WSClient } from "./http";
* @property {UsersClient} users - users client.
* @property {QuestionsClient} questions - questions client.
* @property {() => WSClient} ws - Agama WebSocket client.
* @property {() => Promise<Issues>} issues - issues from all contexts.
* @property {(handler: IssuesHandler) => (() => void)} onIssuesChange - registers a handler to run
* when issues from any context change. It returns a function to deregister the handler.
* @property {() => boolean} isConnected - determines whether the client is connected
* @property {() => boolean} isRecoverable - determines whether the client is recoverable after disconnected
* @property {(handler: () => void) => (() => void)} onConnect - registers a handler to run
Expand All @@ -55,25 +52,6 @@ import { HTTPClient, WSClient } from "./http";
* handler.
*/

/**
* @typedef {import ("~/client/mixins").Issue} Issue
*
* @typedef {object} Issues
* @property {Issue[]} [product] - Issues from product.
* @property {Issue[]} [storage] - Issues from storage.
* @property {Issue[]} [software] - Issues from software.
* @property {Issue[]} [users] - Issues from users.
* @property {boolean} [isEmpty] - Whether the list is empty
*
* @typedef {(issues: Issues) => void} IssuesHandler
*/

const createIssuesList = (product = [], software = [], storage = [], users = []) => {
const list = { product, storage, software, users };
list.isEmpty = !Object.values(list).some((v) => v.length > 0);
return list;
};

/**
* Creates the Agama client
*
Expand All @@ -93,41 +71,6 @@ const createClient = (url) => {
const users = new UsersClient(client);
const questions = new QuestionsClient(client);

/**
* Gets all issues, grouping them by context.
*
* TODO: issues are requested by several components (e.g., overview sections, notifications
* provider, issues page, storage page, etc). There should be an issues provider.
*
* @returns {Promise<Issues>}
*/
const issues = async () => {
const productIssues = await product.getIssues();
const storageIssues = await storage.getIssues();
const softwareIssues = await software.getIssues();
const usersIssues = await users.getIssues();
return createIssuesList(productIssues, softwareIssues, storageIssues, usersIssues);
};

/**
* Registers a callback to be executed when issues change.
*
* @param {IssuesHandler} handler - Callback function.
* @return {() => void} - Function to deregister the callback.
*/
const onIssuesChange = (handler) => {
const unsubscribeCallbacks = [];

unsubscribeCallbacks.push(product.onIssuesChange((i) => handler({ product: i })));
unsubscribeCallbacks.push(storage.onIssuesChange((i) => handler({ storage: i })));
unsubscribeCallbacks.push(software.onIssuesChange((i) => handler({ software: i })));
unsubscribeCallbacks.push(users.onIssuesChange((i) => handler({ users: i })));

return () => {
unsubscribeCallbacks.forEach((cb) => cb());
};
};

const isConnected = () => client.ws().isConnected() || false;
const isRecoverable = () => !!client.ws().isRecoverable();

Expand All @@ -141,8 +84,6 @@ const createClient = (url) => {
storage,
users,
questions,
issues,
onIssuesChange,
isConnected,
isRecoverable,
onConnect: (handler) => client.ws().onOpen(handler),
Expand All @@ -157,4 +98,4 @@ const createDefaultClient = async () => {
return createClient(httpUrl);
};

export { createClient, createDefaultClient, phase, createIssuesList };
export { createClient, createDefaultClient, phase };
53 changes: 1 addition & 52 deletions web/src/client/mixins.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,57 +72,6 @@ const buildIssue = ({ description, details, source, severity }) => {
};
};

/**
* Extends the given class with methods to get the issues over D-Bus
*
* @template {!WithHTTPClient} T
* @param {T} superclass - superclass to extend
* @param {string} issues_path - validation resource path (e.g., "/manager/issues").
* @param {string} service_name - service name (e.g., "org.opensuse.Agama.Manager1").
*/
const WithIssues = (superclass, issues_path, service_name) =>
class extends superclass {
/**
* Returns the issues
*
* @return {Promise<Issue[]>}
*/
async getIssues() {
const response = await this.client.get(issues_path);
if (!response.ok) {
console.log("get issues failed with:", response);
return [];
} else {
const issues = await response.json();
return issues.map(buildIssue);
}
}

/**
* Gets all issues with error severity
*
* @return {Promise<Issue[]>}
*/
async getErrors() {
const issues = await this.getIssues();
return issues.filter((i) => i.severity === "error");
}

/**
* Registers a callback to run when the issues change
*
* @param {IssuesHandler} handler - callback function
* @return {import ("./http").RemoveFn} function to disable the callback
*/
onIssuesChange(handler) {
return this.client.onEvent("IssuesChanged", ({ service, issues }) => {
if (service === service_name) {
handler(issues.map(buildIssue));
}
});
}
};

/**
* Extends the given class with methods to get and track the service status
*
Expand Down Expand Up @@ -265,4 +214,4 @@ const createError = (message) => {
return { message };
};

export { WithIssues, WithProgress, WithStatus };
export { WithProgress, WithStatus };
23 changes: 6 additions & 17 deletions web/src/client/software.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@

// @ts-check

import { WithIssues, WithProgress, WithStatus } from "./mixins";
import { WithProgress, WithStatus } from "./mixins";

const SOFTWARE_SERVICE = "org.opensuse.Agama.Software1";
const PRODUCT_PATH = "/org/opensuse/Agama/Software1/Product";

/**
* Enum for the reasons to select a pattern
Expand Down Expand Up @@ -185,17 +184,13 @@ class SoftwareBaseClient {
/**
* Manages software and product configuration.
*/
class SoftwareClient extends WithIssues(
WithProgress(
WithStatus(SoftwareBaseClient, "/software/status", SOFTWARE_SERVICE),
"/software/progress",
SOFTWARE_SERVICE,
),
"/software/issues/software",
"/org/opensuse/Agama/Software1",
class SoftwareClient extends WithProgress(
WithStatus(SoftwareBaseClient, "/software/status", SOFTWARE_SERVICE),
"/software/progress",
SOFTWARE_SERVICE,
) {}

class ProductBaseClient {
class ProductClient {
/**
* @param {import("./http").HTTPClient} client - HTTP client.
*/
Expand Down Expand Up @@ -336,10 +331,4 @@ class ProductBaseClient {
}
}

class ProductClient extends WithIssues(
ProductBaseClient,
"/software/issues/product",
PRODUCT_PATH,
) {}

export { ProductClient, SelectedBy, SoftwareClient };
12 changes: 4 additions & 8 deletions web/src/client/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
// cspell:ignore ptable

import { compact, hex, uniq } from "~/utils";
import { WithIssues, WithProgress, WithStatus } from "./mixins";
import { WithProgress, WithStatus } from "./mixins";
import { HTTPClient } from "./http";

const SERVICE_NAME = "org.opensuse.Agama.Storage1";
Expand Down Expand Up @@ -1652,13 +1652,9 @@ class StorageBaseClient {
/**
* Allows interacting with the storage settings
*/
class StorageClient extends WithIssues(
WithProgress(
WithStatus(StorageBaseClient, "/storage/status", SERVICE_NAME),
"/storage/progress",
SERVICE_NAME,
),
"/storage/issues",
class StorageClient extends WithProgress(
WithStatus(StorageBaseClient, "/storage/status", SERVICE_NAME),
"/storage/progress",
SERVICE_NAME,
) {}

Expand Down
72 changes: 0 additions & 72 deletions web/src/client/storage.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1346,78 +1346,6 @@ describe.skip("#onDeprecate", () => {
});
});

describe("#getIssues", () => {
beforeEach(() => {
client = new StorageClient(http);
});

describe("if there are no issues", () => {
beforeEach(() => {
mockJsonFn.mockResolvedValue([]);
});

it("returns an empty list", async () => {
const issues = await client.getIssues();
expect(issues).toEqual([]);
});
});

describe("if there are issues", () => {
beforeEach(() => {
mockJsonFn.mockResolvedValue(contexts.withIssues());
});

it("returns the list of issues", async () => {
const issues = await client.getIssues();
expect(issues).toEqual(
expect.arrayContaining([
{ description: "Issue 1", details: "", source: "system", severity: "error" },
{ description: "Issue 2", details: "", source: "system", severity: "warn" },
{ description: "Issue 3", details: "", source: "config", severity: "error" },
]),
);
});
});
});

describe("#getErrors", () => {
beforeEach(() => {
client = new StorageClient(http);
mockJsonFn.mockResolvedValue(contexts.withIssues());
});

it("returns the issues with error severity", async () => {
const errors = await client.getErrors();
expect(errors.map((e) => e.description)).toEqual(
expect.arrayContaining(["Issue 1", "Issue 3"]),
);
});
});

// @fixme See note at the test of onDeprecate about mocking signals
describe.skip("#onIssuesChange", () => {
it("runs the handler when the issues change", async () => {
client = new StorageClient();

const handler = jest.fn();
client.onIssuesChange(handler);

emitSignal("/org/opensuse/Agama/Storage1", "org.opensuse.Agama1.Issues", {
All: {
v: [
["Issue 1", "", 1, 0],
["Issue 2", "", 2, 1],
],
},
});

expect(handler).toHaveBeenCalledWith([
{ description: "Issue 1", details: "", source: "system", severity: "warn" },
{ description: "Issue 2", details: "", source: "config", severity: "error" },
]);
});
});

describe("#system", () => {
describe("#getDevices", () => {
beforeEach(() => {
Expand Down
11 changes: 2 additions & 9 deletions web/src/client/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@

// @ts-check

import { WithIssues } from "./mixins";

const SERVICE_NAME = "org.opensuse.Agama.Manager1";

/**
Expand All @@ -48,11 +46,11 @@ const SERVICE_NAME = "org.opensuse.Agama.Manager1";
*/

/**
* Users client
* Client to interact with the Agama users service
*
* @ignore
*/
class UsersBaseClient {
class UsersClient {
/**
* @param {import("./http").HTTPClient} client - HTTP client.
*/
Expand Down Expand Up @@ -183,9 +181,4 @@ class UsersBaseClient {
}
}

/**
* Client to interact with the Agama users service
*/
class UsersClient extends WithIssues(UsersBaseClient, "/users/issues", SERVICE_NAME) {}

export { UsersClient };
Loading
Loading