From 05f29b82f6d479194cd331a9072a2ed03ec7d1c1 Mon Sep 17 00:00:00 2001 From: Priec Date: Thu, 13 Aug 2026 22:51:57 +0200 Subject: [PATCH] global is being global and i can select it and so on --- web/src/pages/add_table/loader.rs | 111 ++++++------- web/src/pages/admin/admin/loader.rs | 66 ++++---- web/src/pages/admin/admin/ui.rs | 34 ++++ .../pages/admin/table_definition/loader.rs | 87 ++++++---- web/src/pages/admin/table_definition/ui.rs | 44 +++++ web/src/pages/mod.rs | 1 + web/src/pages/table_scope.rs | 155 ++++++++++++++++++ 7 files changed, 368 insertions(+), 130 deletions(-) create mode 100644 web/src/pages/table_scope.rs diff --git a/web/src/pages/add_table/loader.rs b/web/src/pages/add_table/loader.rs index addda5af..a564534e 100644 --- a/web/src/pages/add_table/loader.rs +++ b/web/src/pages/add_table/loader.rs @@ -1,7 +1,10 @@ use axum::http::HeaderMap; use crate::{ - AppState, auth::GetAuthorizationRequest, definitions::common::Empty, + AppState, + auth::GetAuthorizationRequest, + definitions::{common::Empty, table_definition::GetTableCatalogRequest}, + pages::table_scope, services::authenticated_request, }; @@ -59,78 +62,60 @@ pub(crate) async fn load_page( .map_err(|error| LoadError::Backend(error.message().to_string()))? .into_inner(); - let effective_profile = draft.effective_profile_name(); + // The global scope is the catalog's answer, here as everywhere else, so a + // shared table the admin panel lists is a shared table this page can link + // to — see `crate::pages::table_scope`. + let catalog_tables = definitions + .get_table_catalog( + authenticated_request(headers, GetTableCatalogRequest { profile_name: None }) + .map_err(|_| LoadError::Unauthenticated)?, + ) + .await + .map_err(|error| LoadError::Backend(error.message().to_string()))? + .into_inner() + .tables; - // A global table belongs to every profile, so it is a link target from - // every scope. It is listed under each profile in the tree, so it is - // collected once here and deduplicated by name. - let global_tables = tree - .profiles - .iter() - .flat_map(|profile| profile.tables.iter()) - .filter(|table| table.global) - .map(|table| (table.name.clone(), table.table_kind.clone())) - .collect::>() + let effective_profile = draft.effective_profile_name(); + // A global table is a link target from every scope, and a global table + // being created may only link to another global one. + let tables = if draft.global { + table_scope::global_tables(&catalog_tables) + } else { + table_scope::linkable_tables(&tree.profiles, &catalog_tables, &effective_profile) + }; + + let table_options = tables .into_iter() - .map(|(name, table_kind)| RelationTableOption { - name, - global: true, - system: table_kind == "system", + // The profile's ledger accounts are not a link target of their own: an + // ACCOUNTING column is how a row is posted to one. + .filter(|table| table.name != "accounts") + .map(|table| RelationTableOption { + name: table.name, + global: table.global, + system: table.table_kind == "system", }) .collect::>(); - if draft.global { - draft.existing_profile_tables = tree - .profiles + // Every table the new one would sit beside holds its name — including the + // shared ones, which exist in every profile, and including in a profile + // that does not exist yet. A global table would land in every profile at + // once, so for that scope every name in the deployment is taken. + draft.existing_profile_tables = if draft.global { + tree.profiles .iter() .flat_map(|profile| profile.tables.iter()) .map(|table| table.name.clone()) + .chain(catalog_tables.iter().map(|table| table.name.clone())) .collect::>() .into_iter() - .collect(); - draft.set_available_relation_table_options(global_tables); - } else { match tree - .profiles - .iter() - .find(|profile| profile.name == effective_profile) - { - // An existing profile: its tables are the link targets, and their - // names are reserved against duplicate table creation. - Some(profile) => { - let mut table_options = profile - .tables - .iter() - .filter(|table| table.name != "accounts") - .map(|table| RelationTableOption { - name: table.name.clone(), - global: table.global, - system: table.table_kind == "system", - }) - .collect::>(); - // The shared tables belong here whether or not the tree repeated - // them under this profile. - for global in &global_tables { - if !table_options.iter().any(|table| table.name == global.name) { - table_options.push(global.clone()); - } - } - draft.existing_profile_tables = table_options - .iter() - .map(|table| table.name.clone()) - .collect(); - draft.set_available_relation_table_options(table_options); - } - // A brand-new (or not-yet-named) profile has no tables of its own, - // but the shared ones are there to link to — and their names are - // taken, so the new table may not reuse one either. - None => { - draft.existing_profile_tables = global_tables - .iter() - .map(|table| table.name.clone()) - .collect(); - draft.set_available_relation_table_options(global_tables); - } - }} + .collect() + } else { + table_options + .iter() + .map(|table| table.name.clone()) + .collect() + }; + draft.set_available_relation_table_options(table_options); Ok(AddTablePageState { nav: crate::ui::Nav::new(headers, "admin").with_authorization(&authorization), diff --git a/web/src/pages/admin/admin/loader.rs b/web/src/pages/admin/admin/loader.rs index 4fc7da0c..faccba97 100644 --- a/web/src/pages/admin/admin/loader.rs +++ b/web/src/pages/admin/admin/loader.rs @@ -7,7 +7,7 @@ use crate::{ common::Empty, table_definition::GetTableCatalogRequest, table_structure::GetTableStructureRequest, }, - pages::GLOBAL_SCOPE, + pages::{GLOBAL_SCOPE, table_scope}, services::{AuthenticationError, authenticated_request}, }; @@ -46,16 +46,19 @@ pub(crate) async fn load_admin_page( // Global tables belong to every profile, so the tree repeats them under // each one. They are browsed under the global scope instead, and a - // profile shows only the tables it owns. - let global_tables = definitions - .get_table_catalog( - authenticated_request(headers, GetTableCatalogRequest { profile_name: None }) - .map_err(authentication_error)?, - ) - .await - .map_err(|error| LoadError::Backend(error.message().to_string()))? - .into_inner() - .tables; + // profile shows only the tables it owns. The catalog is where every page + // reads the global scope from — see `crate::pages::table_scope`. + let global_tables = table_scope::global_tables( + &definitions + .get_table_catalog( + authenticated_request(headers, GetTableCatalogRequest { profile_name: None }) + .map_err(authentication_error)?, + ) + .await + .map_err(|error| LoadError::Backend(error.message().to_string()))? + .into_inner() + .tables, + ); let mut profiles = vec![ProfileView { label: "Global".to_string(), @@ -72,49 +75,46 @@ pub(crate) async fn load_admin_page( let selected_profile = (!selection.profile.is_empty()).then_some(selection.profile); let selected_tables = match selected_profile.as_deref() { - Some(GLOBAL_SCOPE) => Some(global_tables.as_slice()), - Some(name) => Some( - profile_tree - .profiles - .iter() - .find(|profile| profile.name == name) - .ok_or_else(|| LoadError::InvalidSelection(format!("Unknown profile '{name}'")))? - .tables - .as_slice(), - ), + Some(GLOBAL_SCOPE) => Some(global_tables), + Some(name) => { + if !profile_tree.profiles.iter().any(|profile| profile.name == name) { + return Err(LoadError::InvalidSelection(format!("Unknown profile '{name}'"))); + } + Some(table_scope::profile_owned_tables(&profile_tree.profiles, name)) + } None => None, }; + let scope_is_chosen = selected_tables.is_some(); let tables = selected_tables .map(|tables| { tables - .iter() - .filter(|table| table.global == (selected_profile.as_deref() == Some(GLOBAL_SCOPE))) + .into_iter() .map(|table| TableView { - name: table.name.clone(), + name: table.name, // One entry per link, named by the column carrying it, so a // table pointing at one target twice reads as two links. depends_on: table .depends_on - .iter() + .into_iter() .map(|dependency| { format!("{} ({})", dependency.table_name, dependency.column_name) }) .collect(), - row_display_columns: table.row_display_columns.clone(), - table_kind: table.table_kind.clone(), + row_display_columns: table.row_display_columns, + table_kind: table.table_kind, }) .collect::>() }) .unwrap_or_default(); let selected_table = (!selection.table.is_empty()).then_some(selection.table); - if let Some(table_name) = selected_table.as_deref() { - if selected_tables.is_none() || !tables.iter().any(|table| table.name == table_name) { - return Err(LoadError::InvalidSelection(format!( - "Table '{table_name}' is not part of the selected profile" - ))); - } + if let Some(table_name) = selected_table.as_deref() + && (!scope_is_chosen || !tables.iter().any(|table| table.name == table_name)) + { + return Err(LoadError::InvalidSelection(format!( + "Table '{table_name}' is not part of the selected profile" + ))); } let columns = match (selected_profile.as_deref(), selected_table.as_deref()) { diff --git a/web/src/pages/admin/admin/ui.rs b/web/src/pages/admin/admin/ui.rs index 67ce70fd..44ea6aaf 100644 --- a/web/src/pages/admin/admin/ui.rs +++ b/web/src/pages/admin/admin/ui.rs @@ -159,6 +159,40 @@ mod tests { } } + /// A global table is picked in the global scope, and the actions offered + /// for it carry that scope. The page they lead to has to accept it — see + /// `the_delete_page_accepts_a_global_table` for the other half of this. + #[test] + fn a_global_table_is_offered_the_same_actions() { + use crate::pages::admin::admin::state::TableView; + + let page = AdminPageState { + nav: Nav::default(), + profiles: Vec::new(), + selected_profile: Some(crate::pages::GLOBAL_SCOPE.to_string()), + tables: vec![TableView { + name: "currencies".to_string(), + depends_on: Vec::new(), + row_display_columns: vec!["code".to_string()], + table_kind: "dynamic".to_string(), + }], + selected_table: Some("currencies".to_string()), + columns: Vec::new(), + can_manage_tables: true, + can_manage_scripts: true, + can_manage_validations: true, + can_export: true, + can_ecb: true, + }; + + let html = render_workspace(&page); + assert!( + html.contains("/admin/tables/delete?profile=__global&table=currencies"), + "{html}" + ); + assert!(html.contains("/admin/tables/new?global=true")); + } + #[test] fn system_columns_render_after_the_user_defined_ones() { let column = |name: &str, system: bool| crate::pages::admin::admin::state::ColumnView { diff --git a/web/src/pages/admin/table_definition/loader.rs b/web/src/pages/admin/table_definition/loader.rs index ddeb3255..72f4e631 100644 --- a/web/src/pages/admin/table_definition/loader.rs +++ b/web/src/pages/admin/table_definition/loader.rs @@ -20,10 +20,11 @@ use crate::{ definitions::{ common::Empty, table_definition::{ - GetColumnAliasRenameHistoryRequest, GetProfileDetailsRequest, MoneyRounding, - table_definition_client::TableDefinitionClient, + GetColumnAliasRenameHistoryRequest, GetProfileDetailsRequest, GetTableCatalogRequest, + MoneyRounding, table_definition_client::TableDefinitionClient, }, }, + pages::table_scope, schema::{ColumnCatalog, column_catalog}, services::authenticated_request, }; @@ -98,50 +99,68 @@ pub(crate) async fn load_page( .map(|profile| profile.name.as_str()) .collect::>(); - // A profile that no longer exists takes the table selection with it. - if inputs.selection.profile != GLOBAL_SCOPE + // A profile that no longer exists takes the table selection with it, and + // says so — the commonest way to get here with a stale one is having just + // deleted its last table, which is worth being told. + if inputs.selection.has_profile() + && inputs.selection.profile != GLOBAL_SCOPE && !profiles.contains(&inputs.selection.profile.as_str()) { - inputs.selection.profile.clear(); + let missing = std::mem::take(&mut inputs.selection.profile); inputs.selection.table.clear(); + inputs + .error + .get_or_insert(format!("Profile `{missing}` no longer exists.")); } + // The same two sources the admin panel browses by, so a table it offers is + // a table this page can act on. Reading the global scope out of the tree + // instead is what used to answer "No table chosen" for a table the panel + // had just listed. let selected_tables = if inputs.selection.is_global() { - tree.profiles.first().map(|profile| profile.tables.as_slice()) + table_scope::global_tables( + &definitions + .get_table_catalog( + authenticated_request(headers, GetTableCatalogRequest { profile_name: None }) + .map_err(|_| LoadError::Unauthenticated)?, + ) + .await + .map_err(|error| LoadError::Backend(error.message().to_string()))? + .into_inner() + .tables, + ) } else { - tree.profiles - .iter() - .find(|profile| profile.name == inputs.selection.profile) - .map(|profile| profile.tables.as_slice()) + table_scope::profile_owned_tables(&tree.profiles, &inputs.selection.profile) }; let tables = selected_tables - .map(|tables| { - tables - .iter() - .filter(|table| table.global == inputs.selection.is_global()) - .map(|table| TableSummary { - name: table.name.clone(), - table_kind: table.table_kind.clone(), - global: table.global, - // One entry per link, named by the column carrying it, so a - // table pointing at one target twice reads as two links. - depends_on: table - .depends_on - .iter() - .map(|dependency| { - format!("{} ({})", dependency.table_name, dependency.column_name) - }) - .collect(), - }) - .collect::>() + .into_iter() + .map(|table| TableSummary { + name: table.name, + table_kind: table.table_kind, + global: table.global, + // One entry per link, named by the column carrying it, so a table + // pointing at one target twice reads as two links. + depends_on: table + .depends_on + .into_iter() + .map(|dependency| format!("{} ({})", dependency.table_name, dependency.column_name)) + .collect(), }) - .unwrap_or_default(); + .collect::>(); - if !tables - .iter() - .any(|table| table.name == inputs.selection.table) + // A table the scope does not hold is dropped rather than acted on. It is + // said out loud, though: dropping it in silence is what left the panel + // telling someone who had just picked a table to go and pick one. + if inputs.selection.has_table() + && !tables + .iter() + .any(|table| table.name == inputs.selection.table) { - inputs.selection.table.clear(); + let missing = std::mem::take(&mut inputs.selection.table); + let scope = inputs.selection.scope_label(); + inputs + .error + .get_or_insert(format!("`{missing}` is not a table of {scope}.")); } let detail = match inputs.selection.has_table() { diff --git a/web/src/pages/admin/table_definition/ui.rs b/web/src/pages/admin/table_definition/ui.rs index 3e7f6570..176452f2 100644 --- a/web/src/pages/admin/table_definition/ui.rs +++ b/web/src/pages/admin/table_definition/ui.rs @@ -289,6 +289,50 @@ mod tests { assert!(!html.contains("/admin/profiles/copy?profile=billing\" method")); } + /// The other half of `a_global_table_is_offered_the_same_actions`: what the + /// admin panel links to for a global table is a page that deletes it, + /// rather than one that says no table was chosen. + #[test] + fn the_delete_page_accepts_a_global_table() { + let mut state = page(); + state.selection = Selection { + profile: crate::pages::GLOBAL_SCOPE.to_string(), + table: "currencies".to_string(), + }; + state.tables = vec![TableSummary { + name: "currencies".to_string(), + table_kind: "dynamic".to_string(), + global: true, + depends_on: Vec::new(), + }]; + state.active = "delete"; + + let html = render_delete_page(&state); + + assert!(!html.contains("Template error"), "{html}"); + assert!(html.contains("Type currencies to confirm"), "{html}"); + assert!(!html.contains("No table chosen")); + // The sentinel is never shown as if it were a profile's name. + assert!(html.contains("Global — all profiles")); + } + + /// And when the table really is gone, the page says which one rather than + /// telling someone who just picked one to go and pick one. The loader is + /// what clears the selection and sets the reason. + #[test] + fn a_dropped_selection_is_explained() { + let mut state = page(); + state.selection.table = String::new(); + state.detail = None; + state.error = Some("`invoice` is not a table of billing.".to_string()); + state.active = "delete"; + + let html = render_delete_page(&state); + + assert!(html.contains("`invoice` is not a table of billing."), "{html}"); + assert!(html.contains("No table chosen")); + } + /// A system table is the backend's own; every write is refused for it, so /// none of the write pages offer their form. #[test] diff --git a/web/src/pages/mod.rs b/web/src/pages/mod.rs index 20afce8c..9236b41e 100644 --- a/web/src/pages/mod.rs +++ b/web/src/pages/mod.rs @@ -14,3 +14,4 @@ pub(crate) mod import_export; pub(crate) mod login; pub(crate) mod permissions; pub(crate) mod register; +pub(crate) mod table_scope; diff --git a/web/src/pages/table_scope.rs b/web/src/pages/table_scope.rs new file mode 100644 index 00000000..ad8fec38 --- /dev/null +++ b/web/src/pages/table_scope.rs @@ -0,0 +1,155 @@ +//! Which tables a scope holds, decided in one place for every page that asks. +//! +//! Three pages used to answer this for themselves — the admin panel from the +//! table catalog, the table-definition workspace and the Add-table builder from +//! the profile tree — and they did not agree. The tree lists a global table +//! under every profile, so a deployment whose tree has no profiles in it (or a +//! profile the tree does not repeat them under) hid every global table from the +//! two pages that read it, while the admin panel still listed them. Picking one +//! there and acting on it then landed on "No table chosen", because the page +//! being asked to act could not see the table the panel had just offered. +//! +//! So the global scope is the catalog's answer and nothing else, and a +//! profile's own tables are the tree's. Neither is derived from the other. + +use crate::definitions::table_definition::profile_tree_response::{Profile, Table}; + +/// The tables of the global scope: the catalog for `profile_name: None`, which +/// is every shared table in the deployment, whether or not any profile exists +/// to repeat it. +pub(crate) fn global_tables(catalog: &[Table]) -> Vec { + // The catalog for the global scope reports shared tables only, but the + // filter is what makes that a property of this function rather than of the + // call that happened to fill `catalog`. + catalog.iter().filter(|table| table.global).cloned().collect() +} + +/// The tables a profile owns. Global tables are shared by every profile and +/// are browsed under the global scope, so they are not a profile's own. +pub(crate) fn profile_owned_tables(profiles: &[Profile], profile_name: &str) -> Vec
{ + profiles + .iter() + .find(|profile| profile.name == profile_name) + .map(|profile| { + profile + .tables + .iter() + .filter(|table| !table.global) + .cloned() + .collect() + }) + .unwrap_or_default() +} + +/// Everything a table in `profile_name` may point a link at: the profile's own +/// tables and the shared ones. +/// +/// A profile that does not exist yet still gets the shared tables — they are +/// there to be linked to before the profile holding the new table is created. +pub(crate) fn linkable_tables( + profiles: &[Profile], + catalog: &[Table], + profile_name: &str, +) -> Vec
{ + let mut tables = profile_owned_tables(profiles, profile_name); + for global in global_tables(catalog) { + if !tables.iter().any(|table| table.name == global.name) { + tables.push(global); + } + } + tables +} + +#[cfg(test)] +pub(crate) mod tests { + use super::*; + + pub(crate) fn table(name: &str, global: bool) -> Table { + Table { + id: 0, + name: name.to_string(), + depends_on: Vec::new(), + row_display_columns: Vec::new(), + table_kind: "dynamic".to_string(), + global, + } + } + + fn profile(name: &str, tables: Vec
) -> Profile { + Profile { + name: name.to_string(), + tables, + } + } + + /// The regression: the admin panel reads the global scope from the catalog, + /// so every other page has to see the same tables there — including when + /// the profile tree the other pages used to read has no profiles in it at + /// all, which left the global scope looking empty and cleared the selection + /// the panel had just made. + #[test] + fn the_global_scope_is_the_catalog_even_with_no_profiles_in_the_tree() { + let catalog = vec![table("currencies", true), table("countries", true)]; + + assert_eq!( + global_tables(&catalog) + .iter() + .map(|table| table.name.as_str()) + .collect::>(), + ["currencies", "countries"] + ); + // And a profile-shaped question about an empty tree is empty, rather + // than falling back to some other profile's tables. + assert!(profile_owned_tables(&[], "billing").is_empty()); + } + + #[test] + fn a_profile_owns_its_own_tables_and_not_another_profiles() { + let profiles = vec![ + profile( + "billing", + vec![table("invoice", false), table("currencies", true)], + ), + profile("payroll", vec![table("employee", false)]), + ]; + + assert_eq!( + profile_owned_tables(&profiles, "billing") + .iter() + .map(|table| table.name.as_str()) + .collect::>(), + ["invoice"], + "a global table is browsed under the global scope" + ); + assert!(profile_owned_tables(&profiles, "unknown").is_empty()); + } + + /// A link may point at a shared table, so the two lists are joined here — + /// once, without repeating a table the tree already listed under the + /// profile. + #[test] + fn link_targets_are_the_profiles_own_tables_and_the_shared_ones() { + let profiles = vec![profile( + "billing", + vec![table("invoice", false), table("currencies", true)], + )]; + let catalog = vec![table("currencies", true), table("countries", true)]; + + let names = |tables: Vec
| { + tables + .iter() + .map(|table| table.name.clone()) + .collect::>() + }; + + assert_eq!( + names(linkable_tables(&profiles, &catalog, "billing")), + ["invoice", "currencies", "countries"] + ); + // A profile that does not exist yet can still link to the shared ones. + assert_eq!( + names(linkable_tables(&profiles, &catalog, "")), + ["currencies", "countries"] + ); + } +}