From afc1a15959a106a2a14d3ce47eb64cf00e562687 Mon Sep 17 00:00:00 2001 From: Marius Vollmer Date: Tue, 15 Oct 2024 12:43:12 +0300 Subject: [PATCH] shell: Take docs from the correct manifest item For example, "network/firewall" has no manifest entry of its own. Rather, it is a sub-page of "network" and should show the same documentation. We also need to check the parent first now, since for example "metrics" is a sub-page of "system", but still wants to show its own docs. Fixes #21040 --- pkg/shell/state.jsx | 14 ++++++++++++++ pkg/shell/topnav.jsx | 31 ++++++++----------------------- pkg/shell/util.jsx | 23 +++++++++++++++-------- test/verify/check-pages | 5 +++++ 4 files changed, 42 insertions(+), 31 deletions(-) diff --git a/pkg/shell/state.jsx b/pkg/shell/state.jsx index ca5ee15cddaa..f7ffb9ea82c0 100644 --- a/pkg/shell/state.jsx +++ b/pkg/shell/state.jsx @@ -471,6 +471,18 @@ export function ShellState() { * * And then, the "metrics" path has the "Overview" item associated * with it, although the two come from different packages. + * + * - current_manifest + * + * The manifest corresponding to the "path" part of + * "current_location". The "current_manifest_item" is not + * necessarily part of this manifest, but this manifest is always + * from the same package as the files loaded for the current + * location. + * + * For example, for the "metrics" path the "current_manifest" will + * be for the "metrics" package, while "current_manifest_item" is + * for the "Overview" menu entry from the "system" package. */ const self = { @@ -486,6 +498,7 @@ export function ShellState() { current_machine: null, current_manifest_item: null, current_machine_manifest_items: null, + current_manifest: null, // Methods jump, @@ -554,6 +567,7 @@ export function ShellState() { self.current_machine = machine; self.current_machine_manifest_items = compiled; self.current_manifest_item = item; + self.current_manifest = compiled.find_path_manifest(location.path); let frame = null; if (location.path && (machine.state == "connected" || machine.state == "connecting")) diff --git a/pkg/shell/topnav.jsx b/pkg/shell/topnav.jsx index c1ceff5c5fc3..f4a6d98ef486 100644 --- a/pkg/shell/topnav.jsx +++ b/pkg/shell/topnav.jsx @@ -90,9 +90,9 @@ export class TopNav extends React.Component { render() { const Dialogs = this.context; const { - current_location, current_machine, - current_machine_manifest_items, + current_manifest_item, + current_manifest, current_frame, } = this.props.state; @@ -113,27 +113,12 @@ export class TopNav extends React.Component { } } - // NOTE: The following is arguably wrong. We should not index - // the manifest items with the location path. Instead, we - // should use state.current_manifest_item. - // - // See https://github.com/cockpit-project/cockpit/issues/21040 - // - const item = current_machine_manifest_items.items[current_location.path]; - if (item && item.docs) - docs = item.docs; - - // NOTE: The following is also arguably wrong. We should not - // index the manifests with the location path either. We - // should use only the first component after splitting the - // path at "/". Also, we should look into - // current_machine.manifests instead of cockpit.manifests. - // - if (docs.length === 0) { - const comp = cockpit.manifests[current_location.path]; - if (comp && comp.parent && comp.parent.docs) - docs = comp.parent.docs; - } + // Check first whether we have docs in the "parent" section of + // the manifest. + if (current_manifest.parent && current_manifest.parent.docs) + docs = current_manifest.parent.docs; + else if (current_manifest_item.docs) + docs = current_manifest_item.docs; const docItems = []; diff --git a/pkg/shell/util.jsx b/pkg/shell/util.jsx index 0c44455448cb..1098b121009f 100644 --- a/pkg/shell/util.jsx +++ b/pkg/shell/util.jsx @@ -86,11 +86,11 @@ export function push_window_location(location) { window.history.pushState(null, "", encode_location(location)); } -function CompiledComponents() { +function CompiledComponents(manifests) { const self = this; self.items = {}; - self.load = function(manifests, section) { + self.load = function(section) { Object.entries(manifests || { }).forEach(([name, manifest]) => { Object.entries(manifest[section] || { }).forEach(([prop, info]) => { const item = { @@ -167,21 +167,28 @@ function CompiledComponents() { } // Still don't know where it comes from, check for parent - if (!component) { - const comp = cockpit.manifests[path]; + if (!component && manifests) { + const comp = manifests[path]; if (comp && comp.parent) component = comp.parent.component; } return self.items[component] || { path, label: path, section: "menu" }; }; + + self.find_path_manifest = function(path) { + const parts = path.split("/"); + const pkg = parts[0]; + + return (manifests && manifests[pkg]) || { }; + }; } export function compile_manifests(manifests) { - const compiled = new CompiledComponents(); - compiled.load(manifests, "tools"); - compiled.load(manifests, "dashboard"); - compiled.load(manifests, "menu"); + const compiled = new CompiledComponents(manifests); + compiled.load("tools"); + compiled.load("dashboard"); + compiled.load("menu"); return compiled; } diff --git a/test/verify/check-pages b/test/verify/check-pages index f7e0a8742eda..71239ba55aa2 100755 --- a/test/verify/check-pages +++ b/test/verify/check-pages @@ -130,6 +130,11 @@ OnCalendar=daily b.click("#toggle-docs") b.wait_not_present("#toggle-docs-menu") b.go("/network") + self.checkDocs(["Managing networking bonds", "Managing networking teams", + "Managing networking bridges", "Managing VLANs", "Managing firewall"]) + b.go("/system") + self.checkDocs(["Configuring system settings"]) + b.go("/network/firewall") self.checkDocs(["Managing networking bonds", "Managing networking teams", "Managing networking bridges", "Managing VLANs", "Managing firewall"]) b.go("/system/services")